move String.prototype.match taint step to a general AdditionalTaintStep

Author: erik-krogh

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

@@ -558,6 +558,23 @@ module TaintTracking {
       succ = this
     }
   }
+  
+
+  /**
+   * A taint propagating data flow edge arising from calling `String.prototype.match()`.
+   */
+  private class StringMatchTaintStep extends AdditionalTaintStep, DataFlow::MethodCallNode {
+    StringMatchTaintStep() {
+      this.getMethodName() = "match" and
+      this.getNumArgument() = 1 and
+      this.getArgument(0) .analyze().getAType() = TTRegExp()
+    }
+
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      pred = this.getReceiver() and
+      succ = this
+    }
+  }
 
   /**
    * A taint propagating data flow edge arising from JSON unparsing.

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

@@ -75,19 +75,6 @@ module ExceptionXss {
         or
         pred = any(DataFlow::FunctionNode func).getExceptionalReturn()
       )
-      or
-      // String.prototype.match()
-      exists(DataFlow::MethodCallNode call |
-        call = succ and
-        pred = call.getReceiver() and
-        call.getMethodName() = "match" and
-        call.getNumArgument() = 1 and
-        // TODO: Better way of detecting regExp / String.prototype.match() calls?
-        (
-          call.getArgument(0).getALocalSource().asExpr() instanceof RegExpLiteral or
-          call.getArgument(0).getALocalSource().(DataFlow::NewNode).getCalleeName() = "RegExp"
-        )
-      )
     }
   }
 

javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss.expected

@@ -32,6 +32,9 @@ nodes
 | addEventListener.js:12:24:12:33 | event.data |
 | addEventListener.js:12:24:12:33 | event.data |
 | exception-xss.js:2:9:2:31 | foo |
+| exception-xss.js:2:9:2:31 | foo |
+| exception-xss.js:2:15:2:31 | document.location |
+| exception-xss.js:2:15:2:31 | document.location |
 | exception-xss.js:2:15:2:31 | document.location |
 | exception-xss.js:2:15:2:31 | document.location |
 | exception-xss.js:4:20:4:20 | x |
@@ -75,6 +78,10 @@ nodes
 | exception-xss.js:82:10:82:10 | e |
 | exception-xss.js:83:18:83:18 | e |
 | exception-xss.js:83:18:83:18 | e |
+| exception-xss.js:86:17:86:19 | foo |
+| exception-xss.js:86:17:86:19 | foo |
+| exception-xss.js:86:17:86:19 | foo |
+| exception-xss.js:86:17:86:19 | foo |
 | jquery.js:2:7:2:40 | tainted |
 | jquery.js:2:7:2:40 | tainted |
 | jquery.js:2:17:2:33 | document.location |
@@ -743,6 +750,12 @@ edges
 | exception-xss.js:2:9:2:31 | foo | exception-xss.js:33:19:33:21 | foo |
 | exception-xss.js:2:9:2:31 | foo | exception-xss.js:46:16:46:18 | foo |
 | exception-xss.js:2:9:2:31 | foo | exception-xss.js:81:16:81:18 | foo |
+| exception-xss.js:2:9:2:31 | foo | exception-xss.js:86:17:86:19 | foo |
+| exception-xss.js:2:9:2:31 | foo | exception-xss.js:86:17:86:19 | foo |
+| exception-xss.js:2:9:2:31 | foo | exception-xss.js:86:17:86:19 | foo |
+| exception-xss.js:2:9:2:31 | foo | exception-xss.js:86:17:86:19 | foo |
+| exception-xss.js:2:15:2:31 | document.location | exception-xss.js:2:9:2:31 | foo |
+| exception-xss.js:2:15:2:31 | document.location | exception-xss.js:2:9:2:31 | foo |
 | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:2:9:2:31 | foo |
 | exception-xss.js:2:15:2:31 | document.location | exception-xss.js:2:9:2:31 | foo |
 | exception-xss.js:4:20:4:20 | x | exception-xss.js:5:14:5:14 | x |

javascript/ql/test/query-tests/Security/CWE-079/exception-xss.js

@@ -82,4 +82,6 @@
 	} catch(e) {
 		$('myId').html(e); // NOT OK! 
 	}
+	
+	$('myId').html(foo); // Direct leak, reported by other query.
 });