Fix

Author: haby0

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.java

@@ -15,36 +15,7 @@ public ModelAndView bad1(String url) {
 	}
 
 	@GetMapping("/bad2")
-	public ModelAndView bad2(String url) {
-		ModelAndView modelAndView = new ModelAndView();
-		modelAndView.setViewName(url);
-		return modelAndView;
-	}
-
-	@GetMapping("/bad3")
-	public String bad3(String url) {
-		return "forward:" + url + "/swagger-ui/index.html";
-	}
-
-	@GetMapping("/bad4")
-	public ModelAndView bad4(String url) {
-		ModelAndView modelAndView = new ModelAndView("forward:" + url);
-		return modelAndView;
-	}
-
-	@GetMapping("/bad5")
-	public void bad5(String url, HttpServletRequest request, HttpServletResponse response) {
-		try {
-			request.getRequestDispatcher(url).include(request, response);
-		} catch (ServletException e) {
-			e.printStackTrace();
-		} catch (IOException e) {
-			e.printStackTrace();
-		}
-	}
-
-	@GetMapping("/bad6")
-	public void bad6(String url, HttpServletRequest request, HttpServletResponse response) {
+	public void bad2(String url, HttpServletRequest request, HttpServletResponse response) {
 		try {
 			request.getRequestDispatcher("/WEB-INF/jsp/" + url + ".jsp").include(request, response);
 		} catch (ServletException e) {

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp

@@ -17,8 +17,7 @@
 <example>
 
 <p>The following examples show the bad case and the good case respectively.
-In <code>bad1</code> method and <code>bad2</code> method and <code>bad3</code> method and 
-<code>bad4</code> method and <code>bad5</code> method and <code>bad6</code> method, shows an HTTP request parameter being used directly in a URL forward
+The <code>bad</code> methods show an HTTP request parameter being used directly in a URL forward
 without validating the input, which may cause file leakage. In <code>good1</code> method, 
 ordinary forwarding requests are shown, which will not cause file leakage.
 </p>

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll

@@ -3,6 +3,7 @@ import DataFlow
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.frameworks.Servlets
 import semmle.code.java.frameworks.spring.SpringWeb
+import semmle.code.java.security.RequestForgery
 
 /** A sanitizer for unsafe url forward vulnerabilities. */
 abstract class UnsafeUrlForwardSanitizer extends DataFlow::Node { }
@@ -32,88 +33,6 @@ private Expr getAUnsafeUrlForwardSanitizingPrefix() {
   result.(AddExpr).getAnOperand() = getAUnsafeUrlForwardSanitizingPrefix()
 }
 
-/** A call to `StringBuilder.append` method. */
-class StringBuilderAppend extends MethodAccess {
-  StringBuilderAppend() {
-    this.getMethod().getDeclaringType() instanceof StringBuildingType and
-    this.getMethod().hasName("append")
-  }
-}
-
-private Expr getQualifier(Expr e) { result = e.(MethodAccess).getQualifier() }
-
-/**
- * An extension of `StringBuilderVar` that also accounts for strings appended in StringBuilder/Buffer's constructor
- * and in `append` calls chained onto the constructor call.
- *
- * The original `StringBuilderVar` doesn't care about these because it is designed to model taint, and
- * in taint rules terms these are not needed, as the connection between construction, appends and the
- * eventual `toString` is more obvious.
- */
-private class StringBuilderVarExt extends StringBuilderVar {
-  /**
-   * Returns a first assignment after this StringBuilderVar is first assigned.
-   *
-   * For example, for `StringBuilder sbv = new StringBuilder("1").append("2"); sbv.append("3").append("4");`
-   * this returns the append of `"3"`.
-   */
-  private StringBuilderAppend getAFirstAppendAfterAssignment() {
-    result = this.getAnAppend() and not result = this.getNextAppend(_)
-  }
-
-  /**
-   * Gets the next `append` after `prev`, where `prev` is, perhaps after some more `append` or other
-   * chained calls, assigned to this `StringBuilderVar`.
-   */
-  private StringBuilderAppend getNextAssignmentChainedAppend(StringBuilderConstructorOrAppend prev) {
-    getQualifier*(result) = this.getAnAssignedValue() and
-    result.getQualifier() = prev
-  }
-
-  /**
-   * Get a constructor call or `append` call that contributes a string to this string builder.
-   */
-  StringBuilderConstructorOrAppend getAConstructorOrAppend() {
-    exists(this.getNextAssignmentChainedAppend(result)) or
-    result = this.getAnAssignedValue() or
-    result = this.getAnAppend()
-  }
-
-  /**
-   * Like `StringBuilderVar.getNextAppend`, except including appends and constructors directly
-   * assigned to this `StringBuilderVar`.
-   */
-  private StringBuilderAppend getNextAppendIncludingAssignmentChains(
-    StringBuilderConstructorOrAppend prev
-  ) {
-    result = getNextAssignmentChainedAppend(prev)
-    or
-    prev = this.getAnAssignedValue() and
-    result = this.getAFirstAppendAfterAssignment()
-    or
-    result = this.getNextAppend(prev)
-  }
-
-  /**
-   * Implements `StringBuilderVarExt.getNextAppendIncludingAssignmentChains+(prev)`.
-   */
-  pragma[nomagic]
-  StringBuilderAppend getSubsequentAppendIncludingAssignmentChains(
-    StringBuilderConstructorOrAppend prev
-  ) {
-    result = this.getNextAppendIncludingAssignmentChains(prev) or
-    result =
-      this.getSubsequentAppendIncludingAssignmentChains(this.getNextAppendIncludingAssignmentChains(prev))
-  }
-}
-
-private class StringBuilderConstructorOrAppend extends Call {
-  StringBuilderConstructorOrAppend() {
-    this instanceof StringBuilderAppend or
-    this.(ClassInstanceExpr).getConstructedType() instanceof StringBuildingType
-  }
-}
-
 private class UnsafeUrlForwardSanitizedExpr extends Expr {
   UnsafeUrlForwardSanitizedExpr() {
     // Sanitize expressions that come after a sanitizing prefix in a tree of string additions: