Python: ReDoS conservative

[yoff]

Just enough to get going...

[nickrolfe]

I have a suggestion for parsing backreferences correctly and not as normal characters.

diff --git a/python/ql/src/semmle/python/regex.qll b/python/ql/src/semmle/python/regex.qll
index e35a373016..36abe17424 100644
--- a/python/ql/src/semmle/python/regex.qll
+++ b/python/ql/src/semmle/python/regex.qll
@@ -368,7 +368,8 @@ abstract class RegexString extends Expr {
       or
       this.escapedCharacter(start, end)
     ) and
-    not exists(int x, int y | this.group_start(x, y) and x <= start and y >= end)
+    not exists(int x, int y | this.group_start(x, y) and x <= start and y >= end) and
+    not exists(int x, int y | this.backreference(x, y) and x <= start and y >= end)
   }
 
   predicate normalCharacter(int start, int end) {
@@ -650,6 +651,8 @@ abstract class RegexString extends Expr {
     this.group(start, end)
     or
     this.charSet(start, end)
+    or
+    this.backreference(start, end)
   }
 
   private predicate qualifier(int start, int end, boolean maybe_empty, boolean may_repeat_forever) {
@@ -748,7 +751,8 @@ abstract class RegexString extends Expr {
   private predicate item_start(int start) {
     this.character(start, _) or
     this.isGroupStart(start) or
-    this.charSet(start, _)
+    this.charSet(start, _) or
+    this.backreference(start, _)
   }
 
   private predicate item_end(int end) {

[nickrolfe]

Do I understand correctly that this doesn't attempt to strip whitespace and comments in VERBOSE regexes? (Ruby has a similar mechanism with the x flag).

[yoff]

Do I understand correctly that this doesn't attempt to strip whitespace and comments in VERBOSE regexes? (Ruby has a similar mechanism with the x flag).

Yes, I believe no such attempt is made. In fact we currently exclude regexes in verbose mode, which is of course not desirable.

[yoff]

Your suggestion regarding back references looks reasonable. In fact I found the whole character/normalchar code a little fuzzy...

[erik-krogh]

If we get a toUnicode() method, then here how that could be used to implement getValue() for escaped unicode chars.

[erik-krogh]

If we get a toUnicode() method, then here how that could be used to implement getValue() for escaped unicode chars.

I've pushed the unicode parsing to this PR.
So you now need a freshly build CLI to run the code in this PR.

[nickrolfe]

Here's what I believe is a false positive:

r"\A(?:\w|\w-\w|\n|\t)+\z"
    ^-----------------^

This part of the regular expression may cause exponential backtracking on strings starting with 'A' and containing many repetitions of 't'.

I guess what's happening is that it thinks t will match \t as well as \w. It's also suggesting the prefix A, which I think means it's confused about \A.

[yoff]

Here's what I believe is a false positive:

r"\A(?:\w|\w-\w|\n|\t)+\z"
    ^-----------------^

This part of the regular expression may cause exponential backtracking on strings starting with 'A' and containing many repetitions of 't'.

I guess what's happening is that it thinks t will match \t as well as \w. It's also suggesting the prefix A, which I think means it's confused about \A.

I think you are right. Some escapes code for themselves while others do not. We currently have too many in the first group, it seems (everyone except \n and \r):

  override string getValue() {
    this.isIdentityEscape() and result = this.getUnescaped()
    or
    this.getUnescaped() = "n" and result = "\n"
    or
    this.getUnescaped() = "r" and result = "\r"
    or
    isUnicode() and
    result = getUnicode()
  }

  predicate isIdentityEscape() { not this.getUnescaped() in ["n", "r"] }

[yoff]

Superseded by #6175.