Skip to content

Commit e01cbb2

Browse files
erik-krogherik-krogh
authored
Merge pull request #10378 from erik-krogh/aliasFlow
JS: expand localFieldStep to use access-paths, and build access-paths in more cases
2 parents 552c524 + fc38bf0 commit e01cbb2

File tree

7 files changed

+297
-3
lines changed

7 files changed

+297
-3
lines changed

‎javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,8 @@ module AccessPath {
233233
baseName = fromReference(write.getBase(), root)
234234
or
235235
baseName = fromRhs(write.getBase(), root)
236+
or
237+
baseName = fromRhs(GetLaterAccess::getLaterBaseAccess(write), root)
236238
)
237239
or
238240
exists(GlobalVariable var |
@@ -266,6 +268,100 @@ module AccessPath {
266268
)
267269
}
268270

271+
/** A module for computing an access to a variable that happens after a property has been written onto it */
272+
private module GetLaterAccess {
273+
/**
274+
* Gets an access to a variable that is written to in `write`, where the access is after the write.
275+
*
276+
* This allows `fromRhs` to compute an access path for e.g. the below example:
277+
* ```JavaScript
278+
* function foo(x) {
279+
* var obj = {
280+
* bar: x // `x` has the access path "foo.bar" starting from the root `this`.
281+
* };
282+
* this.foo = obj;
283+
* }
284+
* ```
285+
*/
286+
pragma[noopt]
287+
DataFlow::Node getLaterBaseAccess(DataFlow::PropWrite write) {
288+
exists(
289+
ControlFlowNode writeNode, BindingPattern access, VarRef otherAccess, Variable variable,
290+
StmtContainer container
291+
|
292+
access = getBaseVar(write) and
293+
writeNode = write.getWriteNode() and
294+
access = getAnAccessInContainer(variable, container, true) and
295+
variable = getARelevantVariable() and // manual magic
296+
otherAccess = getAnAccessInContainer(variable, container, false) and
297+
access != otherAccess and
298+
result.asExpr() = otherAccess
299+
|
300+
exists(BasicBlock bb, int i, int j |
301+
bb.getNode(i) = writeNode and
302+
bb.getNode(j) = otherAccess and
303+
i < j
304+
)
305+
or
306+
otherAccess.getBasicBlock() = getASuccessorBBThatReadsVar(write) // more manual magic - outlined into a helper predicate.
307+
)
308+
}
309+
310+
/** Gets a variable ref that `write` writes a property to. */
311+
VarRef getBaseVar(DataFlow::PropWrite write) {
312+
result = write.getBase().asExpr()
313+
or
314+
exists(Assignment assign |
315+
write.getBase().asExpr() = assign.getRhs() and
316+
result = assign.getLhs()
317+
)
318+
or
319+
exists(VariableDeclarator decl |
320+
write.getBase().asExpr() = decl.getInit() and
321+
result = decl.getBindingPattern()
322+
)
323+
}
324+
325+
/** Gets an access to `var` inside `container` where `usedInWrite` indicates whether the access is the base of a property write. */
326+
private VarRef getAnAccessInContainer(Variable var, StmtContainer container, boolean usedInWrite) {
327+
result.getVariable() = var and
328+
result.getContainer() = container and
329+
var.isLocal() and
330+
if result = getBaseVar(_) then usedInWrite = true else usedInWrite = false
331+
}
332+
333+
/** Gets a variable that is relevant for the computations in the `GetLaterAccess` module. */
334+
private Variable getARelevantVariable() {
335+
// The variable might be used where `getLaterBaseAccess()` is called.
336+
exists(DataFlow::Node node |
337+
exists(fromRhs(node, _)) and
338+
node.asExpr().(VarAccess).getVariable() = result
339+
) and
340+
// There is a write that writes to the variable.
341+
getBaseVar(_).getVariable() = result and
342+
// It's local.
343+
result.isLocal() and // we skip global variables, because that turns messy quick.
344+
// There is both a "write" and "read" in the same container of the variable.
345+
exists(StmtContainer container |
346+
exists(getAnAccessInContainer(result, container, true)) and // a "write", an access to the variable that is the base of a property reference.
347+
exists(getAnAccessInContainer(result, container, false)) // a "read", an access to the variable that is not the base of a property reference.
348+
)
349+
}
350+
351+
/** Gets a basic-block that has a read of the variable that is written to by `write`, where the basicblock occurs after `start`. */
352+
private ReachableBasicBlock getASuccessorBBThatReadsVar(DataFlow::PropWrite write) {
353+
exists(VarAccess baseExpr, Variable var, ControlFlowNode writeNode |
354+
baseExpr = getBaseVar(write) and
355+
var = baseExpr.getVariable() and
356+
var = getARelevantVariable() and
357+
writeNode = write.getWriteNode() and
358+
writeNode.getBasicBlock().(ReachableBasicBlock).strictlyDominates(result) and
359+
// manual magic.
360+
result = getAnAccessInContainer(getARelevantVariable(), _, false).getBasicBlock()
361+
)
362+
}
363+
}
364+
269365
/**
270366
* Gets a node that refers to the given access path relative to the given `root` node,
271367
* or `root` itself if the access path is empty.

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1692,10 +1692,10 @@ module DataFlow {
16921692
*/
16931693
predicate localFieldStep(DataFlow::Node pred, DataFlow::Node succ) {
16941694
exists(ClassNode cls, string prop |
1695-
pred = cls.getADirectSuperClass*().getAReceiverNode().getAPropertyWrite(prop).getRhs() or
1695+
pred = AccessPath::getAnAssignmentTo(cls.getADirectSuperClass*().getAReceiverNode(), prop) or
16961696
pred = cls.getInstanceMethod(prop)
16971697
|
1698-
succ = cls.getAReceiverNode().getAPropertyRead(prop)
1698+
succ = AccessPath::getAReferenceTo(cls.getAReceiverNode(), prop)
16991699
)
17001700
}
17011701

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,10 @@ nodes
170170
| lib/lib.js:277:23:277:26 | opts |
171171
| lib/lib.js:277:23:277:30 | opts.bla |
172172
| lib/lib.js:277:23:277:30 | opts.bla |
173+
| lib/lib.js:279:19:279:22 | opts |
174+
| lib/lib.js:279:19:279:26 | opts.bla |
175+
| lib/lib.js:281:23:281:35 | this.opts.bla |
176+
| lib/lib.js:281:23:281:35 | this.opts.bla |
173177
| lib/lib.js:307:39:307:42 | name |
174178
| lib/lib.js:307:39:307:42 | name |
175179
| lib/lib.js:308:23:308:26 | name |
@@ -504,8 +508,13 @@ edges
504508
| lib/lib.js:268:22:268:24 | obj | lib/lib.js:268:22:268:32 | obj.version |
505509
| lib/lib.js:276:8:276:11 | opts | lib/lib.js:277:23:277:26 | opts |
506510
| lib/lib.js:276:8:276:11 | opts | lib/lib.js:277:23:277:26 | opts |
511+
| lib/lib.js:276:8:276:11 | opts | lib/lib.js:279:19:279:22 | opts |
512+
| lib/lib.js:276:8:276:11 | opts | lib/lib.js:279:19:279:22 | opts |
507513
| lib/lib.js:277:23:277:26 | opts | lib/lib.js:277:23:277:30 | opts.bla |
508514
| lib/lib.js:277:23:277:26 | opts | lib/lib.js:277:23:277:30 | opts.bla |
515+
| lib/lib.js:279:19:279:22 | opts | lib/lib.js:279:19:279:26 | opts.bla |
516+
| lib/lib.js:279:19:279:26 | opts.bla | lib/lib.js:281:23:281:35 | this.opts.bla |
517+
| lib/lib.js:279:19:279:26 | opts.bla | lib/lib.js:281:23:281:35 | this.opts.bla |
509518
| lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name |
510519
| lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name |
511520
| lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name |
@@ -714,6 +723,7 @@ edges
714723
| lib/lib.js:261:11:261:33 | "rm -rf ... + name | lib/lib.js:257:35:257:38 | name | lib/lib.js:261:30:261:33 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:257:35:257:38 | name | library input | lib/lib.js:261:3:261:34 | cp.exec ... + name) | shell command |
715724
| lib/lib.js:268:10:268:32 | "rm -rf ... version | lib/lib.js:267:46:267:48 | obj | lib/lib.js:268:22:268:32 | obj.version | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:267:46:267:48 | obj | library input | lib/lib.js:268:2:268:33 | cp.exec ... ersion) | shell command |
716725
| lib/lib.js:277:11:277:30 | "rm -rf " + opts.bla | lib/lib.js:276:8:276:11 | opts | lib/lib.js:277:23:277:30 | opts.bla | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:276:8:276:11 | opts | library input | lib/lib.js:277:3:277:31 | cp.exec ... ts.bla) | shell command |
726+
| lib/lib.js:281:11:281:35 | "rm -rf ... pts.bla | lib/lib.js:276:8:276:11 | opts | lib/lib.js:281:23:281:35 | this.opts.bla | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:276:8:276:11 | opts | library input | lib/lib.js:281:3:281:36 | cp.exec ... ts.bla) | shell command |
717727
| lib/lib.js:308:11:308:26 | "rm -rf " + name | lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:307:39:307:42 | name | library input | lib/lib.js:308:3:308:27 | cp.exec ... + name) | shell command |
718728
| lib/lib.js:315:10:315:25 | "rm -rf " + name | lib/lib.js:314:40:314:43 | name | lib/lib.js:315:22:315:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:314:40:314:43 | name | library input | lib/lib.js:315:2:315:26 | cp.exec ... + name) | shell command |
719729
| lib/lib.js:320:11:320:26 | "rm -rf " + name | lib/lib.js:314:40:314:43 | name | lib/lib.js:320:23:320:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:314:40:314:43 | name | library input | lib/lib.js:320:3:320:27 | cp.exec ... + name) | shell command |

‎javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -278,7 +278,7 @@ module.exports.Foo = class Foo {
278278
this.opts = {};
279279
this.opts.bla = opts.bla
280280

281-
cp.exec("rm -rf " + this.opts.bla); // NOT OK - but FN [INCONSISTENCY]
281+
cp.exec("rm -rf " + this.opts.bla); // NOT OK
282282
}
283283
}
284284

‎javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/UnsafeCodeConstruction.expected

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,41 @@ nodes
1515
| lib/index.js:19:26:19:29 | data |
1616
| lib/index.js:22:7:22:10 | data |
1717
| lib/index.js:22:7:22:10 | data |
18+
| lib/index.js:41:32:41:35 | opts |
19+
| lib/index.js:41:32:41:35 | opts |
20+
| lib/index.js:42:3:42:19 | opts |
21+
| lib/index.js:42:10:42:13 | opts |
22+
| lib/index.js:42:10:42:19 | opts \|\| {} |
23+
| lib/index.js:44:21:44:24 | opts |
24+
| lib/index.js:44:21:44:32 | opts.varName |
25+
| lib/index.js:51:21:51:32 | opts.varName |
26+
| lib/index.js:51:21:51:32 | opts.varName |
27+
| lib/index.js:51:21:51:32 | opts.varName |
28+
| lib/index.js:86:15:86:19 | taint |
29+
| lib/index.js:86:15:86:19 | taint |
30+
| lib/index.js:87:18:87:22 | taint |
31+
| lib/index.js:89:36:89:40 | taint |
32+
| lib/index.js:93:32:93:36 | taint |
33+
| lib/index.js:98:30:98:34 | taint |
34+
| lib/index.js:103:21:103:47 | this.op ... dOption |
35+
| lib/index.js:103:21:103:47 | this.op ... dOption |
36+
| lib/index.js:104:21:104:47 | this.op ... dOption |
37+
| lib/index.js:104:21:104:47 | this.op ... dOption |
38+
| lib/index.js:105:21:105:47 | this.op ... dOption |
39+
| lib/index.js:105:21:105:47 | this.op ... dOption |
40+
| lib/index.js:106:21:106:30 | this.taint |
41+
| lib/index.js:106:21:106:30 | this.taint |
42+
| lib/index.js:112:17:112:21 | taint |
43+
| lib/index.js:112:17:112:21 | taint |
44+
| lib/index.js:113:20:113:24 | taint |
45+
| lib/index.js:121:34:121:38 | taint |
46+
| lib/index.js:129:32:129:36 | taint |
47+
| lib/index.js:136:23:136:49 | this.op ... dOption |
48+
| lib/index.js:136:23:136:49 | this.op ... dOption |
49+
| lib/index.js:137:23:137:49 | this.op ... dOption |
50+
| lib/index.js:137:23:137:49 | this.op ... dOption |
51+
| lib/index.js:138:23:138:32 | this.taint |
52+
| lib/index.js:138:23:138:32 | this.taint |
1853
edges
1954
| lib/index.js:1:35:1:38 | data | lib/index.js:2:21:2:24 | data |
2055
| lib/index.js:1:35:1:38 | data | lib/index.js:2:21:2:24 | data |
@@ -32,8 +67,53 @@ edges
3267
| lib/index.js:19:26:19:29 | data | lib/index.js:22:7:22:10 | data |
3368
| lib/index.js:19:26:19:29 | data | lib/index.js:22:7:22:10 | data |
3469
| lib/index.js:19:26:19:29 | data | lib/index.js:22:7:22:10 | data |
70+
| lib/index.js:41:32:41:35 | opts | lib/index.js:42:10:42:13 | opts |
71+
| lib/index.js:41:32:41:35 | opts | lib/index.js:42:10:42:13 | opts |
72+
| lib/index.js:42:3:42:19 | opts | lib/index.js:44:21:44:24 | opts |
73+
| lib/index.js:42:10:42:13 | opts | lib/index.js:42:10:42:19 | opts \|\| {} |
74+
| lib/index.js:42:10:42:19 | opts \|\| {} | lib/index.js:42:3:42:19 | opts |
75+
| lib/index.js:44:21:44:24 | opts | lib/index.js:44:21:44:32 | opts.varName |
76+
| lib/index.js:44:21:44:32 | opts.varName | lib/index.js:51:21:51:32 | opts.varName |
77+
| lib/index.js:44:21:44:32 | opts.varName | lib/index.js:51:21:51:32 | opts.varName |
78+
| lib/index.js:44:21:44:32 | opts.varName | lib/index.js:51:21:51:32 | opts.varName |
79+
| lib/index.js:86:15:86:19 | taint | lib/index.js:87:18:87:22 | taint |
80+
| lib/index.js:86:15:86:19 | taint | lib/index.js:87:18:87:22 | taint |
81+
| lib/index.js:86:15:86:19 | taint | lib/index.js:89:36:89:40 | taint |
82+
| lib/index.js:86:15:86:19 | taint | lib/index.js:89:36:89:40 | taint |
83+
| lib/index.js:86:15:86:19 | taint | lib/index.js:93:32:93:36 | taint |
84+
| lib/index.js:86:15:86:19 | taint | lib/index.js:93:32:93:36 | taint |
85+
| lib/index.js:86:15:86:19 | taint | lib/index.js:98:30:98:34 | taint |
86+
| lib/index.js:86:15:86:19 | taint | lib/index.js:98:30:98:34 | taint |
87+
| lib/index.js:87:18:87:22 | taint | lib/index.js:106:21:106:30 | this.taint |
88+
| lib/index.js:87:18:87:22 | taint | lib/index.js:106:21:106:30 | this.taint |
89+
| lib/index.js:89:36:89:40 | taint | lib/index.js:103:21:103:47 | this.op ... dOption |
90+
| lib/index.js:89:36:89:40 | taint | lib/index.js:103:21:103:47 | this.op ... dOption |
91+
| lib/index.js:93:32:93:36 | taint | lib/index.js:104:21:104:47 | this.op ... dOption |
92+
| lib/index.js:93:32:93:36 | taint | lib/index.js:104:21:104:47 | this.op ... dOption |
93+
| lib/index.js:98:30:98:34 | taint | lib/index.js:105:21:105:47 | this.op ... dOption |
94+
| lib/index.js:98:30:98:34 | taint | lib/index.js:105:21:105:47 | this.op ... dOption |
95+
| lib/index.js:112:17:112:21 | taint | lib/index.js:113:20:113:24 | taint |
96+
| lib/index.js:112:17:112:21 | taint | lib/index.js:113:20:113:24 | taint |
97+
| lib/index.js:112:17:112:21 | taint | lib/index.js:121:34:121:38 | taint |
98+
| lib/index.js:112:17:112:21 | taint | lib/index.js:121:34:121:38 | taint |
99+
| lib/index.js:112:17:112:21 | taint | lib/index.js:129:32:129:36 | taint |
100+
| lib/index.js:112:17:112:21 | taint | lib/index.js:129:32:129:36 | taint |
101+
| lib/index.js:113:20:113:24 | taint | lib/index.js:138:23:138:32 | this.taint |
102+
| lib/index.js:113:20:113:24 | taint | lib/index.js:138:23:138:32 | this.taint |
103+
| lib/index.js:121:34:121:38 | taint | lib/index.js:136:23:136:49 | this.op ... dOption |
104+
| lib/index.js:121:34:121:38 | taint | lib/index.js:136:23:136:49 | this.op ... dOption |
105+
| lib/index.js:129:32:129:36 | taint | lib/index.js:137:23:137:49 | this.op ... dOption |
106+
| lib/index.js:129:32:129:36 | taint | lib/index.js:137:23:137:49 | this.op ... dOption |
35107
#select
36108
| lib/index.js:2:21:2:24 | data | lib/index.js:1:35:1:38 | data | lib/index.js:2:21:2:24 | data | This string concatenation which depends on $@ is later $@. | lib/index.js:1:35:1:38 | data | library input | lib/index.js:2:15:2:30 | "(" + data + ")" | interpreted as code |
37109
| lib/index.js:6:26:6:29 | name | lib/index.js:5:35:5:38 | name | lib/index.js:6:26:6:29 | name | This string concatenation which depends on $@ is later $@. | lib/index.js:5:35:5:38 | name | library input | lib/index.js:6:17:6:29 | "obj." + name | interpreted as code |
38110
| lib/index.js:14:21:14:24 | data | lib/index.js:13:38:13:41 | data | lib/index.js:14:21:14:24 | data | This string concatenation which depends on $@ is later $@. | lib/index.js:13:38:13:41 | data | library input | lib/index.js:14:15:14:30 | "(" + data + ")" | interpreted as code |
39111
| lib/index.js:22:7:22:10 | data | lib/index.js:19:26:19:29 | data | lib/index.js:22:7:22:10 | data | This string concatenation which depends on $@ is later $@. | lib/index.js:19:26:19:29 | data | library input | lib/index.js:25:24:25:26 | str | interpreted as code |
112+
| lib/index.js:51:21:51:32 | opts.varName | lib/index.js:41:32:41:35 | opts | lib/index.js:51:21:51:32 | opts.varName | This string concatenation which depends on $@ is later $@. | lib/index.js:41:32:41:35 | opts | library input | lib/index.js:51:10:51:52 | " var ... ing();" | interpreted as code |
113+
| lib/index.js:103:21:103:47 | this.op ... dOption | lib/index.js:86:15:86:19 | taint | lib/index.js:103:21:103:47 | this.op ... dOption | This string concatenation which depends on $@ is later $@. | lib/index.js:86:15:86:19 | taint | library input | lib/index.js:103:10:103:67 | " var ... ing();" | interpreted as code |
114+
| lib/index.js:104:21:104:47 | this.op ... dOption | lib/index.js:86:15:86:19 | taint | lib/index.js:104:21:104:47 | this.op ... dOption | This string concatenation which depends on $@ is later $@. | lib/index.js:86:15:86:19 | taint | library input | lib/index.js:104:10:104:67 | " var ... ing();" | interpreted as code |
115+
| lib/index.js:105:21:105:47 | this.op ... dOption | lib/index.js:86:15:86:19 | taint | lib/index.js:105:21:105:47 | this.op ... dOption | This string concatenation which depends on $@ is later $@. | lib/index.js:86:15:86:19 | taint | library input | lib/index.js:105:10:105:67 | " var ... ing();" | interpreted as code |
116+
| lib/index.js:106:21:106:30 | this.taint | lib/index.js:86:15:86:19 | taint | lib/index.js:106:21:106:30 | this.taint | This string concatenation which depends on $@ is later $@. | lib/index.js:86:15:86:19 | taint | library input | lib/index.js:106:10:106:50 | " var ... ing();" | interpreted as code |
117+
| lib/index.js:136:23:136:49 | this.op ... dOption | lib/index.js:112:17:112:21 | taint | lib/index.js:136:23:136:49 | this.op ... dOption | This string concatenation which depends on $@ is later $@. | lib/index.js:112:17:112:21 | taint | library input | lib/index.js:136:12:136:69 | " var ... ing();" | interpreted as code |
118+
| lib/index.js:137:23:137:49 | this.op ... dOption | lib/index.js:112:17:112:21 | taint | lib/index.js:137:23:137:49 | this.op ... dOption | This string concatenation which depends on $@ is later $@. | lib/index.js:112:17:112:21 | taint | library input | lib/index.js:137:12:137:69 | " var ... ing();" | interpreted as code |
119+
| lib/index.js:138:23:138:32 | this.taint | lib/index.js:112:17:112:21 | taint | lib/index.js:138:23:138:32 | this.taint | This string concatenation which depends on $@ is later $@. | lib/index.js:112:17:112:21 | taint | library input | lib/index.js:138:12:138:52 | " var ... ing();" | interpreted as code |

0 commit comments

Comments
 (0)