JS: Add query to detect bad code sanitizers #3680
Conversation
| // TODO: Proper customizations module, Source class Sink class etc. | ||
| import javascript | ||
| import DataFlow::PathGraph | ||
| private import semmle.javascript.heuristics.AdditionalSinks |
There was a problem hiding this comment.
We can wait with the following suggestion, but we must not forget about it.
AdditionalSinks.qll is designed to have the giant side effect of enabling all kinds of new sinks, and I think it is a footgun to import that library by default in any checked-in query - even if it doesn't hurt right here.
Can you pull out the relevant sink from AdditionalSinks.qll? AdditionalSinks.qll can then pull it back in and extend the right sink for the side effect.
| exists(StringOps::ConcatenationRoot root, int i | | ||
| root.getOperand(i) = result and | ||
| not exists(result.getStringValue()) and | ||
| not root = endsInCodeInjectionSink() |
There was a problem hiding this comment.
Hmm, my suggestion for this filter was not completely thought through. This line alone will cause us to miss completely out on the cases where we do not detect a remote-flow input to the source. That is not a trade-off I am willing to make in order to avoid duplicate alerts.
Could we perhaps check in the where clause below that there also isn't taint-type-tracking flow to the source from a remote-flow source? That won't cost us any results.
Please include a test case too.
| t.start() and | ||
| result instanceof RemoteFlowSource | ||
| or | ||
| exists(DataFlow::TypeTracker t2 | result = remoteFlow(t2).track(t2, t)) |
There was a problem hiding this comment.
See
codeql/javascript/ql/src/semmle/javascript/Regexp.qll
Lines 893 to 898 in cafbe14
CVE-2019-13506: TP/TN
CVE-2018-18282: TP/TN
Seems to easy.
The CVEs are both described as XSS, but I think that is because XSS is a catch-all for "the attacker can run some code".
In both CVEs the issue is a
function () {..}being constructed as a string and run, so that seems more like code-injection, which is why the query is in theCWE-094folder.No results on our standard 236 benchmarks.
TODO:
String.prototype.replacesanitizer more precise?