Java: Clean up ContainerFlow, consider more methods #3918

aibaars · 2020-07-07T17:54:02Z

aibaars · 2020-07-07T18:24:34Z

Thanks for the detailed list @Marcono1234 . This PR should address #3883 . It should include all of the suggestions you made except

reduce methods from ConcurrentHashMap. While it is possible that taint is preserved by the reducer, I think this is quite unlikely
methods that taint their arguments (copyInto, drainTo, toArray, etc). These should be put in taintPreservingQualifierToArgument .

If you spot any methods in this PR that I missed, other than the ones mentioned above, please let me know.

Marcono1234

Thanks! I missed some Deque methods in #3883 and incorrectly listed Set.spliterator(), but luckily you spotted this. I have adjusted 3883 accordingly.

Often you only restricted the method using (CollectionMethod) or (MapMethod). I assume that this the reason why you then left out some methods, which - while not overriding any other method - are already being covered by one of the more generic (CollectionMethod) or (MapMethod) checks.
However, in my opinion it would be good to at least add them commented out (and mention where they are matched) so it is clear that the method was not left out by accident. Otherwise if later one of the more generic checks is refined, it might not match that (currently not listed) method anymore and that false negative would not be that obvious. I have left review comments at (hopefully) all places where this applies.

Additionally, what do you think about checking for Object.clone() as well? Unless I overlooked it, it appears there is currently no taint tracking for it.

java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll

While most flow for a qualifierToArgumentStep goes through a variable use this is not always the case. Therefore it is best to remove the restriction to RValue to allow taint steps to use postupdate nodes. See also: ba86dea

aibaars · 2020-07-09T10:44:37Z

@Marcono1234 thanks for the detailed review. I addressed all your comments and also added a test case that should cover all the new flow that was implemented.

Some method variants are captured by a super class. Added some comments to indicate where this happens to make review of missing methods easier in the future.

aschackmull · 2020-07-09T12:24:08Z

java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll

+  // java.util.Map
+  m
+      .(MapMethod)
+      .hasName(["computeIfAbsent", "entrySet", "get", "getOrDefault", "merge", "put", "putIfAbsent",


I'm not sure we should include merge, as this is dependent on the merge function preserving taint.

java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll

Stack.push(E) returns its argument, it does not propagate taint from the stack to the return value.

aschackmull · 2020-07-09T14:47:51Z

This is btw. definitely a PR for which we'll want a Java-Differences job before merging, as there might be a performance impact.

aibaars · 2020-07-09T14:59:52Z

https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/830/

aschackmull

If we remove merge from taintPreservingQualifierToMethod then I think this is ready to merge.

Marcono1234

Thanks, looks really good!
I noticed a few more minor things which could possibly be improved.

Marcono1234 · 2020-07-10T21:42:16Z

java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll

+  m.(CollectionMethod).hasName(["peek", "pop"])
  or
  // java.util.Queue
-  m.(CollectionMethod).hasName(["element", /*"peek", "remove"*/ "poll"])


Did you remove the comment for peek by accident?
It would probably good to add a comment as well:

// covered by Stack: peek()

Marcono1234 · 2020-07-10T21:45:39Z

java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll

+  // covered by Deque: offerFirst(E, long, TimeUnit), offerLast(E, long, TimeUnit)
  method.(CollectionMethod).hasName(["putFirst", "putLast"]) and arg = 0
+  or
+  //java.util.Dictionary


Would probably be good to add a space between // and java... to be consistent:

// java.util.Dictionary

Marcono1234 · 2020-07-10T21:47:39Z

java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll

 */
 private predicate taintPreservingArgumentToMethod(Method method, int arg) {
+  // java.util.Stack
+  method.(CollectionMethod).hasName("push") and arg = 0


@aschackmull, isn't that even data flow then and not only tainting?

Java: ContainerFlow: organize taintPreservingQualifierToMethod

5d73b99

aibaars added Java WIP This is a work-in-progress, do not merge yet! labels Jul 7, 2020

aibaars added 2 commits July 7, 2020 20:32

Java: ContainerFlow: organize taintPreservingArgumentToQualifier

c9ae2c8

Java: add Vector::copyInto, BlockingQueue::drainTo

441bf98

aibaars changed the title ~~Java: ContainerFlow: organize taintPreservingQualifierToMethod~~ Java: Clean up ContainerFlow, consider more methods Jul 7, 2020

Marcono1234 reviewed Jul 8, 2020

View reviewed changes

aibaars added 3 commits July 8, 2020 22:08

Address review comments

6367eb9

Java: add tests for Container taint steps

0bd103a

Java: ContainerFlow: RValue -> Expr

24c6e50

While most flow for a qualifierToArgumentStep goes through a variable use this is not always the case. Therefore it is best to remove the restriction to RValue to allow taint steps to use postupdate nodes. See also: ba86dea

aibaars marked this pull request as ready for review July 9, 2020 10:43

aibaars requested a review from a team as a code owner July 9, 2020 10:43

aibaars removed the WIP This is a work-in-progress, do not merge yet! label Jul 9, 2020

Java: ContainerFlow add comments

d3d5879

Some method variants are captured by a super class. Added some comments to indicate where this happens to make review of missing methods easier in the future.

aschackmull reviewed Jul 9, 2020

View reviewed changes

java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll Outdated Show resolved Hide resolved

aschackmull reviewed Jul 9, 2020

View reviewed changes

java/ql/src/semmle/code/java/dataflow/internal/ContainerFlow.qll Show resolved Hide resolved

Fix modelling of Stack.push

0d33a77

Stack.push(E) returns its argument, it does not propagate taint from the stack to the return value.

aibaars requested a review from a team July 9, 2020 14:55

aschackmull requested changes Jul 10, 2020

View reviewed changes

Drop Map.merge as taint step

43b6103

aschackmull approved these changes Jul 10, 2020

View reviewed changes

aschackmull merged commit a1d272e into github:master Jul 10, 2020

Marcono1234 reviewed Jul 10, 2020

View reviewed changes

aibaars mentioned this pull request Jul 13, 2020

Java: Clean up ContainerFlow: address outstanding comments #3946

Merged

Java: Clean up ContainerFlow, consider more methods #3918

Java: Clean up ContainerFlow, consider more methods #3918

Uh oh!

Conversation

aibaars commented Jul 7, 2020

Uh oh!

aibaars commented Jul 7, 2020

Uh oh!

Marcono1234 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aibaars commented Jul 9, 2020

Uh oh!

aschackmull Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aschackmull commented Jul 9, 2020

Uh oh!

aibaars commented Jul 9, 2020

Uh oh!

aschackmull left a comment

Choose a reason for hiding this comment

Uh oh!

Marcono1234 left a comment

Choose a reason for hiding this comment

Uh oh!

Marcono1234 Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

Marcono1234 Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

Marcono1234 Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

intrigus-lgtm Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Marcono1234 left a comment •

edited

Loading