JS: recognize .split("?")[0] in more cases for URL redirection#3391
Conversation
I think the 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) { ... }
} |
|
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. |
| /** | ||
| * Gets a the SourceNode for the string before it is split. | ||
| */ | ||
| DataFlow::SourceNode getUnsplit() { result = getReceiver().getALocalSource() } |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I went with getBaseString, and dropped the getALocalSource().
Co-authored-by: Esben Sparre Andreasen <esbena@github.com>
esbena
left a comment
There was a problem hiding this comment.
LGTM. Modulo the missing change note part and a missing flowsTo.
Co-authored-by: Esben Sparre Andreasen <esbena@github.com>
|
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. |
|
A security/nightly evaluation came back with mixed results. So I think this is ready to land. |
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.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.