JS: Add precise data-flow steps for Promise.all

[erik-krogh]

We can now precisely analyze something like the below in a DataFlow::Configuration.

var [clean, tainted] = await Promise.all(["clean", source]);
sink(clean); // OK - but flagged by taint-tracking
sink(tainted); // NOT OK

A TaintTracking::Configuration will still mix up the two properties.
This is caused both by the heap/directory taint-steps in TaintTracking.qll, and by the taint-steps in Promise.qll.
(You have to remove both to get rid of the imprecision).

[esbena]

LGTM, just a bit of polish required, and a change note for "promises" and bluebird.

The taint step precision can be addressed later - lets continue that discussion in https://github.com/github/codeql-javascript-team/issues/117.

[esbena]

I think we should have the following instead of the ad-hoc conversion with .toString, and also in the name of hypothetical performance.

string arrayElement(int i) { i < 5 and result = i.toString() or result = arrayElement() }

[esbena]

Approved. See inline comment.

[esbena]

We can eliminate getAnElement() because we happen to always know the element order in an ArrayCreationNode?
I think that is forwards compatible in practice, but please indicate with a 👍 that this change was intentional.

[asgerf]

Hm, I'm a little sceptical of doing something that only works for pure data-flow, not taint-tracking.

I was thinking we could locally recognize tuple steps through Promise.all, add those as taint steps, and use them to filter out the usual heap steps. With pseudo-code, something like this:

predicate promiseAllStep(Node pred, Node succ) { ... }

predicate heapStep(Node pred, Node succ) {
  // ...
   exists(ArrayExpr array | array = succ.asExpr() |
     not promiseAllStep(pred, _)
     pred = array.getAnElement().flow()
    )
)

[erik-krogh]

I was thinking we could locally recognize tuple steps through Promise.all, add those as taint steps, and use them to filter out the usual heap steps. With pseudo-code, something like this:

I like the idea, I'll look at that afterwards.
For now I think this is good to merge after an evaluation.

[asgerf]

Could you add a test case where the inputs to Promise.all are promises?

I can't quite picture how this would work. It seems to me we end up with the sequence of steps:

• Store step into promise with promise-value pseudo-property
• Store step into array object with array-index property
• Load step from the array object to Promise.all with promise-value pseudo-property
• Load step from Promise.all to array access with array-index property

The above sequence is not reducible to plan data flow because the innermost store/load pair have different property names.

The way I see it, Promise.all converts an "array of promises" to a "promise of an array", which in general requires us to swap the order of two properties. This requires two loads steps followed by two store steps (which we unfortunately can't do).

[erik-krogh]

The way I see it, Promise.all converts an "array of promises" to a "promise of an array", which in general requires us to swap the order of two properties. This requires two loads steps followed by two store steps (which we unfortunately can't do).

Your analysis is correct, and we end up not tracking data-flow when the inputs to Promise.all are promises.

[asgerf]

Yeah that's really unfortunate. This PR is a bit of a hard sell with that restriction. Do you mind withholding this PR for a bit while trying out the local heuristic?

[erik-krogh]

Yeah that's really unfortunate. This PR is a bit of a hard sell with that restriction. Do you mind withholding this PR for a bit while trying out the local heuristic?

I think I got it working nicely.
The last test-case is still not flagged by a DataFlow::Configuration, but the TaintTracking::Configuration no longer has any FPs in the test.

[erik-krogh]

An evaluation came back with mixed results (redo of the 10 worst).

I tried to restrict the precise array-elements to Promise.all.
Evaluation on that looks good. (After I initially got this weird result).

[asgerf]

Thanks! LGTM now

[erik-krogh]

@asgerf can I get a re-approve?

[erik-krogh]

@esbena can I get a re-approve/merge?
QLucie won't let me merge without your approval.