diff --git a/java/ql/lib/change-notes/2024-11-28-model-resttemplate-getforobject-third-parameter.md b/java/ql/lib/change-notes/2024-11-28-model-resttemplate-getforobject-third-parameter.md new file mode 100644 index 000000000000..4f45d19e5e8c --- /dev/null +++ b/java/ql/lib/change-notes/2024-11-28-model-resttemplate-getforobject-third-parameter.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Added a sink for "Server-side request forgery" (`java/ssrf`) for the third parameter to org.springframework.web.client.RestTemplate.getForObject, when we cannot statically determine that it does not affect the host in the URL. diff --git a/java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll b/java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll index 3a8d4bb084a4..e84108394704 100644 --- a/java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll +++ b/java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll @@ -4,6 +4,7 @@ import java import SpringHttp +private import semmle.code.java.security.RequestForgery /** The class `org.springframework.web.client.RestTemplate`. */ class SpringRestTemplate extends Class { @@ -27,3 +28,76 @@ class SpringWebClient extends Interface { this.hasQualifiedName("org.springframework.web.reactive.function.client", "WebClient") } } + +/** The method `getForObject` on `org.springframework.web.reactive.function.client.RestTemplate`. */ +class SpringRestTemplateGetForObjectMethod extends Method { + SpringRestTemplateGetForObjectMethod() { + this.getDeclaringType() instanceof SpringRestTemplate and + this.hasName("getForObject") + } +} + +/** 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, 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) + ) + } +} + +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`. For the latter we + // cannot deal with MapValue content easily but there is a default + // implicit taint read at sinks that will catch it. + i >= 0 and + this.asExpr() = mc.getArgument(i + 2) + | + // If we can determine that part of mc.getArgument(0) is a hostname + // sanitizing prefix, then we count how many placeholders occur before it + // and only consider that many arguments beyond the first two as sinks. + // For the `Map` overload this has the effect of only + // 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() + ) + or + // If we cannot determine that part of mc.getArgument(0) is a hostname + // sanitizing prefix, but it is a compile time constant and we can get + // its string value, then we count how many placeholders occur in it + // and only consider that many arguments beyond the first two as sinks. + // For the `Map` 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, _) + 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()) + ) + } +} diff --git a/java/ql/lib/semmle/code/java/security/RequestForgery.qll b/java/ql/lib/semmle/code/java/security/RequestForgery.qll index a4e824c1cfeb..1f3ce61406f7 100644 --- a/java/ql/lib/semmle/code/java/security/RequestForgery.qll +++ b/java/ql/lib/semmle/code/java/security/RequestForgery.qll @@ -63,14 +63,17 @@ abstract class RequestForgerySanitizer extends DataFlow::Node { } private class PrimitiveSanitizer extends RequestForgerySanitizer instanceof SimpleTypeSanitizer { } -private class HostnameSanitizingPrefix extends InterestingPrefix { +/** + * A string constant that contains a prefix which looks like when it is prepended to untrusted + * input, it will restrict the host or entity addressed. + * + * For example, anything containing `?` or `#`, or a slash that doesn't appear to be a protocol + * specifier (e.g. `http://` is not sanitizing), or specifically the string "/". + */ +class HostnameSanitizingPrefix extends InterestingPrefix { int offset; HostnameSanitizingPrefix() { - // Matches strings that look like when prepended to untrusted input, they will restrict - // the host or entity addressed: for example, anything containing `?` or `#`, or a slash that - // doesn't appear to be a protocol specifier (e.g. `http://` is not sanitizing), or specifically - // the string "/". exists(this.getStringValue().regexpFind("([?#]|[^?#:/\\\\][/\\\\])|^/$", 0, offset)) } @@ -81,8 +84,10 @@ private class HostnameSanitizingPrefix extends InterestingPrefix { * A value that is the result of prepending a string that prevents any value from controlling the * host of a URL. */ -private class HostnameSantizer extends RequestForgerySanitizer { - HostnameSantizer() { this.asExpr() = any(HostnameSanitizingPrefix hsp).getAnAppendedExpression() } +private class HostnameSanitizer extends RequestForgerySanitizer { + HostnameSanitizer() { + this.asExpr() = any(HostnameSanitizingPrefix hsp).getAnAppendedExpression() + } } /** diff --git a/java/ql/test/query-tests/security/CWE-918/SpringSSRF.java b/java/ql/test/query-tests/security/CWE-918/SpringSSRF.java index 6af4829ba024..895c68eda69a 100644 --- a/java/ql/test/query-tests/security/CWE-918/SpringSSRF.java +++ b/java/ql/test/query-tests/security/CWE-918/SpringSSRF.java @@ -13,6 +13,7 @@ import java.net.http.HttpRequest; import java.net.Proxy.Type; import java.io.InputStream; +import java.util.Map; import org.apache.http.client.methods.HttpGet; import javax.servlet.ServletException; @@ -32,6 +33,14 @@ protected void doGet(HttpServletRequest request2, HttpServletResponse response2) restTemplate.exchange(fooResourceUrl, HttpMethod.POST, request, String.class); // $ SSRF restTemplate.execute(fooResourceUrl, HttpMethod.POST, null, null, "test"); // $ SSRF restTemplate.getForObject(fooResourceUrl, String.class, "test"); // $ SSRF + restTemplate.getForObject("http://{foo}", String.class, fooResourceUrl); // $ SSRF + restTemplate.getForObject("http://{foo}/a/b", String.class, fooResourceUrl); // $ SSRF + restTemplate.getForObject("http://safe.com/{foo}", String.class, fooResourceUrl); // not bad - the tainted value does not affect the host + restTemplate.getForObject("http://{foo}", String.class, "safe.com", fooResourceUrl); // not bad - the tainted value is unused + restTemplate.getForObject("http://{foo}", String.class, Map.of("foo", fooResourceUrl)); // $ SSRF + restTemplate.getForObject("http://safe.com/{foo}", String.class, Map.of("foo", fooResourceUrl)); // not bad - the tainted value does not affect the host + restTemplate.getForObject("http://{foo}", String.class, Map.of("foo", "safe.com", "unused", fooResourceUrl)); // $ SPURIOUS: SSRF // not bad - the key for the tainted value is unused + restTemplate.getForObject("http://{foo}", String.class, Map.of("foo", "safe.com", fooResourceUrl, "unused")); // not bad - the tainted value is in a map key restTemplate.patchForObject(fooResourceUrl, new String("object"), String.class, "hi"); // $ SSRF restTemplate.postForEntity(new URI(fooResourceUrl), new String("object"), String.class); // $ SSRF restTemplate.postForLocation(fooResourceUrl, new String("object")); // $ SSRF