SSRF query: add sanitizer looking for a variety of ways of prepending a sanitizing prefix, such as one that restricts the hostname a URI will refer to.

java/ql/src/Security/CWE/CWE-918/RequestForgery.ql

@@ -25,6 +25,8 @@ class RequestForgeryConfiguration extends TaintTracking::Configuration {
   override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
     requestForgeryStep(pred, succ)
   }
+
+  override predicate isSanitizer(DataFlow::Node node) { node instanceof RequestForgerySanitizer }
 }
 
 from DataFlow::PathNode source, DataFlow::PathNode sink, RequestForgeryConfiguration conf

java/ql/src/Security/CWE/CWE-918/RequestForgery.qll

@@ -5,6 +5,8 @@ import semmle.code.java.frameworks.spring.Spring
 import semmle.code.java.frameworks.JaxWS
 import semmle.code.java.frameworks.javase.Http
 import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.dataflow.TaintTracking
+private import semmle.code.java.StringFormat
 
 predicate requestForgeryStep(DataFlow::Node pred, DataFlow::Node succ) {
   // propagate to a URI when its host is assigned to
@@ -190,3 +192,83 @@ private class SpringRestTemplateUrlMethods extends Method {
     result = ma.getArgument(0)
   }
 }
+
+/** A sanitizer for request forgery vulnerabilities. */
+abstract class RequestForgerySanitizer extends DataFlow::Node { }
+
+private class HostnameSanitzingPrefix extends CompileTimeConstantExpr {
+  int offset;
+
+  HostnameSanitzingPrefix() {
+    exists(
+      this.getStringValue().regexpFind(".*([?#]|[^?#:/\\\\][/\\\\]).*|[/\\\\][^/\\\\].*", 0, offset)
+    )
+  }
+
+  int getOffset() { result = offset }
+}
+
+private AddExpr getParentAdd(AddExpr e) { result = e.getParent() }
+
+private AddExpr getAnAddContainingHostnameSanitizingPrefix() {
+  result = getParentAdd*(any(HostnameSanitzingPrefix p).getParent())
+}
+
+private Expr getASanitizedAddOperand() {
+  exists(AddExpr e |
+    e = getAnAddContainingHostnameSanitizingPrefix() and
+    (
+      e.getLeftOperand() = getAnAddContainingHostnameSanitizingPrefix() or
+      e.getLeftOperand() instanceof HostnameSanitzingPrefix
+    ) and
+    result = e.getRightOperand()
+  )
+}
+
+private MethodAccess getNextAppend(MethodAccess append) {
+  result = any(StringBuilderVar sbv).getNextAppend(append)
+}
+
+class HostnameSanitizedExpr extends Expr {
+  HostnameSanitizedExpr() {
+    // Sanitize expressions that come after a sanitizing prefix in a tree of string additions:
+    this = getASanitizedAddOperand()
+    or
+    // Sanitize expressions that come after a sanitizing prefix in a sequence of StringBuilder operations:
+    exists(MethodAccess appendSanitizingConstant, MethodAccess subsequentAppend |
+      appendSanitizingConstant.getArgument(0) instanceof HostnameSanitzingPrefix and
+      getNextAppend*(appendSanitizingConstant) = subsequentAppend and
+      this = subsequentAppend.getArgument(0)
+    )
+    or
+    // Sanitize expressions that come after a sanitizing prefix in the args to a format call:
+    exists(
+      FormattingCall formatCall, FormatString formatString, HostnameSanitzingPrefix prefix,
+      int sanitizedFromOffset, int laterOffset, int sanitizedArg
+    |
+      formatString = unique(FormatString fs | fs = formatCall.getAFormatString()) and
+      (
+        // An argument that sanitizes will be come before this:
+        exists(int argIdx |
+          formatCall.getArgumentToBeFormatted(argIdx) = prefix and
+          sanitizedFromOffset = formatString.getAnArgUsageOffset(argIdx)
+        )
+        or
+        // The format string itself sanitizes subsequent arguments:
+        formatString = prefix.getStringValue() and
+        sanitizedFromOffset = prefix.getOffset()
+      ) and
+      laterOffset > sanitizedFromOffset and
+      laterOffset = formatString.getAnArgUsageOffset(sanitizedArg) and
+      this = formatCall.getArgumentToBeFormatted(sanitizedArg)
+    )
+  }
+}
+
+/**
+ * A value that is the result of prepending a string that prevents any value from controlling the
+ * host of a URL.
+ */
+class HostnameSantizer extends RequestForgerySanitizer {
+  HostnameSantizer() { this.asExpr() instanceof HostnameSanitizedExpr }
+}

java/ql/src/semmle/code/java/StringFormat.qll

@@ -175,6 +175,16 @@ class FormattingCall extends Call {
     )
   }
 
+  /** Gets the `i`th argument to be formatted. */
+  Expr getArgumentToBeFormatted(int i) {
+    i >= 0 and
+    if this.hasExplicitVarargsArray()
+    then
+      result =
+        this.getArgument(1 + this.getFormatStringIndex()).(ArrayCreationExpr).getInit().getInit(i)
+    else result = this.getArgument(this.getFormatStringIndex() + 1 + i)
+  }
+
   /** Holds if the varargs argument is given as an explicit array. */
   private predicate hasExplicitVarargsArray() {
     this.getNumArgument() = this.getFormatStringIndex() + 2 and
@@ -353,6 +363,11 @@ class FormatString extends string {
    * is not referred by any format specifier.
    */
   /*abstract*/ int getASkippedFmtSpecIndex() { none() }
+
+  /**
+   * Gets an offset in this format string where argument `argNo` will be interpolated, if any.
+   */
+  int getAnArgUsageOffset(int argNo) { none() }
 }
 
 private class PrintfFormatString extends FormatString {
@@ -425,6 +440,16 @@ private class PrintfFormatString extends FormatString {
     result > count(int i | fmtSpecRefersToSequentialIndex(i)) and
     not result = fmtSpecRefersToSpecificIndex(_)
   }
+
+  override int getAnArgUsageOffset(int argNo) {
+    argNo = fmtSpecRefersToSpecificIndex(result)
+    or
+    fmtSpecRefersToSequentialIndex(result) and
+    argNo = count(int i | i < result and fmtSpecRefersToSequentialIndex(i))
+    or
+    fmtSpecRefersToPrevious(result) and
+    argNo = count(int i | i < result and fmtSpecRefersToSequentialIndex(i)) - 1
+  }
 }
 
 private class LoggerFormatString extends FormatString {
@@ -449,4 +474,9 @@ private class LoggerFormatString extends FormatString {
   }
 
   override int getMaxFmtSpecIndex() { result = count(int i | fmtPlaceholder(i)) }
+
+  override int getAnArgUsageOffset(int argNo) {
+    fmtPlaceholder(result) and
+    argNo = count(int i | fmtPlaceholder(i) and i < result)
+  }
 }

java/ql/test/query-tests/security/CWE-918/RequestForgery.expected

@@ -16,6 +16,12 @@ edges
 | RequestForgery.java:19:31:19:57 | getParameter(...) : String | RequestForgery.java:19:23:19:58 | new URI(...) : URI |
 | RequestForgery.java:19:31:19:57 | getParameter(...) : String | RequestForgery.java:22:52:22:54 | uri |
 | RequestForgery.java:19:31:19:57 | getParameter(...) : String | RequestForgery.java:27:57:27:59 | uri |
+| RequestForgery.java:61:33:61:63 | getParameter(...) : String | RequestForgery.java:62:59:62:77 | new URI(...) |
+| RequestForgery.java:65:49:65:79 | getParameter(...) : String | RequestForgery.java:66:59:66:77 | new URI(...) |
+| RequestForgery.java:70:31:70:61 | getParameter(...) : String | RequestForgery.java:71:59:71:88 | new URI(...) |
+| RequestForgery.java:74:73:74:103 | getParameter(...) : String | RequestForgery.java:75:59:75:77 | new URI(...) |
+| RequestForgery.java:78:56:78:86 | getParameter(...) : String | RequestForgery.java:79:59:79:77 | new URI(...) |
+| RequestForgery.java:82:55:82:85 | getParameter(...) : String | RequestForgery.java:83:59:83:77 | new URI(...) |
 | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:32:47:32:67 | ... + ... |
 | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:37:43:37:56 | fooResourceUrl |
 | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:41:42:41:55 | fooResourceUrl |
@@ -44,6 +50,18 @@ nodes
 | RequestForgery.java:19:31:19:57 | getParameter(...) : String | semmle.label | getParameter(...) : String |
 | RequestForgery.java:22:52:22:54 | uri | semmle.label | uri |
 | RequestForgery.java:27:57:27:59 | uri | semmle.label | uri |
+| RequestForgery.java:61:33:61:63 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| RequestForgery.java:62:59:62:77 | new URI(...) | semmle.label | new URI(...) |
+| RequestForgery.java:65:49:65:79 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| RequestForgery.java:66:59:66:77 | new URI(...) | semmle.label | new URI(...) |
+| RequestForgery.java:70:31:70:61 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| RequestForgery.java:71:59:71:88 | new URI(...) | semmle.label | new URI(...) |
+| RequestForgery.java:74:73:74:103 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| RequestForgery.java:75:59:75:77 | new URI(...) | semmle.label | new URI(...) |
+| RequestForgery.java:78:56:78:86 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| RequestForgery.java:79:59:79:77 | new URI(...) | semmle.label | new URI(...) |
+| RequestForgery.java:82:55:82:85 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| RequestForgery.java:83:59:83:77 | new URI(...) | semmle.label | new URI(...) |
 | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | semmle.label | getParameter(...) : String |
 | SpringSSRF.java:32:47:32:67 | ... + ... | semmle.label | ... + ... |
 | SpringSSRF.java:37:43:37:56 | fooResourceUrl | semmle.label | fooResourceUrl |
@@ -66,6 +84,12 @@ nodes
 | RequestForgery2.java:69:29:69:32 | uri2 | RequestForgery2.java:23:27:23:53 | getParameter(...) : String | RequestForgery2.java:69:29:69:32 | uri2 | Potential server side request forgery due to $@. | RequestForgery2.java:23:27:23:53 | getParameter(...) | a user-provided value |
 | RequestForgery.java:22:52:22:54 | uri | RequestForgery.java:19:31:19:57 | getParameter(...) : String | RequestForgery.java:22:52:22:54 | uri | Potential server side request forgery due to $@. | RequestForgery.java:19:31:19:57 | getParameter(...) | a user-provided value |
 | RequestForgery.java:27:57:27:59 | uri | RequestForgery.java:19:31:19:57 | getParameter(...) : String | RequestForgery.java:27:57:27:59 | uri | Potential server side request forgery due to $@. | RequestForgery.java:19:31:19:57 | getParameter(...) | a user-provided value |
+| RequestForgery.java:62:59:62:77 | new URI(...) | RequestForgery.java:61:33:61:63 | getParameter(...) : String | RequestForgery.java:62:59:62:77 | new URI(...) | Potential server side request forgery due to $@. | RequestForgery.java:61:33:61:63 | getParameter(...) | a user-provided value |
+| RequestForgery.java:66:59:66:77 | new URI(...) | RequestForgery.java:65:49:65:79 | getParameter(...) : String | RequestForgery.java:66:59:66:77 | new URI(...) | Potential server side request forgery due to $@. | RequestForgery.java:65:49:65:79 | getParameter(...) | a user-provided value |
+| RequestForgery.java:71:59:71:88 | new URI(...) | RequestForgery.java:70:31:70:61 | getParameter(...) : String | RequestForgery.java:71:59:71:88 | new URI(...) | Potential server side request forgery due to $@. | RequestForgery.java:70:31:70:61 | getParameter(...) | a user-provided value |
+| RequestForgery.java:75:59:75:77 | new URI(...) | RequestForgery.java:74:73:74:103 | getParameter(...) : String | RequestForgery.java:75:59:75:77 | new URI(...) | Potential server side request forgery due to $@. | RequestForgery.java:74:73:74:103 | getParameter(...) | a user-provided value |
+| RequestForgery.java:79:59:79:77 | new URI(...) | RequestForgery.java:78:56:78:86 | getParameter(...) : String | RequestForgery.java:79:59:79:77 | new URI(...) | Potential server side request forgery due to $@. | RequestForgery.java:78:56:78:86 | getParameter(...) | a user-provided value |
+| RequestForgery.java:83:59:83:77 | new URI(...) | RequestForgery.java:82:55:82:85 | getParameter(...) : String | RequestForgery.java:83:59:83:77 | new URI(...) | Potential server side request forgery due to $@. | RequestForgery.java:82:55:82:85 | getParameter(...) | a user-provided value |
 | SpringSSRF.java:32:47:32:67 | ... + ... | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:32:47:32:67 | ... + ... | Potential server side request forgery due to $@. | SpringSSRF.java:26:33:26:60 | getParameter(...) | a user-provided value |
 | SpringSSRF.java:37:43:37:56 | fooResourceUrl | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:37:43:37:56 | fooResourceUrl | Potential server side request forgery due to $@. | SpringSSRF.java:26:33:26:60 | getParameter(...) | a user-provided value |
 | SpringSSRF.java:41:42:41:55 | fooResourceUrl | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:41:42:41:55 | fooResourceUrl | Potential server side request forgery due to $@. | SpringSSRF.java:26:33:26:60 | getParameter(...) | a user-provided value |

java/ql/test/query-tests/security/CWE-918/RequestForgery.java

@@ -27,6 +27,62 @@ public class RequestForgery extends HttpServlet {
                 HttpRequest r2 = HttpRequest.newBuilder(uri).build();
                 client.send(r2, null);
             }
+
+            // GOOD: sanitisation by concatenation with a prefix that prevents targeting an arbitrary host.
+            // We test a few different ways of sanitisation: via string conctentation (perhaps nested),
+            // via a stringbuilder and via String.format.
+            String safeUri3 = "https://example.com/" + request.getParameter("uri3");
+            HttpRequest r3 = HttpRequest.newBuilder(new URI(safeUri3)).build();
+            client.send(r3, null);
+
+            String safeUri4 = "https://example.com/" + ("someprefix" + request.getParameter("uri4"));
+            HttpRequest r4 = HttpRequest.newBuilder(new URI(safeUri4)).build();
+            client.send(r4, null);
+
+            StringBuilder safeUri5 = new StringBuilder();
+            safeUri5.append("https://example.com/").append(request.getParameter("uri5"));
+            HttpRequest r5 = HttpRequest.newBuilder(new URI(safeUri5.toString())).build();
+            client.send(r5, null);
+
+            String safeUri6 = String.format("https://example.com/%s", request.getParameter("uri6"));
+            HttpRequest r6 = HttpRequest.newBuilder(new URI(safeUri6)).build();
+            client.send(r6, null);
+
+            String safeUri7 = String.format("%s/%s", "https://example.com", request.getParameter("uri7"));
+            HttpRequest r7 = HttpRequest.newBuilder(new URI(safeUri7)).build();
+            client.send(r7, null);
+
+            String safeUri8 = String.format("%s%s", "https://example.com/", request.getParameter("uri8"));
+            HttpRequest r8 = HttpRequest.newBuilder(new URI(safeUri8)).build();
+            client.send(r8, null);
+
+            // BAD: cases where a string that would sanitise is used, but occurs in the wrong
+            // place to sanitise user input:
+            String unsafeUri3 = request.getParameter("baduri3") + "https://example.com/";
+            HttpRequest unsafer3 = HttpRequest.newBuilder(new URI(unsafeUri3)).build();
+            client.send(unsafer3, null);
+
+            String unsafeUri4 = ("someprefix" + request.getParameter("baduri4")) + "https://example.com/";
+            HttpRequest unsafer4 = HttpRequest.newBuilder(new URI(unsafeUri4)).build();
+            client.send(unsafer4, null);
+
+            StringBuilder unsafeUri5 = new StringBuilder();
+            unsafeUri5.append(request.getParameter("baduri5")).append("https://example.com/");
+            HttpRequest unsafer5 = HttpRequest.newBuilder(new URI(unsafeUri5.toString())).build();
+            client.send(unsafer5, null);
+
+            String unsafeUri6 = String.format("%shttps://example.com/", request.getParameter("baduri6"));
+            HttpRequest unsafer6 = HttpRequest.newBuilder(new URI(unsafeUri6)).build();
+            client.send(unsafer6, null);
+
+            String unsafeUri7 = String.format("%s/%s", request.getParameter("baduri7"), "https://example.com");
+            HttpRequest unsafer7 = HttpRequest.newBuilder(new URI(unsafeUri7)).build();
+            client.send(unsafer7, null);
+
+            String unsafeUri8 = String.format("%s%s", request.getParameter("baduri8"), "https://example.com/");
+            HttpRequest unsafer8 = HttpRequest.newBuilder(new URI(unsafeUri8)).build();
+            client.send(unsafer8, null);
+
         } catch (Exception e) {
             // TODO: handle exception
         }