-
Notifications
You must be signed in to change notification settings - Fork 1.7k
JS: Detect more expensive regexps in js/polynomial-redos #4579
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Yeah that's a bit tricky, but I think it might be worth special-casing the handling of The way I see it, if We can convert the class PolynomialBackTrackingTerm extends InfiniteRepetitionQuantifier {
string reason;
PolynomialBackTrackingTerm() {
// the regexp may fail to match ...
exists(RegExpTerm succ | this.getSuccessor+() = succ | not matchesEpsilon(succ)) andIf I understand correctly, we use If the taint source is found to be a |
Correct. The regexp engine will then try all of the N^2 sequences that lead to that unsatisfiable |
I'll try to do that.
I've found the |
I've done it now. But I'm not proud of the implementation. I did the pruning of results in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Putting the check in the where clause is exactly what I had in mind.
Just a minor typo otherwise looks good to merge.
javascript/ql/src/semmle/javascript/security/performance/SuperlinearBackTracking.qll
Outdated
Show resolved
Hide resolved
…linearBackTracking.qll Co-authored-by: Asger F <asgerf@github.com>


Recognizes the sink in CVE-2018-20801.
An evaluation shows good performance, but the two new results are both FPs.
However, they are only FPs because the input cannot contain newlines, so I'm not sure how we should handle that.
(We already had such FPs, see line 7, 20, 25, and 30 of the existing test case, the only difference is that we now have more).