JS: recognize .split("?")[0] in more cases for URL redirection

[erik-krogh]

Fixes these FPs that showed up in an anomalous query results a while ago.

The issue is we didn't recognize the .split("?")[0] pattern if there was any data-flow. E.g.

var parts = document.location.href.split( '?' );
var safe = parts[0];

With this PR a .getALocalSource() is added, such that we recognize the safe way of constructing a redirect URL in more cases.

The pattern is not common, and I found no instances of it on our standard 236 benchmarks.

I know it looks like an ad-hoc addon, but the alternative solution would be to add another flow-label, and I don't think that would perform well.

[esbena]

the alternative solution would be to add another flow-label, and I don't think that would perform well.

I think the split-and-destruct pattern is common enough that you can generalize QueryPrefixSanitizer to be a class we may be able to use elsewhere (ag '"split"' -G qll gives some results), that is another alternative that avoids the adhoc feeling.

I am thinking of something like:

class StringSplitCall extends DataFlow::MethodCallNode {
  string getSplitAt() { getArgument(0).mayHaveStringValue(result) }
  DataFlow::Node getUnsplit() { result = getReceiver() }
  DataFlow::Node getAnElementRead(int i) { ... }
}

[asgerf]

I believe this also affects the DOM-based XSS query. Might be worth sharing the sanitizer if doing so doesn't have any adverse side effects.

[esbena]

Some minor comments.

[esbena]

I dislike the getUnsplit name (even though it is my own suggestion).

Perhaps getStringSource, and dually getASubstringRead(int i) below would be better?

This has no result for "foo|bar".split("|").map(...). I can live with that if a renaming like the above is implemented.
If we need to reason about the shorthand above, we can introduce string getString() and string getSubstring(int i).

[asgerf]

In the StringOps module I tried to stick with getBaseString as it was meaningful yet generic enough to be used consistently across the different string operations. I think we can use the same here, and possibly drop the getALocalSource.

[erik-krogh]

I went with getBaseString, and dropped the getALocalSource().

[esbena]

LGTM. Modulo the missing change note part and a missing flowsTo.

[erik-krogh]

An evaluation came back with not great performance, and an FP for the following pattern:

window.location.replace(window.location.href.split("#")[0] + "#mappage");

I fixed the FP, and I'll look into the performance.

[erik-krogh]

A security/nightly evaluation came back with mixed results.
But a redo of the 10 worst looks OK.

So I think this is ready to land.