Skip to content

Commit 517e17d

Browse files
erik-krogherik-krogh
committed
support more property writes in js/prototype-pollution-utility, and generalize ObjectDefinePropertyAsPropWrite
1 parent 5ee9612 commit 517e17d

File tree

4 files changed

+138
-20
lines changed

4 files changed

+138
-20
lines changed

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

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -651,10 +651,18 @@ module DataFlow {
651651
override string getPropertyName() { result = astNode.getArgument(1).getStringValue() }
652652

653653
override Node getRhs() {
654-
exists(ObjectExpr obj | obj = astNode.getArgument(2) |
655-
result = obj.getPropertyByName("value").getInit().flow()
654+
exists(DataFlow::SourceNode descriptor |
655+
descriptor = astNode.getArgument(2).flow().getALocalSource()
656+
|
657+
result =
658+
descriptor
659+
.getAPropertyWrite("get")
660+
.getRhs()
661+
.getALocalSource()
662+
.(DataFlow::FunctionNode)
663+
.getAReturn()
656664
or
657-
result = obj.getPropertyByName("get").getInit().flow().(DataFlow::FunctionNode).getAReturn()
665+
result = descriptor.getAPropertyWrite("value").getRhs()
658666
)
659667
}
660668

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

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -165,23 +165,24 @@ predicate isPotentiallyObjectPrototype(SourceNode node) {
165165
* would typically not happen in a merge function.
166166
*/
167167
predicate dynamicPropWrite(DataFlow::Node base, DataFlow::Node prop, DataFlow::Node rhs) {
168-
exists(AssignExpr write, IndexExpr index |
169-
index = write.getLhs() and
170-
base = index.getBase().flow() and
171-
prop = index.getPropertyNameExpr().flow() and
172-
rhs = write.getRhs().flow() and
173-
not exists(prop.getStringValue()) and
174-
not arePropertiesEnumerated(base.getALocalSource()) and
175-
// Prune writes that are unlikely to modify Object.prototype.
176-
// This is mainly for performance, but may block certain results due to
177-
// not tracking out of function returns and into callbacks.
178-
isPotentiallyObjectPrototype(base.getALocalSource()) and
179-
// Ignore writes with an obviously safe RHS.
180-
not exists(Expr e | e = rhs.asExpr() |
181-
e instanceof Literal or
182-
e instanceof ObjectExpr or
183-
e instanceof ArrayExpr
184-
)
168+
exists(
169+
DataFlow::PropWrite write // includes e.g. Object.defineProperty
170+
|
171+
write.getBase() = base and
172+
write.getPropertyNameExpr().flow() = prop and
173+
rhs = write.getRhs()
174+
) and
175+
not exists(prop.getStringValue()) and
176+
not arePropertiesEnumerated(base.getALocalSource()) and
177+
// Prune writes that are unlikely to modify Object.prototype.
178+
// This is mainly for performance, but may block certain results due to
179+
// not tracking out of function returns and into callbacks.
180+
isPotentiallyObjectPrototype(base.getALocalSource()) and
181+
// Ignore writes with an obviously safe RHS.
182+
not exists(Expr e | e = rhs.asExpr() |
183+
e instanceof Literal or
184+
e instanceof ObjectExpr or
185+
e instanceof ArrayExpr
185186
)
186187
}
187188

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

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1320,6 +1320,48 @@ nodes
13201320
| tests.js:502:24:502:28 | value |
13211321
| tests.js:502:24:502:28 | value |
13221322
| tests.js:502:24:502:28 | value |
1323+
| tests.js:508:30:508:32 | dst |
1324+
| tests.js:508:30:508:32 | dst |
1325+
| tests.js:508:35:508:37 | src |
1326+
| tests.js:508:35:508:37 | src |
1327+
| tests.js:511:13:511:25 | key |
1328+
| tests.js:511:13:511:25 | key |
1329+
| tests.js:511:19:511:25 | keys[i] |
1330+
| tests.js:511:19:511:25 | keys[i] |
1331+
| tests.js:511:19:511:25 | keys[i] |
1332+
| tests.js:513:33:513:35 | dst |
1333+
| tests.js:513:33:513:35 | dst |
1334+
| tests.js:513:33:513:40 | dst[key] |
1335+
| tests.js:513:33:513:40 | dst[key] |
1336+
| tests.js:513:33:513:40 | dst[key] |
1337+
| tests.js:513:33:513:40 | dst[key] |
1338+
| tests.js:513:37:513:39 | key |
1339+
| tests.js:513:37:513:39 | key |
1340+
| tests.js:513:43:513:45 | src |
1341+
| tests.js:513:43:513:45 | src |
1342+
| tests.js:513:43:513:50 | src[key] |
1343+
| tests.js:513:43:513:50 | src[key] |
1344+
| tests.js:513:43:513:50 | src[key] |
1345+
| tests.js:513:43:513:50 | src[key] |
1346+
| tests.js:513:43:513:50 | src[key] |
1347+
| tests.js:513:47:513:49 | key |
1348+
| tests.js:513:47:513:49 | key |
1349+
| tests.js:516:32:516:34 | src |
1350+
| tests.js:516:32:516:34 | src |
1351+
| tests.js:516:32:516:39 | src[key] |
1352+
| tests.js:516:32:516:39 | src[key] |
1353+
| tests.js:516:32:516:39 | src[key] |
1354+
| tests.js:516:32:516:39 | src[key] |
1355+
| tests.js:516:32:516:39 | src[key] |
1356+
| tests.js:516:32:516:39 | src[key] |
1357+
| tests.js:516:36:516:38 | key |
1358+
| tests.js:516:36:516:38 | key |
1359+
| tests.js:517:35:517:37 | dst |
1360+
| tests.js:517:35:517:37 | dst |
1361+
| tests.js:517:35:517:37 | dst |
1362+
| tests.js:517:40:517:42 | key |
1363+
| tests.js:517:40:517:42 | key |
1364+
| tests.js:517:40:517:42 | key |
13231365
edges
13241366
| examples/PrototypePollutingFunction.js:1:16:1:18 | dst | examples/PrototypePollutingFunction.js:5:19:5:21 | dst |
13251367
| examples/PrototypePollutingFunction.js:1:16:1:18 | dst | examples/PrototypePollutingFunction.js:5:19:5:21 | dst |
@@ -2982,6 +3024,57 @@ edges
29823024
| tests.js:498:25:498:27 | key | tests.js:498:21:498:28 | src[key] |
29833025
| tests.js:500:38:500:42 | value | tests.js:494:32:494:34 | src |
29843026
| tests.js:500:38:500:42 | value | tests.js:494:32:494:34 | src |
3027+
| tests.js:508:30:508:32 | dst | tests.js:513:33:513:35 | dst |
3028+
| tests.js:508:30:508:32 | dst | tests.js:513:33:513:35 | dst |
3029+
| tests.js:508:30:508:32 | dst | tests.js:517:35:517:37 | dst |
3030+
| tests.js:508:30:508:32 | dst | tests.js:517:35:517:37 | dst |
3031+
| tests.js:508:30:508:32 | dst | tests.js:517:35:517:37 | dst |
3032+
| tests.js:508:30:508:32 | dst | tests.js:517:35:517:37 | dst |
3033+
| tests.js:508:35:508:37 | src | tests.js:513:43:513:45 | src |
3034+
| tests.js:508:35:508:37 | src | tests.js:513:43:513:45 | src |
3035+
| tests.js:508:35:508:37 | src | tests.js:516:32:516:34 | src |
3036+
| tests.js:508:35:508:37 | src | tests.js:516:32:516:34 | src |
3037+
| tests.js:511:13:511:25 | key | tests.js:513:37:513:39 | key |
3038+
| tests.js:511:13:511:25 | key | tests.js:513:37:513:39 | key |
3039+
| tests.js:511:13:511:25 | key | tests.js:513:47:513:49 | key |
3040+
| tests.js:511:13:511:25 | key | tests.js:513:47:513:49 | key |
3041+
| tests.js:511:13:511:25 | key | tests.js:516:36:516:38 | key |
3042+
| tests.js:511:13:511:25 | key | tests.js:516:36:516:38 | key |
3043+
| tests.js:511:13:511:25 | key | tests.js:517:40:517:42 | key |
3044+
| tests.js:511:13:511:25 | key | tests.js:517:40:517:42 | key |
3045+
| tests.js:511:13:511:25 | key | tests.js:517:40:517:42 | key |
3046+
| tests.js:511:13:511:25 | key | tests.js:517:40:517:42 | key |
3047+
| tests.js:511:19:511:25 | keys[i] | tests.js:511:13:511:25 | key |
3048+
| tests.js:511:19:511:25 | keys[i] | tests.js:511:13:511:25 | key |
3049+
| tests.js:511:19:511:25 | keys[i] | tests.js:511:13:511:25 | key |
3050+
| tests.js:511:19:511:25 | keys[i] | tests.js:511:13:511:25 | key |
3051+
| tests.js:513:33:513:35 | dst | tests.js:513:33:513:40 | dst[key] |
3052+
| tests.js:513:33:513:35 | dst | tests.js:513:33:513:40 | dst[key] |
3053+
| tests.js:513:33:513:40 | dst[key] | tests.js:508:30:508:32 | dst |
3054+
| tests.js:513:33:513:40 | dst[key] | tests.js:508:30:508:32 | dst |
3055+
| tests.js:513:33:513:40 | dst[key] | tests.js:508:30:508:32 | dst |
3056+
| tests.js:513:33:513:40 | dst[key] | tests.js:508:30:508:32 | dst |
3057+
| tests.js:513:37:513:39 | key | tests.js:513:33:513:40 | dst[key] |
3058+
| tests.js:513:37:513:39 | key | tests.js:513:33:513:40 | dst[key] |
3059+
| tests.js:513:43:513:45 | src | tests.js:513:43:513:50 | src[key] |
3060+
| tests.js:513:43:513:45 | src | tests.js:513:43:513:50 | src[key] |
3061+
| tests.js:513:43:513:50 | src[key] | tests.js:508:35:508:37 | src |
3062+
| tests.js:513:43:513:50 | src[key] | tests.js:508:35:508:37 | src |
3063+
| tests.js:513:43:513:50 | src[key] | tests.js:508:35:508:37 | src |
3064+
| tests.js:513:43:513:50 | src[key] | tests.js:508:35:508:37 | src |
3065+
| tests.js:513:43:513:50 | src[key] | tests.js:508:35:508:37 | src |
3066+
| tests.js:513:43:513:50 | src[key] | tests.js:508:35:508:37 | src |
3067+
| tests.js:513:47:513:49 | key | tests.js:513:43:513:50 | src[key] |
3068+
| tests.js:513:47:513:49 | key | tests.js:513:43:513:50 | src[key] |
3069+
| tests.js:516:32:516:34 | src | tests.js:516:32:516:39 | src[key] |
3070+
| tests.js:516:32:516:34 | src | tests.js:516:32:516:39 | src[key] |
3071+
| tests.js:516:32:516:34 | src | tests.js:516:32:516:39 | src[key] |
3072+
| tests.js:516:32:516:34 | src | tests.js:516:32:516:39 | src[key] |
3073+
| tests.js:516:32:516:39 | src[key] | tests.js:516:32:516:39 | src[key] |
3074+
| tests.js:516:36:516:38 | key | tests.js:516:32:516:39 | src[key] |
3075+
| tests.js:516:36:516:38 | key | tests.js:516:32:516:39 | src[key] |
3076+
| tests.js:516:36:516:38 | key | tests.js:516:32:516:39 | src[key] |
3077+
| tests.js:516:36:516:38 | key | tests.js:516:32:516:39 | src[key] |
29853078
#select
29863079
| 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 |
29873080
| 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 |
@@ -3010,3 +3103,4 @@ edges
30103103
| tests.js:467:30:467:32 | dst | tests.js:460:25:460:27 | key | tests.js:467:30:467:32 | dst | Properties are copied from $@ to $@ without guarding against prototype pollution. | tests.js:460:12:460:14 | src | src | tests.js:467:30:467:32 | dst | dst |
30113104
| tests.js:477:13:477:15 | dst | tests.js:473:25:473:27 | key | tests.js:477:13:477:15 | dst | Properties are copied from $@ to $@ without guarding against prototype pollution. | tests.js:473:12:473:14 | src | src | tests.js:477:13:477:15 | dst | dst |
30123105
| tests.js:489:13:489:15 | dst | tests.js:484:14:484:16 | key | tests.js:489:13:489:15 | dst | Properties are copied from $@ to $@ without guarding against prototype pollution. | tests.js:484:21:484:23 | src | src | tests.js:489:13:489:15 | dst | dst |
3106+
| tests.js:517:35:517:37 | dst | tests.js:511:19:511:25 | keys[i] | tests.js:517:35:517:37 | dst | Properties are copied from $@ to $@ without guarding against prototype pollution. | tests.js:509:28:509:30 | src | src | tests.js:517:35:517:37 | dst | dst |

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -503,3 +503,18 @@ function copyPlainObject2(dst, src) {
503503
}
504504
}
505505
}
506+
507+
508+
function usingDefineProperty(dst, src) {
509+
let keys = Object.keys(src);
510+
for (let i = 0; i < keys.length; ++i) {
511+
let key = keys[i];
512+
if (dst[key]) {
513+
usingDefineProperty(dst[key], src[key]);
514+
} else {
515+
var descriptor = {};
516+
descriptor.value = src[key];
517+
Object.defineProperty(dst, key, descriptor); // NOT OK
518+
}
519+
}
520+
}

0 commit comments

Comments
 (0)