From 532a10bfdfe580a18eed2b84c12ef01b88e1bbf4 Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Thu, 15 Apr 2021 14:47:50 +0100
Subject: [PATCH] Java SSRF query: Provide hook for custom taint-propagating
 steps; make all default sinks/sanitizers/steps private.

---
 .../Security/CWE/CWE-918/RequestForgery.ql    |  2 +-
 .../Security/CWE/CWE-918/RequestForgery.qll   | 74 ++++++++++---------
 2 files changed, 42 insertions(+), 34 deletions(-)

diff --git a/java/ql/src/Security/CWE/CWE-918/RequestForgery.ql b/java/ql/src/Security/CWE/CWE-918/RequestForgery.ql
index cbae9afdb17..424edf3b36a 100644
--- a/java/ql/src/Security/CWE/CWE-918/RequestForgery.ql
+++ b/java/ql/src/Security/CWE/CWE-918/RequestForgery.ql
@@ -29,7 +29,7 @@ class RequestForgeryConfiguration extends TaintTracking::Configuration {
   override predicate isSink(DataFlow::Node sink) { sink instanceof RequestForgerySink }
 
   override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
-    requestForgeryStep(pred, succ)
+    any(RequestForgeryAdditionalTaintStep r).propagatesTaint(pred, succ)
   }
 
   override predicate isSanitizer(DataFlow::Node node) { node instanceof RequestForgerySanitizer }
diff --git a/java/ql/src/Security/CWE/CWE-918/RequestForgery.qll b/java/ql/src/Security/CWE/CWE-918/RequestForgery.qll
index 370ceb8fe1c..f9694f1ca25 100644
--- a/java/ql/src/Security/CWE/CWE-918/RequestForgery.qll
+++ b/java/ql/src/Security/CWE/CWE-918/RequestForgery.qll
@@ -8,39 +8,47 @@ import semmle.code.java.dataflow.DataFlow
 import semmle.code.java.dataflow.TaintTracking
 private import semmle.code.java.StringFormat
 
-/**
- * Holds if taint is propagated from `pred` to `succ`.
- */
-predicate requestForgeryStep(DataFlow::Node pred, DataFlow::Node succ) {
-  // propagate to a URI when its host is assigned to
-  exists(UriCreation c | c.getHostArg() = pred.asExpr() | succ.asExpr() = c)
-  or
-  // propagate to a URL when its host is assigned to
-  exists(UrlConstructorCall c | c.getHostArg() = pred.asExpr() | succ.asExpr() = c)
-  or
-  // propagate to a RequestEntity when its url is assigned to
-  exists(MethodAccess m |
-    m.getMethod().getDeclaringType() instanceof SpringRequestEntity and
-    (
-      m.getMethod().hasName(["get", "post", "head", "delete", "options", "patch", "put"]) and
-      m.getArgument(0) = pred.asExpr() and
-      m = succ.asExpr()
-      or
-      m.getMethod().hasName("method") and
-      m.getArgument(1) = pred.asExpr() and
+abstract class RequestForgeryAdditionalTaintStep extends string {
+  bindingset[this]
+  RequestForgeryAdditionalTaintStep() { any() }
+
+  abstract predicate propagatesTaint(DataFlow::Node pred, DataFlow::Node succ);
+}
+
+private class DefaultRequestForgeryAdditionalTaintStep extends RequestForgeryAdditionalTaintStep {
+  DefaultRequestForgeryAdditionalTaintStep() { this = "DefaultRequestForgeryAdditionalTaintStep" }
+
+  override predicate propagatesTaint(DataFlow::Node pred, DataFlow::Node succ) {
+    // propagate to a URI when its host is assigned to
+    exists(UriCreation c | c.getHostArg() = pred.asExpr() | succ.asExpr() = c)
+    or
+    // propagate to a URL when its host is assigned to
+    exists(UrlConstructorCall c | c.getHostArg() = pred.asExpr() | succ.asExpr() = c)
+    or
+    // propagate to a RequestEntity when its url is assigned to
+    exists(MethodAccess m |
+      m.getMethod().getDeclaringType() instanceof SpringRequestEntity and
+      (
+        m.getMethod().hasName(["get", "post", "head", "delete", "options", "patch", "put"]) and
+        m.getArgument(0) = pred.asExpr() and
+        m = succ.asExpr()
+        or
+        m.getMethod().hasName("method") and
+        m.getArgument(1) = pred.asExpr() and
+        m = succ.asExpr()
+      )
+    )
+    or
+    // propagate from a `RequestEntity<>$BodyBuilder` to a `RequestEntity`
+    // when the builder is tainted
+    exists(MethodAccess m, RefType t |
+      m.getMethod().getDeclaringType() = t and
+      t.hasQualifiedName("org.springframework.http", "RequestEntity<>$BodyBuilder") and
+      m.getMethod().hasName("body") and
+      m.getQualifier() = pred.asExpr() and
       m = succ.asExpr()
     )
-  )
-  or
-  // propagate from a `RequestEntity<>$BodyBuilder` to a `RequestEntity`
-  // when the builder is tainted
-  exists(MethodAccess m, RefType t |
-    m.getMethod().getDeclaringType() = t and
-    t.hasQualifiedName("org.springframework.http", "RequestEntity<>$BodyBuilder") and
-    m.getMethod().hasName("body") and
-    m.getQualifier() = pred.asExpr() and
-    m = succ.asExpr()
-  )
+  }
 }
 
 /** A data flow sink for request forgery vulnerabilities. */
@@ -257,7 +265,7 @@ private MethodAccess getAChainedAppend(Expr e) {
  * a hostname or a URL separator, preventing the appended string from arbitrarily controlling
  * the addressed server.
  */
-class HostnameSanitizedExpr extends Expr {
+private class HostnameSanitizedExpr extends Expr {
   HostnameSanitizedExpr() {
     // Sanitize expressions that come after a sanitizing prefix in a tree of string additions:
     this = getASanitizedAddOperand()
@@ -311,6 +319,6 @@ class HostnameSanitizedExpr extends Expr {
  * 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 {
+private class HostnameSantizer extends RequestForgerySanitizer {
   HostnameSantizer() { this.asExpr() instanceof HostnameSanitizedExpr }
 }