Skip to content

Commit

Permalink
Refactor to avoid duplicated logic
Browse files Browse the repository at this point in the history
  • Loading branch information
owen-mc committed Dec 5, 2024
1 parent b20b7c7 commit 347fd57
Showing 1 changed file with 48 additions and 41 deletions.
89 changes: 48 additions & 41 deletions java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll
Original file line number Diff line number Diff line change
Expand Up @@ -30,41 +30,60 @@ 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<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.
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<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.
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<String, ?>` 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
Expand All @@ -74,24 +93,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.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())
)
}
}

0 comments on commit 347fd57

Please sign in to comment.