From 65fb895ed5a4b8702a641051bc864f9300e05659 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 28 Nov 2024 15:51:01 +0000 Subject: [PATCH 01/10] (Unrelated) Fix typo in class name --- java/ql/lib/semmle/code/java/security/RequestForgery.qll | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/RequestForgery.qll b/java/ql/lib/semmle/code/java/security/RequestForgery.qll index a4e824c1cfeb..a59bacb1fe76 100644 --- a/java/ql/lib/semmle/code/java/security/RequestForgery.qll +++ b/java/ql/lib/semmle/code/java/security/RequestForgery.qll @@ -81,8 +81,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() + } } /** From b5fbf2e9441dfcd5b424eb1fb9f4f52c420d9c9b Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 28 Nov 2024 13:03:09 +0000 Subject: [PATCH 02/10] Add models for third arg of getForObject No attempt to stop FPs. --- java/ql/lib/ext/org.springframework.web.client.model.yml | 3 +++ .../ql/test/query-tests/security/CWE-918/SpringSSRF.java | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/java/ql/lib/ext/org.springframework.web.client.model.yml b/java/ql/lib/ext/org.springframework.web.client.model.yml index 79a7f577c3de..90abe1df71db 100644 --- a/java/ql/lib/ext/org.springframework.web.client.model.yml +++ b/java/ql/lib/ext/org.springframework.web.client.model.yml @@ -16,6 +16,9 @@ extensions: - ["org.springframework.web.client", "RestTemplate", False, "execute", "", "", "Argument[0]", "request-forgery", "manual"] - ["org.springframework.web.client", "RestTemplate", False, "getForEntity", "", "", "Argument[0]", "request-forgery", "manual"] - ["org.springframework.web.client", "RestTemplate", False, "getForObject", "", "", "Argument[0]", "request-forgery", "manual"] + - ["org.springframework.web.client", "RestTemplate", False, "getForObject", "", "", "Argument[2]", "request-forgery", "manual"] # This is a workaround for the fact that sink model can't currently have access paths + # - ["org.springframework.web.client", "RestTemplate", False, "getForObject", "", "", "Argument[2].ArrayElement", "request-forgery", "manual"] + # - ["org.springframework.web.client", "RestTemplate", False, "getForObject", "", "", "Argument[2].MapValue", "request-forgery", "manual"] - ["org.springframework.web.client", "RestTemplate", False, "headForHeaders", "", "", "Argument[0]", "request-forgery", "manual"] - ["org.springframework.web.client", "RestTemplate", False, "optionsForAllow", "", "", "Argument[0]", "request-forgery", "manual"] - ["org.springframework.web.client", "RestTemplate", False, "patchForObject", "", "", "Argument[0]", "request-forgery", "manual"] 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..917d8b29ac08 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); // $ SPURIOUS: SSRF // not bad - the tainted value does not affect the host + restTemplate.getForObject("http://{foo}", String.class, "safe.com", fooResourceUrl); // $ SPURIOUS: SSRF // 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)); // $ SPURIOUS: SSRF // 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 From ba3f9d61346689ba61e69ffd17090e49f2a20fd0 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 28 Nov 2024 13:44:55 +0000 Subject: [PATCH 03/10] Convert model to QL --- .../org.springframework.web.client.model.yml | 3 --- .../java/frameworks/spring/SpringWebClient.qll | 18 ++++++++++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/java/ql/lib/ext/org.springframework.web.client.model.yml b/java/ql/lib/ext/org.springframework.web.client.model.yml index 90abe1df71db..79a7f577c3de 100644 --- a/java/ql/lib/ext/org.springframework.web.client.model.yml +++ b/java/ql/lib/ext/org.springframework.web.client.model.yml @@ -16,9 +16,6 @@ extensions: - ["org.springframework.web.client", "RestTemplate", False, "execute", "", "", "Argument[0]", "request-forgery", "manual"] - ["org.springframework.web.client", "RestTemplate", False, "getForEntity", "", "", "Argument[0]", "request-forgery", "manual"] - ["org.springframework.web.client", "RestTemplate", False, "getForObject", "", "", "Argument[0]", "request-forgery", "manual"] - - ["org.springframework.web.client", "RestTemplate", False, "getForObject", "", "", "Argument[2]", "request-forgery", "manual"] # This is a workaround for the fact that sink model can't currently have access paths - # - ["org.springframework.web.client", "RestTemplate", False, "getForObject", "", "", "Argument[2].ArrayElement", "request-forgery", "manual"] - # - ["org.springframework.web.client", "RestTemplate", False, "getForObject", "", "", "Argument[2].MapValue", "request-forgery", "manual"] - ["org.springframework.web.client", "RestTemplate", False, "headForHeaders", "", "", "Argument[0]", "request-forgery", "manual"] - ["org.springframework.web.client", "RestTemplate", False, "optionsForAllow", "", "", "Argument[0]", "request-forgery", "manual"] - ["org.springframework.web.client", "RestTemplate", False, "patchForObject", "", "", "Argument[0]", "request-forgery", "manual"] 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..d245f5ed244d 100644 --- a/java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll +++ b/java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll @@ -27,3 +27,21 @@ class SpringWebClient extends Interface { this.hasQualifiedName("org.springframework.web.reactive.function.client", "WebClient") } } + +private import semmle.code.java.security.RequestForgery + +private class SpringWebClientRestTemplateGetForObject extends RequestForgerySink { + SpringWebClientRestTemplateGetForObject() { + exists(Method m, MethodCall mc, int i | + m.getDeclaringType() instanceof SpringRestTemplate and + m.hasName("getForObject") and + mc.getMethod() = m + | + // Deal with two overloads, with third parameter type `Object...` and + // `Map`. We cannot deal with mapvalue content easily but + // there is a default implicit taint read at sinks that will catch it. + this.asExpr() = mc.getArgument(i) and + i >= 2 + ) + } +} From 617f4f140e9fb4b2c64c8c8b91257c4c138512fb Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 28 Nov 2024 15:51:37 +0000 Subject: [PATCH 04/10] Make HostnameSanitizingPrefix public --- java/ql/lib/semmle/code/java/security/RequestForgery.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/RequestForgery.qll b/java/ql/lib/semmle/code/java/security/RequestForgery.qll index a59bacb1fe76..c670d92b5ea5 100644 --- a/java/ql/lib/semmle/code/java/security/RequestForgery.qll +++ b/java/ql/lib/semmle/code/java/security/RequestForgery.qll @@ -63,7 +63,7 @@ abstract class RequestForgerySanitizer extends DataFlow::Node { } private class PrimitiveSanitizer extends RequestForgerySanitizer instanceof SimpleTypeSanitizer { } -private class HostnameSanitizingPrefix extends InterestingPrefix { +class HostnameSanitizingPrefix extends InterestingPrefix { int offset; HostnameSanitizingPrefix() { From 7648d397f8291e99e53f804b82d516ceb3cbb661 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 28 Nov 2024 16:50:26 +0000 Subject: [PATCH 05/10] Improve model to remove some false positives --- .../frameworks/spring/SpringWebClient.qll | 57 +++++++++++++++++-- .../security/CWE-918/SpringSSRF.java | 6 +- 2 files changed, 54 insertions(+), 9 deletions(-) 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 d245f5ed244d..79f0cb9c8bb7 100644 --- a/java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll +++ b/java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll @@ -35,13 +35,58 @@ private class SpringWebClientRestTemplateGetForObject extends RequestForgerySink exists(Method m, MethodCall mc, int i | m.getDeclaringType() instanceof SpringRestTemplate and m.hasName("getForObject") and - mc.getMethod() = m + mc.getMethod() = m and + // Note that mc.getArgument(0) is modeled 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. + this.asExpr() = mc.getArgument(i + 2) and + i >= 0 | - // Deal with two overloads, with third parameter type `Object...` and - // `Map`. We cannot deal with mapvalue content easily but - // there is a default implicit taint read at sinks that will catch it. - this.asExpr() = mc.getArgument(i) and - 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(HostnameSanitizingPrefix hsp | + hsp = mc.getArgument(0) and + i <= + max(int occurrenceIndex, int occurrenceOffset | + exists( + hsp.getStringValue().regexpFind("\\{[^}]*\\}", occurrenceIndex, occurrenceOffset) + ) and + occurrenceOffset < hsp.getOffset() + | + occurrenceIndex + ) + ) + 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.getArgument(0) instanceof HostnameSanitizingPrefix and + i <= + max(int occurrenceIndex | + exists( + mc.getArgument(0) + .(CompileTimeConstantExpr) + .getStringValue() + .regexpFind("\\{[^}]*\\}", occurrenceIndex, _) + ) + | + occurrenceIndex + ) + or + // If we cannot determine the string value of mc.getArgument(0), then we + // conservatively consider all arguments as sinks. + not exists(mc.getArgument(0).(CompileTimeConstantExpr).getStringValue()) ) } } 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 917d8b29ac08..895c68eda69a 100644 --- a/java/ql/test/query-tests/security/CWE-918/SpringSSRF.java +++ b/java/ql/test/query-tests/security/CWE-918/SpringSSRF.java @@ -35,10 +35,10 @@ protected void doGet(HttpServletRequest request2, HttpServletResponse response2) 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); // $ SPURIOUS: SSRF // not bad - the tainted value does not affect the host - restTemplate.getForObject("http://{foo}", String.class, "safe.com", fooResourceUrl); // $ SPURIOUS: SSRF // not bad - the tainted value is unused + 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)); // $ SPURIOUS: SSRF // not bad - the tainted value does not affect the host + 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 From 7f8a1ae941703e6e01717f5777eb6b072df852a7 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 28 Nov 2024 17:03:41 +0000 Subject: [PATCH 06/10] Add change note --- ...4-11-28-model-resttemplate-getforobject-third-parameter.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 java/ql/lib/change-notes/2024-11-28-model-resttemplate-getforobject-third-parameter.md 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. From 2c061b0d560dd8a0fd18e6cbabacf500428ab761 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Fri, 29 Nov 2024 09:46:08 +0000 Subject: [PATCH 07/10] Add QLDoc for HostnameSanitizingPrefix --- .../lib/semmle/code/java/security/RequestForgery.qll | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/RequestForgery.qll b/java/ql/lib/semmle/code/java/security/RequestForgery.qll index c670d92b5ea5..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 { } +/** + * 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)) } From b20b7c7572104495ec8fddf957beb085c2597554 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 5 Dec 2024 10:43:13 +0000 Subject: [PATCH 08/10] Remove escaped "{" and "}" before counting placeholders --- .../semmle/code/java/frameworks/spring/SpringWebClient.qll | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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 79f0cb9c8bb7..9e6ac2c86014 100644 --- a/java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll +++ b/java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll @@ -56,7 +56,10 @@ private class SpringWebClientRestTemplateGetForObject extends RequestForgerySink i <= max(int occurrenceIndex, int occurrenceOffset | exists( - hsp.getStringValue().regexpFind("\\{[^}]*\\}", occurrenceIndex, occurrenceOffset) + hsp.getStringValue() + .replaceAll("\\{", " ") + .replaceAll("\\}", " ") + .regexpFind("\\{[^}]*\\}", occurrenceIndex, occurrenceOffset) ) and occurrenceOffset < hsp.getOffset() | @@ -78,6 +81,8 @@ private class SpringWebClientRestTemplateGetForObject extends RequestForgerySink mc.getArgument(0) .(CompileTimeConstantExpr) .getStringValue() + .replaceAll("\\{", " ") + .replaceAll("\\}", " ") .regexpFind("\\{[^}]*\\}", occurrenceIndex, _) ) | From 347fd575a2c52e9d8ed7d227c7702c2541711ba3 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 5 Dec 2024 11:15:43 +0000 Subject: [PATCH 09/10] Refactor to avoid duplicated logic --- .../frameworks/spring/SpringWebClient.qll | 89 ++++++++++--------- 1 file changed, 48 insertions(+), 41 deletions(-) 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 9e6ac2c86014..a9c3cd3cdd88 100644 --- a/java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll +++ b/java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll @@ -30,20 +30,50 @@ class SpringWebClient extends Interface { private import semmle.code.java.security.RequestForgery +/** 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(Method m, MethodCall mc, int i | - m.getDeclaringType() instanceof SpringRestTemplate and - m.hasName("getForObject") and - mc.getMethod() = m and - // Note that mc.getArgument(0) is modeled 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. - this.asExpr() = mc.getArgument(i + 2) and - i >= 0 + 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 @@ -51,20 +81,9 @@ private class SpringWebClientRestTemplateGetForObject extends RequestForgerySink // 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(HostnameSanitizingPrefix hsp | - hsp = mc.getArgument(0) and - i <= - max(int occurrenceIndex, int occurrenceOffset | - exists( - hsp.getStringValue() - .replaceAll("\\{", " ") - .replaceAll("\\}", " ") - .regexpFind("\\{[^}]*\\}", occurrenceIndex, occurrenceOffset) - ) and - occurrenceOffset < hsp.getOffset() - | - occurrenceIndex - ) + 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 @@ -74,24 +93,12 @@ private class SpringWebClientRestTemplateGetForObject extends RequestForgerySink // 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.getArgument(0) instanceof HostnameSanitizingPrefix and - i <= - max(int occurrenceIndex | - exists( - mc.getArgument(0) - .(CompileTimeConstantExpr) - .getStringValue() - .replaceAll("\\{", " ") - .replaceAll("\\}", " ") - .regexpFind("\\{[^}]*\\}", occurrenceIndex, _) - ) - | - occurrenceIndex - ) + 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.getArgument(0).(CompileTimeConstantExpr).getStringValue()) + not exists(mc.getConstantUrl().getStringValue()) ) } } From 1420bce36aafcf114b4636997a1a4cfd4787e474 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com> Date: Wed, 11 Dec 2024 14:19:24 +0000 Subject: [PATCH 10/10] Move import statement in SpringWebClient.qll --- .../lib/semmle/code/java/frameworks/spring/SpringWebClient.qll | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 a9c3cd3cdd88..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 { @@ -28,8 +29,6 @@ class SpringWebClient extends Interface { } } -private import semmle.code.java.security.RequestForgery - /** The method `getForObject` on `org.springframework.web.reactive.function.client.RestTemplate`. */ class SpringRestTemplateGetForObjectMethod extends Method { SpringRestTemplateGetForObjectMethod() {