Skip to content

Commit 82c6c22

Browse files
erik-krogherik-krogh
committed
make a model for hasOwnProperty calls and similar
1 parent 2a97dd9 commit 82c6c22

File tree

6 files changed

+108
-33
lines changed

6 files changed

+108
-33
lines changed

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

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -229,18 +229,10 @@ module MembershipCandidate {
229229
membersNode = inExpr.getRightOperand()
230230
)
231231
or
232-
exists(MethodCallExpr hasOwn |
233-
this = hasOwn.getArgument(0).flow() and
234-
test = hasOwn and
235-
hasOwn.calls(membersNode, "hasOwnProperty")
236-
)
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()
232+
exists(HasOwnPropertyCall hasOwn |
233+
this = hasOwn.getProperty() and
234+
test = hasOwn.asExpr() and
235+
membersNode = hasOwn.getObject().asExpr()
244236
)
245237
}
246238

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,3 +192,35 @@ class StringSplitCall extends DataFlow::MethodCallNode {
192192
bindingset[i]
193193
DataFlow::Node getASubstringRead(int i) { result = this.getAPropertyRead(i.toString()) }
194194
}
195+
196+
/**
197+
* A call to `Object.prototype.hasOwnProperty`, `Object.hasOwn`, or a library that implements
198+
* the same functionality.
199+
*/
200+
class HasOwnPropertyCall extends DataFlow::Node instanceof DataFlow::CallNode {
201+
DataFlow::Node object;
202+
DataFlow::Node property;
203+
204+
HasOwnPropertyCall() {
205+
// Make sure we handle reflective calls since libraries love to do that.
206+
super.getCalleeNode().getALocalSource().(DataFlow::PropRead).getPropertyName() =
207+
"hasOwnProperty" and
208+
object = super.getReceiver() and
209+
property = super.getArgument(0)
210+
or
211+
this =
212+
[
213+
DataFlow::globalVarRef("Object").getAMemberCall("hasOwn"), //
214+
DataFlow::moduleImport("has").getACall(), //
215+
LodashUnderscore::member("has").getACall()
216+
] and
217+
object = super.getArgument(0) and
218+
property = super.getArgument(1)
219+
}
220+
221+
/** Gets the object whose property is being checked. */
222+
DataFlow::Node getObject() { result = object }
223+
224+
/** Gets the property being checked. */
225+
DataFlow::Node getProperty() { result = property }
226+
}

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

Lines changed: 3 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,6 @@ 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
290289
node instanceof InExprGuard or
291290
node instanceof InstanceOfGuard or
292291
node instanceof TypeofGuard or
@@ -340,32 +339,16 @@ class AllowListEqualityGuard extends DataFlow::LabeledBarrierGuardNode, ValueNod
340339
* but the destination object generally doesn't. It is therefore only a sanitizer when
341340
* used on the destination object.
342341
*/
343-
class HasOwnPropertyGuard extends DataFlow::BarrierGuardNode, CallNode {
342+
class HasOwnPropertyGuard extends DataFlow::BarrierGuardNode instanceof HasOwnPropertyCall {
344343
HasOwnPropertyGuard() {
345-
// Make sure we handle reflective calls since libraries love to do that.
346-
getCalleeNode().getALocalSource().(DataFlow::PropRead).getPropertyName() = "hasOwnProperty" and
347-
exists(getReceiver()) and
348344
// Try to avoid `src.hasOwnProperty` by requiring that the receiver
349345
// does not locally have its properties enumerated. Typically there is no
350346
// reason to enumerate the properties of the destination object.
351-
not arePropertiesEnumerated(getReceiver().getALocalSource())
347+
not arePropertiesEnumerated(super.getObject().getALocalSource())
352348
}
353349

354350
override predicate blocks(boolean outcome, Expr e) {
355-
e = getArgument(0).asExpr() and outcome = true
356-
}
357-
}
358-
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
351+
e = super.getProperty().asExpr() and outcome = true
369352
}
370353
}
371354

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,6 @@ sanitizingGuard
131131
| tst.js:395:16:395:38 | o.hasOw ... (v.p.q) | tst.js:395:33:395:37 | v.p.q | true |
132132
| tst.js:397:16:397:36 | o.hasOw ... ty(v.p) | tst.js:397:33:397:35 | v.p | true |
133133
| 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 |
135134
| tst.js:401:16:401:34 | Object.hasOwn(o, v) | tst.js:401:33:401:33 | v | true |
136135
taintedSink
137136
| tst.js:2:13:2:20 | SOURCE() | tst.js:3:10:3:10 | v |

‎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
@@ -1503,6 +1503,31 @@ nodes
15031503
| tests.js:559:24:559:31 | src[key] |
15041504
| tests.js:559:28:559:30 | key |
15051505
| tests.js:559:28:559:30 | key |
1506+
| tests.js:564:35:564:37 | src |
1507+
| tests.js:564:35:564:37 | src |
1508+
| tests.js:565:14:565:16 | key |
1509+
| tests.js:565:14:565:16 | key |
1510+
| tests.js:565:14:565:16 | key |
1511+
| tests.js:569:43:569:45 | src |
1512+
| tests.js:569:43:569:45 | src |
1513+
| tests.js:569:43:569:50 | src[key] |
1514+
| tests.js:569:43:569:50 | src[key] |
1515+
| tests.js:569:43:569:50 | src[key] |
1516+
| tests.js:569:43:569:50 | src[key] |
1517+
| tests.js:569:43:569:50 | src[key] |
1518+
| tests.js:571:17:571:19 | key |
1519+
| tests.js:571:17:571:19 | key |
1520+
| tests.js:571:17:571:19 | key |
1521+
| tests.js:571:24:571:26 | src |
1522+
| tests.js:571:24:571:26 | src |
1523+
| tests.js:571:24:571:31 | src[key] |
1524+
| tests.js:571:24:571:31 | src[key] |
1525+
| tests.js:571:24:571:31 | src[key] |
1526+
| tests.js:571:24:571:31 | src[key] |
1527+
| tests.js:571:24:571:31 | src[key] |
1528+
| tests.js:571:24:571:31 | src[key] |
1529+
| tests.js:571:28:571:30 | key |
1530+
| tests.js:571:28:571:30 | key |
15061531
edges
15071532
| examples/PrototypePollutingFunction.js:1:16:1:18 | dst | examples/PrototypePollutingFunction.js:5:19:5:21 | dst |
15081533
| examples/PrototypePollutingFunction.js:1:16:1:18 | dst | examples/PrototypePollutingFunction.js:5:19:5:21 | dst |
@@ -3404,6 +3429,38 @@ edges
34043429
| tests.js:559:28:559:30 | key | tests.js:559:24:559:31 | src[key] |
34053430
| tests.js:559:28:559:30 | key | tests.js:559:24:559:31 | src[key] |
34063431
| tests.js:559:28:559:30 | key | tests.js:559:24:559:31 | src[key] |
3432+
| tests.js:564:35:564:37 | src | tests.js:569:43:569:45 | src |
3433+
| tests.js:564:35:564:37 | src | tests.js:569:43:569:45 | src |
3434+
| tests.js:564:35:564:37 | src | tests.js:571:24:571:26 | src |
3435+
| tests.js:564:35:564:37 | src | tests.js:571:24:571:26 | src |
3436+
| tests.js:565:14:565:16 | key | tests.js:571:17:571:19 | key |
3437+
| tests.js:565:14:565:16 | key | tests.js:571:17:571:19 | key |
3438+
| tests.js:565:14:565:16 | key | tests.js:571:17:571:19 | key |
3439+
| tests.js:565:14:565:16 | key | tests.js:571:17:571:19 | key |
3440+
| tests.js:565:14:565:16 | key | tests.js:571:17:571:19 | key |
3441+
| tests.js:565:14:565:16 | key | tests.js:571:17:571:19 | key |
3442+
| tests.js:565:14:565:16 | key | tests.js:571:17:571:19 | key |
3443+
| tests.js:565:14:565:16 | key | tests.js:571:28:571:30 | key |
3444+
| tests.js:565:14:565:16 | key | tests.js:571:28:571:30 | key |
3445+
| tests.js:565:14:565:16 | key | tests.js:571:28:571:30 | key |
3446+
| tests.js:565:14:565:16 | key | tests.js:571:28:571:30 | key |
3447+
| tests.js:569:43:569:45 | src | tests.js:569:43:569:50 | src[key] |
3448+
| tests.js:569:43:569:45 | src | tests.js:569:43:569:50 | src[key] |
3449+
| tests.js:569:43:569:50 | src[key] | tests.js:564:35:564:37 | src |
3450+
| tests.js:569:43:569:50 | src[key] | tests.js:564:35:564:37 | src |
3451+
| tests.js:569:43:569:50 | src[key] | tests.js:564:35:564:37 | src |
3452+
| tests.js:569:43:569:50 | src[key] | tests.js:564:35:564:37 | src |
3453+
| tests.js:569:43:569:50 | src[key] | tests.js:564:35:564:37 | src |
3454+
| tests.js:569:43:569:50 | src[key] | tests.js:564:35:564:37 | src |
3455+
| tests.js:571:24:571:26 | src | tests.js:571:24:571:31 | src[key] |
3456+
| tests.js:571:24:571:26 | src | tests.js:571:24:571:31 | src[key] |
3457+
| tests.js:571:24:571:26 | src | tests.js:571:24:571:31 | src[key] |
3458+
| tests.js:571:24:571:26 | src | tests.js:571:24:571:31 | src[key] |
3459+
| tests.js:571:24:571:31 | src[key] | tests.js:571:24:571:31 | src[key] |
3460+
| tests.js:571:28:571:30 | key | tests.js:571:24:571:31 | src[key] |
3461+
| tests.js:571:28:571:30 | key | tests.js:571:24:571:31 | src[key] |
3462+
| tests.js:571:28:571:30 | key | tests.js:571:24:571:31 | src[key] |
3463+
| tests.js:571:28:571:30 | key | tests.js:571:24:571:31 | src[key] |
34073464
#select
34083465
| 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 |
34093466
| 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 |

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -560,3 +560,15 @@ function copyHasOwnProperty2(dst, src) {
560560
}
561561
}
562562
}
563+
564+
function copyHasOwnProperty3(dst, src) {
565+
for (let key in src) {
566+
// Guarding the recursive case by dst.hasOwnProperty (or Object.hasOwn) is safe,
567+
// since '__proto__' and 'constructor' are not own properties of the destination object.
568+
if (_.has(dst, key)) {
569+
copyHasOwnProperty3(dst[key], src[key]);
570+
} else {
571+
dst[key] = src[key]; // OK
572+
}
573+
}
574+
}

0 commit comments

Comments
 (0)