github · semmle-qlci · May 18, 2020 · May 4, 2020 · May 4, 2020 · May 4, 2020
diff --git a/change-notes/1.25/analysis-javascript.md b/change-notes/1.25/analysis-javascript.md
@@ -20,6 +20,10 @@
 | Misspelled variable name (`js/misspelled-variable-name`) | Message changed | The message for this query now correctly identifies the misspelled variable in additional cases. |
 | Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional file system calls. |
 | Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional command execution calls. |
+| Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | Less results | This query now recognizes additional safe patterns of doing URL redirects. |
+| Client-side cross-site scripting (`js/xss`) | Less results | This query now recognizes additional safe strings based on URLs. |
+| Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | More results | This query now recognizes additional url scheme checks. |
+| Prototype pollution in utility function (`js/prototype-pollution-utility`) | More results | This query now recognizes additional utility functions as vulnerable to prototype polution. |
 | Expression has no effect (`js/useless-expression`) | Less results | This query no longer flags an expression when that expression is the only content of the containing file. |
 
 ## Changes to libraries

diff --git a/javascript/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql b/javascript/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql
@@ -27,11 +27,10 @@ class DangerousScheme extends string {
 /** Returns a node that refers to the scheme of `url`. */
 DataFlow::SourceNode schemeOf(DataFlow::Node url) {
   // url.split(":")[0]
-  exists(DataFlow::MethodCallNode split |
-    split.getMethodName() = "split" and
-    split.getArgument(0).getStringValue() = ":" and
-    result = split.getAPropertyRead("0") and
-    url = split.getReceiver()
+  exists(StringSplitCall split |
+    split.getSeparator() = ":" and
+    result = split.getASubstringRead(0) and
+    url = split.getBaseString()
   )
   or
   // url.getScheme(), url.getProtocol(), getScheme(url), getProtocol(url)

diff --git a/javascript/ql/src/Security/CWE-400/PrototypePollutionUtility.ql b/javascript/ql/src/Security/CWE-400/PrototypePollutionUtility.ql
@@ -21,11 +21,10 @@ import semmle.javascript.DynamicPropertyAccess
  *
  * We restrict this to parameter nodes to focus on "deep assignment" functions.
  */
-class SplitCall extends MethodCallNode {
+class SplitCall extends StringSplitCall {
   SplitCall() {
-    getMethodName() = "split" and
-    getArgument(0).mayHaveStringValue(".") and
-    getReceiver().getALocalSource() instanceof ParameterNode
+    getSeparator() = "." and
+    getBaseString().getALocalSource() instanceof ParameterNode
   }
 }
 

diff --git a/javascript/ql/src/semmle/javascript/StandardLibrary.qll b/javascript/ql/src/semmle/javascript/StandardLibrary.qll
@@ -146,3 +146,37 @@ class StringReplaceCall extends DataFlow::MethodCallNode {
     )
   }
 }
+
+/**
+ * A call to `String.prototype.split`.
+ *
+ * We heuristically include any call to a method called `split`, provided it either
+ * has one or two arguments, or local data flow suggests that the receiver may be a string.
+ */
+class StringSplitCall extends DataFlow::MethodCallNode {
+  StringSplitCall() {
+    this.getMethodName() = "split" and
+    (getNumArgument() = [1, 2] or getReceiver().mayHaveStringValue(_))
+  }
+
+  /**
+   * Gets a string that determines where the string is split.
+   */
+  string getSeparator() {
+    getArgument(0).mayHaveStringValue(result)
+    or
+    result =
+      getArgument(0).getALocalSource().(DataFlow::RegExpCreationNode).getRoot().getAMatchedString()
+  }
+
+  /**
+   * Gets the DataFlow::Node for the base string that is split.
+   */
+  DataFlow::Node getBaseString() { result = getReceiver() }
+
+  /**
+   * Gets a read of the `i`th element from the split string.
+   */
+  bindingset[i]
+  DataFlow::Node getASubstringRead(int i) { result = getAPropertyRead(i.toString()) }
+}
diff --git a/...script/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll b/...script/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll
@@ -9,6 +9,8 @@ import semmle.javascript.security.dataflow.RemoteFlowSources
 import UrlConcatenation
 
 module ClientSideUrlRedirect {
+  private import Xss::DomBasedXss as DomBasedXss
+
   /**
    * A data flow source for unvalidated URL redirect vulnerabilities.
    */
@@ -52,7 +54,7 @@ module ClientSideUrlRedirect {
       mce = queryAccess.asExpr() and mce.calls(nd.asExpr(), methodName)
     |
       methodName = "split" and
-      // exclude `location.href.split('?')[0]`, which can never refer to the query string
+      // exclude all splits where only the prefix is accessed, which is safe for url-redirects.
       not exists(PropAccess pacc | mce = pacc.getBase() | pacc.getPropertyName() = "0")
       or
       (methodName = "substring" or methodName = "substr" or methodName = "slice") and
@@ -68,6 +70,11 @@ module ClientSideUrlRedirect {
     )
   }
 
+  /**
+   * A sanitizer that reads the first part a location split by "?", e.g. `location.href.split('?')[0]`.
+   */
+  class QueryPrefixSanitizer extends Sanitizer, DomBasedXss::QueryPrefixSanitizer { }
+
   /**
    * A sink which is used to set the window location.
    */

diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll
@@ -97,23 +97,17 @@ module TaintedPath {
         )
       )
       or
+      // A `str.split()` call can either split into path elements (`str.split("/")`) or split by some other string.
+      exists(StringSplitCall mcn | dst = mcn and mcn.getBaseString() = src |
+        if mcn.getSeparator() = "/"
+        then
+          srclabel.(Label::PosixPath).canContainDotDotSlash() and
+          dstlabel instanceof Label::SplitPath
+        else srclabel = dstlabel
+      )
+      or
       // array method calls of interest
       exists(DataFlow::MethodCallNode mcn, string name | dst = mcn and mcn.calls(src, name) |
-        // A `str.split()` call can either split into path elements (`str.split("/")`) or split by some other string.
-        name = "split" and
-        (
-          if
-            exists(DataFlow::Node splitBy | splitBy = mcn.getArgument(0) |
-              splitBy.mayHaveStringValue("/") or
-              any(DataFlow::RegExpCreationNode reg | reg.getRoot().getAMatchedString() = "/")
-                  .flowsTo(splitBy)
-            )
-          then
-            srclabel.(Label::PosixPath).canContainDotDotSlash() and
-            dstlabel instanceof Label::SplitPath
-          else srclabel = dstlabel
-        )
-        or
         (
           name = "pop" or
           name = "shift"

diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll b/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll
@@ -278,6 +278,20 @@ module DomBasedXss {
     }
   }
 
+  /**
+   * A sanitizer that reads the first part a location split by "?", e.g. `location.href.split('?')[0]`.
+   */
+  class QueryPrefixSanitizer extends Sanitizer {
+    StringSplitCall splitCall;
+
+    QueryPrefixSanitizer() {
+      this = splitCall.getASubstringRead(0) and
+      splitCall.getSeparator() = "?" and
+      splitCall.getBaseString().getALocalSource() =
+        [DOM::locationRef(), DOM::locationRef().getAPropertyRead("href")]
+    }
+  }
+
   /**
    * A regexp replacement involving an HTML meta-character, viewed as a sanitizer for
    * XSS vulnerabilities.

diff --git a/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.expected b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.expected
@@ -3,3 +3,4 @@
 | IncompleteUrlSchemeCheck.js:23:9:23:43 | badProt ... scheme) | This check does not consider vbscript:. |
 | IncompleteUrlSchemeCheck.js:30:9:30:43 | badProt ... scheme) | This check does not consider vbscript:. |
 | IncompleteUrlSchemeCheck.js:37:9:37:31 | scheme  ... script" | This check does not consider data: and vbscript:. |
+| IncompleteUrlSchemeCheck.js:51:9:51:31 | scheme  ... script" | This check does not consider data: and vbscript:. |
diff --git a/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.js b/javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.js
@@ -45,3 +45,10 @@ function test6(url) {
         return "about:blank";
     return url;
 }
+
+function test7(url) {
+    let scheme = url.split(/:/)[0];
+    if (scheme === "javascript") // NOT OK
+        return "about:blank";
+    return url;
+}
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected b/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected
@@ -357,6 +357,12 @@ nodes
 | tst.js:366:21:366:26 | target |
 | tst.js:369:18:369:23 | target |
 | tst.js:369:18:369:23 | target |
+| tst.js:377:7:377:39 | target |
+| tst.js:377:16:377:32 | document.location |
+| tst.js:377:16:377:32 | document.location |
+| tst.js:377:16:377:39 | documen ... .search |
+| tst.js:380:18:380:23 | target |
+| tst.js:380:18:380:23 | target |
 | typeahead.js:20:13:20:45 | target |
 | typeahead.js:20:22:20:38 | document.location |
 | typeahead.js:20:22:20:38 | document.location |
@@ -689,6 +695,11 @@ edges
 | tst.js:361:19:361:35 | document.location | tst.js:361:19:361:42 | documen ... .search |
 | tst.js:361:19:361:35 | document.location | tst.js:361:19:361:42 | documen ... .search |
 | tst.js:361:19:361:42 | documen ... .search | tst.js:361:10:361:42 | target |
+| tst.js:377:7:377:39 | target | tst.js:380:18:380:23 | target |
+| tst.js:377:7:377:39 | target | tst.js:380:18:380:23 | target |
+| tst.js:377:16:377:32 | document.location | tst.js:377:16:377:39 | documen ... .search |
+| tst.js:377:16:377:32 | document.location | tst.js:377:16:377:39 | documen ... .search |
+| tst.js:377:16:377:39 | documen ... .search | tst.js:377:7:377:39 | target |
 | typeahead.js:20:13:20:45 | target | typeahead.js:21:12:21:17 | target |
 | typeahead.js:20:22:20:38 | document.location | typeahead.js:20:22:20:45 | documen ... .search |
 | typeahead.js:20:22:20:38 | document.location | typeahead.js:20:22:20:45 | documen ... .search |
@@ -794,6 +805,7 @@ edges
 | tst.js:362:16:362:21 | target | tst.js:361:19:361:35 | document.location | tst.js:362:16:362:21 | target | Cross-site scripting vulnerability due to $@. | tst.js:361:19:361:35 | document.location | user-provided value |
 | tst.js:366:21:366:26 | target | tst.js:361:19:361:35 | document.location | tst.js:366:21:366:26 | target | Cross-site scripting vulnerability due to $@. | tst.js:361:19:361:35 | document.location | user-provided value |
 | tst.js:369:18:369:23 | target | tst.js:361:19:361:35 | document.location | tst.js:369:18:369:23 | target | Cross-site scripting vulnerability due to $@. | tst.js:361:19:361:35 | document.location | user-provided value |
+| tst.js:380:18:380:23 | target | tst.js:377:16:377:32 | document.location | tst.js:380:18:380:23 | target | Cross-site scripting vulnerability due to $@. | tst.js:377:16:377:32 | document.location | user-provided value |
 | typeahead.js:25:18:25:20 | val | typeahead.js:20:22:20:38 | document.location | typeahead.js:25:18:25:20 | val | Cross-site scripting vulnerability due to $@. | typeahead.js:20:22:20:38 | document.location | user-provided value |
 | v-html.vue:2:8:2:23 | v-html=tainted | v-html.vue:6:42:6:58 | document.location | v-html.vue:2:8:2:23 | v-html=tainted | Cross-site scripting vulnerability due to $@. | v-html.vue:6:42:6:58 | document.location | user-provided value |
 | winjs.js:3:43:3:49 | tainted | winjs.js:2:17:2:33 | document.location | winjs.js:3:43:3:49 | tainted | Cross-site scripting vulnerability due to $@. | winjs.js:2:17:2:33 | document.location | user-provided value |

diff --git a/javascript/ql/test/query-tests/Security/CWE-079/XssWithAdditionalSources.expected b/javascript/ql/test/query-tests/Security/CWE-079/XssWithAdditionalSources.expected
@@ -357,6 +357,12 @@ nodes
 | tst.js:366:21:366:26 | target |
 | tst.js:369:18:369:23 | target |
 | tst.js:369:18:369:23 | target |
+| tst.js:377:7:377:39 | target |
+| tst.js:377:16:377:32 | document.location |
+| tst.js:377:16:377:32 | document.location |
+| tst.js:377:16:377:39 | documen ... .search |
+| tst.js:380:18:380:23 | target |
+| tst.js:380:18:380:23 | target |
 | typeahead.js:9:28:9:30 | loc |
 | typeahead.js:9:28:9:30 | loc |
 | typeahead.js:10:16:10:18 | loc |
@@ -693,6 +699,11 @@ edges
 | tst.js:361:19:361:35 | document.location | tst.js:361:19:361:42 | documen ... .search |
 | tst.js:361:19:361:35 | document.location | tst.js:361:19:361:42 | documen ... .search |
 | tst.js:361:19:361:42 | documen ... .search | tst.js:361:10:361:42 | target |
+| tst.js:377:7:377:39 | target | tst.js:380:18:380:23 | target |
+| tst.js:377:7:377:39 | target | tst.js:380:18:380:23 | target |
+| tst.js:377:16:377:32 | document.location | tst.js:377:16:377:39 | documen ... .search |
+| tst.js:377:16:377:32 | document.location | tst.js:377:16:377:39 | documen ... .search |
+| tst.js:377:16:377:39 | documen ... .search | tst.js:377:7:377:39 | target |
 | typeahead.js:9:28:9:30 | loc | typeahead.js:10:16:10:18 | loc |
 | typeahead.js:9:28:9:30 | loc | typeahead.js:10:16:10:18 | loc |
 | typeahead.js:9:28:9:30 | loc | typeahead.js:10:16:10:18 | loc |

diff --git a/javascript/ql/test/query-tests/Security/CWE-079/tst.js b/javascript/ql/test/query-tests/Security/CWE-079/tst.js
@@ -372,3 +372,13 @@ function thisNodes() {
 	$.fn[pluginName] = myPlugin; 
 
 }
+
+function test() {
+  var target = document.location.search
+
+  // NOT OK
+  $('myId').html(target)
+
+  // OK
+  $('myid').html(document.location.href.split("?")[0]);
+}