Merge pull request #3391 from erik-krogh/SplitFPs

Approved by esbena

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()) }
+}

javascript/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.
    */

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"

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.