Modify Sanitizer

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

@@ -15,6 +15,18 @@ import UnsafeUrlForward
 import semmle.code.java.dataflow.FlowSources
 import DataFlow::PathGraph
 
+private class StartsWithSanitizer extends DataFlow::BarrierGuard {
+  StartsWithSanitizer() {
+    this.(MethodAccess).getMethod().hasName("startsWith") and
+    this.(MethodAccess).getMethod().getDeclaringType() instanceof TypeString and
+    this.(MethodAccess).getMethod().getNumberOfParameters() = 1
+  }
+
+  override predicate checks(Expr e, boolean branch) {
+    e = this.(MethodAccess).getQualifier() and branch = true
+  }
+}
+
 class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration {
   UnsafeUrlForwardFlowConfig() { this = "UnsafeUrlForwardFlowConfig" }
 
@@ -25,39 +37,18 @@ class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration {
       ma.getMethod()
           .getDeclaringType()
           .getASupertype*()
-          .hasQualifiedName("javax.servlet.http", "HttpServletRequest")
+          .hasQualifiedName("javax.servlet.http", "HttpServletRequest") and
+      ma = source.asExpr()
     )
   }
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeUrlForwardSink }
 
-  override predicate isSanitizer(DataFlow::Node node) {
-    node.getType() instanceof BoxedType
-    or
-    node.getType() instanceof PrimitiveType
-    or
-    exists(AddExpr ae |
-      ae.getRightOperand() = node.asExpr() and
-      (
-        not ae.getLeftOperand().(CompileTimeConstantExpr).getStringValue().matches("/WEB-INF/%") and
-        not ae.getLeftOperand().(CompileTimeConstantExpr).getStringValue() = "forward:"
-      )
-    )
-    or
-    exists(MethodAccess ma, int i |
-      ma.getMethod().hasName("format") and
-      ma.getMethod().getDeclaringType() instanceof TypeString and
-      ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "redirect:" and
-      ma.getArgument(i) = node.asExpr() and
-      i != 0
-    )
-    or
-    exists(StringBuilderAppendCall ma1, StringBuilderAppendCall ma2 |
-      DataFlow2::localExprFlow(ma1.getQualifier(), ma2.getQualifier()) and
-      ma1.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "redirect:" and
-      ma2.getArgument(0) = node.asExpr()
-    )
+  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    guard instanceof StartsWithSanitizer
   }
+
+  override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer }
 }
 
 from DataFlow::PathNode source, DataFlow::PathNode sink, UnsafeUrlForwardFlowConfig conf

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

@@ -3,11 +3,141 @@ import DataFlow
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.frameworks.Servlets
 
+/** A sanitizer for unsafe url forward vulnerabilities. */
+abstract class UnsafeUrlForwardSanitizer extends DataFlow::Node { }
+
+private class PrimitiveSanitizer extends UnsafeUrlForwardSanitizer {
+  PrimitiveSanitizer() {
+    this.getType() instanceof PrimitiveType or
+    this.getType() instanceof BoxedType or
+    this.getType() instanceof NumberType
+  }
+}
+
+private class UnsafeUrlForwardSantizer extends UnsafeUrlForwardSanitizer {
+  UnsafeUrlForwardSantizer() { this.asExpr() instanceof UnsafeUrlForwardSanitizedExpr }
+}
+
+private class UnsafeUrlForwardSanitizingConstantPrefix extends CompileTimeConstantExpr {
+  UnsafeUrlForwardSanitizingConstantPrefix() {
+    not this.getStringValue().matches("/WEB-INF/%") and
+    not this.getStringValue() = "forward:"
+  }
+}
+
+private Expr getAUnsafeUrlForwardSanitizingPrefix() {
+  result instanceof UnsafeUrlForwardSanitizingConstantPrefix
+  or
+  result.(AddExpr).getAnOperand() = getAUnsafeUrlForwardSanitizingPrefix()
+}
+
 /** A call to `StringBuilder.append` method. */
-class StringBuilderAppendCall extends MethodAccess {
-  StringBuilderAppendCall() {
-    this.getMethod().hasName("append") and
-    this.getMethod().getDeclaringType() instanceof StringBuildingType
+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:
+    this =
+      any(AddExpr add | add.getLeftOperand() = getAUnsafeUrlForwardSanitizingPrefix())
+          .getRightOperand()
+    or
+    // Sanitize expressions that come after a sanitizing prefix in a sequence of StringBuilder operations:
+    exists(
+      StringBuilderConstructorOrAppend appendSanitizingConstant,
+      StringBuilderAppend subsequentAppend, StringBuilderVarExt v
+    |
+      appendSanitizingConstant = v.getAConstructorOrAppend() and
+      appendSanitizingConstant.getArgument(0) = getAUnsafeUrlForwardSanitizingPrefix() and
+      v.getSubsequentAppendIncludingAssignmentChains(appendSanitizingConstant) = subsequentAppend and
+      this = subsequentAppend.getArgument(0)
+    )
+    or
+    exists(MethodAccess ma, int i |
+      ma.getMethod().hasName("format") and
+      ma.getMethod().getDeclaringType() instanceof TypeString and
+      ma.getArgument(0) instanceof UnsafeUrlForwardSanitizingConstantPrefix and
+      ma.getArgument(i) = this and
+      i != 0
+    )
   }
 }
 
@@ -16,7 +146,7 @@ class StringBuilderAppendCall extends MethodAccess {
  *
  * E.g: `"forward:" + url`
  */
-class ForwardBuilderExpr extends AddExpr {
+private class ForwardBuilderExpr extends AddExpr {
   ForwardBuilderExpr() {
     this.getLeftOperand().(CompileTimeConstantExpr).getStringValue() = "forward:"
   }
@@ -27,7 +157,7 @@ class ForwardBuilderExpr extends AddExpr {
  *
  * E.g: `StringBuilder.append("forward:")`
  */
-class ForwardAppendCall extends StringBuilderAppendCall {
+private class ForwardAppendCall extends StringBuilderAppend {
   ForwardAppendCall() {
     this.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "forward:"
   }

java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected

@@ -20,6 +20,7 @@ nodes
 | UnsafeUrlForward.java:38:33:38:35 | url | semmle.label | url |
 | UnsafeUrlForward.java:47:19:47:28 | url : String | semmle.label | url : String |
 | UnsafeUrlForward.java:49:33:49:62 | ... + ... | semmle.label | ... + ... |
+subpaths
 #select
 | UnsafeUrlForward.java:14:27:14:29 | url | UnsafeUrlForward.java:13:27:13:36 | url : String | UnsafeUrlForward.java:14:27:14:29 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:13:27:13:36 | url | user-provided value |
 | UnsafeUrlForward.java:20:28:20:30 | url | UnsafeUrlForward.java:18:27:18:36 | url : String | UnsafeUrlForward.java:20:28:20:30 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:18:27:18:36 | url | user-provided value |

java/ql/test/experimental/query-tests/security/CWE-552/options

@@ -1 +1 @@
-//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.2.3/
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.3.8/