Java: Add flow steps for Guava collection utilities #4963
Conversation
| // ImmutableMultiset.Builder<E> addCopies(E element, int occurrences) | ||
| // ImmutableMultiset.Builder<E> setCount(E element, int count) | ||
| this.hasName(["add", "addAll", "addCopies", "setCount"]) and | ||
| argument = 0 |
There was a problem hiding this comment.
This will only match the first element for the varargs method add(E...), won't it?
There was a problem hiding this comment.
The taint tracking library takes varargs into account and will match each element when a varargs parameter is specified by TaintPreservingCallable.
|
|
||
| ImmutableContainerType() { | ||
| this.getSourceDeclaration().getASourceSupertype*().hasQualifiedName(guavaCollectPackage(), kind) and | ||
| kind = ["ImmutableCollection", "ImmutableMap", "ImmutableMultimap", "ImmutableTable"] |
There was a problem hiding this comment.
There is also ImmutableClassToInstanceMap which interestingly does not extend ImmutableMap.
| // static <E> CopyOnWriteArraySet<E> newCopyOnWriteArraySet(Iterable<? extends E> elements) | ||
| // static <E extends Enum<E>>EnumSet<E> newEnumSet(Iterable<E> iterable, Class<E> elementType) | ||
| // etc | ||
| this.getName().matches("new%Set") and |
There was a problem hiding this comment.
This will erroneously taint the argument of newTreeSet(Comparator)
There was a problem hiding this comment.
@joefarebrother, do you think this will be an issue?
There was a problem hiding this comment.
Probably not, I don't imagine it being common for a Comparator to carry user-tainted data.
| private class SetsMethod extends TaintPreservingCallable { | ||
| int arg; | ||
|
|
||
| SetsMethod() { |
There was a problem hiding this comment.
Should this also match the immutableEnumSet(...) methods? (given that it matches the newEnumSet methods)
There was a problem hiding this comment.
Flow through enum sets is not relevant for security analysis, since enum sets are internally bitvectors so it's not really possibe for them to be tainted by user input.
joefarebrother
changed the title
joefarebrother
changed the title
|
QL looks good to me, but I don't think we should be adding |
|
We should probably squash the commits that add and remove those |
joefarebrother
force-pushed
the
guava-collections
branch
from
a35ba8a to
d69ecde
Compare
3 participants
Models the methods of several classes in the collections package (
com.google.common.collect) of the Guava framework, includingTable,Multimap, the variousImmutableSomethingclasses and their builders, and the static utility methods inSets.Part of https://github.com/github/codeql-java-team/issues/39
https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/1121/https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/1128/