JS: Make sanitizers no longer block taint inside an object

[asgerf]

Taint can now be tracked across sanitizers that don't operate on the tainted value directly, but only on an object containing it. For example:

if (isSafe(x)) {
  sink(x.foo); // NOT OK - 'x' is sanitized, but not 'x.foo'
}

This was done through the following change to taint-tracking configurations:

• Taint sources are now associated with the taint label by default (previously data).
• Sanitizers only block the taint label.

The old behavior of completely blocking flow through a label can be achieved by overriding isBarrier instead, or using a labeled barrier to block the data label.

The flow label mapping data->taint is still generated by all taint steps. This ensures backwards compatibility for queries that use data as the source (although the change to sanitizers is still in effect).

Note that we have some taint-tracking configurations that relies on data being the source, such as HardcodedDataInterpretedAsCode, where we are only interested in data that has gone through some transformation before reaching the sink. So there are use-cases for having data being the source, it's just no longer the default.

Evaluation was pretty quiet.

[erik-krogh]

It looks good to me.
It makes sense to have taint-tracking configurations primarily work with the taint label.

But I think we should get yet another pair of eyes on this (@esbena?).

Tests are failing.

[esbena]

I have one minor architectural concern.

[esbena]

It is a shame that we have to introduce a dependency form Configuration.qll to TaintTracking.qll (I think this is the first, at least). We could avoid that by introducing cfg.getDefaultSourceLabel(), which in TaintTracking::configuration is implemented as result = FlowLabel::taint().
I will leave it to you to decide what to do here.

Ditto for isSink below.

[asgerf]

That's very true, but on the other hand, there's also a downside to extending our public API with stuff that users don't actually need.

[asgerf]

There was a test failure that revealed an interesting, but generally benign side effect of the change.

The prototype pollution utility query had a barrier guard for x in obj where x was sanitized in the false case, while a standard sanitizer guard sanitizes x in the true case. The latter one shouldn't matter because we don't use a taint-tracking configuration, but because its underlying instance (the InExpr) was included in isBarrierGuard the standard sanitizer was pulled in that way. Since the standard sanitizer guard only blocks the taint label now, it's still being pulled in, but has no effect as we're not using the taint label in that query.

The change thus fixed a spurious sanitizer which revealed that a test was passing for the wrong reason. I've updated the test accordingly.

[esbena]

I have restarted the tests. The Jenkins logs were no longer present.

[asgerf]

This will need another evaluation. After rebasing some join orderings have changed and it's not performing as well anymore.

[asgerf]

Evaluation still looks fine.

[erik-krogh]

👍

[asgerf]

Thanks for the review @erik-krogh!

@esbena I believe I addressed your comment in in b6ca4fb but in any case, I've gone ahead and merged to unblock Erik's PR and since there are now a bunch of performance-related PRs floating around whose evaluations ought to be based on this one from now on.