Java: Review changes for https://github.com/github/codeql/pull/3653 #1
Conversation
lcartey
left a comment
lcartey
left a comment
There was a problem hiding this comment.
There are some gaps between the removed container modeling, and the default container modeling, although I do not think they are critical for Spring support.
| or | ||
| m instanceof MapMethod and | ||
| ( | ||
| m.getName().regexpMatch("get|entrySet|keySet|values") |
There was a problem hiding this comment.
Just noting that keySet is not covered in ContainerFlow (unlike the others). However, I believe this is fine for Spring, because a Map type parameter to a @RequestMapping method will be unlikely to contain tainted keys, and ContainerFlow only taints Maps via the value not the key parameter.
(ref: https://docs.spring.io/spring/docs/current/spring-framework-reference/web.html#mvc-ann-arguments)
There was a problem hiding this comment.
keySet is excluded on purpose as we currently otherwise won't be able to distinguish keys and values in a tainted map. If at some point tainted keys become important to track, then we'll most likely attempt this through a more precise modelling via container-content-as-field-flow.
| ( | ||
| m.getName().regexpMatch("get|toArray|subList|spliterator|set|iterator|listIterator") or | ||
| (m.getName().regexpMatch("remove") and not m.getReturnType() instanceof BooleanType) | ||
| ) |
There was a problem hiding this comment.
spliterator, set and listIterator are not covered by ContainerFlow.
…aflow-tests Small fixups to your PR to my PR
Rewrite XQuery injection to use an additional taint step instead of multiple configurations
2 participants
No description provided.