Merge pull request #1182 from esben-semmle/js/sourcenode-regexp-literals

Approved by xiemaisi

Author: semmle-qlci

change-notes/1.21/analysis-javascript.md

@@ -17,10 +17,15 @@
 
 | **Query**                      | **Expected impact**          | **Change**                                                                |
 |--------------------------------|------------------------------|---------------------------------------------------------------------------|
-| Expression has no effect       | Fewer false-positive results | This rule now treats uses of `Object.defineProperty` more conservatively. |
-| Useless assignment to property | Fewer false-positive results | This rule now ignores reads of additional getters. |
 | Arbitrary file write during zip extraction ("Zip Slip") | More results | This rule now considers more libraries, including tar as well as zip. |
-| Client-side URL redirect       | Fewer false-positive results | This rule now treats URLs as safe in more cases where the hostname cannot be tampered with. |
+| Client-side URL redirect       | More results and fewer false-positive results | This rule now recognizes additional uses of the document URL. This rule now treats URLs as safe in more cases where the hostname cannot be tampered with. |
+| Double escaping or unescaping | More results | This rule now considers the flow of regular expressions literals. |
+| Expression has no effect       | Fewer false-positive results | This rule now treats uses of `Object.defineProperty` more conservatively. |
+| Incomplete string escaping or encoding | More results | This rule now considers the flow of regular expressions literals. |
+| Replacement of a substring with itself | More results | This rule now considers the flow of regular expressions literals. |
 | Server-side URL redirect       | Fewer false-positive results | This rule now treats URLs as safe in more cases where the hostname cannot be tampered with. |
+| Useless assignment to property | Fewer false-positive results | This rule now ignore reads of additional getters. |
 
 ## Changes to QL libraries
+
+* `RegExpLiteral` is now a `DataFlow::SourceNode`.

javascript/ql/src/RegExp/IdentityReplacement.ql

@@ -18,7 +18,7 @@ import javascript
  */
 predicate matchesString(Expr e, string s) {
   exists(RegExpLiteral rl |
-    rl = e and
+    rl.flow().(DataFlow::SourceNode).flowsToExpr(e) and
     not rl.isIgnoreCase() and
     regExpMatchesString(rl.getRoot(), s)
   )

javascript/ql/src/Security/CWE-116/DoubleEscaping.ql

@@ -70,7 +70,7 @@ class Replacement extends DataFlow::Node {
   Replacement() {
     exists(DataFlow::MethodCallNode mcn | this = mcn |
       mcn.getMethodName() = "replace" and
-      mcn.getArgument(0).asExpr() = pattern and
+      pattern.flow().(DataFlow::SourceNode).flowsTo(mcn.getArgument(0))and
       mcn.getNumArgument() = 2 and
       pattern.isGlobal()
     )

javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql

@@ -59,7 +59,7 @@ predicate isSimple(RegExpTerm t) {
  */
 predicate isBackslashEscape(MethodCallExpr mce, RegExpLiteral re) {
   mce.getMethodName() = "replace" and
-  re = mce.getArgument(0) and
+  re.flow().(DataFlow::SourceNode).flowsToExpr(mce.getArgument(0)) and
   re.isGlobal() and
   exists(string new | new = mce.getArgument(1).getStringValue() |
     // `new` is `\$&`, `\$1` or similar
@@ -104,7 +104,7 @@ predicate allBackslashesEscaped(DataFlow::Node nd) {
 from MethodCallExpr repl, Expr old, string msg
 where
   repl.getMethodName() = "replace" and
-  old = repl.getArgument(0) and
+  (old = repl.getArgument(0) or old.flow().(DataFlow::SourceNode).flowsToExpr(repl.getArgument(0))) and
   (
     not old.(RegExpLiteral).isGlobal() and
     msg = "This replaces only the first occurrence of " + old + "." and

javascript/ql/src/semmle/javascript/dataflow/Sources.qll

@@ -211,6 +211,7 @@ module SourceNode {
    *   - object expressions
    *   - array expressions
    *   - JSX literals
+   *   - regular expression literals
    *
    * This class is for internal use only and should not normally be used directly.
    */
@@ -224,7 +225,8 @@ module SourceNode {
         astNode instanceof ArrayExpr or
         astNode instanceof JSXNode or
         astNode instanceof GlobalVarAccess or
-        astNode instanceof ExternalModuleReference
+        astNode instanceof ExternalModuleReference or
+        astNode instanceof RegExpLiteral
       )
       or
       exists(SsaExplicitDefinition ssa, VarDef def |

javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll

@@ -100,7 +100,7 @@ module ClientSideUrlRedirect {
     or
     exists(MethodCallExpr mce |
       queryAccess.asExpr() = mce and
-      mce.calls(any(RegExpLiteral re), "exec") and
+      mce = any(RegExpLiteral re).flow().(DataFlow::SourceNode).getAMethodCall("exec").asExpr() and
       nd.asExpr() = mce.getArgument(0)
     )
   }

javascript/ql/test/library-tests/DataFlow/sources.expected

@@ -11,6 +11,7 @@
 | sources.js:3:2:3:1 | this |
 | sources.js:3:2:5:1 | functio ... x+19;\\n} |
 | sources.js:3:11:3:11 | x |
+| sources.js:7:1:7:3 | /x/ |
 | tst.js:1:1:1:0 | this |
 | tst.js:1:1:1:24 | import  ... m 'fs'; |
 | tst.js:1:10:1:11 | fs |

javascript/ql/test/library-tests/DataFlow/sources.js

@@ -3,3 +3,5 @@ new (x => x);
 (function(x) {
   return x+19;
 })(23);
+
+/x/;

javascript/ql/test/query-tests/RegExp/IdentityReplacement/IdentityReplacement.expected

@@ -1,4 +1,5 @@
 | IdentityReplacement.js:1:27:1:30 | /"/g | This replaces '"' with itself. |
+| IdentityReplacement.js:4:14:4:21 | indirect | This replaces '"' with itself. |
 | tst.js:1:13:1:16 | "\\\\" | This replaces '\\' with itself. |
 | tst.js:2:13:2:18 | /(\\\\)/ | This replaces '\\' with itself. |
 | tst.js:3:13:3:17 | /["]/ | This replaces '"' with itself. |

javascript/ql/test/query-tests/RegExp/IdentityReplacement/IdentityReplacement.js

@@ -1 +1,5 @@
 var escaped = raw.replace(/"/g, '\"');
+(function() {
+	var indirect = /"/g;
+	raw.replace(indirect, '\"');
+});