Merge pull request #20930 from owen-mc/java/spring-rest-template-request-forgery-sinks

Java: add more Spring RestTemplate request forgery sinks

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

@@ -31,50 +31,55 @@ 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.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.getParameter(pos).getName() = "uriVariables"
   }
+
+  int getUriVariablesPosition() { result = pos }
 }
 
-/** A call to the method `getForObject` on `org.springframework.web.reactive.function.client.RestTemplate`. */
-class SpringRestTemplateGetForObjectMethodCall extends MethodCall {
-  SpringRestTemplateGetForObjectMethodCall() {
-    this.getMethod() instanceof SpringRestTemplateGetForObjectMethod
-  }
+/** Gets the first argument of `mc`, if it is a compile-time constant. */
+pragma[inline]
+private CompileTimeConstantExpr getConstantUrl(MethodCall mc) { result = mc.getArgument(0) }
 
-  /** Gets the first argument, if it is a compile time constant. */
-  CompileTimeConstantExpr getConstantUrl() { result = this.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)
-    )
-  }
+/**
+ * Holds if the first argument of `mc` is a compile-time constant URL template
+ * which has its `idx`-th placeholder at the offset `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 |
-      // 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.
+private class SpringWebClientRestTemplateUriVariable extends RequestForgerySink {
+  SpringWebClientRestTemplateUriVariable() {
+    exists(SpringRestTemplateMethodWithUriVariablesParameter m, MethodCall mc, int i |
+      // Note that the first argument of `m` is modeled as a request forgery
+      // sink separately. This model is for arguments corresponding to the
+      // `uriVariables` parameter. There are always two relevant overloads, one
+      // with parameter type `Object...` and one with 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 +88,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 +99,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())
     )
   }
 }