JS: Add library input as source to js/prototype-polluting-assignment

[erik-krogh]

I did an investigation into some prototype related vulnerabilities where I checked which CVEs we could flag by adding library inputs to js/prototype-polluting-assignment.
I didn’t expect much to come from that, but I found 23 CVEs that were flagged using this approach, so now I want to see if that could get merged.

A run across a bunch of projects definitely reveals some noise.
But I had expected worse, so I think it's OK for this warning query.
(But maybe we should lower the precision?)

Performance evaluation looks OK.

Most of the commits (after the initial commit) added one TP or TN to the list of CVEs below.

Edit: It's likely that the current PR does not flag all the below CVEs.
I'll look into which are flagged after the PR has been merged.

CVEs (23):

TP/TN (14): CVE-2020-7724, CVE-2020-7600, CVE-2019-10792, CVE-2020-7638, CVE-2020-8203, CVE-2020-7720, CVE-2020-7788, CVE-2021-25912, CVE-2020-28273, CVE-2020-7774, CVE-2020-28448, CVE-2020-7708, CVE-2019-10806, CVE-2020-7709
TP (9): CVE-2020-7701, CVE-2018-16490, CVE-2020-15256, CVE-2020-28460, CVE-2020-7707, CVE-2020-28268, CVE-2021-23434, CVE-2021-3805, CVE-2020-28282

---

There are other CVEs that basically boil down to:

export function myGetter(name) {
  return obj[name]; // <- name can be "__proto__", this is just a small gadget in a larger exploit. 
}

I don't think we will ever flag those without getting way to much noise.

[asgerf]

I like the refactorings and sanitizer improvements, but I'm worried about making this query more noisy than it already is.

Some of the paths in the linked query console run start at a parameter but then uses a return step to flow out of the library function to some other library code. These are clearly not interesting as it does not represent an external user calling the library. I seem remember you did something to exclude such paths in other contexts, but I'm not sure if it applies to this query? I'd say we should get that working for this query, if we're to go ahead with this.

[asgerf]

We have ConcatenationNode.isCoercion() for this

[erik-krogh]

I seem remember you did something to exclude such paths in other contexts, but I'm not sure if it applies to this query? I'd say we should get that working for this query, if we're to go ahead with this.

I did exclude such paths in other "library-input" queries, but I forgot to add it here.
It's an extremely simple addition, but it requires another performance evaluation (I'll get that going).
The change might remove some CVE TPs, but I'll look at that after merge.

[erik-krogh]

but it requires another performance evaluation (I'll get that going).

New evaluation done.
Performance is still OK (given that it's single-query performance).
And there are about half the number of new results, so it seems to have removed a decent amount of noise.

[asgerf]

Unfortunately the results still seem very noisy.

In addition to #6789, I'd suggest excluding sinks that aren't dynamic property assignments when the source was a library input (by restricting valid source/sink pairs after the data flow paths have been found). That might bring it down to reasonable levels.

[erik-krogh]

In addition to #6789, I'd suggest excluding sinks that aren't dynamic property assignments when the source was a library input (by restricting valid source/sink pairs after the data flow paths have been found). That might bring it down to reasonable levels.

I've implemented it.
I like the suggestion, and I don't think it will cause many FPs.
(I've rebased on top of main and started a new evaluation).

[erik-krogh]

The new evaluation looks better.
Still a bit noisy, but less than before.

[asgerf]

Ok it's getting close but I think we go the extra mile to cut down the remaining FPs.

I've opened #7044 which fixes the FPs in angular and relay.

[erik-krogh]

Do you want another evaluation now that I've merged in #7044?

[asgerf]

Yes, please.

[erik-krogh]

Yes, please.

New evaluation is ready.

[asgerf]

Great, I think the results look reasonable now. Apart from a nit in the change note LGTM now.