Skip to content

JS: Manually prune data flow in prototype-pollution-utility#2735

Merged
semmle-qlci merged 4 commits intogithub:masterfrom
asger-semmle:prototype-pollution-manual-dataflow
Feb 5, 2020
Merged

JS: Manually prune data flow in prototype-pollution-utility#2735
semmle-qlci merged 4 commits intogithub:masterfrom
asger-semmle:prototype-pollution-manual-dataflow

Conversation

@asgerf
Copy link
Contributor

@asgerf asgerf commented Jan 31, 2020

Speeds up the prototype-pollution-utility query by pruning sinks much more aggressively.

To recap: a dynamic write base[key] = rhs potentially gives rise to three sinks (base, key, rhs). We must find a path to all three sinks for this write to be flagged. However, if one of the sinks is obviously not reachable from a source (e.g. it's a constant), the analysis will still try to find paths to the other sinks, even though they can never lead to an alert.

This PR prunes out dynamic writes by doing a bit of preliminary data flow. In particular, we check that

  • key might come from a property enumeration and
  • base might refer to Object.prototype, which requires flow from a dynamic property read whose key comes from a property enumeration.

In this pruning flow, we deliberately do not track flow out of returns because this simply doesn't happen in the cases we're looking for (but we do summarize functions).

Evaluation shows a moderate improvement and we even lose the pesky FP in jQuery.

@asgerf asgerf added the JS label Jan 31, 2020
@asgerf asgerf requested a review from a team as a code owner January 31, 2020 15:17
@asgerf
Copy link
Contributor Author

asgerf commented Feb 3, 2020

Looking at this again I realise I made the query depend on an internal API (argumentPassing). We should find a nice way to expose that.

@asgerf asgerf added the WIP This is a work-in-progress, do not merge yet! label Feb 3, 2020
@asgerf asgerf force-pushed the prototype-pollution-manual-dataflow branch from 1d3deaf to 3ccdaa9 Compare February 4, 2020 15:08
@asgerf
Copy link
Contributor Author

asgerf commented Feb 4, 2020

I've exposed the previously-internal argumentPassing as DataFlow::argumentPassingStep. It embodies a fairly general concept, so it seems innocent enough. (ping @max-schaefer)

I opted to keep it as a predicate without a receiver or return type as this minimises the chance for breaking changes later. In particular, the type of the invocation node may be loosened when adding flow from property writes into setters.

Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

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

That is a nice surgical improvement.
All of my comments are requests for additional explanation-comments for the changes.

asgerf and others added 2 commits February 5, 2020 09:47
@asgerf asgerf removed the WIP This is a work-in-progress, do not merge yet! label Feb 5, 2020
@semmle-qlci semmle-qlci merged commit 163285b into github:master Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants