Skip to content

Commit 2a97dd9

Browse files
erik-krogherik-krogh
committed
add support for Object.hasOwn(obj, key)
1 parent 1717d17 commit 2a97dd9

File tree

11 files changed

+171
-10
lines changed

11 files changed

+171
-10
lines changed

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,14 @@ module MembershipCandidate {
234234
test = hasOwn and
235235
hasOwn.calls(membersNode, "hasOwnProperty")
236236
)
237+
or
238+
exists(DataFlow::CallNode hasOwn |
239+
hasOwn = DataFlow::globalVarRef("Object").getAMemberCall("hasOwn")
240+
|
241+
hasOwn.getArgument(0).asExpr() = membersNode and
242+
this = hasOwn.getArgument(1) and
243+
test = hasOwn.asExpr()
244+
)
237245
}
238246

239247
override DataFlow::Node getTest() { result = test.flow() }

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

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1027,18 +1027,16 @@ module TaintTracking {
10271027
class WhitelistContainmentCallSanitizer extends AdditionalSanitizerGuardNode,
10281028
DataFlow::MethodCallNode {
10291029
WhitelistContainmentCallSanitizer() {
1030-
exists(string name |
1031-
name = "contains" or
1032-
name = "has" or
1033-
name = "hasOwnProperty"
1034-
|
1035-
this.getMethodName() = name
1036-
)
1030+
this.getMethodName() = ["contains", "has", "hasOwnProperty", "hasOwn"]
10371031
}
10381032

10391033
override predicate sanitizes(boolean outcome, Expr e) {
1040-
outcome = true and
1041-
e = this.getArgument(0).asExpr()
1034+
exists(int propertyIndex |
1035+
if this.getMethodName() = "hasOwn" then propertyIndex = 1 else propertyIndex = 0
1036+
|
1037+
outcome = true and
1038+
e = this.getArgument(propertyIndex).asExpr()
1039+
)
10421040
}
10431041

10441042
override predicate appliesTo(Configuration cfg) { any() }

‎javascript/ql/src/Declarations/UnusedProperty.ql

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ predicate hasUnknownPropertyRead(LocalObject obj) {
2727
or
2828
exists(obj.getAPropertyRead("hasOwnProperty"))
2929
or
30+
obj.flowsTo(DataFlow::globalVarRef("Object").getAMemberCall("hasOwn").getArgument(0))
31+
or
3032
exists(obj.getAPropertyRead("propertyIsEnumerable"))
3133
}
3234

‎javascript/ql/src/Security/CWE-915/PrototypePollutingFunction.ql

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,7 @@ class PropNameTracking extends DataFlow::Configuration {
286286
node instanceof DenyListEqualityGuard or
287287
node instanceof AllowListEqualityGuard or
288288
node instanceof HasOwnPropertyGuard or
289+
node instanceof HasOwnGuard or
289290
node instanceof InExprGuard or
290291
node instanceof InstanceOfGuard or
291292
node instanceof TypeofGuard or
@@ -355,6 +356,19 @@ class HasOwnPropertyGuard extends DataFlow::BarrierGuardNode, CallNode {
355356
}
356357
}
357358

359+
/** Sanitizer guard for calls to `Object.hasOwn(obj, prop)`. */
360+
class HasOwnGuard extends DataFlow::BarrierGuardNode, CallNode {
361+
HasOwnGuard() {
362+
this = DataFlow::globalVarRef("Object").getAMemberCall("hasOwn") and
363+
// same check as in HasOwnPropertyGuard
364+
not arePropertiesEnumerated(getArgument(0).getALocalSource())
365+
}
366+
367+
override predicate blocks(boolean outcome, Expr e) {
368+
e = getArgument(1).asExpr() and outcome = true
369+
}
370+
}
371+
358372
/**
359373
* A sanitizer guard for `key in dst`.
360374
*

‎javascript/ql/test/library-tests/TaintBarriers/tests.expected

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,10 @@ isLabeledBarrier
4343
| ExampleConfiguration | tst.js:361:14:361:14 | v | taint |
4444
| ExampleConfiguration | tst.js:371:14:371:16 | o.p | taint |
4545
| ExampleConfiguration | tst.js:378:14:378:17 | o[p] | taint |
46+
| ExampleConfiguration | tst.js:392:14:392:14 | v | taint |
47+
| ExampleConfiguration | tst.js:394:14:394:16 | v.p | taint |
48+
| ExampleConfiguration | tst.js:396:14:396:18 | v.p.q | taint |
49+
| ExampleConfiguration | tst.js:402:14:402:14 | v | taint |
4650
isSanitizer
4751
| ExampleConfiguration | tst.js:176:18:176:18 | v |
4852
sanitizingGuard
@@ -122,6 +126,13 @@ sanitizingGuard
122126
| tst.js:370:9:370:29 | o.p == ... listed" | tst.js:370:16:370:29 | "white-listed" | true |
123127
| tst.js:377:11:377:32 | o[p] == ... listed" | tst.js:377:11:377:14 | o[p] | true |
124128
| tst.js:377:11:377:32 | o[p] == ... listed" | tst.js:377:19:377:32 | "white-listed" | true |
129+
| tst.js:391:9:391:27 | o.hasOwnProperty(v) | tst.js:391:26:391:26 | v | true |
130+
| tst.js:393:16:393:36 | o.hasOw ... ty(v.p) | tst.js:393:33:393:35 | v.p | true |
131+
| tst.js:395:16:395:38 | o.hasOw ... (v.p.q) | tst.js:395:33:395:37 | v.p.q | true |
132+
| tst.js:397:16:397:36 | o.hasOw ... ty(v.p) | tst.js:397:33:397:35 | v.p | true |
133+
| tst.js:399:16:399:41 | o.hasOw ... "p.q"]) | tst.js:399:33:399:40 | v["p.q"] | true |
134+
| tst.js:401:16:401:34 | Object.hasOwn(o, v) | tst.js:401:30:401:30 | o | true |
135+
| tst.js:401:16:401:34 | Object.hasOwn(o, v) | tst.js:401:33:401:33 | v | true |
125136
taintedSink
126137
| tst.js:2:13:2:20 | SOURCE() | tst.js:3:10:3:10 | v |
127138
| tst.js:2:13:2:20 | SOURCE() | tst.js:8:14:8:14 | v |
@@ -186,3 +197,6 @@ taintedSink
186197
| tst.js:367:13:367:20 | SOURCE() | tst.js:373:14:373:16 | o.p |
187198
| tst.js:367:13:367:20 | SOURCE() | tst.js:380:14:380:17 | o[p] |
188199
| tst.js:367:13:367:20 | SOURCE() | tst.js:382:14:382:17 | o[p] |
200+
| tst.js:388:13:388:20 | SOURCE() | tst.js:389:10:389:14 | v.p.q |
201+
| tst.js:388:13:388:20 | SOURCE() | tst.js:398:14:398:14 | v |
202+
| tst.js:388:13:388:20 | SOURCE() | tst.js:400:14:400:18 | v.p.q |

‎javascript/ql/test/library-tests/TaintBarriers/tst.js

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -383,3 +383,22 @@ function constantComparisonSanitizer2() {
383383
}
384384
}
385385
}
386+
387+
function propertySanitization(o) {
388+
var v = SOURCE();
389+
SINK(v.p.q); // NOT OK
390+
391+
if (o.hasOwnProperty(v)) {
392+
SINK(v); // OK
393+
} else if (o.hasOwnProperty(v.p)) {
394+
SINK(v.p); // OK
395+
} else if (o.hasOwnProperty(v.p.q)) {
396+
SINK(v.p.q); // OK
397+
} else if (o.hasOwnProperty(v.p)) {
398+
SINK(v); // NOT OK
399+
} else if (o.hasOwnProperty(v["p.q"])) {
400+
SINK(v.p.q); // NOT OK
401+
} else if (Object.hasOwn(o, v)) {
402+
SINK(v); // OK
403+
}
404+
}

‎javascript/ql/test/query-tests/Declarations/UnusedProperty/tst.js

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,3 +81,11 @@
8181
(function(){
8282
({ unusedProp: 42 }, 42);
8383
});
84+
85+
(function(){
86+
var foo = {
87+
unused: 42
88+
};
89+
foo.unused = 42;
90+
Object.hasOwn(foo, blab);
91+
});

‎javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,13 @@ nodes
5050
| express.js:146:16:146:24 | query.foo |
5151
| express.js:146:16:146:24 | query.foo |
5252
| express.js:146:16:146:24 | query.foo |
53+
| express.js:150:7:150:34 | target |
54+
| express.js:150:16:150:34 | req.param("target") |
55+
| express.js:150:16:150:34 | req.param("target") |
56+
| express.js:155:18:155:23 | target |
57+
| express.js:155:18:155:23 | target |
58+
| express.js:160:18:160:23 | target |
59+
| express.js:160:18:160:23 | target |
5360
| koa.js:6:6:6:27 | url |
5461
| koa.js:6:12:6:27 | ctx.query.target |
5562
| koa.js:6:12:6:27 | ctx.query.target |
@@ -140,6 +147,12 @@ edges
140147
| express.js:136:22:136:36 | req.params.user | express.js:136:16:136:36 | 'u' + r ... ms.user |
141148
| express.js:143:16:143:28 | req.query.foo | express.js:143:16:143:28 | req.query.foo |
142149
| express.js:146:16:146:24 | query.foo | express.js:146:16:146:24 | query.foo |
150+
| express.js:150:7:150:34 | target | express.js:155:18:155:23 | target |
151+
| express.js:150:7:150:34 | target | express.js:155:18:155:23 | target |
152+
| express.js:150:7:150:34 | target | express.js:160:18:160:23 | target |
153+
| express.js:150:7:150:34 | target | express.js:160:18:160:23 | target |
154+
| express.js:150:16:150:34 | req.param("target") | express.js:150:7:150:34 | target |
155+
| express.js:150:16:150:34 | req.param("target") | express.js:150:7:150:34 | target |
143156
| koa.js:6:6:6:27 | url | koa.js:7:15:7:17 | url |
144157
| koa.js:6:6:6:27 | url | koa.js:7:15:7:17 | url |
145158
| koa.js:6:6:6:27 | url | koa.js:8:18:8:20 | url |
@@ -199,6 +212,8 @@ edges
199212
| express.js:136:16:136:36 | 'u' + r ... ms.user | express.js:136:22:136:36 | req.params.user | express.js:136:16:136:36 | 'u' + r ... ms.user | Untrusted URL redirection due to $@. | express.js:136:22:136:36 | req.params.user | user-provided value |
200213
| express.js:143:16:143:28 | req.query.foo | express.js:143:16:143:28 | req.query.foo | express.js:143:16:143:28 | req.query.foo | Untrusted URL redirection due to $@. | express.js:143:16:143:28 | req.query.foo | user-provided value |
201214
| express.js:146:16:146:24 | query.foo | express.js:146:16:146:24 | query.foo | express.js:146:16:146:24 | query.foo | Untrusted URL redirection due to $@. | express.js:146:16:146:24 | query.foo | user-provided value |
215+
| express.js:155:18:155:23 | target | express.js:150:16:150:34 | req.param("target") | express.js:155:18:155:23 | target | Untrusted URL redirection due to $@. | express.js:150:16:150:34 | req.param("target") | user-provided value |
216+
| express.js:160:18:160:23 | target | express.js:150:16:150:34 | req.param("target") | express.js:160:18:160:23 | target | Untrusted URL redirection due to $@. | express.js:150:16:150:34 | req.param("target") | user-provided value |
202217
| koa.js:7:15:7:17 | url | koa.js:6:12:6:27 | ctx.query.target | koa.js:7:15:7:17 | url | Untrusted URL redirection due to $@. | koa.js:6:12:6:27 | ctx.query.target | user-provided value |
203218
| koa.js:8:15:8:26 | `${url}${x}` | koa.js:6:12:6:27 | ctx.query.target | koa.js:8:15:8:26 | `${url}${x}` | Untrusted URL redirection due to $@. | koa.js:6:12:6:27 | ctx.query.target | user-provided value |
204219
| koa.js:14:16:14:18 | url | koa.js:6:12:6:27 | ctx.query.target | koa.js:14:16:14:18 | url | Untrusted URL redirection due to $@. | koa.js:6:12:6:27 | ctx.query.target | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/express.js

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,4 +144,18 @@ app.get("foo", (req, res) => {
144144
});
145145
app.get("bar", ({query}, res) => {
146146
res.redirect(query.foo); // NOT OK
147-
})
147+
})
148+
149+
app.get('/some/path', function(req, res) {
150+
let target = req.param("target");
151+
152+
if (SAFE_TARGETS.hasOwnProperty(target))
153+
res.redirect(target); // OK: request parameter is checked against whitelist
154+
else
155+
res.redirect(target); // NOT OK
156+
157+
if (Object.hasOwn(SAFE_TARGETS, target))
158+
res.redirect(target); // OK: request parameter is checked against whitelist
159+
else
160+
res.redirect(target); // NOT OK
161+
});

‎javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingFunction/PrototypePollutingFunction.expected

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1478,6 +1478,31 @@ nodes
14781478
| tests.js:547:24:547:28 | value |
14791479
| tests.js:547:24:547:28 | value |
14801480
| tests.js:547:24:547:28 | value |
1481+
| tests.js:552:35:552:37 | src |
1482+
| tests.js:552:35:552:37 | src |
1483+
| tests.js:553:14:553:16 | key |
1484+
| tests.js:553:14:553:16 | key |
1485+
| tests.js:553:14:553:16 | key |
1486+
| tests.js:557:43:557:45 | src |
1487+
| tests.js:557:43:557:45 | src |
1488+
| tests.js:557:43:557:50 | src[key] |
1489+
| tests.js:557:43:557:50 | src[key] |
1490+
| tests.js:557:43:557:50 | src[key] |
1491+
| tests.js:557:43:557:50 | src[key] |
1492+
| tests.js:557:43:557:50 | src[key] |
1493+
| tests.js:559:17:559:19 | key |
1494+
| tests.js:559:17:559:19 | key |
1495+
| tests.js:559:17:559:19 | key |
1496+
| tests.js:559:24:559:26 | src |
1497+
| tests.js:559:24:559:26 | src |
1498+
| tests.js:559:24:559:31 | src[key] |
1499+
| tests.js:559:24:559:31 | src[key] |
1500+
| tests.js:559:24:559:31 | src[key] |
1501+
| tests.js:559:24:559:31 | src[key] |
1502+
| tests.js:559:24:559:31 | src[key] |
1503+
| tests.js:559:24:559:31 | src[key] |
1504+
| tests.js:559:28:559:30 | key |
1505+
| tests.js:559:28:559:30 | key |
14811506
edges
14821507
| examples/PrototypePollutingFunction.js:1:16:1:18 | dst | examples/PrototypePollutingFunction.js:5:19:5:21 | dst |
14831508
| examples/PrototypePollutingFunction.js:1:16:1:18 | dst | examples/PrototypePollutingFunction.js:5:19:5:21 | dst |
@@ -3347,6 +3372,38 @@ edges
33473372
| tests.js:545:43:545:47 | value | tests.js:542:35:542:37 | src |
33483373
| tests.js:545:43:545:47 | value | tests.js:542:35:542:37 | src |
33493374
| tests.js:545:43:545:47 | value | tests.js:542:35:542:37 | src |
3375+
| tests.js:552:35:552:37 | src | tests.js:557:43:557:45 | src |
3376+
| tests.js:552:35:552:37 | src | tests.js:557:43:557:45 | src |
3377+
| tests.js:552:35:552:37 | src | tests.js:559:24:559:26 | src |
3378+
| tests.js:552:35:552:37 | src | tests.js:559:24:559:26 | src |
3379+
| tests.js:553:14:553:16 | key | tests.js:559:17:559:19 | key |
3380+
| tests.js:553:14:553:16 | key | tests.js:559:17:559:19 | key |
3381+
| tests.js:553:14:553:16 | key | tests.js:559:17:559:19 | key |
3382+
| tests.js:553:14:553:16 | key | tests.js:559:17:559:19 | key |
3383+
| tests.js:553:14:553:16 | key | tests.js:559:17:559:19 | key |
3384+
| tests.js:553:14:553:16 | key | tests.js:559:17:559:19 | key |
3385+
| tests.js:553:14:553:16 | key | tests.js:559:17:559:19 | key |
3386+
| tests.js:553:14:553:16 | key | tests.js:559:28:559:30 | key |
3387+
| tests.js:553:14:553:16 | key | tests.js:559:28:559:30 | key |
3388+
| tests.js:553:14:553:16 | key | tests.js:559:28:559:30 | key |
3389+
| tests.js:553:14:553:16 | key | tests.js:559:28:559:30 | key |
3390+
| tests.js:557:43:557:45 | src | tests.js:557:43:557:50 | src[key] |
3391+
| tests.js:557:43:557:45 | src | tests.js:557:43:557:50 | src[key] |
3392+
| tests.js:557:43:557:50 | src[key] | tests.js:552:35:552:37 | src |
3393+
| tests.js:557:43:557:50 | src[key] | tests.js:552:35:552:37 | src |
3394+
| tests.js:557:43:557:50 | src[key] | tests.js:552:35:552:37 | src |
3395+
| tests.js:557:43:557:50 | src[key] | tests.js:552:35:552:37 | src |
3396+
| tests.js:557:43:557:50 | src[key] | tests.js:552:35:552:37 | src |
3397+
| tests.js:557:43:557:50 | src[key] | tests.js:552:35:552:37 | src |
3398+
| tests.js:559:24:559:26 | src | tests.js:559:24:559:31 | src[key] |
3399+
| tests.js:559:24:559:26 | src | tests.js:559:24:559:31 | src[key] |
3400+
| tests.js:559:24:559:26 | src | tests.js:559:24:559:31 | src[key] |
3401+
| tests.js:559:24:559:26 | src | tests.js:559:24:559:31 | src[key] |
3402+
| tests.js:559:24:559:31 | src[key] | tests.js:559:24:559:31 | src[key] |
3403+
| tests.js:559:28:559:30 | key | tests.js:559:24:559:31 | src[key] |
3404+
| tests.js:559:28:559:30 | key | tests.js:559:24:559:31 | src[key] |
3405+
| tests.js:559:28:559:30 | key | tests.js:559:24:559:31 | src[key] |
3406+
| tests.js:559:28:559:30 | key | tests.js:559:24:559:31 | src[key] |
33503407
#select
33513408
| examples/PrototypePollutingFunction.js:7:13:7:15 | dst | examples/PrototypePollutingFunction.js:2:14:2:16 | key | examples/PrototypePollutingFunction.js:7:13:7:15 | dst | Properties are copied from $@ to $@ without guarding against prototype pollution. | examples/PrototypePollutingFunction.js:2:21:2:23 | src | src | examples/PrototypePollutingFunction.js:7:13:7:15 | dst | dst |
33523409
| path-assignment.js:15:13:15:18 | target | path-assignment.js:8:19:8:25 | keys[i] | path-assignment.js:15:13:15:18 | target | The property chain $@ is recursively assigned to $@ without guarding against prototype pollution. | path-assignment.js:8:19:8:25 | keys[i] | here | path-assignment.js:15:13:15:18 | target | target |

0 commit comments

Comments
 (0)