Skip to content

Data flow: Track simple call contexts in nodeCand[Fwd]1#2822

Merged
aschackmull merged 3 commits into
github:masterfrom
hvitved:dataflow/node-cand-simple-call-context
Feb 21, 2020
Merged

Data flow: Track simple call contexts in nodeCand[Fwd]1#2822
aschackmull merged 3 commits into
github:masterfrom
hvitved:dataflow/node-cand-simple-call-context

Conversation

@hvitved
Copy link
Copy Markdown
Contributor

@hvitved hvitved commented Feb 12, 2020 •
edited
Loading

This PR add a simple fromArg Boolean call context to nodeCandFwd1 (and similarly, toReturn is added to nodeCand1). This enables us to restrict flow back out of method to call sites where we have seen flow in, in the same way that field reads are restricted to fields that have also been written to.

In addition to making the two predicates more precise, this means we can also restrict taint summary calculations to Nodes n for which nodeCand1(n, true, config) holds, i.e., nodes that can reach a sink via a return.

Profiling runs (internal links):

@hvitved hvitved force-pushed the dataflow/node-cand-simple-call-context branch 3 times, most recently from a0ebf63 to 4f647c1 Compare February 17, 2020 13:39
@hvitved hvitved force-pushed the dataflow/node-cand-simple-call-context branch from 4f647c1 to a695b56 Compare February 17, 2020 18:40
@hvitved hvitved marked this pull request as ready for review February 18, 2020 08:48
@hvitved hvitved requested review from a team as code owners February 18, 2020 08:48
@hvitved hvitved requested a review from aschackmull February 18, 2020 08:48
aschackmull
aschackmull reviewed Feb 20, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor things, otherwise looks good.

Comment thread cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl.qll
Comment thread cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl.qll
Comment thread cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl.qll Outdated
Comment thread cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl.qll Outdated
@aschackmull aschackmull merged commit 771cb75 into github:master Feb 21, 2020
@hvitved hvitved deleted the dataflow/node-cand-simple-call-context branch February 21, 2020 09:02
max-schaefer pushed a commit to max-schaefer/codeql-go that referenced this pull request Feb 21, 2020
Track simple call contexts in nodeCand[Fwd]1.
7230912 
See github/codeql#2822.
@max-schaefer max-schaefer mentioned this pull request Feb 21, 2020
Merged
@yo-h
Copy link
Copy Markdown
Contributor

yo-h commented Feb 24, 2020

For the record, the Java result changes were:

=== Changes per project (+new -fixed) ===

g/apache/lucene-solr                                                        +1 -0
g/apache/hadoop-common                                                      +3 -0
g/AdoptOpenJDK/openjdk-jdk11u                                               +8 -69

=== Changes per query (+new -fixed) ===

java/hardcoded-credential-api-call                                          +2 -0
java/path-injection                                                         +1 -0
java/similar-file                                                           +2 -2
java/stack-trace-exposure                                                   +1 -0
java/tainted-arithmetic                                                     +5 -67
java/xxe                                                                    +1 -0

@jbj
Copy link
Copy Markdown
Contributor

jbj commented Feb 25, 2020

I didn't think this PR was supposed to have a large impact on results. Did I misunderstand?

@aschackmull
Copy link
Copy Markdown
Contributor

aschackmull commented Feb 25, 2020

I don't exactly know why java/tainted-arithmetic lost those results, but the query is quite volatile on jdk11. For the result increases I believe this is because an improved pruning in step 1 means that fewer dispatch edges are pruned due to falling below the fieldFlowBranchLimit. At the very least I know that this is the reason for the two added java/hardcoded-credential-api-call.

@hvitved hvitved mentioned this pull request Mar 23, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aschackmull aschackmull aschackmull approved these changes

Assignees

@aschackmull aschackmull

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hvitved @yo-h @jbj @aschackmull