Merge pull request github#6460 from yoff/python-regex-parsing-consistency-checks

Python: Add regex parsing consistency checks

Author: tausbn

python/ql/lib/semmle/python/RegexTreeView.qll

@@ -7,16 +7,29 @@ private import semmle.python.regex
  * An element containing a regular expression term, that is, either
  * a string literal (parsed as a regular expression)
  * or another regular expression term.
+ *
+ * For sequences and alternations, we require at least one child.
+ * Otherwise, we wish to represent the term differently.
+ * This avoids multiple representations of the same term.
  */
 newtype TRegExpParent =
   /** A string literal used as a regular expression */
   TRegExpLiteral(Regex re) or
   /** A quantified term */
   TRegExpQuantifier(Regex re, int start, int end) { re.qualifiedItem(start, end, _, _) } or
   /** A sequence term */
-  TRegExpSequence(Regex re, int start, int end) { re.sequence(start, end) } or
-  /** An alternatio term */
-  TRegExpAlt(Regex re, int start, int end) { re.alternation(start, end) } or
+  TRegExpSequence(Regex re, int start, int end) {
+    re.sequence(start, end) and
+    exists(seqChild(re, start, end, 1)) // if a sequence does not have more than one element, it should be treated as that element instead.
+  } or
+  /** An alternation term */
+  TRegExpAlt(Regex re, int start, int end) {
+    re.alternation(start, end) and
+    exists(int part_end |
+      re.alternationOption(start, end, start, part_end) and
+      part_end < end
+    ) // if an alternation does not have more than one element, it should be treated as that element instead.
+  } or
   /** A character class term */
   TRegExpCharacterClass(Regex re, int start, int end) { re.charSet(start, end) } or
   /** A character range term */
@@ -93,8 +106,7 @@ class RegExpTerm extends RegExpParent {
     or
     this = TRegExpQuantifier(re, start, end)
     or
-    this = TRegExpSequence(re, start, end) and
-    exists(seqChild(re, start, end, 1)) // if a sequence does not have more than one element, it should be treated as that element instead.
+    this = TRegExpSequence(re, start, end)
     or
     this = TRegExpSpecialChar(re, start, end)
   }
@@ -341,10 +353,7 @@ class RegExpRange extends RegExpQuantifier {
  * This is a sequence with the elements `(ECMA|Java)` and `Script`.
  */
 class RegExpSequence extends RegExpTerm, TRegExpSequence {
-  RegExpSequence() {
-    this = TRegExpSequence(re, start, end) and
-    exists(seqChild(re, start, end, 1)) // if a sequence does not have more than one element, it should be treated as that element instead.
-  }
+  RegExpSequence() { this = TRegExpSequence(re, start, end) }
 
   override RegExpTerm getChild(int i) { result = seqChild(re, start, end, i) }
 

python/ql/lib/semmle/python/regex.qll

@@ -369,12 +369,12 @@ abstract class RegexString extends Expr {
       // hex value \xhh
       this.getChar(start + 1) = "x" and end = start + 4
       or
-      // octal value \ooo
+      // octal value \o, \oo, or \ooo
       end in [start + 2 .. start + 4] and
-      this.getText().substring(start + 1, end).toInt() >= 0 and
+      forall(int i | i in [start + 1 .. end - 1] | this.isOctal(i)) and
       not (
         end < start + 4 and
-        exists(this.getText().substring(start + 1, end + 1).toInt())
+        this.isOctal(end)
       )
       or
       // 16-bit hex value \uhhhh
@@ -392,6 +392,9 @@ abstract class RegexString extends Expr {
     )
   }
 
+  pragma[inline]
+  private predicate isOctal(int index) { this.getChar(index) = [0 .. 7].toString() }
+
   /** Holds if `index` is inside a character set. */
   predicate inCharSet(int index) {
     exists(int x, int y | this.charSet(x, y) and index in [x + 1 .. y - 2])
@@ -690,6 +693,7 @@ abstract class RegexString extends Expr {
 
   private predicate numbered_backreference(int start, int end, int value) {
     this.escapingChar(start) and
+    // starting with 0 makes it an octal escape
     not this.getChar(start + 1) = "0" and
     exists(string text, string svalue, int len |
       end = start + len and
@@ -698,8 +702,16 @@ abstract class RegexString extends Expr {
     |
       svalue = text.substring(start + 1, start + len) and
       value = svalue.toInt() and
-      not exists(text.substring(start + 1, start + len + 1).toInt()) and
-      value > 0
+      // value is composed of digits
+      forall(int i | i in [start + 1 .. start + len - 1] | this.getChar(i) = [0 .. 9].toString()) and
+      // a longer reference is not possible
+      not (
+        len = 2 and
+        exists(text.substring(start + 1, start + len + 1).toInt())
+      ) and
+      // 3 octal digits makes it an octal escape
+      not forall(int i | i in [start + 1 .. start + 4] | this.isOctal(i))
+      // TODO: Inside a character set, all numeric escapes are treated as characters.
     )
   }
 

python/ql/test/library-tests/regex/Alternation.expected

@@ -19,4 +19,4 @@
 | x\| | 0 | 2 | x\| | 0 | 1 | x |
 | x\| | 0 | 2 | x\| | 2 | 2 |  |
 | x\|(?<!\\w)l | 0 | 10 | x\|(?<!\\w)l | 0 | 1 | x |
-| x\|(?<!\\w)l | 0 | 10 | x\|(?<!\\w)l | 2 | 10 | (?<!\\w)l |
+| x\|(?<!\\w)l | 0 | 10 | x\|(?<!\\w)l | 2 | 10 | (?<!\\w)l |

python/ql/test/library-tests/regex/Characters.expected

@@ -52,6 +52,8 @@
 | [^A-Z] | 2 | 3 |
 | [^A-Z] | 4 | 5 |
 | [^]] | 2 | 3 |
+| \\+0 | 0 | 2 |
+| \\+0 | 2 | 3 |
 | \\A[+-]?\\d+ | 0 | 2 |
 | \\A[+-]?\\d+ | 3 | 4 |
 | \\A[+-]?\\d+ | 4 | 5 |

python/ql/test/library-tests/regex/Consistency.expected

@@ -0,0 +1,12 @@
+/**
+ * Flags regular expressions that are parsed ambigously
+ */
+
+import python
+import semmle.python.regex
+
+from string str, Location loc, int counter
+where
+  counter = strictcount(Regex term | term.getLocation() = loc and term.getText() = str) and
+  counter > 1
+select str, counter, loc

python/ql/test/library-tests/regex/Consistency.ql

@@ -42,6 +42,8 @@
 | [^A-Z] | last | 0 | 6 |
 | [^]] | first | 0 | 4 |
 | [^]] | last | 0 | 4 |
+| \\+0 | first | 0 | 2 |
+| \\+0 | last | 2 | 3 |
 | \\A[+-]?\\d+ | first | 0 | 2 |
 | \\A[+-]?\\d+ | last | 7 | 9 |
 | \\A[+-]?\\d+ | last | 7 | 10 |

python/ql/test/library-tests/regex/FirstLast.expected

@@ -113,6 +113,9 @@
 | [^]] | char | 2 | 3 |
 | [^]] | char-set | 0 | 4 |
 | [^]] | sequence | 0 | 4 |
+| \\+0 | char | 0 | 2 |
+| \\+0 | char | 2 | 3 |
+| \\+0 | sequence | 0 | 3 |
 | \\A[+-]?\\d+ | char | 0 | 2 |
 | \\A[+-]?\\d+ | char | 3 | 4 |
 | \\A[+-]?\\d+ | char | 4 | 5 |

python/ql/test/library-tests/regex/Regex.expected

@@ -24,7 +24,8 @@
 
 re.compile(r'[^A-Z]') #$ charRange=2:3-4:5
 
-re.compile(r'[\0-\09]') #$ charRange=1:3-4:7
+re.compile(r'[\0-\09]') #$ charRange=1:3-4:6
+re.compile(r'[\0-\07]') #$ charRange=1:3-4:7
 
 re.compile(r'[\0123-5]') #$ charRange=5:6-7:8
 

python/ql/test/library-tests/regex/charRangeTest.py

@@ -10,8 +10,10 @@
 re.compile(r'[--\-]') #$ escapedCharacter=3:5
 re.compile(r'[\--\-]') #$ escapedCharacter=1:3 escapedCharacter=4:6
 re.compile(r'[0\-9-A-Z]') #$ escapedCharacter=2:4
-re.compile(r'[\0-\09]') #$ escapedCharacter=1:3 escapedCharacter=4:7
+re.compile(r'[\0-\09]') #$ escapedCharacter=1:3 escapedCharacter=4:6
+re.compile(r'[\0-\07]') #$ escapedCharacter=1:3 escapedCharacter=4:7
 re.compile(r'[\0123-5]') #$ escapedCharacter=1:5
+re.compile(r'\1754\1854\17\18\07\08') #$ escapedCharacter=0:4 escapedCharacter=16:19 escapedCharacter=19:21
 
 #ODASA-3985
 #Half Surrogate pairs
@@ -21,3 +23,9 @@
 
 #Misparsed on LGTM
 re.compile(r"\[(?P<txt>[^[]*)\]\((?P<uri>[^)]*)") #$ escapedCharacter=0:2 escapedCharacter=16:18 escapedCharacter=18:20
+
+#Non-raw string
+re_blank = re.compile('(\n|\r|\\s)*\n', re.M) #$ escapedCharacter=5:7
+
+#Backreference confusion
+re.compile(r'\+0') #$ escapedCharacter=0:2