Merge pull request github#3034 from esbena/js/sharpen-useless-regexp-character-escape

Approved by asgerf

Author: semmle-qlci

change-notes/1.24/analysis-javascript.md

@@ -67,6 +67,7 @@
 | Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional ways of constructing arguments to `cmd.exe` and `/bin/sh`. |
 | Syntax error (`js/syntax-error`) | Lower severity | This results of this query are now displayed with lower severity. |
 | Use of password hash with insufficient computational effort (`js/insufficient-password-hash`) | Fewer false positive results | This query now recognizes additional cases that do not require secure hashing. |
+| Useless regular-expression character escape (`js/useless-regexp-character-escape`) | Fewer false positive results | This query now distinguishes escapes in strings and regular expression literals. |
 
 ## Changes to libraries
 

javascript/ql/src/semmle/javascript/CharacterEscapes.qll

@@ -93,6 +93,12 @@ module CharacterEscapes {
       // conservative formulation: we do not know in general if the sequence is enclosed in a character class `[...]`
       result = Sets::regexpMetaChars().charAt(_) and
       mistake = "may still represent a meta-character"
+    ) and
+    // avoid the benign case where preceding escaped backslashes turns into backslashes when the regexp is constructed
+    not exists(string raw |
+      not rawStringNode instanceof RegExpLiteral and
+      hasRawStringAndQuote(_, _, rawStringNode, raw) and
+      result = raw.regexpFind("(?<=(^|[^\\\\])((\\\\{3})|(\\\\{7}))).", _, i)
     )
   }
 }

javascript/ql/test/query-tests/Security/CWE-020/UselessCharacterEscape.expected

@@ -1,4 +1,5 @@
 | tst-IncompleteHostnameRegExp.js:42:13:42:65 | '^http[ ... \\/(.+)' | The escape sequence '\\/' is equivalent to just '/'. |
+| tst-SemiAnchoredRegExp.js:72:13:72:40 | '^good\\ ... \\\\.com' | The escape sequence '\\.' is equivalent to just '.'. |
 | tst-SemiAnchoredRegExp.js:109:2:109:45 | /^((\\+\| ... ?\\d\\d)/ | The escape sequence '\\:' is equivalent to just ':'. |
 | tst-escapes.js:19:8:19:11 | "\\ " | The escape sequence '\\ ' is equivalent to just ' '. |
 | tst-escapes.js:20:1:20:54 | /\\a\\b\\c ... x\\y\\z"/ | The escape sequence '\\a' is equivalent to just 'a'. |
@@ -56,3 +57,6 @@
 | tst-escapes.js:42:1:42:4 | "\\." | The escape sequence '\\.' is equivalent to just '.'. |
 | tst-escapes.js:48:8:48:15 | "'\\'\\\\'" | The escape sequence '\\'' is equivalent to just '''. |
 | tst-escapes.js:50:8:50:15 | '"\\"\\\\"' | The escape sequence '\\"' is equivalent to just '"'. |
+| tst-escapes.js:66:8:66:13 | "\\\\\\]" | The escape sequence '\\]' is equivalent to just ']'. |
+| tst-escapes.js:67:8:67:14 | "x\\\\\\]" | The escape sequence '\\]' is equivalent to just ']'. |
+| tst-escapes.js:71:8:71:17 | "\\\\\\\\\\\\\\]" | The escape sequence '\\]' is equivalent to just ']'. |

javascript/ql/test/query-tests/Security/CWE-020/UselessRegExpCharacterEscape.expected

@@ -1,8 +1,6 @@
 | tst-IncompleteHostnameRegExp.js:55:26:55:27 | '\\.' is equivalent to just '.', so the sequence may still represent a meta-character | The escape sequence '\\.' is equivalent to just '.', so the sequence may still represent a meta-character when it is used in a $@. | tst-IncompleteHostnameRegExp.js:55:13:55:39 | '^http: ... le.com' | regular expression |
 | tst-SemiAnchoredRegExp.js:70:19:70:20 | '\\.' is equivalent to just '.', so the sequence may still represent a meta-character | The escape sequence '\\.' is equivalent to just '.', so the sequence may still represent a meta-character when it is used in a $@. | tst-SemiAnchoredRegExp.js:70:13:70:36 | '^good\\ ... r\\.com' | regular expression |
 | tst-SemiAnchoredRegExp.js:70:31:70:32 | '\\.' is equivalent to just '.', so the sequence may still represent a meta-character | The escape sequence '\\.' is equivalent to just '.', so the sequence may still represent a meta-character when it is used in a $@. | tst-SemiAnchoredRegExp.js:70:13:70:36 | '^good\\ ... r\\.com' | regular expression |
-| tst-SemiAnchoredRegExp.js:72:21:72:22 | '\\.' is equivalent to just '.', so the sequence may still represent a meta-character | The escape sequence '\\.' is equivalent to just '.', so the sequence may still represent a meta-character when it is used in a $@. | tst-SemiAnchoredRegExp.js:72:13:72:40 | '^good\\ ... \\\\.com' | regular expression |
-| tst-SemiAnchoredRegExp.js:72:35:72:36 | '\\.' is equivalent to just '.', so the sequence may still represent a meta-character | The escape sequence '\\.' is equivalent to just '.', so the sequence may still represent a meta-character when it is used in a $@. | tst-SemiAnchoredRegExp.js:72:13:72:40 | '^good\\ ... \\\\.com' | regular expression |
 | tst-escapes.js:13:11:13:12 | '\\b' is a backspace, and not a word-boundary assertion | The escape sequence '\\b' is a backspace, and not a word-boundary assertion when it is used in a $@. | tst-escapes.js:13:8:13:61 | "\\a\\b\\c ... \\x\\y\\z" | regular expression |
 | tst-escapes.js:13:13:13:14 | '\\c' is equivalent to just 'c', so the sequence is not a character class | The escape sequence '\\c' is equivalent to just 'c', so the sequence is not a character class when it is used in a $@. | tst-escapes.js:13:8:13:61 | "\\a\\b\\c ... \\x\\y\\z" | regular expression |
 | tst-escapes.js:13:15:13:16 | '\\d' is equivalent to just 'd', so the sequence is not a character class | The escape sequence '\\d' is equivalent to just 'd', so the sequence is not a character class when it is used in a $@. | tst-escapes.js:13:8:13:61 | "\\a\\b\\c ... \\x\\y\\z" | regular expression |
@@ -43,3 +41,6 @@
 | tst-escapes.js:60:14:60:15 | '\\d' is equivalent to just 'd', so the sequence is not a character class | The escape sequence '\\d' is equivalent to just 'd', so the sequence is not a character class when it is used in a $@. | tst-escapes.js:60:8:60:19 | `\\k\\\\k\\d\\\\d` | regular expression |
 | tst-escapes.js:61:9:61:10 | '\\k' is equivalent to just 'k', so the sequence is not a backreference | The escape sequence '\\k' is equivalent to just 'k', so the sequence is not a backreference when it is used in a $@. | tst-escapes.js:61:8:61:25 | `\\k\\\\k${foo}\\d\\\\d` | regular expression |
 | tst-escapes.js:61:20:61:21 | '\\d' is equivalent to just 'd', so the sequence is not a character class | The escape sequence '\\d' is equivalent to just 'd', so the sequence is not a character class when it is used in a $@. | tst-escapes.js:61:8:61:25 | `\\k\\\\k${foo}\\d\\\\d` | regular expression |
+| tst-escapes.js:64:9:64:10 | '\\]' is equivalent to just ']', so the sequence may still represent a meta-character | The escape sequence '\\]' is equivalent to just ']', so the sequence may still represent a meta-character when it is used in a $@. | tst-escapes.js:64:8:64:11 | "\\]" | regular expression |
+| tst-escapes.js:69:13:69:14 | '\\]' is equivalent to just ']', so the sequence may still represent a meta-character | The escape sequence '\\]' is equivalent to just ']', so the sequence may still represent a meta-character when it is used in a $@. | tst-escapes.js:69:8:69:15 | "\\\\\\\\\\]" | regular expression |
+| tst-escapes.js:73:17:73:18 | '\\]' is equivalent to just ']', so the sequence may still represent a meta-character | The escape sequence '\\]' is equivalent to just ']', so the sequence may still represent a meta-character when it is used in a $@. | tst-escapes.js:73:8:73:19 | "\\\\\\\\\\\\\\\\\\]" | regular expression |

javascript/ql/test/query-tests/Security/CWE-020/tst-escapes.js

@@ -59,3 +59,15 @@ RegExp("\b");
 RegExp(`\b`);
 RegExp(`\k\\k\d\\d`)
 RegExp(`\k\\k${foo}\d\\d`)
+
+// effective escapes
+RegExp("\]")
+RegExp("\\]")
+RegExp("\\\]"); // effectively escaped after all
+RegExp("x\\\]"); // effectively escaped after all
+RegExp("\\\\]")
+RegExp("\\\\\]")
+RegExp("\\\\\\]")
+RegExp("\\\\\\\]") // effectively escaped after all
+RegExp("\\\\\\\\]")
+RegExp("\\\\\\\\\]")