JS: add query js/exploitable-polynomial-redos

[esbena]

Regular expressions with superlinear time-complexity on contemporary regular expression engines are the cause of several CVEs. We already flag the exponential cases with js/redos, regardless of how the regular expression is used. This is fine as the exponential case is bad regardless of malicious users.

It is possible to identify a large class of regular expression terms that multiplies the time-complexity of the enclosing regular expression by a linear time factor, so one such term results in quadratic time-complexity, and two such terms results in cubic time-complexity. (The 11 Class A CVEs in https://github.com/github/codeql-javascript-team/issues/63 contain patterns that are flagged by PolynomialBackTrackingTerm!)

These superlinear polynomial cases are extremely common, and mostly benign in practice. So it will be too noisy to flag all of them in general, even though many can easily be rewritten to have a linear time complexity.

To flag only the interesting cases, this PR introduces a taint tracking query that requires remote flow to be matched with the expensive regular expression. Note that the alert location is the expensive regular expression term , and that the path is for the remote flow.

In practice, client-side ReDoS is uninteresting, so the query only considers HTTP::RequestInputAccess as a source. By default, NodeJS servers only allow 8KB of data outside the body of HTTP requests, so most sources in the query will be limited to a length of less than 8000 characters, which in practice translates to roughly 100ms evaluation time for a quadratic regular expression. This is not a lot, so I have set the severity of the query to warning.

There's still room for a few improvement in the query (for instance, handing of negated character classes), but that work starts encroaching on the js/redos query implementation, so that can be done later.

For a sneak-peek at the results, check out the link hidden at https://git.semmle.com/gist/esben/313a7bfdfc6a1f30383e6891a138a2a4.

[asgerf]

Looks very reasonable. I like the decision to use taint tracking, and using RequestInputSource to restrict to server code.

I think we should remove the word "exploitable" from the name, though, as the exploitability of these results are no different from the other taint queries. "Polynomial ReDoS" sounds fine to me.

[asgerf]

I think this is already available through RegExpTerm.isNullable

[esbena]

Almost, but not exactly. The two ^$ anchors in particular are nullable, but they do not match "at least an epsilon symbol", they match the implicit start and end symbols instead.

[esbena]

this term does not restrict the language of the enclosing regular expression

I think this makes it clear that ^ and $ are not epsilon matchers.

[asgerf]

Could you rephrase this? I don't understand what this means, and I can't figure it out from the implementation.

[esbena]

I have tried that already several times, I think I stuck in the wrong corner. Let me state the purpose of this predicate, and then you may view this problem from the right angle to help me get a better naming/docstring.

The predicate is used to find a term t1:

1. t1 consumes the last symbol just before an infinitely repeating term t2.
   • for example: t1 is a, and t2 is b+ here: /...ab+.../ and here: /...a(foo)?b+.../. (this is what getAMatchPredecessor(this.getPredecessor()) implements)
2. t1 is infinitely repeating (this is what InfiniteRepetitionQuantifier implements)
3. t1 can consume at least one of the same symbols as t2 (this is what compatible implements)

[asgerf]

Yeah I can see how it's not easy to capture in a short sentence.

It might be enough to just add some examples, like:

• For b? in ab?c this gets a and b (in addition to b? itself).
• For (ab|cd) this gets b and d (in addition to ab|cd and (ab|cd)).

[esbena]

The performance is again surprisingly good.
The comparison is for js/redos vs js/redos+js/polynomial-redos, so the additional taint-tracking seems to be for free.

[asgerf]

The performance is again surprisingly good.

Those wall clock timings are clearly biased in favor of the second run. Could you try running with --dpm?

[esbena]

with dpm, now the timing barely favors the run with only js/redos, so it should be good enough to land IMO.

Ping @mchammer01 for docreview.
NB:

• the QHelp Preview is broken for this PR
• the content in the two <include> qhelp files has simply been moved out of ReDoS.qhelp.

[asgerf]

LGTM - just waiting for doc review.

[mchammer01]

@esbena - apologies for the late review but I am currently in the US for a team mini-summit.
Unfortunately the preview failed so I wasn't able to see the example snippets in-situ, nor what the overview or the references look like.
I have made a few comments for your consideration. Feel free to ignore the ones you don't agree with.

[esbena]

Thank you @mchammer01. Except for the capitalisation comment, I have addressed all of your comments with minor fixes.

[mchammer01]

Thanks for the doc updates @esbena - LGTM 👍

[erik-krogh]

Doesn't look like the qhelp preview likes your inline regexp:
PolynomialReDoS.qhelp:48:21: The content of elements must consist of well-formed character data or markup.

Line 48: (<code>/^\s+|(?<!\s)\s+$/g</code>), or just by using the built-in trim

[asgerf]

I get a build error when building the qhelp offline:

Error on line 48 column 21 of PolynomialReDoS.qhelp:
  SXXP0003: Error reported by XML parser: The content of elements must consist of
  well-formed character data or markup.

It seems to be caused by the < in this regexp:

(<code>/^\s+|(?<!\s)\s+$/g</code>), or just by using the built-in trim

Edit: erik beat me to it