Java: Add models for flow- and taint-preserving functions in Commons ObjectUtils by smowton · Pull Request #5344 · github/codeql

smowton · 2021-03-05T18:04:27Z

These should all be value-preserving, but we don't support value-preserving varargs methods yet.

java/change-notes/2021-03-05-commons-object-utils.md

Marcono1234 · 2021-03-06T02:21:36Z

java/ql/test/library-tests/frameworks/apache-commons-lang3/flow.ql

+    exists(DataFlow::Node src, DataFlow::Node sink, TaintFlowConf conf | conf.hasFlow(src, sink) |
+      sink.getLocation() = location and
+      element = sink.toString() and
+      value = "y"


You could also match the empty string "" here, see #5347

Marcono1234 · 2021-03-06T02:24:18Z

java/ql/src/semmle/code/java/frameworks/apache/Lang.qll

+        // but we don't support value-preserving varargs functions at the moment.
+        "org.apache.commons.lang3;ObjectUtils;false;clone;;;Argument;ReturnValue;value",
+        "org.apache.commons.lang3;ObjectUtils;false;cloneIfPossible;;;Argument;ReturnValue;value",
+        "org.apache.commons.lang3;ObjectUtils;false;CONST;;;Argument;ReturnValue;value",


Should this also match CONST_BYTE and CONST_SHORT?

I believe these won't work due to a type mismatch? Not 100% sure though

Do you mean they won't work for dataflow because CodeQL does not support dataflow with different types? Would it at least be good then to model them as taint?

They should work just fine. The type system that's used for pruning data flow allows for implicit casts, it effectively does not distinguish between the different numeric primitive types.

aschackmull · 2021-03-08T13:42:17Z

but we don't support value-preserving varargs methods yet.

Please elaborate.

smowton · 2021-03-09T08:22:45Z

but we don't support value-preserving varargs methods yet.

Please elaborate.

I tried defining value-preserving edges from the actual arguments to the implicit varargs array created at the callsite, and from the array to the return value, but no flow resulted, presumably due to type differences between the array and its elements?

smowton · 2021-03-09T15:56:32Z

@aschackmull applied the above comments, believe this is ready

Marcono1234

You probably had some internal discussion about this, but in case varargs arguments should be covered with value, the following review comments mention the necessary changes.
Please ignore them if flow from a varargs argument should still be treated as taint.

Though there is some ambiguity for dataflow then, e.g. consider this (contrived) example:

public static Object[] copy(Object[]... args) {
    return args.clone();
}

public static Object[] get(Object[]... args) {
    return args[r.nextInt(args.length)];
}

Unless there is some special CSV model syntax, for one of the methods dataflow cannot be modeled correctly.
Maybe it would make sense to introduce a syntax similar to Argument[...] to indicate that the input / output is an argument of the implicit varargs array?
E.g. for the above example code:

org.example;MyClass;false;copy;;;Argument;ReturnValue;value
org.example;MyClass;false;get;;;Argument[...];ReturnValue;value

Marcono1234 · 2021-03-10T21:28:22Z

java/ql/src/semmle/code/java/frameworks/apache/Lang.qll

+  override predicate row(string row) {
+    row =
+      [
+        // Note all the functions annotated with `taint` flow really should have `value` flow,


This comment might now be redundant.

Marcono1234 · 2021-03-10T21:28:56Z

java/ql/src/semmle/code/java/frameworks/apache/Lang.qll

+        "org.apache.commons.lang3;ObjectUtils;false;CONST_BYTE;;;Argument;ReturnValue;value",
+        "org.apache.commons.lang3;ObjectUtils;false;CONST_SHORT;;;Argument;ReturnValue;value",
+        "org.apache.commons.lang3;ObjectUtils;false;defaultIfNull;;;Argument;ReturnValue;value",
+        "org.apache.commons.lang3;ObjectUtils;false;firstNonNull;;;Argument;ReturnValue;taint",


Suggested change

"org.apache.commons.lang3;ObjectUtils;false;firstNonNull;;;Argument;ReturnValue;taint",

"org.apache.commons.lang3;ObjectUtils;false;firstNonNull;;;Argument;ReturnValue;value",

No, this has to be "taint" a while yet until we support proper specification of array contents. The argument is an array (possibly implicitly created), but the method does not return that exact array, so this is not value-preserving from Argument to ReturnValue. The proper value-preserving spec is from Content[Array] of Argument to ReturnValue, but that isn't supported yet.

Marcono1234 · 2021-03-10T21:33:02Z

java/ql/src/semmle/code/java/frameworks/apache/Lang.qll

+        "org.apache.commons.lang3;ObjectUtils;false;max;;;Argument;ReturnValue;taint",
+        "org.apache.commons.lang3;ObjectUtils;false;median;;;Argument;ReturnValue;taint",
+        "org.apache.commons.lang3;ObjectUtils;false;min;;;Argument;ReturnValue;taint",
+        "org.apache.commons.lang3;ObjectUtils;false;mode;;;Argument;ReturnValue;taint",


Suggested change

"org.apache.commons.lang3;ObjectUtils;false;max;;;Argument;ReturnValue;taint",

"org.apache.commons.lang3;ObjectUtils;false;median;;;Argument;ReturnValue;taint",

"org.apache.commons.lang3;ObjectUtils;false;min;;;Argument;ReturnValue;taint",

"org.apache.commons.lang3;ObjectUtils;false;mode;;;Argument;ReturnValue;taint",

"org.apache.commons.lang3;ObjectUtils;false;max;;;Argument;ReturnValue;value",

"org.apache.commons.lang3;ObjectUtils;false;median;;;Argument;ReturnValue;value",

"org.apache.commons.lang3;ObjectUtils;false;min;;;Argument;ReturnValue;value",

"org.apache.commons.lang3;ObjectUtils;false;mode;;;Argument;ReturnValue;value",

No. Same as above.

aschackmull

LGTM, but needs rebase to solve conflict.

…Utils. These should all be value-preserving, but we don't support value-preserving varargs methods yet.

Co-authored-by: Marcono1234 <Marcono1234@users.noreply.github.com>

smowton · 2021-03-11T16:23:50Z

@aschackmull rebased.

aschackmull · 2021-03-12T09:28:49Z

java/ql/test/library-tests/frameworks/apache-commons-lang3/flow.ql

  override predicate hasActualResult(Location location, string element, string tag, string value) {
    tag = "hasTaintFlow" and
-    exists(DataFlow::Node src, DataFlow::Node sink, Conf conf | conf.hasFlow(src, sink) |
+    exists(DataFlow::Node src, DataFlow::Node sink, TaintFlowConf conf | conf.hasFlow(src, sink) |


Might as well add and not any(ValueFlowConf conf | conf.hasFlow(src, sink)) to make the expected output shorter - there's no need to report taint-flow if we have value-flow as that will always be the case.

smowton requested a review from a team as a code owner March 5, 2021 18:04

github-actions bot added documentation Java labels Mar 5, 2021

Marcono1234 reviewed Mar 6, 2021

View reviewed changes

smowton force-pushed the smowton/feature/commons-object-utils branch from 18af0ff to 80b0ab7 Compare March 9, 2021 15:55

Marcono1234 reviewed Mar 10, 2021

View reviewed changes

aschackmull reviewed Mar 11, 2021

View reviewed changes

smowton and others added 4 commits March 11, 2021 16:22

Add models for flow- and taint-preserving functions in Commons Object…

1c1ca70

…Utils. These should all be value-preserving, but we don't support value-preserving varargs methods yet.

Add models for CONST_BYTE and CONST_SHORT

b5268de

Remove useless =y value specification from inline test expectations

6508a22

Improve change note

82a000b

Co-authored-by: Marcono1234 <Marcono1234@users.noreply.github.com>

smowton force-pushed the smowton/feature/commons-object-utils branch from 04614d4 to 82a000b Compare March 11, 2021 16:23

aschackmull reviewed Mar 12, 2021

View reviewed changes

Abbreviate redundant value-flow / taint-flow tests

58d5c2c

smowton changed the title ~~Add models for flow- and taint-preserving functions in Commons ObjectUtils~~ Java: Add models for flow- and taint-preserving functions in Commons ObjectUtils Mar 12, 2021

aschackmull approved these changes Mar 12, 2021

View reviewed changes

aschackmull merged commit c9786df into github:main Mar 12, 2021

	"org.apache.commons.lang3;ObjectUtils;false;firstNonNull;;;Argument;ReturnValue;taint",
	"org.apache.commons.lang3;ObjectUtils;false;firstNonNull;;;Argument;ReturnValue;value",

Conversation

smowton commented Mar 5, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aschackmull commented Mar 8, 2021

Uh oh!

smowton commented Mar 9, 2021

Uh oh!

smowton commented Mar 9, 2021

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aschackmull left a comment

Choose a reason for hiding this comment

Uh oh!

smowton commented Mar 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Marcono1234 left a comment •

edited

Loading