Java: add SSRF sink model for the third parameter of RestTemplate.getForObject

[owen-mc]

I ran MRVA for the ssrf query restricted to only this new sink (before I had added the commit to reduce FPs) and got no alerts. So I don't think this is a commonly used sink.

Note to reviewers: currently I have had to make HostSanitizingPrefix public to make this work, and I have introduced a bidirectional import between Spring and RequestForgery. I am very open to suggestions for how to do this better.

[smowton]

I'm not immediately familiar, but is there an escaping for literal { in these strings that ought to be checked and disregarded?

[owen-mc]

The strings will be something like "http://{domain}.com/users/{userid}". Depending on which overload of getForObject you are calling the placeholders are then replaced with either (a) the arguments corresponding to the varargs parameter, in order (ignoring the string in the placeholder), or (b) the value in the provided map, where the key is the string in the placeholder.

In this particular part of the code we're trying to find how many placeholders there are before the first character which lets us know that the hostname is over("?", "#", unescaped "/"). I guess it would be possible that someone would put an escaped "{" in there. I'm not sure how it would be interpreted. I guess we could first replaceall "{" and "}" with some other string with the same number of characters, say " ". I'll do that.

[smowton]

Pull this out as something like

class GetForObjectTemplateStringConstant extends CompileTimeConstantExpr {
  GetForObjectTemplateStringConstant() { this = any(Method m | ... is a getForObject method ... ).getACall().getArgument(0) }

  predicate hasPlaceholderAtOffset(int idx, int offset) { ... }
}

[owen-mc]

@smowton I've addressed the review comments. What about the cyclic imports I've created? Is it okay because I've added a private import?

[egregius313]

What about the cyclic imports I've created? Is it okay because I've added a private import?

@owen-mc that is a normal pattern, and in this case SpringWebClient is indirectly imported by RequestForgery. Cf this ql4ql query: https://github.com/github/codeql/blob/main/ql/ql/src/queries/performance/AbstractClassImport.ql

RequestForgery.qll <- Spring.qll <- SpringController.qll <- SpringWebClient.qll

Ideally RequestForgery would privately import the Spring library instead, but I don't know if that's too much of a breaking change since it changes the API surface of RequestForgery.qll

[egregius313]

LGTM