Dataflow: Add support for implicit reads #6107

aschackmull · 2021-06-18T13:47:29Z

This adds a new overridable predicate on configurations:

  /**
   * Holds if an arbitrary number of implicit read steps of content `c` may be
   * taken at `node`.
   */
  predicate allowImplicitRead(Node node, Content c)

The purpose is to support sinks and taint steps accepting non-empty accesspaths.

I've also added a default override in taint-tracking configurations, which the language specific implementations can use to add such default support. This is just a (hopefully helpful) default and can be overridden by specific configurations. For Java, the default implementation is all array-, collection-, and map-value-read steps that match the type at the given sink / additional taint step.

This means that we ought to be able to drop the temporary store-as-taint workaround, which was costing quite some performance.

asgerf · 2021-06-21T08:24:18Z

Quick question about how this relates to something we do in the JS libraries. We support custom load steps via:

  /**
   * EXPERIMENTAL. This API may change in the future.
   *
   * Holds if the property `prop` of the object `pred` should be loaded into `succ`.
   */
  predicate isAdditionalLoadStep(DataFlow::Node pred, DataFlow::Node succ, string prop)

I'm wondering if allowImplicitRead(node, c) would be equivalent to isAdditionalLoadStep(node, node, c), or is there something else going on here?

aschackmull · 2021-06-21T08:28:10Z

I'm wondering if allowImplicitRead(node, c) would be equivalent to isAdditionalLoadStep(node, node, c), or is there something else going on here?

It is almost equivalent, yes. There's a slight difference if you also use type pruning, because this would easily remove the flow you gained from isAdditionalLoadStep(node, node, c) (the types don't match up). So conceptually you may think of this as an in-place additional read step with the quirks related to type pruning sorted such that it actually works.

github-actions · 2021-06-21T13:52:25Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The following differences were found:

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    `Apache Commons Lang <https://commons.apache.org/proper/commons-lang/>`_,``org.apache.commons.lang3``,,417,,,,,,,,
+    `Apache Commons Lang <https://commons.apache.org/proper/commons-lang/>`_,``org.apache.commons.lang3``,,420,,,,,,,,
-    Totals,,84,1622,181,13,6,6,,33,1,58
+    Totals,,84,1625,181,13,6,6,,33,1,58

Changes to framework-coverage-java.csv:

- org.apache.commons.lang3,,,417,,,,,,,,,,,,,,324,93
+ org.apache.commons.lang3,,,420,,,,,,,,,,,,,,292,128

github-actions · 2021-06-21T14:10:56Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The following differences were found:

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    `Apache Commons Lang <https://commons.apache.org/proper/commons-lang/>`_,``org.apache.commons.lang3``,,417,,,,,,,,
+    `Apache Commons Lang <https://commons.apache.org/proper/commons-lang/>`_,``org.apache.commons.lang3``,,420,,,,,,,,
-    Totals,,84,1622,181,13,6,6,,33,1,58
+    Totals,,84,1625,181,13,6,6,,33,1,58

Changes to framework-coverage-java.csv:

- org.apache.commons.lang3,,,417,,,,,,,,,,,,,,324,93
+ org.apache.commons.lang3,,,420,,,,,,,,,,,,,,292,128

smowton · 2021-06-21T15:23:36Z

Started a benchmark to check effects on codeql-go (without any default implementation)

aschackmull · 2021-06-22T07:22:38Z

Java test of 38319a4 (just the dataflow lib changes) ~~https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/1464/~~
https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/1469/

Testing PR head (c06e152):
~~https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/1465/~~
~~https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/1470/~~ (bad magic)
https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/1472/

~~https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/1149/~~
https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/1153/

~~https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/2099/~~
https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/2106/

~~https://jenkins.internal.semmle.com/job/Changes/job/Python-Differences/585/~~
https://jenkins.internal.semmle.com/job/Changes/job/Python-Differences/588/

smowton

Go benchmark results are good (either neutral or slightly beneficial)

aschackmull · 2021-06-23T12:15:15Z

Java perf measurements are thrown off by a case of bad magic. I'll need to fix it and restart them.

github-actions · 2021-06-23T12:41:27Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The following differences were found:

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    `Apache Commons Lang <https://commons.apache.org/proper/commons-lang/>`_,``org.apache.commons.lang3``,,417,,,,,,,,
+    `Apache Commons Lang <https://commons.apache.org/proper/commons-lang/>`_,``org.apache.commons.lang3``,,420,,,,,,,,
-    Totals,,84,1624,181,13,6,6,,33,1,58
+    Totals,,84,1627,181,13,6,6,,33,1,58

Changes to framework-coverage-java.csv:

- org.apache.commons.lang3,,,417,,,,,,,,,,,,,,324,93
+ org.apache.commons.lang3,,,420,,,,,,,,,,,,,,292,128

aschackmull · 2021-06-24T08:21:41Z

Looks like the performance regression by the store-as-taint workaround is indeed fixed:

Security/CWE/CWE-090/LdapInjection.ql	363	138	-225
Security/CWE/CWE-078/ExecUnescaped.ql	477	162	-315
Security/CWE/CWE-078/ExecTainted.ql	481	155	-326

But the new feature that's added to the dataflow library to do this does come with an added performance overhead - mainly due to tuple-numbering, as far as I have been able to see.

hvitved

Looks solid to me.

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl.qll

hvitved · 2021-06-24T12:00:32Z

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll

+
+/** A reference through the contents of some collection-like container. */
+private class CollectionContent extends Content, TCollectionContent {
+  override string toString() { result = "<element>" }


Why did you change the C++ content toString()s?

To match Java. They were originally just copy-pasted from Java and weren't used in either language. When I added support for collection-flow in Java I updated them to something I thought made more sense (but the old somewhat arbitrary values still lingered in C++). So now that I moved them I thought I might as well update them. I did check with @MathiasVP beforehand that this was ok with him.

github-actions · 2021-06-24T12:31:42Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The following differences were found:

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    `Apache Commons Lang <https://commons.apache.org/proper/commons-lang/>`_,``org.apache.commons.lang3``,,417,,,,,,,,
+    `Apache Commons Lang <https://commons.apache.org/proper/commons-lang/>`_,``org.apache.commons.lang3``,,420,,,,,,,,
-    Totals,,84,1634,181,13,6,6,,33,1,58
+    Totals,,84,1637,181,13,6,6,,33,1,58

Changes to framework-coverage-java.csv:

- org.apache.commons.lang3,,,417,,,,,,,,,,,,,,324,93
+ org.apache.commons.lang3,,,420,,,,,,,,,,,,,,292,128

MathiasVP · 2021-06-24T12:39:46Z

https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/2106/

CPP-Differences doesn't look super good. There's a clear performance regression on all of the dataflow queries. Is such a big slowdown expected?

aschackmull · 2021-06-24T12:56:30Z

Is such a big slowdown expected?

I'd say unfortunately yes for the time being. It seems like all dataflow queries are taking a hit across all languages. I've been trying to dig into it, but there's no clear single culprit - rather it seems like the wrapping of Node as TNodeNormal with all the necessary accompanying joins are to blame: It's a bunch of additional Node-sized scans and joins and tuplenumberings. On the plus side, I think a lot of the additional overhead can be run in parallel, and I'm not sure whether the LANG-Differences jobs does that (so the real cost may be less severe).

I do have some ideas for improving the situation, but I'd rather look into those as follow-up work.

MathiasVP

Thanks for the explanation!

aschackmull requested review from a team as code owners June 18, 2021 13:47

github-actions bot added C# C++ Java Python labels Jun 18, 2021

aschackmull added 6 commits June 21, 2021 14:41

DataFlow: Add support for configuration-specific implicit reads.

b7ac329

Dataflow: Sync.

8088032

Java: Make Content public as DataFlow::Content.

aa82d0b

C/C++: Make Content public as DataFlow::Content.

38319a4

Java: Add defaultImplicitTaintRead and sync.

65ac8be

Java: Remove temporary store-as-taint.

d383c0f

aschackmull force-pushed the dataflow/implicit-reads branch from 5cbbae0 to d383c0f Compare June 21, 2021 12:43

C/C++: Update qltest expected output.

810de73

aschackmull added 2 commits June 21, 2021 16:08

Java: Fix some qltests.

27c973e

Java: Remove outdated test.

c06e152

aschackmull force-pushed the dataflow/implicit-reads branch from e2fa670 to c06e152 Compare June 21, 2021 14:09

smowton previously approved these changes Jun 22, 2021

View reviewed changes

Java: Fix bad magic.

6374914

aschackmull dismissed smowton’s stale review via 6374914 June 23, 2021 12:39

hvitved reviewed Jun 24, 2021

View reviewed changes

aschackmull added 3 commits June 24, 2021 14:18

DataFlow: Address review comments.

1c1d11a

Dataflow: Sync.

cd0efbe

C++/C#/Java/Python: Add change notes.

01fc3e6

github-actions bot added the documentation label Jun 24, 2021

hvitved approved these changes Jun 24, 2021

View reviewed changes

MathiasVP approved these changes Jun 24, 2021

View reviewed changes

aschackmull merged commit 95ad8b5 into github:main Jun 24, 2021

aschackmull deleted the dataflow/implicit-reads branch June 24, 2021 13:38

aschackmull mentioned this pull request Jul 1, 2021

Java: Convert SQL sinks to CSV format #5487

Merged

Dataflow: Add support for implicit reads #6107

Dataflow: Add support for implicit reads #6107

Uh oh!

Conversation

aschackmull commented Jun 18, 2021

Uh oh!

asgerf commented Jun 21, 2021

Uh oh!

aschackmull commented Jun 21, 2021

Uh oh!

github-actions bot commented Jun 21, 2021

java

Generated file changes for java

Uh oh!

github-actions bot commented Jun 21, 2021

java

Generated file changes for java

Uh oh!

smowton commented Jun 21, 2021

Uh oh!

aschackmull commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

aschackmull commented Jun 23, 2021

Uh oh!

github-actions bot commented Jun 23, 2021

java

Generated file changes for java

Uh oh!

aschackmull commented Jun 24, 2021

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hvitved Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

aschackmull Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 24, 2021

java

Generated file changes for java

Uh oh!

MathiasVP commented Jun 24, 2021

Uh oh!

aschackmull commented Jun 24, 2021

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aschackmull commented Jun 22, 2021 •

edited

Loading