JS: Do not taint whole array when storing into ArrayElement

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

@@ -409,7 +409,7 @@ module TaintTracking {
         not assgn.getWriteNode() instanceof Property and // not a write inside an object literal
         pred = assgn.getRhs() and
         assgn = obj.getAPropertyWrite() and
-        succ = obj
+        succ = assgn.getBase().getPostUpdateNode()
       |
         obj instanceof DataFlow::ObjectLiteralNode
         or

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

@@ -1432,6 +1432,23 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
       c = ContentSet::arrayElementLowerBound(pos.asPositionalLowerBound())
     )
   )
+  or
+  // Implicitly read array elements before stringification
+  stringifiedNode(node1) and
+  node2 = node1 and
+  c = ContentSet::arrayElement()
+}
+
+private predicate stringifiedNode(Node node) {
+  exists(Expr e | node = TValueNode(e) |
+    e = any(AddExpr add).getAnOperand() and
+    not e instanceof StringLiteral
+    or
+    e = any(TemplateLiteral t).getAnElement() and
+    not e instanceof TemplateElement
+  )
+  or
+  node = DataFlow::globalVarRef("String").getAnInvocation().getArgument(0)
 }
 
 /** Gets the post-update node for which `node` is the corresponding pre-update node. */

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

@@ -14,13 +14,10 @@ predicate defaultAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2)
   FlowSummaryPrivate::Steps::summaryLocalStep(node1.(FlowSummaryNode).getSummaryNode(),
     node2.(FlowSummaryNode).getSummaryNode(), false, _) // TODO: preserve 'model' parameter
   or
-  // Convert steps into and out of array elements to plain taint steps
+  // Convert steps out of array elements to plain taint steps
   FlowSummaryPrivate::Steps::summaryReadStep(node1.(FlowSummaryNode).getSummaryNode(),
     ContentSet::arrayElement(), node2.(FlowSummaryNode).getSummaryNode())
   or
-  FlowSummaryPrivate::Steps::summaryStoreStep(node1.(FlowSummaryNode).getSummaryNode(),
-    ContentSet::arrayElement(), node2.(FlowSummaryNode).getSummaryNode())
-  or
   // If the spread argument itself is tainted (not inside a content), store it into the dynamic argument array.
   exists(InvokeExpr invoke, Content c |
     node1 = TValueNode(invoke.getAnArgument().stripParens().(SpreadElement).getOperand()) and

javascript/ql/lib/semmle/javascript/frameworks/LodashUnderscore.qll

@@ -96,7 +96,7 @@ module LodashUnderscore {
   /**
    * A data flow step propagating an exception thrown from a callback to a Lodash/Underscore function.
    */
-  private class ExceptionStep extends DataFlow::SharedFlowStep {
+  private class ExceptionStep extends DataFlow::LegacyFlowStep {
     override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
       exists(DataFlow::CallNode call, string name |
         // Members ending with By, With, or While indicate that they are a variant of
@@ -144,7 +144,13 @@ module LodashUnderscore {
       name = ["union", "zip"] and
       pred = call.getAnArgument() and
       succ = call
-      or
+    )
+  }
+
+  private predicate underscoreTaintStepLegacy(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(string name, DataFlow::CallNode call |
+      call = any(Member member | member.getName() = name).getACall()
+    |
       name =
         ["each", "map", "every", "some", "max", "min", "sortBy", "partition", "mapObject", "tap"] and
       pred = call.getArgument(0) and
@@ -168,6 +174,173 @@ module LodashUnderscore {
       underscoreTaintStep(pred, succ)
     }
   }
+
+  private class UnderscoreTaintStepLegacy extends TaintTracking::LegacyTaintStep {
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      underscoreTaintStepLegacy(pred, succ)
+    }
+  }
+
+  private class LodashEach extends DataFlow::SummarizedCallable {
+    LodashEach() { this = "_.each-like" }
+
+    override DataFlow::CallNode getACall() {
+      result = member(["each", "eachRight", "forEach", "forEachRight", "every", "some"]).getACall()
+    }
+
+    override predicate propagatesFlow(string input, string output, boolean preservesValue) {
+      preservesValue = true and
+      input = "Argument[0].ArrayElement" and
+      output = "Argument[1].Parameter[0]"
+    }
+  }
+
+  private class LodashMap extends DataFlow::SummarizedCallable {
+    LodashMap() { this = "_.map" }
+
+    override DataFlow::CallNode getACall() { result = member("map").getACall() }
+
+    override predicate propagatesFlow(string input, string output, boolean preservesValue) {
+      (
+        input = "Argument[0].ArrayElement" and
+        output = "Argument[1].Parameter[0]"
+        or
+        input = "Argument[1].ReturnValue" and
+        output = "ReturnValue.ArrayElement"
+      ) and
+      preservesValue = true
+    }
+  }
+
+  private class LodashFlatMap extends DataFlow::SummarizedCallable {
+    LodashFlatMap() { this = "_.flatMap" }
+
+    override DataFlow::CallNode getACall() { result = member("flatMap").getACall() }
+
+    override predicate propagatesFlow(string input, string output, boolean preservesValue) {
+      (
+        input = "Argument[0].ArrayElement" and
+        output = "Argument[1].Parameter[0]"
+        or
+        input = "Argument[1].ReturnValue.WithoutArrayElement" and
+        output = "ReturnValue.ArrayElement"
+        or
+        input = "Argument[1].ReturnValue.ArrayElement" and
+        output = "ReturnValue.ArrayElement"
+      ) and
+      preservesValue = true
+    }
+  }
+
+  private class LodashFlatMapDeep extends DataFlow::SummarizedCallable {
+    LodashFlatMapDeep() { this = "_.flatMapDeep" }
+
+    override DataFlow::CallNode getACall() {
+      result = member(["flatMapDeep", "flatMapDepth"]).getACall()
+    }
+
+    override predicate propagatesFlow(string input, string output, boolean preservesValue) {
+      (
+        input = "Argument[0].ArrayElement" and
+        output = "Argument[1].Parameter[0]"
+        or
+        input = "Argument[1].ReturnValue.WithoutArrayElement" and
+        output = "ReturnValue.ArrayElement"
+        or
+        input = "Argument[1].ReturnValue.ArrayElementDeep" and
+        output = "ReturnValue.ArrayElement"
+      ) and
+      preservesValue = true
+    }
+  }
+
+  private class LodashReduce extends DataFlow::SummarizedCallable {
+    LodashReduce() { this = "_.reduce-like" }
+
+    override DataFlow::CallNode getACall() { result = member(["reduce", "reduceRight"]).getACall() }
+
+    override predicate propagatesFlow(string input, string output, boolean preservesValue) {
+      (
+        input = "Argument[0].ArrayElement" and
+        output = "Argument[1].Parameter[1]"
+        or
+        input = ["Argument[1].ReturnValue", "Argument[2]"] and
+        output = ["ReturnValue", "Argument[1].Parameter[0]"]
+      ) and
+      preservesValue = true
+    }
+  }
+
+  private class LoashSortBy extends DataFlow::SummarizedCallable {
+    LoashSortBy() { this = "_.sortBy-like" }
+
+    override DataFlow::CallNode getACall() { result = member(["sortBy", "orderBy"]).getACall() }
+
+    override predicate propagatesFlow(string input, string output, boolean preservesValue) {
+      input = "Argument[0].ArrayElement" and
+      output =
+        [
+          "Argument[1].Parameter[0]", "Argument[1].ArrayElement.Parameter[0]",
+          "ReturnValue.ArrayElement"
+        ] and
+      preservesValue = true
+    }
+  }
+
+  private class LodashMinMaxBy extends DataFlow::SummarizedCallable {
+    LodashMinMaxBy() { this = "_.minBy / _.maxBy" }
+
+    override DataFlow::CallNode getACall() { result = member(["minBy", "maxBy"]).getACall() }
+
+    override predicate propagatesFlow(string input, string output, boolean preservesValue) {
+      input = "Argument[0].ArrayElement" and
+      output = ["Argument[1].Parameter[1]", "ReturnValue"] and
+      preservesValue = true
+    }
+  }
+
+  private class LodashPartition extends DataFlow::SummarizedCallable {
+    LodashPartition() { this = "_.partition" }
+
+    override DataFlow::CallNode getACall() { result = member("partition").getACall() }
+
+    override predicate propagatesFlow(string input, string output, boolean preservesValue) {
+      input = "Argument[0].ArrayElement" and
+      output = ["Argument[1].Parameter[1]", "ReturnValue.ArrayElement.ArrayElement"] and
+      preservesValue = true
+    }
+  }
+
+  private class UnderscoreMapObject extends DataFlow::SummarizedCallable {
+    UnderscoreMapObject() { this = "_.mapObject" }
+
+    override DataFlow::CallNode getACall() { result = member("mapObject").getACall() }
+
+    override predicate propagatesFlow(string input, string output, boolean preservesValue) {
+      // Just collapse all properties with AnyMember. We could be more precise by generating a summary
+      // for each property name, but for a rarely-used method like this it dosn't seem worth it.
+      (
+        input = "Argument[0].AnyMember" and
+        output = "Argument[1].Parameter[1]"
+        or
+        input = "Argument[1].ReturnValue" and
+        output = "ReturnValue.AnyMember"
+      ) and
+      preservesValue = true
+    }
+  }
+
+  private class LodashTap extends DataFlow::SummarizedCallable {
+    LodashTap() { this = "_.tap" }
+
+    override DataFlow::CallNode getACall() { result = member("tap").getACall() }
+
+    override predicate propagatesFlow(string input, string output, boolean preservesValue) {
+      input = "Argument[0]" and
+      output = ["Argument[1].Parameter[0]", "ReturnValue"] and
+      preservesValue = true
+    }
+  }
 }
 
 /**

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

@@ -24,6 +24,7 @@
 private import javascript
 private import semmle.javascript.dataflow.internal.DataFlowNode
 private import semmle.javascript.dataflow.FlowSummary
+private import Arrays
 private import FlowSummaryUtil
 
 class At extends SummarizedCallable {
@@ -41,15 +42,18 @@ class At extends SummarizedCallable {
 }
 
 class Concat extends SummarizedCallable {
-  Concat() { this = "Array#concat / String#concat" }
+  Concat() { this = "Array#concat / String#concat / Buffer.concat" }
 
   override InstanceCall getACallSimple() { result.getMethodName() = "concat" }
 
   override predicate propagatesFlow(string input, string output, boolean preservesValue) {
+    // Array#concat.
+    // Also models Buffer.concat as this happens to out work well with our toString() model.
     preservesValue = true and
     input = "Argument[this,0..].ArrayElement" and
     output = "ReturnValue.ArrayElement"
     or
+    // String#concat
     preservesValue = false and
     input = "Argument[this,0..]" and
     output = "ReturnValue"
@@ -149,3 +153,20 @@ class Values extends SummarizedCallable {
     output = "ReturnValue.IteratorElement"
   }
 }
+
+class ToString extends SummarizedCallable {
+  ToString() { this = "Object#toString / Array#toString" }
+
+  override InstanceCall getACallSimple() {
+    result.(DataFlow::MethodCallNode).getMethodName() = "toString"
+    or
+    result = arrayConstructorRef().getAPropertyRead("prototype").getAMemberCall("toString")
+  }
+
+  override predicate propagatesFlow(string input, string output, boolean preservesValue) {
+    preservesValue = false and
+    // Arrays stringify their contents and joins by ","
+    input = "Argument[this].ArrayElementDeep" and
+    output = "ReturnValue"
+  }
+}

javascript/ql/src/change-notes/2025-02-18-no-implicit-array-taint.md

@@ -0,0 +1,5 @@
+---
+category: majorAnalysis
+---
+* Improved precision of data flow through arrays, fixing some spurious flows
+  that would sometimes cause the `length` property of an array to be seen as tainted.

javascript/ql/test/library-tests/FlowSummary/DataFlowConsistency.expected

@@ -20,6 +20,7 @@ reverseRead
 | tst.js:267:28:267:31 | map3 | Origin of readStep is missing a PostUpdateNode. |
 argHasPostUpdate
 postWithInFlow
+| file://:0:0:0:0 | [summary] to write: Argument[0] in _.tap | PostUpdateNode should not be the target of local flow. |
 | file://:0:0:0:0 | [summary] to write: Argument[1] in Array method with flow into callback | PostUpdateNode should not be the target of local flow. |
 | file://:0:0:0:0 | [summary] to write: Argument[1] in Array#filter | PostUpdateNode should not be the target of local flow. |
 | file://:0:0:0:0 | [summary] to write: Argument[1] in Array#find / Array#findLast | PostUpdateNode should not be the target of local flow. |
@@ -29,6 +30,7 @@ postWithInFlow
 | file://:0:0:0:0 | [summary] to write: Argument[1] in Array#reduce / Array#reduceRight | PostUpdateNode should not be the target of local flow. |
 | file://:0:0:0:0 | [summary] to write: Argument[2] in 'array.prototype.find' / 'array-find' | PostUpdateNode should not be the target of local flow. |
 | file://:0:0:0:0 | [summary] to write: Argument[2] in Array.from(arg, callback, [thisArg]) | PostUpdateNode should not be the target of local flow. |
+| file://:0:0:0:0 | [summary] to write: Argument[2] in _.reduce-like | PostUpdateNode should not be the target of local flow. |
 | file://:0:0:0:0 | [summary] to write: Argument[this] in Array#flatMap | PostUpdateNode should not be the target of local flow. |
 | file://:0:0:0:0 | [summary] to write: Argument[this] in Array#forEach / Map#forEach / Set#forEach | PostUpdateNode should not be the target of local flow. |
 | file://:0:0:0:0 | [summary] to write: Argument[this] in Array#map | PostUpdateNode should not be the target of local flow. |

javascript/ql/test/library-tests/TripleDot/arrays.js

@@ -20,3 +20,27 @@ function shiftTaint() {
     sink(array.shift()); // $ hasTaintFlow=shift.directly-tainted
     sink(array.shift()); // $ hasTaintFlow=shift.directly-tainted
 }
+
+function implicitToString() {
+    const array = [source('implicitToString.1')];
+    array.push(source('implicitToString.2'))
+
+    sink(array + "foo"); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
+    sink("foo" + array); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
+    sink("" + array); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
+    sink(array + 1); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
+    sink(1 + array); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
+    sink(unknown() + array); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
+    sink(array + unknown()); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
+
+    sink(`${array}`); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
+    sink(`${array} foo`); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
+
+    sink(String(array)); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
+
+    sink(array.toString()); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
+    sink(array.toString("utf8")); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
+
+    sink(Array.prototype.toString.call(array)); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
+    sink(Object.prototype.toString.call(array)); // OK - returns "[object Array]"
+}

javascript/ql/test/library-tests/TripleDot/buffer.js

@@ -0,0 +1,7 @@
+import 'dummy';
+
+function t1() {
+    const b1 = Buffer.from(source("t1.1"));
+    const b2 = Buffer.from(source("t1.2"));
+    sink(Buffer.concat([b1, b2]).toString("utf8")); // $ hasTaintFlow=t1.1 hasTaintFlow=t1.2
+}

javascript/ql/test/query-tests/Security/CWE-078/IndirectCommandInjection/IndirectCommandInjection.expected

@@ -27,7 +27,6 @@ edges
 | command-line-parameter-command-injection.js:14:6:14:30 | fewerArgs [ArrayElement] | command-line-parameter-command-injection.js:18:13:18:21 | fewerArgs [ArrayElement] | provenance |  |
 | command-line-parameter-command-injection.js:14:18:14:21 | args | command-line-parameter-command-injection.js:14:18:14:30 | args.slice(1) | provenance |  |
 | command-line-parameter-command-injection.js:14:18:14:21 | args | command-line-parameter-command-injection.js:14:18:14:30 | args.slice(1) [ArrayElement] | provenance |  |
-| command-line-parameter-command-injection.js:14:18:14:21 | args [ArrayElement] | command-line-parameter-command-injection.js:14:18:14:30 | args.slice(1) | provenance |  |
 | command-line-parameter-command-injection.js:14:18:14:21 | args [ArrayElement] | command-line-parameter-command-injection.js:14:18:14:30 | args.slice(1) [ArrayElement] | provenance |  |
 | command-line-parameter-command-injection.js:14:18:14:30 | args.slice(1) | command-line-parameter-command-injection.js:14:6:14:30 | fewerArgs | provenance |  |
 | command-line-parameter-command-injection.js:14:18:14:30 | args.slice(1) [ArrayElement] | command-line-parameter-command-injection.js:14:6:14:30 | fewerArgs [ArrayElement] | provenance |  |