Refactorizations of the ReDoS libraries #9422

erik-krogh · 2022-06-02T15:05:23Z

Many things are happening in this PR, all of which are outlined below.

Commit-by-commit review is highly recommended.
All the tests should pass on every commit.

1: Disable parts of QL-for-QL

The consistency and ql/dead-code queries are very noisy until we have proper support for parameterized modules.
So I've disabled those.
No longer needed now that #9575 has been merged.

2: Introduce a `ReDoSPruning` parameterized module

The class Config extends string pattern used previously was ugly.
This is just a straight-forward conversion of that pattern into a parameterized module.

3: Implement a more efficient algorithm for computing strings from chains

Based on @hvitved's work: #8589

But moved into a parameterized module in order to share more code between exponential and superlinear ReDoS.
It does the same thing as @hvitved's implementation, but in a slightly different way (to make a pretty signature module).

4: add further normalization of char classses

Inspired by @esbena: #4847 (comment)

Now \d and [\d] and [0-9] will literally be represented with a single value.
(an arbitrary one of them is chosen as the canonical representative).

The extra normalization has a real, but limited, impact.
The largest impact is on the size of the SuperlinearBackTracking::getAThreewayIntersect predicate, the size of that predicate drops by ~15% on large JS benchmarks.

I also drive-by fixed a bug related to case-normalization.
(The bug didn't affect anything before the changes in this PR).

5: make a parameterized module out of the RegexpMatching implementation

I didn't like the configuration pattern used previously, which was also why I hadn't made a real library out of it.
It can be better with parameterized modules, so I did that.

6: Rename `ReDoSUtil` to `NfaUtils`, and rename the `performance` folder to `regexp`.

The ReDoSUtil file was used for more than just ReDoS: it's a general library for constructing and reasoning about NFAs.
And the performance folder was also a bad name, so I renamed it to regexp.

7: Improve performance.

Lots of states in the process() predicate could be eliminated.
Because we know that if a regexp ever "reaches" the accept-all state, then it can never be rejected, and thus we can skip them early.

I've also ported this exponential-redos fix from Shack to the superlinear library.

Evaluations.

All look good, and mostly show a significant speedup (except for Python, which has neutral performance).
JavaScript, Ruby, Java, Python.

Previous version of this PR, which got filled up with noise: #8522

aschackmull · 2022-06-13T10:29:49Z

Ping @ginsbach : Parameterised modules in here.

ginsbach · 2022-06-13T13:45:00Z

Thanks for pinging. The use of parameterised modules here looks good to me, no concerns!

erik-krogh · 2022-06-17T13:10:19Z

I've rebased on the latest main now that QL-for-QL supports parameterized modules: #9575

I've also re-enabled the QL-for-QL consistency- and dead-code-query in this PR.

java/ql/lib/semmle/code/java/security/regexp/NfaUtils.qll

…odule

…"regexp"

…ttern for js/polynomial-redos

erik-krogh · 2022-08-09T14:25:55Z

I did a merge with main to fix a merge conflict.

@github/codeql-python can I get a review?

github-advanced-security

Found 27 potential problems in the proposed changes. Check the Files changed tab for more details.

yoff

I really love this work (although it could arguably have been in several PRs).

The requested changes are all minor.

yoff · 2022-08-12T08:09:58Z

python/ql/lib/semmle/python/security/regexp/NfaUtils.qll

+     */
+    pragma[noinline]
+    private predicate isLikelyRejectable(State s) {
+      s = stateInPumpableRegexp() and


I suppose we cannot define classes inside parameterised modules yet?

Yep.

I can only extend database types inside a parameterised module.
And that's not feasible here.

yoff · 2022-08-12T08:13:43Z

python/ql/lib/semmle/python/security/regexp/SuperlinearBackTracking.qll

+/**
+ * Holds if repetitions of `pump` at `t` will cause polynomial backtracking.
+ */
+predicate polynimalReDoS(RegExpTerm t, string pump, string prefixMsg, RegExpTerm prev) {


This predicate should probably be renamed to polynomialReDoS everywhere. Although, while the code looks new here, I think this name was not introduced in this PR.

yoff · 2022-08-12T08:29:54Z

python/ql/lib/semmle/python/security/BadTagFilterQuery.qll

-/** DEPRECATED: Alias for HtmlMatchingRegExp */
-deprecated class HTMLMatchingRegExp = HtmlMatchingRegExp;


Are we OK to delete this?

No, I'll reintroduce it.
It's in a Query.qll so it's likely fine, but I'll get it back anyway.

yoff · 2022-08-12T08:33:33Z

python/ql/lib/semmle/python/security/regexp/RegexpMatching.qll

+    (
+      test(reg, str, ignorePrefix)
+      or
+      testWithGroups(reg, str, ignorePrefix)
+    ) and


Suggested change

(

test(reg, str, ignorePrefix)

or

testWithGroups(reg, str, ignorePrefix)

) and

isCandidate(reg, str, ignorePrefix, _)

and

yoff · 2022-08-12T08:36:32Z

python/ql/lib/semmle/python/security/regexp/RegexpMatching.qll

+ * A module for determining if a regexp matches a given string,
+ * and reasoning about which capture groups are filled by a given string.
+ *
+ * The module parameter `isRegexpMatchingCandidateSig` determines which string should be tested,


Suggested change

* The module parameter `isRegexpMatchingCandidateSig` determines which string should be tested,

* The module parameter `isCandidate` determines which strings should be tested,

yoff · 2022-08-12T09:04:34Z

python/ql/lib/semmle/python/security/regexp/RegexpMatching.qll

+  }
+
+  /**
+   * Holds if `reg` matches `str`, where `str` is either in the `test` or `testWithGroups` predicate.


Suggested change

* Holds if `reg` matches `str`, where `str` is either in the `test` or `testWithGroups` predicate.

* Holds if `reg` matches `str`, where `str` is in the `isCandidate` predicate.

yoff · 2022-08-12T09:17:36Z

python/ql/lib/semmle/python/security/regexp/NfaUtils.qll

+  private predicate isRelevant(Node n) {
+    isARelevantEnd(n)
+    or
+    exists(Node prev | isRelevant(prev) | n = getPrev(prev))


Suggested change

exists(Node prev | isRelevant(prev) | n = getPrev(prev))

exists(Node succ | isRelevant(succ) | n = getPrev(succ))

yoff · 2022-08-12T09:24:42Z

python/ql/lib/semmle/python/security/regexp/NfaUtils.qll

+  /** Gets an ancestor of `end`, where `end` is a node that should have a result in `concretize`. */
+  private Node getANodeInLongChain(Node end) {


Could this just be called getAnAncestor? And could it be written via prev*?

Maybe it can be written like that, I don't remember if I wrote the recursion manually for a reason 🤷

I'll do another evaluation to check.

yoff · 2022-08-12T09:31:03Z

python/ql/lib/semmle/python/security/regexp/NfaUtils.qll

+  /** Gets the `i`th character on the path from the root to `n`. */
+  pragma[noinline]
+  private string getPrefixChar(Node n, int i) {
+    exists(Node prev |


Maybe rename prev to ancestor (or an abbreviation thereof)

yoff · 2022-08-12T09:38:29Z

python/ql/lib/semmle/python/security/regexp/NfaUtils.qll

+    predicate reachesOnlyRejectableSuffixes(State fork, string w) {
+      isReDoSCandidate(fork, w) and
+      forex(State next | next = process(fork, w, w.length() - 1) | isLikelyRejectable(next)) and
+      not epsilonSucc*(getProcessPrevious(fork, _, w)) = AcceptAnySuffix(_) // we stop `process(..)` early if we can, check here if it happened.


I think you can rewrite it like this

Suggested change

not epsilonSucc*(getProcessPrevious(fork, _, w)) = AcceptAnySuffix(_) // we stop `process(..)` early if we can, check here if it happened.

not getProcessPrevious(fork, _, w) = acceptsAnySuffix() // we stop `process(..)` early if we can, check here if it happened.

github-advanced-security

Found 27 potential problems in the proposed changes. Check the Files changed tab for more details.

erik-krogh · 2022-08-12T18:42:01Z

@yoff I think I've addressed your review comments, and I also addressed some additional documentation issues identified by QL-for-QL.

The remaining QL-for-QL alerts are false positives that will be fixed by #9884

I've started new evaluations for JavaScript, Python, Java, and Ruby.

github-advanced-security

Found 20 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 27 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 20 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 27 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 20 potential problems in the proposed changes. Check the Files changed tab for more details.

yoff · 2022-08-13T09:45:05Z

Yep, looks great :-)

github-advanced-security

Found 27 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 20 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 20 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 27 potential problems in the proposed changes. Check the Files changed tab for more details.

yoff

Lgtm

erik-krogh · 2022-08-14T12:52:08Z

I've started new evaluations for JavaScript, Python, Java, and Ruby.

The evaluations are all looking good.

github-advanced-security

Found 20 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 27 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 20 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 27 potential problems in the proposed changes. Check the Files changed tab for more details.

erik-krogh requested review from a team as code owners June 2, 2022 15:05

github-actions bot added documentation Java JS Python QL-for-QL Ruby labels Jun 2, 2022

calumgrant assigned alexrford Jun 13, 2022

aschackmull requested a review from ginsbach June 13, 2022 10:30

calumgrant requested a review from tausbn June 14, 2022 08:18

erik-krogh force-pushed the refacReDoS branch from f5c729c to 3540c3c Compare June 17, 2022 13:08

joefarebrother reviewed Jun 17, 2022

View reviewed changes

java/ql/lib/semmle/code/java/security/regexp/NfaUtils.qll Outdated Show resolved Hide resolved

github-actions bot removed the QL-for-QL label Jun 17, 2022

erik-krogh added 10 commits June 23, 2022 14:36

move predicates that depend on isReDoSCandidate into a ReDoSPruning m…

dc06e9d

…odule

make ReDoSPruning into a parameterized module

3be4a86

add further normalization of char classses

7fb3d81

make a parameterized module out of the RegexpMatching implementation

dbeae9a

refactor the concretize algorithm

6b0df9b

update expected output from disabling the dead-code query

bbc8cb3

rename ReDoSUtil to NfaUtils, and rename the "performance" folder to …

13482fc

…"regexp"

move the JS ReDoS test to a more appropriate folder

2e4c2df

add deprecated aliases in the old locations, and use the Query.qll pa…

3bea7df

…ttern for js/polynomial-redos

add missing qldoc

14204be

move tests to new test location

da4da22

github-advanced-security bot found potential problems Aug 9, 2022

View reviewed changes

yoff requested changes Aug 12, 2022

View reviewed changes

erik-krogh added 4 commits August 12, 2022 20:25

apply suggestions from code review

3403e2f

simplify code after review

97681ea

sync changes to other languages

b9e96fb

Merge branch 'main' into refacReDoS

b54f037

github-advanced-security bot found potential problems Aug 12, 2022

View reviewed changes

fix some QL-for-QL warnings

3a4a343

erik-krogh requested a review from yoff August 12, 2022 18:39

github-advanced-security bot found potential problems Aug 12, 2022

View reviewed changes

github-advanced-security bot found potential problems Aug 13, 2022

View reviewed changes

github-advanced-security bot found potential problems Aug 14, 2022

View reviewed changes

yoff approved these changes Aug 14, 2022

View reviewed changes

github-advanced-security bot found potential problems Aug 15, 2022

View reviewed changes

erik-krogh merged commit f106e06 into github:main Aug 16, 2022

github deleted a comment Sep 10, 2024

		/** DEPRECATED: Alias for HtmlMatchingRegExp */
		deprecated class HTMLMatchingRegExp = HtmlMatchingRegExp;

	* The module parameter `isRegexpMatchingCandidateSig` determines which string should be tested,
	* The module parameter `isCandidate` determines which strings should be tested,

	* Holds if `reg` matches `str`, where `str` is either in the `test` or `testWithGroups` predicate.
	* Holds if `reg` matches `str`, where `str` is in the `isCandidate` predicate.

	exists(Node prev \| isRelevant(prev) \| n = getPrev(prev))
	exists(Node succ \| isRelevant(succ) \| n = getPrev(succ))

		/** Gets an ancestor of `end`, where `end` is a node that should have a result in `concretize`. */
		private Node getANodeInLongChain(Node end) {

	not epsilonSucc*(getProcessPrevious(fork, _, w)) = AcceptAnySuffix(_) // we stop `process(..)` early if we can, check here if it happened.
	not getProcessPrevious(fork, _, w) = acceptsAnySuffix() // we stop `process(..)` early if we can, check here if it happened.

Refactorizations of the ReDoS libraries #9422

Refactorizations of the ReDoS libraries #9422

Uh oh!

Conversation

erik-krogh commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1: Disable parts of QL-for-QL

2: Introduce a ReDoSPruning parameterized module

3: Implement a more efficient algorithm for computing strings from chains

4: add further normalization of char classses

5: make a parameterized module out of the RegexpMatching implementation

6: Rename ReDoSUtil to NfaUtils, and rename the performance folder to regexp.

7: Improve performance.

Evaluations.

Uh oh!

aschackmull commented Jun 13, 2022

Uh oh!

ginsbach commented Jun 13, 2022

Uh oh!

erik-krogh commented Jun 17, 2022

Uh oh!

Uh oh!

erik-krogh commented Aug 9, 2022

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

yoff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erik-krogh Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

erik-krogh commented Aug 12, 2022

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

yoff commented Aug 13, 2022

Uh oh!

erik-krogh commented Jun 2, 2022 •

edited

Loading

2: Introduce a `ReDoSPruning` parameterized module

6: Rename `ReDoSUtil` to `NfaUtils`, and rename the `performance` folder to `regexp`.

erik-krogh Aug 12, 2022 •

edited

Loading