From bb562643c6d00dd5ff8ceee5ec14679df3018c52 Mon Sep 17 00:00:00 2001
From: Joe Farebrother <joefarebrother@github.com>
Date: Tue, 8 Mar 2022 16:47:39 +0000
Subject: [PATCH] Support possessive quantifiers, which cannot backtrack.

They are approximated by limiting them to up to one repetition (effectively making *+ like ? and ++ like a no-op).
---
 .../ql/lib/semmle/code/java/regex/RegexTreeView.qll | 13 +++++++++----
 .../code/java/security/performance/ReDoSUtil.qll    |  9 +++++++--
 .../java/security/performance/RegExpTreeView.qll    |  5 +++++
 .../query-tests/security/CWE-730/ExpRedosTest.java  | 11 +++++++++++
 4 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/java/ql/lib/semmle/code/java/regex/RegexTreeView.qll b/java/ql/lib/semmle/code/java/regex/RegexTreeView.qll
index a7e3928f085..27074913a7e 100644
--- a/java/ql/lib/semmle/code/java/regex/RegexTreeView.qll
+++ b/java/ql/lib/semmle/code/java/regex/RegexTreeView.qll
@@ -253,7 +253,12 @@ class RegExpQuantifier extends RegExpTerm, TRegExpQuantifier {
   predicate mayRepeatForever() { may_repeat_forever = true }
 
   /** Gets the quantifier for this term. That is e.g "?" for "a?". */
-  string getquantifier() { result = re.getText().substring(part_end, end) }
+  string getQuantifier() { result = re.getText().substring(part_end, end) }
+
+  /** Holds if this is a possessive quantifier, e.g. a*+. */
+  predicate isPossessive() {
+    exists(string q | q = this.getQuantifier() | q.length() > 1 and q.charAt(q.length() - 1) = "+")
+  }
 
   override string getPrimaryQLClass() { result = "RegExpQuantifier" }
 }
@@ -275,7 +280,7 @@ class InfiniteRepetitionQuantifier extends RegExpQuantifier {
  * ```
  */
 class RegExpStar extends InfiniteRepetitionQuantifier {
-  RegExpStar() { this.getquantifier().charAt(0) = "*" }
+  RegExpStar() { this.getQuantifier().charAt(0) = "*" }
 
   override string getPrimaryQLClass() { result = "RegExpStar" }
 }
@@ -290,7 +295,7 @@ class RegExpStar extends InfiniteRepetitionQuantifier {
  * ```
  */
 class RegExpPlus extends InfiniteRepetitionQuantifier {
-  RegExpPlus() { this.getquantifier().charAt(0) = "+" }
+  RegExpPlus() { this.getQuantifier().charAt(0) = "+" }
 
   override string getPrimaryQLClass() { result = "RegExpPlus" }
 }
@@ -305,7 +310,7 @@ class RegExpPlus extends InfiniteRepetitionQuantifier {
  * ```
  */
 class RegExpOpt extends RegExpQuantifier {
-  RegExpOpt() { this.getquantifier().charAt(0) = "?" }
+  RegExpOpt() { this.getQuantifier().charAt(0) = "?" }
 
   override string getPrimaryQLClass() { result = "RegExpOpt" }
 }
diff --git a/java/ql/lib/semmle/code/java/security/performance/ReDoSUtil.qll b/java/ql/lib/semmle/code/java/security/performance/ReDoSUtil.qll
index 824eb9de7ae..f0e26580158 100644
--- a/java/ql/lib/semmle/code/java/security/performance/ReDoSUtil.qll
+++ b/java/ql/lib/semmle/code/java/security/performance/ReDoSUtil.qll
@@ -608,10 +608,15 @@ State after(RegExpTerm t) {
   or
   exists(RegExpGroup grp | t = grp.getAChild() | result = after(grp))
   or
-  exists(EffectivelyStar star | t = star.getAChild() | result = before(star))
+  exists(EffectivelyStar star | t = star.getAChild() |
+    not isPossessive(star) and
+    result = before(star)
+  )
   or
   exists(EffectivelyPlus plus | t = plus.getAChild() |
-    result = before(plus) or
+    not isPossessive(plus) and
+    result = before(plus)
+    or
     result = after(plus)
   )
   or
diff --git a/java/ql/lib/semmle/code/java/security/performance/RegExpTreeView.qll b/java/ql/lib/semmle/code/java/security/performance/RegExpTreeView.qll
index 608c03d006a..f59b1f43ca9 100644
--- a/java/ql/lib/semmle/code/java/security/performance/RegExpTreeView.qll
+++ b/java/ql/lib/semmle/code/java/security/performance/RegExpTreeView.qll
@@ -16,6 +16,11 @@ predicate isEscapeClass(RegExpTerm term, string clazz) {
   term.(RegExpNamedProperty).getBackslashEquivalent() = clazz
 }
 
+/**
+ * Holds if `term` is a possessive quantifier, e.g. `a*+`.
+ */
+predicate isPossessive(RegExpQuantifier term) { term.isPossessive() }
+
 /**
  * Holds if the regular expression should not be considered.
  *
diff --git a/java/ql/test/query-tests/security/CWE-730/ExpRedosTest.java b/java/ql/test/query-tests/security/CWE-730/ExpRedosTest.java
index a9fc19e5d50..e7e876cb696 100644
--- a/java/ql/test/query-tests/security/CWE-730/ExpRedosTest.java
+++ b/java/ql/test/query-tests/security/CWE-730/ExpRedosTest.java
@@ -418,6 +418,17 @@ class ExpRedosTest {
         "\\A(\\d|0)*x", // $ hasExpRedos
         "(\\d|0)*\\Z", // $ hasExpRedos
         "\\b(\\d|0)*x", // $ hasExpRedos
+
+        // GOOD - possessive quantifiers don't backtrack
+        "(a*+)*+b",
+        "(a*)*+b",
+        "(a*+)*b",
+
+        // BAD
+        "(a*)*b", // $ hasExpRedos
+
+        // BAD - but not detected due to the way possessive quantifiers are approximated
+        "((aa|a*+)b)*c" // $ MISSING: hasExpRedos
     };
 
     void test() {