Merge pull request #158 from esben-semmle/js/sharpen-regexp-injection

Approved by xiemaisi

Author: semmle-qlci

change-notes/1.19/analysis-javascript.md

@@ -0,0 +1,18 @@
+# Improvements to JavaScript analysis
+
+## General improvements
+
+## New queries
+
+| **Query**                   | **Tags**  | **Purpose**                                                        |
+|-----------------------------|-----------|--------------------------------------------------------------------|
+| *@name of query (Query ID)* | *Tags*    |*Aim of the new query and whether it is enabled by default or not*  |
+
+## Changes to existing queries
+
+| **Query**                      | **Expected impact**        | **Change**                                   |
+|--------------------------------|----------------------------|----------------------------------------------|
+| Regular expression injection | Fewer false-positive results | This rule now identifies calls to `String.prototype.search` with more precision. |
+
+
+## Changes to QL libraries

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

@@ -69,7 +69,13 @@ module RegExpInjection {
         mce.getReceiver().analyze().getAType() = TTString() and
         mce.getMethodName() = methodName |
         (methodName = "match" and this.asExpr() = mce.getArgument(0) and mce.getNumArgument() = 1) or
-        (methodName = "search" and this.asExpr() = mce.getArgument(0) and mce.getNumArgument() = 1)
+        (
+           methodName = "search" and
+           this.asExpr() = mce.getArgument(0) and
+           mce.getNumArgument() = 1 and
+           // `String.prototype.search` returns a number, so exclude chained accesses
+           not exists(PropAccess p | p.getBase() = mce)
+        )
       )
     }
 

javascript/ql/test/query-tests/Security/CWE-730/RegExpInjection.expected

@@ -10,4 +10,5 @@
 | RegExpInjection.js:45:20:45:24 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
 | RegExpInjection.js:46:23:46:27 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
 | RegExpInjection.js:47:22:47:26 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
+| RegExpInjection.js:50:46:50:50 | input | This regular expression is constructed from a $@. | RegExpInjection.js:5:39:5:56 | req.param("input") | user-provided value |
 | tst.js:3:16:3:35 | "^"+ data.name + "$" | This regular expression is constructed from a $@. | tst.js:1:46:1:46 | e | user-provided value |

javascript/ql/test/query-tests/Security/CWE-730/RegExpInjection.js

@@ -1,6 +1,6 @@
 var express = require('express');
 var app = express();
-
+var URI = reuires("urijs");
 app.get('/findKey', function(req, res) {
   var key = req.param("key"), input = req.param("input");
 
@@ -46,4 +46,8 @@ app.get('/findKey', function(req, res) {
   likelyString.search(input); // NOT OK
   maybeString.search(input); // NOT OK
   notString.search(input); // OK
+
+  URI(`${protocol}://${host}${path}`).search(input); // OK, but still flagged
+  URI(`${protocol}://${host}${path}`).search(input).href(); // OK
+  unknown.search(input).unknown; // OK
 });