Skip to content

Conversation

@erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Jan 6, 2021

Regains TP/TN for CVE-2018-3746 and CVE-2020-4066.

Those two CVEs were lost with #4460.
A simple solution would have been to revert that PR.
But with this solution we can keep the precision gain from #4460, while still getting TPs on the two CVEs.


I wondering if we should promote this flow-step to all taint/data-flow queries?
Because without it we miss the following:

class Foo {
  constructor() {
    this.bar = SOURCE;
  }
  baz() {
    var SINK = this.bar; 
  }
}

We only get the above flow if there is some call to baz() (which seems to connect the this nodes).

@erik-krogh erik-krogh added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Jan 6, 2021
@erik-krogh erik-krogh requested a review from a team as a code owner January 6, 2021 13:36
@github-actions github-actions bot added the JS label Jan 6, 2021
@esbena
Copy link
Contributor

esbena commented Jan 6, 2021

LGTM

I wondering if we should promote this flow-step to all taint/data-flow queries?

I think we have discussed such field taint before, and chosen not to make the promotion.
But perhaps that discussion was in the more general context of field flow between class methods that were invoked at different times.. Which would be more prone to false positives.
Lets create a promotion issue if the evaluation is fine.

@erik-krogh
Copy link
Contributor Author

The evaluation looks fine.
(The Relay result was a fluke, a rerun looks fine).

@erik-krogh erik-krogh removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Jan 7, 2021
@codeql-ci codeql-ci merged commit 7db5a99 into github:main Jan 7, 2021
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.

3 participants