Skip to content

Dataflow: Add support for not skipping configuration-specific nodes in big-step #13717

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
aschackmull merged 7 commits into from
Jul 19, 2023
Merged

Dataflow: Add support for not skipping configuration-specific nodes in big-step #13717

aschackmull merged 7 commits into from
Jul 19, 2023

Conversation

@aschackmull
Copy link
Contributor

@aschackmull aschackmull commented Jul 11, 2023

@owen-mc
Copy link
Contributor

owen-mc commented Jul 11, 2023

For a flow config using the new API (ConfigSig and so on) that does what I was expecting. I was a bit surprised that it didn’t do anything for old-style configuration classes. I guess it’s not important, as everything will move over to the new API in time.

@aschackmull
Copy link
Contributor Author

aschackmull commented Jul 14, 2023 •
edited
Loading

The C/C++ XXE query gets a bunch of additional steps due to its

  predicate isAdditionalFlowStep(
    DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2
  ) {
    // create additional flow steps for `XxeFlowStateTransformer`s
    state2 = node2.asIndirectExpr().(XxeFlowStateTransformer).transform(state1) and
    DataFlow::simpleLocalFlowStep(node1, node2)
  }

Is that desirable? Or should we add a predicate neverSkip(Node node) { none() } in the XXE config?

Edit: There are also a bunch of other queries with additional steps where we might also want to consider whether or not to use the new default that this PR introduces.

@aschackmull aschackmull marked this pull request as ready for review July 14, 2023 09:49
@aschackmull aschackmull requested review from a team as code owners July 14, 2023 09:49
owen-mc
owen-mc previously approved these changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

Go 👍🏻

MathiasVP
MathiasVP previously approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

C++/Swift 👍

@MathiasVP
Copy link
Contributor

MathiasVP commented Jul 17, 2023

The C/C++ XXE query gets a bunch of additional steps due to its

  predicate isAdditionalFlowStep(
    DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2
  ) {
    // create additional flow steps for `XxeFlowStateTransformer`s
    state2 = node2.asIndirectExpr().(XxeFlowStateTransformer).transform(state1) and
    DataFlow::simpleLocalFlowStep(node1, node2)
  }

Is that desirable? Or should we add a predicate neverSkip(Node node) { none() } in the XXE config?

Edit: There are also a bunch of other queries with additional steps where we might also want to consider whether or not to use the new default that this PR introduces.

We resolved this on Slack and decided to add a neverSkip override (see 8a71408).

owen-mc
owen-mc previously approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

Go 👍🏻 (but I think you should add a change note to let users know about the new feature)

yoff
yoff previously approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

Python 👍

@aschackmull aschackmull dismissed stale reviews from yoff, owen-mc, and MathiasVP via f79b560 July 19, 2023 07:50
MathiasVP
MathiasVP previously approved these changes Jul 19, 2023
View reviewed changes
owen-mc
owen-mc previously approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

Go 👍🏻

@aschackmull aschackmull dismissed stale reviews from owen-mc and MathiasVP via 8d365b0 July 19, 2023 09:42
@aschackmull aschackmull force-pushed the dataflow/neverskipadditionalsteps branch from f79b560 to 8d365b0 Compare July 19, 2023 09:42
@aschackmull
Copy link
Contributor Author

aschackmull commented Jul 19, 2023

One C/C++ query changed on main, so I had to rebase and add a commit to update its .expected file.

owen-mc
owen-mc approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

Go 👍🏻

MathiasVP
MathiasVP approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

Still LGTM!

@aschackmull
Copy link
Contributor Author

aschackmull commented Jul 19, 2023

🚀

@aschackmull aschackmull merged commit a9c76d4 into github:main Jul 19, 2023
@aschackmull aschackmull deleted the dataflow/neverskipadditionalsteps branch July 19, 2023 12:06
owen-mc pushed a commit to owen-mc/codeql that referenced this pull request Aug 9, 2023
@mbg @owen-mc
Make AllocationSizeOverflow use new API
c51319b 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.
owen-mc added a commit to owen-mc/codeql that referenced this pull request Aug 9, 2023
@owen-mc
Make RequestForgery use new API
37197b5 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.
owen-mc added a commit to owen-mc/codeql that referenced this pull request Aug 9, 2023
@owen-mc
Make ClearTextLogging use new API
5f3d64d 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.
owen-mc added a commit to owen-mc/codeql that referenced this pull request Aug 9, 2023
@owen-mc
Make CookieWithoutHttpOnly use new API
653458f 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.
owen-mc added a commit to owen-mc/codeql that referenced this pull request Aug 9, 2023
@owen-mc
Make SqlInjection use new API
6d8bb15 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.
owen-mc added a commit to owen-mc/codeql that referenced this pull request Aug 9, 2023
@owen-mc
Make DivideByZero use new API
98adf19 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.
owen-mc added a commit to owen-mc/codeql that referenced this pull request Aug 10, 2023
@owen-mc
Make OpenURLRedirect use new API
3392ffc 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.

Removed nodes and edges were only there originally due to multiple
configurations being in scope. `DataFlow::PathNode` has union semantics
for configurations. Nodes are only generated if they are reachable from
a source, but this includes sources from other configurations.
owen-mc added a commit to owen-mc/codeql that referenced this pull request Aug 10, 2023
@owen-mc
Make ClearTextLogging use new API
eecbb5b 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.
owen-mc added a commit to owen-mc/codeql that referenced this pull request Aug 10, 2023
@owen-mc
Make CookieWithoutHttpOnly use new API
618f425 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.
owen-mc added a commit to owen-mc/codeql that referenced this pull request Aug 10, 2023
@owen-mc
Make SqlInjection use new API
4f32ade 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.
owen-mc added a commit to owen-mc/codeql that referenced this pull request Aug 10, 2023
@owen-mc
Make DivideByZero use new API
29589ff 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.
owen-mc pushed a commit to owen-mc/codeql that referenced this pull request Aug 10, 2023
@mbg @owen-mc
Make AllocationSizeOverflow use new API
82a1b15 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.
owen-mc added a commit to owen-mc/codeql that referenced this pull request Aug 10, 2023
@owen-mc
Make RequestForgery use new API
97c3297 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.
owen-mc added a commit to owen-mc/codeql that referenced this pull request Aug 10, 2023
@owen-mc
Make OpenURLRedirect use new API
1f6cdc7 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.

Removed nodes and edges were only there originally due to multiple
configurations being in scope. `DataFlow::PathNode` has union semantics
for configurations. Nodes are only generated if they are reachable from
a source, but this includes sources from other configurations.
owen-mc added a commit to owen-mc/codeql that referenced this pull request Aug 10, 2023
@owen-mc
Make ClearTextLogging use new API
a7382e0 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.
owen-mc added a commit to owen-mc/codeql that referenced this pull request Aug 10, 2023
@owen-mc
Make CookieWithoutHttpOnly use new API
00cc78d 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.
owen-mc added a commit to owen-mc/codeql that referenced this pull request Aug 10, 2023
@owen-mc
Make SqlInjection use new API
ac1670c 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.
owen-mc added a commit to owen-mc/codeql that referenced this pull request Aug 10, 2023
@owen-mc
Make DivideByZero use new API
ea1f396 
The extra nodes in .expected files are due to the changes from
github#13717, which are not applied to
configuration classes extending DataFlow::Configuration or
TaintTracking::Configuration.
MathiasVP added a commit to MathiasVP/ql that referenced this pull request Oct 12, 2023
@MathiasVP
C++: Remove the hacky flow state since this is no longer needed after g…
64fa6c8 
…ithub#13717.
@MathiasVP MathiasVP mentioned this pull request Oct 12, 2023
Merged
@aschackmull aschackmull mentioned this pull request Dec 7, 2023
Merged
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MathiasVP MathiasVP MathiasVP approved these changes

@owen-mc owen-mc owen-mc approved these changes

@yoff yoff yoff left review comments

Assignees

No one assigned

Labels

C# C++ DataFlow Library documentation Go Java Python Ruby Swift

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@aschackmull @owen-mc @MathiasVP @yoff