Skip to content

Commit 5aa948c

Browse files
asger-semmleasger-semmle
committed
JS: Add angular.merge sink to prototype pollution query
1 parent bd32931 commit 5aa948c

File tree

5 files changed

+71
-20
lines changed

5 files changed

+71
-20
lines changed

‎javascript/ql/src/Security/CWE-400/PrototypePollution.ql‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,10 @@ import DataFlow::PathGraph
1717
import semmle.javascript.dependencies.Dependencies
1818

1919
from
20-
Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Dependency dependency,
21-
string dependencyId
20+
Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, string moduleName, Locatable dependencyLoc
2221
where
2322
cfg.hasFlowPath(source, sink) and
24-
dependency = sink.getNode().(Sink).getDependency() and
25-
dependency.info(dependencyId, _)
23+
sink.getNode().(Sink).dependencyInfo(moduleName, dependencyLoc)
2624
select sink.getNode(), source, sink,
2725
"Prototype pollution caused by merging a user-controlled value from $@ using a vulnerable version of $@.",
28-
source, "here", dependency, dependencyId
26+
source, "here", dependencyLoc, moduleName

‎javascript/ql/src/semmle/javascript/Extend.qll‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,8 @@ private class ExtendCallDeep extends ExtendCall {
9393
callee = DataFlow::moduleMember("smart-extend", "deep") or
9494
callee = LodashUnderscore::member("merge") or
9595
callee = LodashUnderscore::member("mergeWith") or
96-
callee = LodashUnderscore::member("defaultsDeep")
96+
callee = LodashUnderscore::member("defaultsDeep") or
97+
callee = AngularJS::angular().getAPropertyRead("merge")
9798
)
9899
}
99100

@@ -122,7 +123,8 @@ private class ExtendCallShallow extends ExtendCall {
122123
callee = DataFlow::moduleImport("util-extend") or
123124
callee = DataFlow::moduleImport("utils-merge") or
124125
callee = DataFlow::moduleImport("xtend/mutable") or
125-
callee = LodashUnderscore::member("extend")
126+
callee = LodashUnderscore::member("extend") or
127+
callee = AngularJS::angular().getAPropertyRead("extend")
126128
)
127129
}
128130

‎javascript/ql/src/semmle/javascript/security/dataflow/PrototypePollutionCustomizations.qll‎

Lines changed: 53 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -52,9 +52,17 @@ module PrototypePollution {
5252
abstract DataFlow::FlowLabel getAFlowLabel();
5353

5454
/**
55-
* Gets the dependency that defines this sink.
55+
* DEPRECATED. Override `dependencyInfo` instead.
5656
*/
57-
abstract Dependency getDependency();
57+
deprecated Dependency getDependency() { none() }
58+
59+
/**
60+
* Holds if `moduleName` is the name of the module that defines this sink,
61+
* and `location` is the declaration of that dependency.
62+
*
63+
* If no meaningful `location` exists, it should be bound to the sink itself.
64+
*/
65+
abstract predicate dependencyInfo(string moduleName, Locatable location);
5866
}
5967

6068
/**
@@ -80,12 +88,21 @@ module PrototypePollution {
8088

8189
class DeepExtendSink extends Sink {
8290
ExtendCall call;
83-
84-
Dependency dependency;
91+
string moduleName;
92+
Locatable location;
8593

8694
DeepExtendSink() {
8795
this = call.getASourceOperand() and
88-
isVulnerableDeepExtendCall(call, dependency)
96+
(
97+
exists(Dependency dep |
98+
isVulnerableVersionOfDeepExtendCall(call, dep) and
99+
dep = location and
100+
dep.info(moduleName, _)
101+
)
102+
or
103+
isVulnerableDeepExtendCallAllVersions(call, moduleName) and
104+
location = call.asExpr()
105+
)
89106
}
90107

91108
override DataFlow::FlowLabel getAFlowLabel() {
@@ -94,13 +111,16 @@ module PrototypePollution {
94111
result = TaintedObjectWrapper::label()
95112
}
96113

97-
override Dependency getDependency() { result = dependency }
114+
override predicate dependencyInfo(string moduleName_, Locatable loc) {
115+
moduleName = moduleName_ and
116+
location = loc
117+
}
98118
}
99119

100120
/**
101121
* Holds if `call` is vulnerable to prototype pollution because the callee is defined by `dep`.
102122
*/
103-
predicate isVulnerableDeepExtendCall(ExtendCall call, Dependency dep) {
123+
predicate isVulnerableVersionOfDeepExtendCall(ExtendCall call, Dependency dep) {
104124
call.isDeep() and
105125
(
106126
call = DataFlow::dependencyModuleImport(dep).getAMemberCall(_) or
@@ -110,8 +130,6 @@ module PrototypePollution {
110130
id = "assign-deep" and
111131
version.maybeBefore("0.4.7")
112132
or
113-
id = "deep"
114-
or
115133
id = "deep-extend" and
116134
version.maybeBefore("0.5.1")
117135
or
@@ -121,13 +139,12 @@ module PrototypePollution {
121139
id = "extend" and
122140
(version.maybeBefore("2.0.2") or version.maybeBetween("3.0.0", "3.0.2"))
123141
or
124-
id = "extend2"
125-
or
126-
id = "js-extend"
127-
or
128142
id = "just-extend" and
129143
version.maybeBefore("4.0.1")
130144
or
145+
id = "jquery" and
146+
version.maybeBefore("3.4.0")
147+
or
131148
id = "lodash" + any(string s) and
132149
version.maybeBefore("4.17.12")
133150
or
@@ -142,8 +159,31 @@ module PrototypePollution {
142159
or
143160
id = "node.extend" and
144161
(version.maybeBefore("1.1.7") or version.maybeBetween("2.0.0", "2.0.1"))
162+
)
163+
}
164+
165+
/**
166+
* Holds if `call` comes from a package named `id` and is vulnerable to prototype pollution
167+
* in every version of that package.
168+
*/
169+
predicate isVulnerableDeepExtendCallAllVersions(ExtendCall call, string id) {
170+
call.isDeep() and
171+
(
172+
call = DataFlow::moduleImport(id).getACall() or
173+
call = DataFlow::moduleImport(id).getAMemberCall(_)
174+
) and
175+
(
176+
id = "deep"
177+
or
178+
id = "extend2"
179+
or
180+
id = "js-extend"
145181
or
146182
id = "smart-extend"
147183
)
184+
or
185+
call.isDeep() and
186+
call = AngularJS::angular().getAMemberCall("merge") and
187+
id = "angular"
148188
}
149189
}

‎javascript/ql/test/query-tests/Security/CWE-400/PrototypePollution.expected‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,23 @@
11
nodes
2+
| angularmerge.js:1:30:1:34 | event |
3+
| angularmerge.js:2:21:2:42 | JSON.pa ... t.data) |
4+
| angularmerge.js:2:32:2:36 | event |
5+
| angularmerge.js:2:32:2:41 | event.data |
26
| src-vulnerable-lodash/tst.js:7:17:7:29 | req.query.foo |
37
| src-vulnerable-lodash/tst.js:10:17:12:5 | {\\n ... K\\n } |
48
| src-vulnerable-lodash/tst.js:11:16:11:30 | req.query.value |
59
| src-vulnerable-lodash/tst.js:15:14:15:28 | req.query.value |
610
| src-vulnerable-lodash/tst.js:17:17:19:5 | {\\n ... K\\n } |
711
| src-vulnerable-lodash/tst.js:18:16:18:25 | opts.thing |
812
edges
13+
| angularmerge.js:1:30:1:34 | event | angularmerge.js:2:32:2:36 | event |
14+
| angularmerge.js:2:32:2:36 | event | angularmerge.js:2:32:2:41 | event.data |
15+
| angularmerge.js:2:32:2:41 | event.data | angularmerge.js:2:21:2:42 | JSON.pa ... t.data) |
916
| src-vulnerable-lodash/tst.js:11:16:11:30 | req.query.value | src-vulnerable-lodash/tst.js:10:17:12:5 | {\\n ... K\\n } |
1017
| src-vulnerable-lodash/tst.js:15:14:15:28 | req.query.value | src-vulnerable-lodash/tst.js:18:16:18:25 | opts.thing |
1118
| src-vulnerable-lodash/tst.js:18:16:18:25 | opts.thing | src-vulnerable-lodash/tst.js:17:17:19:5 | {\\n ... K\\n } |
1219
#select
20+
| angularmerge.js:2:21:2:42 | JSON.pa ... t.data) | angularmerge.js:1:30:1:34 | event | angularmerge.js:2:21:2:42 | JSON.pa ... t.data) | Prototype pollution caused by merging a user-controlled value from $@ using a vulnerable version of $@. | angularmerge.js:1:30:1:34 | event | here | angularmerge.js:2:3:2:43 | angular ... .data)) | angular |
1321
| src-vulnerable-lodash/tst.js:7:17:7:29 | req.query.foo | src-vulnerable-lodash/tst.js:7:17:7:29 | req.query.foo | src-vulnerable-lodash/tst.js:7:17:7:29 | req.query.foo | Prototype pollution caused by merging a user-controlled value from $@ using a vulnerable version of $@. | src-vulnerable-lodash/tst.js:7:17:7:29 | req.query.foo | here | src-vulnerable-lodash/package.json:3:19:3:26 | "4.17.4" | lodash |
1422
| src-vulnerable-lodash/tst.js:10:17:12:5 | {\\n ... K\\n } | src-vulnerable-lodash/tst.js:11:16:11:30 | req.query.value | src-vulnerable-lodash/tst.js:10:17:12:5 | {\\n ... K\\n } | Prototype pollution caused by merging a user-controlled value from $@ using a vulnerable version of $@. | src-vulnerable-lodash/tst.js:11:16:11:30 | req.query.value | here | src-vulnerable-lodash/package.json:3:19:3:26 | "4.17.4" | lodash |
1523
| src-vulnerable-lodash/tst.js:17:17:19:5 | {\\n ... K\\n } | src-vulnerable-lodash/tst.js:15:14:15:28 | req.query.value | src-vulnerable-lodash/tst.js:17:17:19:5 | {\\n ... K\\n } | Prototype pollution caused by merging a user-controlled value from $@ using a vulnerable version of $@. | src-vulnerable-lodash/tst.js:15:14:15:28 | req.query.value | here | src-vulnerable-lodash/package.json:3:19:3:26 | "4.17.4" | lodash |

‎javascript/ql/test/query-tests/Security/CWE-400/angularmerge.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
addEventListener("message", (event) => {
2+
angular.merge({}, JSON.parse(event.data)); // NOT OK
3+
});

0 commit comments

Comments
 (0)