JS: Fix handling of constant array index reads, and fix the fallout

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

@@ -255,7 +255,7 @@ module TaintTracking {
     exists(StringSplitCall c |
       c.getBaseString().getALocalSource() =
         [DOM::locationRef(), DOM::locationRef().getAPropertyRead("href")] and
-      c.getSeparator() = "?" and
+      c.getSeparator() = ["?", "#"] and
       read = c.getAPropertyRead("0")
     )
   }
@@ -356,6 +356,16 @@ module TaintTracking {
     }
   }
 
+  private class LegacySplitTaintStep extends LegacyTaintStep {
+    override predicate stringManipulationStep(DataFlow::Node pred, DataFlow::Node target) {
+      exists(DataFlow::MethodCallNode call |
+        call.getMethodName() = "split" and
+        pred = call.getReceiver() and
+        target = call
+      )
+    }
+  }
+
   /**
    * A taint propagating data flow edge arising from string manipulation
    * functions defined in the standard library.
@@ -372,9 +382,8 @@ module TaintTracking {
               [
                 "anchor", "big", "blink", "bold", "concat", "fixed", "fontcolor", "fontsize",
                 "italics", "link", "padEnd", "padStart", "repeat", "replace", "replaceAll", "slice",
-                "small", "split", "strike", "sub", "substr", "substring", "sup",
-                "toLocaleLowerCase", "toLocaleUpperCase", "toLowerCase", "toUpperCase", "trim",
-                "trimLeft", "trimRight"
+                "small", "strike", "sub", "substr", "substring", "sup", "toLocaleLowerCase",
+                "toLocaleUpperCase", "toLowerCase", "toUpperCase", "trim", "trimLeft", "trimRight"
               ]
             or
             // sorted, interesting, properties of Object.prototype

javascript/ql/lib/semmle/javascript/dataflow/internal/Contents.qll

@@ -93,14 +93,20 @@ module Private {
     // than an ordinary content component. These special content sets should never appear in a step.
     MkAwaited() or
     MkAnyPropertyDeep() or
-    MkArrayElementDeep()
+    MkArrayElementDeep() or
+    MkOptionalStep(string name) { isAccessPathTokenPresent("OptionalStep", name) } or
+    MkOptionalBarrier(string name) { isAccessPathTokenPresent("OptionalBarrier", name) }
 
   /**
    * Holds if `cs` is used to encode a special operation as a content component, but should not
    * be treated as an ordinary content component.
    */
   predicate isSpecialContentSet(ContentSet cs) {
-    cs = MkAwaited() or cs = MkAnyPropertyDeep() or cs = MkArrayElementDeep()
+    cs = MkAwaited() or
+    cs = MkAnyPropertyDeep() or
+    cs = MkArrayElementDeep() or
+    cs instanceof MkOptionalStep or
+    cs instanceof MkOptionalBarrier
   }
 }
 
@@ -254,14 +260,8 @@ module Public {
     /** Gets the singleton content to be accessed. */
     Content asSingleton() { this = MkSingletonContent(result) }
 
-    /** Gets the property name to be accessed. */
-    PropertyName asPropertyName() {
-      // TODO: array indices should be mapped to a ContentSet that also reads from UnknownArrayElement
-      result = this.asSingleton().asPropertyName()
-    }
-
-    /** Gets the array index to be accessed. */
-    int asArrayIndex() { result = this.asSingleton().asArrayIndex() }
+    /** Gets the property name to be accessed, provided that this is a singleton content set. */
+    PropertyName asPropertyName() { result = this.asSingleton().asPropertyName() }
 
     /**
      * Gets a string representation of this content set.
@@ -294,6 +294,16 @@ module Public {
       or
       this = MkAnyCapturedContent() and
       result = "AnyCapturedContent"
+      or
+      exists(string name |
+        this = MkOptionalStep(name) and
+        result = "OptionalStep[" + name + "]"
+      )
+      or
+      exists(string name |
+        this = MkOptionalBarrier(name) and
+        result = "OptionalBarrier[" + name + "]"
+      )
     }
   }
 

javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll

@@ -1035,6 +1035,11 @@ predicate simpleLocalFlowStep(Node node1, Node node2) {
     FlowSummaryPrivate::Steps::summaryReadStep(input, MkAwaited(), output) and
     node1 = TFlowSummaryNode(input) and
     node2 = TFlowSummaryNode(output)
+    or
+    // Add flow through optional barriers. This step is then blocked by the barrier for queries that choose to use the barrier.
+    FlowSummaryPrivate::Steps::summaryReadStep(input, MkOptionalBarrier(_), output) and
+    node1 = TFlowSummaryNode(input) and
+    node2 = TFlowSummaryNode(output)
   )
   or
   VariableCaptureOutput::localFlowStep(getClosureNode(node1), getClosureNode(node2))
@@ -1103,7 +1108,12 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
     node1 = read.getBase() and
     node2 = read
   |
-    c.asPropertyName() = read.getPropertyName()
+    exists(PropertyName name | read.getPropertyName() = name |
+      not exists(name.asArrayIndex()) and
+      c = ContentSet::property(name)
+      or
+      c = ContentSet::arrayElementKnown(name.asArrayIndex())
+    )
     or
     not exists(read.getPropertyName()) and
     c = ContentSet::arrayElement()
@@ -1157,7 +1167,7 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
     node2 = TRestParameterStoreNode(function, content)
   |
     // shift known array indices
-    c.asArrayIndex() = content.asArrayIndex() + restIndex
+    c.asSingleton().asArrayIndex() = content.asArrayIndex() + restIndex
     or
     content.isUnknownArrayElement() and // TODO: don't read unknown array elements from static array
     c = ContentSet::arrayElementUnknown()
@@ -1174,7 +1184,7 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
       c = ContentSet::arrayElement() and // unknown start index when not the first spread operator
       storeContent.isUnknownArrayElement()
     else (
-      storeContent.asArrayIndex() = n + c.asArrayIndex()
+      storeContent.asArrayIndex() = n + c.asSingleton().asArrayIndex()
       or
       storeContent.isUnknownArrayElement() and c.asSingleton() = storeContent
     )
@@ -1185,7 +1195,7 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
     node1 = TFlowSummaryDynamicParameterArrayNode(parameter.getSummarizedCallable()) and
     node2 = parameter and
     (
-      c.asArrayIndex() = pos.asPositional()
+      c.asSingleton().asArrayIndex() = pos.asPositional()
       or
       c = ContentSet::arrayElementLowerBound(pos.asPositionalLowerBound())
     )
@@ -1256,7 +1266,7 @@ predicate storeStep(Node node1, ContentSet c, Node node2) {
   exists(InvokeExpr invoke, int n |
     node1 = TValueNode(invoke.getArgument(n)) and
     node2 = TStaticArgumentArrayNode(invoke) and
-    c.asArrayIndex() = n and
+    c.asSingleton().asArrayIndex() = n and
     not n >= firstSpreadArgumentIndex(invoke)
   )
   or
@@ -1384,3 +1394,20 @@ class ArgumentNode extends DataFlow::Node {
 class ParameterNode extends DataFlow::Node {
   ParameterNode() { isParameterNodeImpl(this, _, _) }
 }
+
+cached
+private module OptionalSteps {
+  cached
+  predicate optionalStep(Node node1, string name, Node node2) {
+    FlowSummaryPrivate::Steps::summaryReadStep(node1.(FlowSummaryNode).getSummaryNode(),
+      MkOptionalStep(name), node2.(FlowSummaryNode).getSummaryNode())
+  }
+
+  cached
+  predicate optionalBarrier(Node node, string name) {
+    FlowSummaryPrivate::Steps::summaryReadStep(_, MkOptionalBarrier(name),
+      node.(FlowSummaryNode).getSummaryNode())
+  }
+}
+
+import OptionalSteps

javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSummaryPrivate.qll

@@ -95,6 +95,10 @@ private string encodeContentAux(ContentSet cs, string arg) {
   cs = MkAnyPropertyDeep() and result = "AnyMemberDeep" and arg = ""
   or
   cs = MkArrayElementDeep() and result = "ArrayElementDeep" and arg = ""
+  or
+  cs = MkOptionalStep(arg) and result = "OptionalStep"
+  or
+  cs = MkOptionalBarrier(arg) and result = "OptionalBarrier"
 }
 
 /**
@@ -122,10 +126,7 @@ string encodeArgumentPosition(ArgumentPosition pos) {
 }
 
 /** Gets the return kind corresponding to specification `"ReturnValue"`. */
-ReturnKind getStandardReturnValueKind() { result = MkNormalReturnKind() }
-
-/** Gets the return kind corresponding to specification `"ReturnValue"`. */
-MkNormalReturnKind getReturnValueKind() { any() }
+ReturnKind getStandardReturnValueKind() { result = MkNormalReturnKind() and Stage::ref() }
 
 private module FlowSummaryStepInput implements Private::StepsInputSig {
   DataFlowCall getACall(SummarizedCallable sc) {
@@ -237,3 +238,12 @@ ContentSet decodeUnknownWithoutContent(AccessPathSyntax::AccessPathTokenBase tok
  */
 bindingset[token]
 ContentSet decodeUnknownWithContent(AccessPathSyntax::AccessPathTokenBase token) { none() }
+
+cached
+module Stage {
+  cached
+  predicate ref() { 1 = 1 }
+
+  cached
+  predicate backref() { optionalStep(_, _, _) }
+}

javascript/ql/lib/semmle/javascript/dataflow/internal/TaintTrackingPrivate.qll

@@ -61,5 +61,7 @@ predicate defaultTaintSanitizer(DataFlow::Node node) {
 bindingset[node]
 predicate defaultImplicitTaintRead(DataFlow::Node node, ContentSet c) {
   exists(node) and
-  c = [ContentSet::promiseValue(), ContentSet::arrayElement()]
+  c = [ContentSet::promiseValue(), ContentSet::arrayElement()] and
+  // Optional steps are added through isAdditionalFlowStep but we don't want the implicit reads
+  not optionalStep(node, _, _)
 }

javascript/ql/lib/semmle/javascript/internal/flow_summaries/Arrays.qll

@@ -484,7 +484,13 @@ class Shift extends SummarizedCallable {
 
   override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
     preservesValue = true and
-    input = "Argument[this].ArrayElement" and
+    input = "Argument[this].ArrayElement[0]" and
+    output = "ReturnValue"
+    or
+    // ArrayElement[0] in the above summary is not automatically converted to a taint step, so manully add
+    // one from the array to the return value.
+    preservesValue = false and
+    input = "Argument[this]" and
     output = "ReturnValue"
   }
 }

javascript/ql/lib/semmle/javascript/internal/flow_summaries/ForOfLoops.qll

@@ -48,10 +48,10 @@ class ForOfLoopStep extends AdditionalFlowInternal {
   ) {
     exists(ForOfStmt stmt |
       pred = getSynthesizedNode(stmt, "for-of-map-key") and
-      contents.asArrayIndex() = 0
+      contents.asSingleton().asArrayIndex() = 0
       or
       pred = getSynthesizedNode(stmt, "for-of-map-value") and
-      contents.asArrayIndex() = 1
+      contents.asSingleton().asArrayIndex() = 1
     |
       succ = DataFlow::lvalueNode(stmt.getLValue())
     )

javascript/ql/lib/semmle/javascript/internal/flow_summaries/Strings.qll

@@ -55,7 +55,9 @@ class StringSplit extends SummarizedCallable {
   StringSplit() { this = "String#split" }
 
   override DataFlow::MethodCallNode getACallSimple() {
-    result.getMethodName() = "split" and result.getNumArgument() = 1
+    result.getMethodName() = "split" and
+    result.getNumArgument() = [1, 2] and
+    not result.getArgument(0).getStringValue() = ["#", "?"]
   }
 
   override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
@@ -64,3 +66,36 @@ class StringSplit extends SummarizedCallable {
     output = "ReturnValue.ArrayElement"
   }
 }
+
+/**
+ * A call of form `x.split("#")` or `x.split("?")`.
+ *
+ * These are of special significance when tracking a tainted URL suffix, such as `window.location.href`,
+ * because the first element of the resulting array should not be considered tainted.
+ *
+ * This summary defaults to the same behaviour as the general `.split()` case, but it contains optional steps
+ * and barriers named `tainted-url-suffix` that should be activated when tracking a tainted URL suffix.
+ */
+class StringSplitHashOrQuestionMark extends SummarizedCallable {
+  StringSplitHashOrQuestionMark() { this = "String#split with '#' or '?'" }
+
+  override DataFlow::MethodCallNode getACallSimple() {
+    result.getMethodName() = "split" and
+    result.getNumArgument() = [1, 2] and
+    result.getArgument(0).getStringValue() = ["#", "?"]
+  }
+
+  override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
+    preservesValue = false and
+    (
+      input = "Argument[this].OptionalBarrier[split-url-suffix]" and
+      output = "ReturnValue.ArrayElement"
+      or
+      input = "Argument[this].OptionalStep[split-url-suffix-pre]" and
+      output = "ReturnValue.ArrayElement[0]"
+      or
+      input = "Argument[this].OptionalStep[split-url-suffix-post]" and
+      output = "ReturnValue.ArrayElement[1]" // TODO: support ArrayElement[1..]
+    )
+  }
+}

javascript/ql/lib/semmle/javascript/security/TaintedUrlSuffix.qll

@@ -4,6 +4,7 @@
  */
 
 import javascript
+private import semmle.javascript.dataflow.internal.DataFlowPrivate as DataFlowPrivate
 
 /**
  * Provides a flow label for reasoning about URLs with a tainted query and fragment part,
@@ -35,14 +36,15 @@ module TaintedUrlSuffix {
     result.getKind().isUrl()
   }
 
-  /** Holds for `pred -> succ` is a step of form `x -> x.p` */
-  private predicate isSafeLocationProp(DataFlow::PropRead read) {
-    // Ignore properties that refer to the scheme, domain, port, auth, or path.
-    read.getPropertyName() =
-      [
-        "protocol", "scheme", "host", "hostname", "domain", "origin", "port", "path", "pathname",
-        "username", "password", "auth"
-      ]
+  /**
+   * Holds if `node` should be a barrier for the given `label`.
+   *
+   * This should be used in the `isBarrier` predicate of a configuration that uses the tainted-url-suffix
+   * label.
+   */
+  predicate isBarrier(Node node, FlowLabel label) {
+    label = label() and
+    DataFlowPrivate::optionalBarrier(node, "split-url-suffix")
   }
 
   /**
@@ -51,11 +53,17 @@ module TaintedUrlSuffix {
    * This handles steps through string operations, promises, URL parsers, and URL accessors.
    */
   predicate step(Node src, Node dst, FlowLabel srclbl, FlowLabel dstlbl) {
-    // Inherit all ordinary taint steps except `x -> x.p` steps
+    // Transition from tainted-url-suffix to general taint when entering the second array element
+    // of a split('#') or split('?') array.
+    //
+    //   x [tainted-url-suffix] --> x.split('#') [array element 1] [taint]
+    //
+    // Technically we should also preverse tainted-url-suffix when entering the first array element of such
+    // a split, but this mostly leads to FPs since we currently don't track if the taint has been through URI-decoding.
+    // (The query/fragment parts are often URI-decoded in practice, but not the other URL parts are not)
     srclbl = label() and
-    dstlbl = label() and
-    TaintTracking::AdditionalTaintStep::step(src, dst) and
-    not isSafeLocationProp(dst)
+    dstlbl.isTaint() and
+    DataFlowPrivate::optionalStep(src, "split-url-suffix-post", dst)
     or
     // Transition from URL suffix to full taint when extracting the query/fragment part.
     srclbl = label() and
@@ -70,11 +78,6 @@ module TaintedUrlSuffix {
         name = StringOps::substringMethodName() and
         not call.getArgument(0).getIntValue() = 0
         or
-        // Split around '#' or '?' and extract the suffix
-        name = "split" and
-        call.getArgument(0).getStringValue() = ["#", "?"] and
-        not exists(call.getAPropertyRead("0")) // Avoid false flow to the prefix
-        or
         // Replace '#' and '?' with nothing
         name = "replace" and
         call.getArgument(0).getStringValue() = ["#", "?"] and

javascript/ql/lib/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll

@@ -35,7 +35,7 @@ module ClientSideUrlRedirect {
    * hence are only partially user-controlled.
    */
   abstract class DocumentUrl extends DataFlow::FlowLabel {
-    DocumentUrl() { this = "document.url" }
+    DocumentUrl() { this = "document.url" } // TODO: replace with TaintedUrlSuffix
   }
 
   /** A source of remote user input, considered as a flow source for unvalidated URL redirects. */

javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll

@@ -76,6 +76,8 @@ module DomBasedXssConfig implements DataFlow::StateConfigSig {
     isOptionallySanitizedNode(node) and
     lbl = [DataFlow::FlowLabel::taint(), prefixLabel(), TaintedUrlSuffix::label()]
     or
+    TaintedUrlSuffix::isBarrier(node, lbl)
+    or
     node = DataFlow::MakeLabeledBarrierGuard<BarrierGuard>::getABarrierNode(lbl)
   }
 

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

@@ -1,11 +1,13 @@
 legacyDataFlowDifference
+| arrays.js:2:16:2:23 | "source" | arrays.js:58:8:58:13 | arr[0] | only flow with NEW data flow library |
 flow
 | arrays.js:2:16:2:23 | "source" | arrays.js:5:8:5:14 | obj.foo |
 | arrays.js:2:16:2:23 | "source" | arrays.js:11:10:11:15 | arr[i] |
 | arrays.js:2:16:2:23 | "source" | arrays.js:15:27:15:27 | e |
 | arrays.js:2:16:2:23 | "source" | arrays.js:16:23:16:23 | e |
 | arrays.js:2:16:2:23 | "source" | arrays.js:20:8:20:16 | arr.pop() |
 | arrays.js:2:16:2:23 | "source" | arrays.js:39:8:39:24 | arr4_spread.pop() |
+| arrays.js:2:16:2:23 | "source" | arrays.js:58:8:58:13 | arr[0] |
 | arrays.js:2:16:2:23 | "source" | arrays.js:61:10:61:10 | x |
 | arrays.js:2:16:2:23 | "source" | arrays.js:65:10:65:10 | x |
 | arrays.js:2:16:2:23 | "source" | arrays.js:69:10:69:10 | x |