Java: Improve modelling of Spring requests, flow steps and XSS sinks #3653
Conversation
This is not entirely accurate. We do already have default taint steps for flow in and out of collections and maps, see https://github.com/github/codeql/blob/master/java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll for details about what we currently include. |
|
Differences job started: https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/775/ |
|
Could you rebase onto latest master, please? That would make it a bit easier to get a working Difference job running. |
- Recognise @<httpverb>Mapping as well as @RequestMapping. - Identify tainted/not tainted parameters of RequestMapping methods.
- Identify ModelMaps correctly - Add extra not tainted param types (Pageable) - Identify ModelAttributes
- Only track if the body is a String type, as that is the only type at risk of XSS.
Look for Spring request methods which return a String value which may be coerced into a text/html output.
Methods annotated with a produces field which indicates a safe content-type should not be considered XSS sinks. For example: @RequestMapping(..., produces = "application/json")
Model the datatypes that may be populated on demand from request parameters.
- Also improve unwrapping of lists/arrays/maps etc.
60c536d to
2978af3
Compare
|
Annoyingly, rebases don't provide notifications. In any case: https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/797/ |
|
The Differences job failed for some reason. Retrying: https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/815/ |
| or | ||
| m.getDeclaringType().getSourceDeclaration().getASourceSupertype*().hasQualifiedName("java.util", "List") and | ||
| ( | ||
| m.getName().regexpMatch("get|toArray|subList|spliterator|set|iterator|listIterator") or |
There was a problem hiding this comment.
@aschackmull Wouldn't it be better to replace these regexMatch calls with something like :
m.hasName([ "get", "toArray", ...])
|
I've made a bunch of suggested changes here: lcartey#1 |
|
The differences job shows a 3% time increase for jdk, so that looks acceptable. |
|
One of the individual commit comments (7d555a7) suggests that some of the added steps are specific to XSS. Is this the case? If so, then we should move them to the XSS query. |
Java: Review changes for github#3653
|
3 tests are failing due to parameters marked with |
I don't think they are specific to XSS, but they are most likely to be seen with XSS. I think they are fine as general taint steps. |
Yes, this is intentional. |
As part of some recent customer engagements I spent some time improving our modelling of Spring sources, flow steps and XSS sink.
The identification of trusted/untrusted data and XSS sinks is primarily taken from the Spring reference documentation for request mapping handler methods, which has a detailed breakdown of how parameters and return values are treated based on the type/annotations.
Review highlights:
Maps andLists. This is required because the tainted parameters to a Spring request method may be either of these types, and we need to track flow out of them. One concern here is whether this can result in spurious flow through these types, although I believe this will not happen in practice because we generally don't track flow into them (i.e. we don't haveList.addetc. as a taint step).String.replace, as I had a customer provided benchmark which required flow through an inadequatereplacecall. If we retain this step, we may need to add some sanitizers to the XSS (and other) queries for "safe" looking replaces.@RequestBody:Userclass will be populated from JSON provided in the request body, with fields populated recursively. To capture this aspect I have introduced the concept ofSpringUntrustedDataType, and added taint flow steps through getter methods on these classes. I also wrote a helper (stripType) to unwrap generics (so that we can identify type parameters on lists etc. as also untrusted).Work that still needs to be done:
This addresses points 1-3 from: https://github.com/github/codeql-java-team/issues/9