JS: Restrict RegExp queries to actual regular expressions

asger-semmle · asger-semmle · commit c21d095d3850 · 2019-11-15T09:27:19.000Z
diff --git a/javascript/ql/src/Performance/ReDoS.ql b/javascript/ql/src/Performance/ReDoS.ql
@@ -108,11 +108,7 @@ class RegExpRoot extends RegExpTerm {
     // there are no lookbehinds
     not exists(RegExpLookbehind lbh | getRoot(lbh) = this) and
     // is actually used as a RegExp
-    (
-      parent instanceof RegExpLiteral
-      or
-      parent.(StringLiteral).flow() instanceof RegExpPatternSource
-    )
+    isUsedAsRegExp()
   }
 }
 
diff --git a/javascript/ql/src/RegExp/BackrefBeforeGroup.ql b/javascript/ql/src/RegExp/BackrefBeforeGroup.ql
@@ -16,5 +16,6 @@ import javascript
 from RegExpBackRef rebr
 where
   rebr.getLocation().getStartColumn() < rebr.getGroup().getLocation().getEndColumn() and
-  not rebr.isInBackwardMatchingContext()
+  not rebr.isInBackwardMatchingContext() and
+  rebr.isPartOfRegExpLiteral()
 select rebr, "This back reference precedes its capture group."
diff --git a/javascript/ql/src/RegExp/BackrefIntoNegativeLookahead.ql b/javascript/ql/src/RegExp/BackrefIntoNegativeLookahead.ql
@@ -17,7 +17,8 @@ from RegExpNegativeLookahead neg, RegExpGroup grp, RegExpBackRef back
 where
   grp.getParent+() = neg and
   grp = back.getGroup() and
-  not back.getParent+() = neg
+  not back.getParent+() = neg and
+  neg.isPartOfRegExpLiteral()
 select back,
   "This back reference always matches the empty string, since it refers to $@, which is contained in $@.",
   grp, "this capture group", neg, "a negative lookahead assertion"
diff --git a/javascript/ql/src/RegExp/BackspaceEscape.ql b/javascript/ql/src/RegExp/BackspaceEscape.ql
@@ -15,4 +15,5 @@ import javascript
 
 from RegExpCharEscape rece
 where rece.toString() = "\\b"
+  and rece.isPartOfRegExpLiteral()
 select rece, "Backspace escape in regular expression."
diff --git a/javascript/ql/src/RegExp/DuplicateCharacterInCharacterClass.ql b/javascript/ql/src/RegExp/DuplicateCharacterInCharacterClass.ql
@@ -29,6 +29,7 @@ from RegExpCharacterClass recc, RegExpConstant first, RegExpConstant repeat, int
 where
   constantInCharacterClass(recc, 1, first, val) and
   constantInCharacterClass(recc, rnk, repeat, val) and
-  rnk > 1
+  rnk > 1 and
+  recc.isPartOfRegExpLiteral()
 select first, "Character '" + first + "' is repeated $@ in the same character class.", repeat,
   "here"
diff --git a/javascript/ql/src/RegExp/EmptyCharacterClass.ql b/javascript/ql/src/RegExp/EmptyCharacterClass.ql
@@ -15,5 +15,6 @@ import javascript
 from RegExpCharacterClass recc
 where
   not exists(recc.getAChild()) and
-  not recc.isInverted()
+  not recc.isInverted() and
+  recc.isPartOfRegExpLiteral()
 select recc, "Empty character class."
diff --git a/javascript/ql/src/RegExp/IdentityReplacement.ql b/javascript/ql/src/RegExp/IdentityReplacement.ql
@@ -31,36 +31,39 @@ predicate matchesString(Expr e, string s) {
  */
 language[monotonicAggregates]
 predicate regExpMatchesString(RegExpTerm t, string s) {
-  // constants match themselves
-  s = t.(RegExpConstant).getValue()
-  or
-  // assertions match the empty string
+  t.isPartOfRegExpLiteral() and
   (
-    t instanceof RegExpCaret or
-    t instanceof RegExpDollar or
-    t instanceof RegExpWordBoundary or
-    t instanceof RegExpNonWordBoundary or
-    t instanceof RegExpLookahead or
-    t instanceof RegExpLookbehind
-  ) and
-  s = ""
-  or
-  // groups match their content
-  regExpMatchesString(t.(RegExpGroup).getAChild(), s)
-  or
-  // single-character classes match that character
-  exists(RegExpCharacterClass recc | recc = t and not recc.isInverted() |
-    recc.getNumChild() = 1 and
-    regExpMatchesString(recc.getChild(0), s)
-  )
-  or
-  // sequences match the concatenation of their elements
-  exists(RegExpSequence seq | seq = t |
-    s = concat(int i, RegExpTerm child |
-        child = seq.getChild(i)
-      |
-        any(string subs | regExpMatchesString(child, subs)) order by i
-      )
+    // constants match themselves
+    s = t.(RegExpConstant).getValue()
+    or
+    // assertions match the empty string
+    (
+      t instanceof RegExpCaret or
+      t instanceof RegExpDollar or
+      t instanceof RegExpWordBoundary or
+      t instanceof RegExpNonWordBoundary or
+      t instanceof RegExpLookahead or
+      t instanceof RegExpLookbehind
+    ) and
+    s = ""
+    or
+    // groups match their content
+    regExpMatchesString(t.(RegExpGroup).getAChild(), s)
+    or
+    // single-character classes match that character
+    exists(RegExpCharacterClass recc | recc = t and not recc.isInverted() |
+      recc.getNumChild() = 1 and
+      regExpMatchesString(recc.getChild(0), s)
+    )
+    or
+    // sequences match the concatenation of their elements
+    exists(RegExpSequence seq | seq = t |
+      s = concat(int i, RegExpTerm child |
+          child = seq.getChild(i)
+        |
+          any(string subs | regExpMatchesString(child, subs)) order by i
+        )
+    )
   )
 }
 
diff --git a/javascript/ql/src/RegExp/UnboundBackref.ql b/javascript/ql/src/RegExp/UnboundBackref.ql
@@ -16,6 +16,7 @@ import javascript
 
 from RegExpBackRef rebr, string ref
 where
+  rebr.isPartOfRegExpLiteral() and
   not exists(rebr.getGroup()) and
   (
     ref = rebr.getNumber().toString()
diff --git a/javascript/ql/src/RegExp/UnmatchableCaret.ql b/javascript/ql/src/RegExp/UnmatchableCaret.ql
@@ -17,6 +17,7 @@ import javascript
 
 from RegExpCaret caret, RegExpTerm t
 where
+  caret.isPartOfRegExpLiteral() and
   t = caret.getPredecessor+() and
   not t.isNullable() and
   // conservative handling of multi-line regular expressions
diff --git a/javascript/ql/src/RegExp/UnmatchableDollar.ql b/javascript/ql/src/RegExp/UnmatchableDollar.ql
@@ -17,6 +17,7 @@ import javascript
 
 from RegExpDollar dollar, RegExpTerm t
 where
+  dollar.isPartOfRegExpLiteral() and
   t = dollar.getSuccessor+() and
   not t.isNullable() and
   // conservative handling of multi-line regular expressions
diff --git a/javascript/ql/src/semmle/javascript/Regexp.qll b/javascript/ql/src/semmle/javascript/Regexp.qll
@@ -53,7 +53,7 @@ class RegExpTerm extends Locatable, @regexpterm {
   RegExpParent getParent() { regexpterm(this, _, result, _, _) }
 
   /** Gets the regular expression literal this term belongs to, if any. */
-  RegExpLiteral getLiteral() { result = getParent+() }
+  RegExpLiteral getLiteral() { result = getRootTerm().getParent() }
 
   override string toString() { regexpterm(this, _, _, _, result) }
 
@@ -112,6 +112,42 @@ class RegExpTerm extends Locatable, @regexpterm {
   predicate isRootTerm() {
     not getParent() instanceof RegExpTerm
   }
+
+  /**
+   * Gets the outermost term of this regular expression.
+   */
+  RegExpTerm getRootTerm() {
+    isRootTerm() and
+    result = this
+    or
+    result = getParent().(RegExpTerm).getRootTerm()
+  }
+
+  /**
+   * Holds if this term occurs as part of a regular expression literal.
+   */
+  predicate isPartOfRegExpLiteral() {
+    exists(getLiteral())
+  }
+
+  /**
+   * Holds if this term occurs as part of a string literal.
+   */
+  predicate isPartOfStringLiteral() {
+    getRootTerm().getParent() instanceof StringLiteral
+  }
+
+  /**
+   * Holds if this term is part of a regular expression literal or a string literal
+   * that is used as a regular expression.
+   */
+  predicate isUsedAsRegExp() {
+    exists(RegExpParent parent | parent = getRootTerm().getParent() |
+      parent instanceof RegExpLiteral
+      or
+      parent.(StringLiteral).flow() instanceof RegExpPatternSource
+    )
+  }
 }
 
 /**
diff --git a/javascript/ql/test/query-tests/RegExp/EmptyCharacterClass/EmptyCharacterClass.expected b/javascript/ql/test/query-tests/RegExp/EmptyCharacterClass/EmptyCharacterClass.expected
@@ -1 +1 @@
-| tst.js:1:2:1:3 | [] | Empty character class. |
+| tst.js:1:2:1:3 | [] | Empty character class. |

Original file line number	Diff line number	Diff line change
`@@ -108,11 +108,7 @@ class RegExpRoot extends RegExpTerm {`
`108`	`108`	`// there are no lookbehinds`
`109`	`109`	`not exists(RegExpLookbehind lbh \| getRoot(lbh) = this) and`
`110`	`110`	`// is actually used as a RegExp`
`111`		`- (`
`112`		`- parent instanceof RegExpLiteral`
`113`		`- or`
`114`		`- parent.(StringLiteral).flow() instanceof RegExpPatternSource`
`115`		`- )`
	`111`	`+ isUsedAsRegExp()`
`116`	`112`	`}`
`117`	`113`	`}`
`118`	`114`
Original file line number	Diff line number	Diff line change
`@@ -16,6 +16,7 @@ import javascript`
`16`	`16`
`17`	`17`	`from RegExpBackRef rebr, string ref`
`18`	`18`	`where`
	`19`	`+ rebr.isPartOfRegExpLiteral() and`
`19`	`20`	`not exists(rebr.getGroup()) and`
`20`	`21`	`(`
`21`	`22`	`ref = rebr.getNumber().toString()`