Add SSRF sinks for uriVariables arguments of more methods on Spring RestTemplate

Author: owen-mc

java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll

@@ -31,50 +31,74 @@ class SpringWebClient extends Interface {
   }
 }
 
-/** The method `getForObject` on `org.springframework.web.reactive.function.client.RestTemplate`. */
-class SpringRestTemplateGetForObjectMethod extends Method {
-  SpringRestTemplateGetForObjectMethod() {
+/**
+ * A method on `org.springframework.web.reactive.function.client.RestTemplate`
+ * which has a parameter `uriVariables` (which can have type `Object..` or
+ * `Map<String, ?>`) which contains variables to be expanded into the URL
+ * template in parameter 0.
+ */
+private class SpringRestTemplateMethodWithUriVariablesParameter extends Method {
+  int pos;
+
+  SpringRestTemplateMethodWithUriVariablesParameter() {
     this.getDeclaringType() instanceof SpringRestTemplate and
-    this.hasName("getForObject")
+    (
+      this.hasName("delete") and pos = 1
+      or
+      this.hasName("exchange") and pos = 4
+      or
+      this.hasName("execute") and pos = 4
+      or
+      this.hasName("getForEntity") and pos = 2
+      or
+      this.hasName("getForObject") and pos = 2
+      or
+      this.hasName("headForHeaders") and pos = 1
+      or
+      this.hasName("optionsForAllow") and pos = 1
+      or
+      this.hasName("patchForObject") and pos = 3
+      or
+      this.hasName("postForEntity") and pos = 3
+      or
+      this.hasName("postForLocation") and pos = 2
+      or
+      this.hasName("postForObject") and pos = 3
+      or
+      this.hasName("put") and pos = 2
+    )
   }
-}
 
-/** A call to the method `getForObject` on `org.springframework.web.reactive.function.client.RestTemplate`. */
-class SpringRestTemplateGetForObjectMethodCall extends MethodCall {
-  SpringRestTemplateGetForObjectMethodCall() {
-    this.getMethod() instanceof SpringRestTemplateGetForObjectMethod
-  }
+  int getUriVariablesPosition() { result = pos }
+}
 
-  /** Gets the first argument, if it is a compile time constant. */
-  CompileTimeConstantExpr getConstantUrl() { result = this.getArgument(0) }
+/** Gets the first argument, if it is a compile time constant. */
+pragma[inline]
+private CompileTimeConstantExpr getConstantUrl(MethodCall mc) { result = mc.getArgument(0) }
 
-  /**
-   * Holds if the first argument is a compile time constant and it has a
-   * placeholder at offset `offset`, and there are `idx` placeholders that
-   * appear before it.
-   */
-  predicate urlHasPlaceholderAtOffset(int idx, int offset) {
-    exists(
-      this.getConstantUrl()
-          .getStringValue()
-          .replaceAll("\\{", "  ")
-          .replaceAll("\\}", "  ")
-          .regexpFind("\\{[^}]*\\}", idx, offset)
-    )
-  }
+pragma[inline]
+private predicate urlHasPlaceholderAtOffset(MethodCall mc, int idx, int offset) {
+  exists(
+    getConstantUrl(mc)
+        .getStringValue()
+        .replaceAll("\\{", "  ")
+        .replaceAll("\\}", "  ")
+        .regexpFind("\\{[^}]*\\}", idx, offset)
+  )
 }
 
 private class SpringWebClientRestTemplateGetForObject extends RequestForgerySink {
   SpringWebClientRestTemplateGetForObject() {
-    exists(SpringRestTemplateGetForObjectMethodCall mc, int i |
+    exists(SpringRestTemplateMethodWithUriVariablesParameter m, MethodCall mc, int i |
       // Note that the first argument is modeled as a request forgery sink
       // separately. This model is for arguments beyond the first two. There
       // are two relevant overloads, one with third parameter type `Object...`
       // and one with third parameter type `Map<String, ?>`. For the latter we
       // cannot deal with MapValue content easily but there is a default
       // implicit taint read at sinks that will catch it.
+      mc.getMethod() = m and
       i >= 0 and
-      this.asExpr() = mc.getArgument(i + 2)
+      this.asExpr() = mc.getArgument(m.getUriVariablesPosition() + i)
     |
       // If we can determine that part of mc.getArgument(0) is a hostname
       // sanitizing prefix, then we count how many placeholders occur before it
@@ -83,8 +107,8 @@ private class SpringWebClientRestTemplateGetForObject extends RequestForgerySink
       // considering the map values as sinks if there is at least one
       // placeholder in the URL before the hostname sanitizing prefix.
       exists(int offset |
-        mc.urlHasPlaceholderAtOffset(i, offset) and
-        offset < mc.getConstantUrl().(HostnameSanitizingPrefix).getOffset()
+        urlHasPlaceholderAtOffset(mc, i, offset) and
+        offset < getConstantUrl(mc).(HostnameSanitizingPrefix).getOffset()
       )
       or
       // If we cannot determine that part of mc.getArgument(0) is a hostname
@@ -94,12 +118,12 @@ private class SpringWebClientRestTemplateGetForObject extends RequestForgerySink
       // For the `Map<String, ?>` overload this has the effect of only
       // considering the map values as sinks if there is at least one
       // placeholder in the URL.
-      not mc.getConstantUrl() instanceof HostnameSanitizingPrefix and
-      mc.urlHasPlaceholderAtOffset(i, _)
+      not getConstantUrl(mc) instanceof HostnameSanitizingPrefix and
+      urlHasPlaceholderAtOffset(mc, i, _)
       or
       // If we cannot determine the string value of mc.getArgument(0), then we
       // conservatively consider all arguments as sinks.
-      not exists(mc.getConstantUrl().getStringValue())
+      not exists(getConstantUrl(mc).getStringValue())
     )
   }
 }