Skip to content

Commit 58c8b5f

Browse files
asgerfasgerf
authored
Merge pull request #18790 from asgerf/js/no-implicit-array-taint
JS: Do not taint whole array when storing into ArrayElement
2 parents e1c2805 + 804a1a6 commit 58c8b5f

File tree

18 files changed

+254
-83
lines changed

18 files changed

+254
-83
lines changed

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -409,7 +409,7 @@ module TaintTracking {
409409
not assgn.getWriteNode() instanceof Property and // not a write inside an object literal
410410
pred = assgn.getRhs() and
411411
assgn = obj.getAPropertyWrite() and
412-
succ = obj
412+
succ = assgn.getBase().getPostUpdateNode()
413413
|
414414
obj instanceof DataFlow::ObjectLiteralNode
415415
or

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

+17
Original file line numberDiff line numberDiff line change
@@ -1432,6 +1432,23 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
14321432
c = ContentSet::arrayElementLowerBound(pos.asPositionalLowerBound())
14331433
)
14341434
)
1435+
or
1436+
// Implicitly read array elements before stringification
1437+
stringifiedNode(node1) and
1438+
node2 = node1 and
1439+
c = ContentSet::arrayElement()
1440+
}
1441+
1442+
private predicate stringifiedNode(Node node) {
1443+
exists(Expr e | node = TValueNode(e) |
1444+
e = any(AddExpr add).getAnOperand() and
1445+
not e instanceof StringLiteral
1446+
or
1447+
e = any(TemplateLiteral t).getAnElement() and
1448+
not e instanceof TemplateElement
1449+
)
1450+
or
1451+
node = DataFlow::globalVarRef("String").getAnInvocation().getArgument(0)
14351452
}
14361453

14371454
/** Gets the post-update node for which `node` is the corresponding pre-update node. */

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

+1-4
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,10 @@ predicate defaultAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2)
1414
FlowSummaryPrivate::Steps::summaryLocalStep(node1.(FlowSummaryNode).getSummaryNode(),
1515
node2.(FlowSummaryNode).getSummaryNode(), false, _) // TODO: preserve 'model' parameter
1616
or
17-
// Convert steps into and out of array elements to plain taint steps
17+
// Convert steps out of array elements to plain taint steps
1818
FlowSummaryPrivate::Steps::summaryReadStep(node1.(FlowSummaryNode).getSummaryNode(),
1919
ContentSet::arrayElement(), node2.(FlowSummaryNode).getSummaryNode())
2020
or
21-
FlowSummaryPrivate::Steps::summaryStoreStep(node1.(FlowSummaryNode).getSummaryNode(),
22-
ContentSet::arrayElement(), node2.(FlowSummaryNode).getSummaryNode())
23-
or
2421
// If the spread argument itself is tainted (not inside a content), store it into the dynamic argument array.
2522
exists(InvokeExpr invoke, Content c |
2623
node1 = TValueNode(invoke.getAnArgument().stripParens().(SpreadElement).getOperand()) and

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

+175-2
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ module LodashUnderscore {
9696
/**
9797
* A data flow step propagating an exception thrown from a callback to a Lodash/Underscore function.
9898
*/
99-
private class ExceptionStep extends DataFlow::SharedFlowStep {
99+
private class ExceptionStep extends DataFlow::LegacyFlowStep {
100100
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
101101
exists(DataFlow::CallNode call, string name |
102102
// Members ending with By, With, or While indicate that they are a variant of
@@ -144,7 +144,13 @@ module LodashUnderscore {
144144
name = ["union", "zip"] and
145145
pred = call.getAnArgument() and
146146
succ = call
147-
or
147+
)
148+
}
149+
150+
private predicate underscoreTaintStepLegacy(DataFlow::Node pred, DataFlow::Node succ) {
151+
exists(string name, DataFlow::CallNode call |
152+
call = any(Member member | member.getName() = name).getACall()
153+
|
148154
name =
149155
["each", "map", "every", "some", "max", "min", "sortBy", "partition", "mapObject", "tap"] and
150156
pred = call.getArgument(0) and
@@ -168,6 +174,173 @@ module LodashUnderscore {
168174
underscoreTaintStep(pred, succ)
169175
}
170176
}
177+
178+
private class UnderscoreTaintStepLegacy extends TaintTracking::LegacyTaintStep {
179+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
180+
underscoreTaintStepLegacy(pred, succ)
181+
}
182+
}
183+
184+
private class LodashEach extends DataFlow::SummarizedCallable {
185+
LodashEach() { this = "_.each-like" }
186+
187+
override DataFlow::CallNode getACall() {
188+
result = member(["each", "eachRight", "forEach", "forEachRight", "every", "some"]).getACall()
189+
}
190+
191+
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
192+
preservesValue = true and
193+
input = "Argument[0].ArrayElement" and
194+
output = "Argument[1].Parameter[0]"
195+
}
196+
}
197+
198+
private class LodashMap extends DataFlow::SummarizedCallable {
199+
LodashMap() { this = "_.map" }
200+
201+
override DataFlow::CallNode getACall() { result = member("map").getACall() }
202+
203+
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
204+
(
205+
input = "Argument[0].ArrayElement" and
206+
output = "Argument[1].Parameter[0]"
207+
or
208+
input = "Argument[1].ReturnValue" and
209+
output = "ReturnValue.ArrayElement"
210+
) and
211+
preservesValue = true
212+
}
213+
}
214+
215+
private class LodashFlatMap extends DataFlow::SummarizedCallable {
216+
LodashFlatMap() { this = "_.flatMap" }
217+
218+
override DataFlow::CallNode getACall() { result = member("flatMap").getACall() }
219+
220+
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
221+
(
222+
input = "Argument[0].ArrayElement" and
223+
output = "Argument[1].Parameter[0]"
224+
or
225+
input = "Argument[1].ReturnValue.WithoutArrayElement" and
226+
output = "ReturnValue.ArrayElement"
227+
or
228+
input = "Argument[1].ReturnValue.ArrayElement" and
229+
output = "ReturnValue.ArrayElement"
230+
) and
231+
preservesValue = true
232+
}
233+
}
234+
235+
private class LodashFlatMapDeep extends DataFlow::SummarizedCallable {
236+
LodashFlatMapDeep() { this = "_.flatMapDeep" }
237+
238+
override DataFlow::CallNode getACall() {
239+
result = member(["flatMapDeep", "flatMapDepth"]).getACall()
240+
}
241+
242+
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
243+
(
244+
input = "Argument[0].ArrayElement" and
245+
output = "Argument[1].Parameter[0]"
246+
or
247+
input = "Argument[1].ReturnValue.WithoutArrayElement" and
248+
output = "ReturnValue.ArrayElement"
249+
or
250+
input = "Argument[1].ReturnValue.ArrayElementDeep" and
251+
output = "ReturnValue.ArrayElement"
252+
) and
253+
preservesValue = true
254+
}
255+
}
256+
257+
private class LodashReduce extends DataFlow::SummarizedCallable {
258+
LodashReduce() { this = "_.reduce-like" }
259+
260+
override DataFlow::CallNode getACall() { result = member(["reduce", "reduceRight"]).getACall() }
261+
262+
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
263+
(
264+
input = "Argument[0].ArrayElement" and
265+
output = "Argument[1].Parameter[1]"
266+
or
267+
input = ["Argument[1].ReturnValue", "Argument[2]"] and
268+
output = ["ReturnValue", "Argument[1].Parameter[0]"]
269+
) and
270+
preservesValue = true
271+
}
272+
}
273+
274+
private class LoashSortBy extends DataFlow::SummarizedCallable {
275+
LoashSortBy() { this = "_.sortBy-like" }
276+
277+
override DataFlow::CallNode getACall() { result = member(["sortBy", "orderBy"]).getACall() }
278+
279+
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
280+
input = "Argument[0].ArrayElement" and
281+
output =
282+
[
283+
"Argument[1].Parameter[0]", "Argument[1].ArrayElement.Parameter[0]",
284+
"ReturnValue.ArrayElement"
285+
] and
286+
preservesValue = true
287+
}
288+
}
289+
290+
private class LodashMinMaxBy extends DataFlow::SummarizedCallable {
291+
LodashMinMaxBy() { this = "_.minBy / _.maxBy" }
292+
293+
override DataFlow::CallNode getACall() { result = member(["minBy", "maxBy"]).getACall() }
294+
295+
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
296+
input = "Argument[0].ArrayElement" and
297+
output = ["Argument[1].Parameter[1]", "ReturnValue"] and
298+
preservesValue = true
299+
}
300+
}
301+
302+
private class LodashPartition extends DataFlow::SummarizedCallable {
303+
LodashPartition() { this = "_.partition" }
304+
305+
override DataFlow::CallNode getACall() { result = member("partition").getACall() }
306+
307+
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
308+
input = "Argument[0].ArrayElement" and
309+
output = ["Argument[1].Parameter[1]", "ReturnValue.ArrayElement.ArrayElement"] and
310+
preservesValue = true
311+
}
312+
}
313+
314+
private class UnderscoreMapObject extends DataFlow::SummarizedCallable {
315+
UnderscoreMapObject() { this = "_.mapObject" }
316+
317+
override DataFlow::CallNode getACall() { result = member("mapObject").getACall() }
318+
319+
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
320+
// Just collapse all properties with AnyMember. We could be more precise by generating a summary
321+
// for each property name, but for a rarely-used method like this it dosn't seem worth it.
322+
(
323+
input = "Argument[0].AnyMember" and
324+
output = "Argument[1].Parameter[1]"
325+
or
326+
input = "Argument[1].ReturnValue" and
327+
output = "ReturnValue.AnyMember"
328+
) and
329+
preservesValue = true
330+
}
331+
}
332+
333+
private class LodashTap extends DataFlow::SummarizedCallable {
334+
LodashTap() { this = "_.tap" }
335+
336+
override DataFlow::CallNode getACall() { result = member("tap").getACall() }
337+
338+
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
339+
input = "Argument[0]" and
340+
output = ["Argument[1].Parameter[0]", "ReturnValue"] and
341+
preservesValue = true
342+
}
343+
}
171344
}
172345

173346
/**

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

+22-1
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
private import javascript
2525
private import semmle.javascript.dataflow.internal.DataFlowNode
2626
private import semmle.javascript.dataflow.FlowSummary
27+
private import Arrays
2728
private import FlowSummaryUtil
2829

2930
class At extends SummarizedCallable {
@@ -41,15 +42,18 @@ class At extends SummarizedCallable {
4142
}
4243

4344
class Concat extends SummarizedCallable {
44-
Concat() { this = "Array#concat / String#concat" }
45+
Concat() { this = "Array#concat / String#concat / Buffer.concat" }
4546

4647
override InstanceCall getACallSimple() { result.getMethodName() = "concat" }
4748

4849
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
50+
// Array#concat.
51+
// Also models Buffer.concat as this happens to out work well with our toString() model.
4952
preservesValue = true and
5053
input = "Argument[this,0..].ArrayElement" and
5154
output = "ReturnValue.ArrayElement"
5255
or
56+
// String#concat
5357
preservesValue = false and
5458
input = "Argument[this,0..]" and
5559
output = "ReturnValue"
@@ -149,3 +153,20 @@ class Values extends SummarizedCallable {
149153
output = "ReturnValue.IteratorElement"
150154
}
151155
}
156+
157+
class ToString extends SummarizedCallable {
158+
ToString() { this = "Object#toString / Array#toString" }
159+
160+
override InstanceCall getACallSimple() {
161+
result.(DataFlow::MethodCallNode).getMethodName() = "toString"
162+
or
163+
result = arrayConstructorRef().getAPropertyRead("prototype").getAMemberCall("toString")
164+
}
165+
166+
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
167+
preservesValue = false and
168+
// Arrays stringify their contents and joins by ","
169+
input = "Argument[this].ArrayElementDeep" and
170+
output = "ReturnValue"
171+
}
172+
}

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

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

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

+2
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ reverseRead
2020
| tst.js:267:28:267:31 | map3 | Origin of readStep is missing a PostUpdateNode. |
2121
argHasPostUpdate
2222
postWithInFlow
23+
| file://:0:0:0:0 | [summary] to write: Argument[0] in _.tap | PostUpdateNode should not be the target of local flow. |
2324
| 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. |
2425
| file://:0:0:0:0 | [summary] to write: Argument[1] in Array#filter | PostUpdateNode should not be the target of local flow. |
2526
| 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
2930
| file://:0:0:0:0 | [summary] to write: Argument[1] in Array#reduce / Array#reduceRight | PostUpdateNode should not be the target of local flow. |
3031
| 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. |
3132
| 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. |
33+
| file://:0:0:0:0 | [summary] to write: Argument[2] in _.reduce-like | PostUpdateNode should not be the target of local flow. |
3234
| file://:0:0:0:0 | [summary] to write: Argument[this] in Array#flatMap | PostUpdateNode should not be the target of local flow. |
3335
| 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. |
3436
| 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

+24
Original file line numberDiff line numberDiff line change
@@ -20,3 +20,27 @@ function shiftTaint() {
2020
sink(array.shift()); // $ hasTaintFlow=shift.directly-tainted
2121
sink(array.shift()); // $ hasTaintFlow=shift.directly-tainted
2222
}
23+
24+
function implicitToString() {
25+
const array = [source('implicitToString.1')];
26+
array.push(source('implicitToString.2'))
27+
28+
sink(array + "foo"); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
29+
sink("foo" + array); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
30+
sink("" + array); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
31+
sink(array + 1); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
32+
sink(1 + array); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
33+
sink(unknown() + array); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
34+
sink(array + unknown()); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
35+
36+
sink(`${array}`); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
37+
sink(`${array} foo`); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
38+
39+
sink(String(array)); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
40+
41+
sink(array.toString()); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
42+
sink(array.toString("utf8")); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
43+
44+
sink(Array.prototype.toString.call(array)); // $ hasTaintFlow=implicitToString.1 hasTaintFlow=implicitToString.2
45+
sink(Object.prototype.toString.call(array)); // OK - returns "[object Array]"
46+
}

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

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

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

-1
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ edges
2727
| 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 | |
2828
| 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 | |
2929
| 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 | |
30-
| 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 | |
3130
| 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 | |
3231
| 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 | |
3332
| 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 | |

0 commit comments

Comments
 (0)