Skip to content

Commit f4104e2

Browse files
author
Stephan Brandauer
authored
Merge pull request github#8886 from kaeluka/add-rest-parameter-flowstep
JS: Add flow step to `...rest` parameters
2 parents 1f1581c + ee280cd commit f4104e2

File tree

4 files changed

+139
-0
lines changed

4 files changed

+139
-0
lines changed

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,11 @@ class Function extends @function, Parameterized, TypeParameterized, StmtContaine
4141
/** Gets the `i`th parameter of this function. */
4242
Parameter getParameter(int i) { result = this.getChildExpr(i) }
4343

44+
/** Gets the `...rest` parameter, if any. */
45+
Parameter getRestParameter() {
46+
result = this.getParameter(this.getNumParameter() - 1) and result.isRestParameter()
47+
}
48+
4449
/** Gets a parameter of this function. */
4550
override Parameter getAParameter() { exists(int idx | result = this.getParameter(idx)) }
4651

‎javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSteps.qll

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,8 @@ private module CachedSteps {
212212
)
213213
or
214214
parm = reflectiveParameterAccess(f, i)
215+
or
216+
parm = restParameterAccess(f, i)
215217
)
216218
or
217219
arg = invk.(DataFlow::CallNode).getReceiver() and
@@ -264,6 +266,21 @@ private module CachedSteps {
264266
result.(DataFlow::PropRead).accesses(argumentsAccess(f), any(string p | i = p.toInt()))
265267
}
266268

269+
/**
270+
* Gets a data-flow node that refers to the `i`th parameter of `f` through its `...rest` argument.
271+
*
272+
* If there is normal arguments before `...rest`, we have to account for them.
273+
* For example, a function `function f(a, ...rest) { console.log(rest[1]); }`:
274+
* Here, `restParameterAccess(_, 2)` will return `rest[1]`, because there is the leading
275+
* `a` parameter.
276+
*/
277+
private DataFlow::SourceNode restParameterAccess(Function f, int i) {
278+
result
279+
.(DataFlow::PropRead)
280+
.accesses(f.getRestParameter().flow().(DataFlow::ParameterNode).getALocalUse(),
281+
any(string idx | i = idx.toInt() + f.getNumParameter() - 1))
282+
}
283+
267284
/**
268285
* Holds if there is a flow step from `pred` to `succ` through parameter passing
269286
* to a function call.

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

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1362,6 +1362,50 @@ nodes
13621362
| tests.js:517:40:517:42 | key |
13631363
| tests.js:517:40:517:42 | key |
13641364
| tests.js:517:40:517:42 | key |
1365+
| tests.js:523:11:523:23 | dst |
1366+
| tests.js:523:11:523:23 | dst |
1367+
| tests.js:523:17:523:23 | args[0] |
1368+
| tests.js:523:17:523:23 | args[0] |
1369+
| tests.js:524:11:524:23 | src |
1370+
| tests.js:524:11:524:23 | src |
1371+
| tests.js:524:17:524:23 | args[1] |
1372+
| tests.js:524:17:524:23 | args[1] |
1373+
| tests.js:525:14:525:16 | key |
1374+
| tests.js:525:14:525:16 | key |
1375+
| tests.js:525:14:525:16 | key |
1376+
| tests.js:527:35:527:37 | dst |
1377+
| tests.js:527:35:527:37 | dst |
1378+
| tests.js:527:35:527:42 | dst[key] |
1379+
| tests.js:527:35:527:42 | dst[key] |
1380+
| tests.js:527:35:527:42 | dst[key] |
1381+
| tests.js:527:35:527:42 | dst[key] |
1382+
| tests.js:527:39:527:41 | key |
1383+
| tests.js:527:39:527:41 | key |
1384+
| tests.js:527:45:527:47 | src |
1385+
| tests.js:527:45:527:47 | src |
1386+
| tests.js:527:45:527:52 | src[key] |
1387+
| tests.js:527:45:527:52 | src[key] |
1388+
| tests.js:527:45:527:52 | src[key] |
1389+
| tests.js:527:45:527:52 | src[key] |
1390+
| tests.js:527:45:527:52 | src[key] |
1391+
| tests.js:527:49:527:51 | key |
1392+
| tests.js:527:49:527:51 | key |
1393+
| tests.js:529:13:529:15 | dst |
1394+
| tests.js:529:13:529:15 | dst |
1395+
| tests.js:529:13:529:15 | dst |
1396+
| tests.js:529:17:529:19 | key |
1397+
| tests.js:529:17:529:19 | key |
1398+
| tests.js:529:17:529:19 | key |
1399+
| tests.js:529:24:529:26 | src |
1400+
| tests.js:529:24:529:26 | src |
1401+
| tests.js:529:24:529:31 | src[key] |
1402+
| tests.js:529:24:529:31 | src[key] |
1403+
| tests.js:529:24:529:31 | src[key] |
1404+
| tests.js:529:24:529:31 | src[key] |
1405+
| tests.js:529:24:529:31 | src[key] |
1406+
| tests.js:529:24:529:31 | src[key] |
1407+
| tests.js:529:28:529:30 | key |
1408+
| tests.js:529:28:529:30 | key |
13651409
edges
13661410
| examples/PrototypePollutingFunction.js:1:16:1:18 | dst | examples/PrototypePollutingFunction.js:5:19:5:21 | dst |
13671411
| examples/PrototypePollutingFunction.js:1:16:1:18 | dst | examples/PrototypePollutingFunction.js:5:19:5:21 | dst |
@@ -3075,6 +3119,66 @@ edges
30753119
| tests.js:516:36:516:38 | key | tests.js:516:32:516:39 | src[key] |
30763120
| tests.js:516:36:516:38 | key | tests.js:516:32:516:39 | src[key] |
30773121
| tests.js:516:36:516:38 | key | tests.js:516:32:516:39 | src[key] |
3122+
| tests.js:523:11:523:23 | dst | tests.js:527:35:527:37 | dst |
3123+
| tests.js:523:11:523:23 | dst | tests.js:527:35:527:37 | dst |
3124+
| tests.js:523:11:523:23 | dst | tests.js:529:13:529:15 | dst |
3125+
| tests.js:523:11:523:23 | dst | tests.js:529:13:529:15 | dst |
3126+
| tests.js:523:11:523:23 | dst | tests.js:529:13:529:15 | dst |
3127+
| tests.js:523:11:523:23 | dst | tests.js:529:13:529:15 | dst |
3128+
| tests.js:523:17:523:23 | args[0] | tests.js:523:11:523:23 | dst |
3129+
| tests.js:523:17:523:23 | args[0] | tests.js:523:11:523:23 | dst |
3130+
| tests.js:524:11:524:23 | src | tests.js:527:45:527:47 | src |
3131+
| tests.js:524:11:524:23 | src | tests.js:527:45:527:47 | src |
3132+
| tests.js:524:11:524:23 | src | tests.js:529:24:529:26 | src |
3133+
| tests.js:524:11:524:23 | src | tests.js:529:24:529:26 | src |
3134+
| tests.js:524:17:524:23 | args[1] | tests.js:524:11:524:23 | src |
3135+
| tests.js:524:17:524:23 | args[1] | tests.js:524:11:524:23 | src |
3136+
| tests.js:525:14:525:16 | key | tests.js:527:39:527:41 | key |
3137+
| tests.js:525:14:525:16 | key | tests.js:527:39:527:41 | key |
3138+
| tests.js:525:14:525:16 | key | tests.js:527:39:527:41 | key |
3139+
| tests.js:525:14:525:16 | key | tests.js:527:39:527:41 | key |
3140+
| tests.js:525:14:525:16 | key | tests.js:527:49:527:51 | key |
3141+
| tests.js:525:14:525:16 | key | tests.js:527:49:527:51 | key |
3142+
| tests.js:525:14:525:16 | key | tests.js:527:49:527:51 | key |
3143+
| tests.js:525:14:525:16 | key | tests.js:527:49:527:51 | key |
3144+
| tests.js:525:14:525:16 | key | tests.js:529:17:529:19 | key |
3145+
| tests.js:525:14:525:16 | key | tests.js:529:17:529:19 | key |
3146+
| tests.js:525:14:525:16 | key | tests.js:529:17:529:19 | key |
3147+
| tests.js:525:14:525:16 | key | tests.js:529:17:529:19 | key |
3148+
| tests.js:525:14:525:16 | key | tests.js:529:17:529:19 | key |
3149+
| tests.js:525:14:525:16 | key | tests.js:529:17:529:19 | key |
3150+
| tests.js:525:14:525:16 | key | tests.js:529:17:529:19 | key |
3151+
| tests.js:525:14:525:16 | key | tests.js:529:28:529:30 | key |
3152+
| tests.js:525:14:525:16 | key | tests.js:529:28:529:30 | key |
3153+
| tests.js:525:14:525:16 | key | tests.js:529:28:529:30 | key |
3154+
| tests.js:525:14:525:16 | key | tests.js:529:28:529:30 | key |
3155+
| tests.js:527:35:527:37 | dst | tests.js:527:35:527:42 | dst[key] |
3156+
| tests.js:527:35:527:37 | dst | tests.js:527:35:527:42 | dst[key] |
3157+
| tests.js:527:35:527:42 | dst[key] | tests.js:523:17:523:23 | args[0] |
3158+
| tests.js:527:35:527:42 | dst[key] | tests.js:523:17:523:23 | args[0] |
3159+
| tests.js:527:35:527:42 | dst[key] | tests.js:523:17:523:23 | args[0] |
3160+
| tests.js:527:35:527:42 | dst[key] | tests.js:523:17:523:23 | args[0] |
3161+
| tests.js:527:39:527:41 | key | tests.js:527:35:527:42 | dst[key] |
3162+
| tests.js:527:39:527:41 | key | tests.js:527:35:527:42 | dst[key] |
3163+
| tests.js:527:45:527:47 | src | tests.js:527:45:527:52 | src[key] |
3164+
| tests.js:527:45:527:47 | src | tests.js:527:45:527:52 | src[key] |
3165+
| tests.js:527:45:527:52 | src[key] | tests.js:524:17:524:23 | args[1] |
3166+
| tests.js:527:45:527:52 | src[key] | tests.js:524:17:524:23 | args[1] |
3167+
| tests.js:527:45:527:52 | src[key] | tests.js:524:17:524:23 | args[1] |
3168+
| tests.js:527:45:527:52 | src[key] | tests.js:524:17:524:23 | args[1] |
3169+
| tests.js:527:45:527:52 | src[key] | tests.js:524:17:524:23 | args[1] |
3170+
| tests.js:527:45:527:52 | src[key] | tests.js:524:17:524:23 | args[1] |
3171+
| tests.js:527:49:527:51 | key | tests.js:527:45:527:52 | src[key] |
3172+
| tests.js:527:49:527:51 | key | tests.js:527:45:527:52 | src[key] |
3173+
| tests.js:529:24:529:26 | src | tests.js:529:24:529:31 | src[key] |
3174+
| tests.js:529:24:529:26 | src | tests.js:529:24:529:31 | src[key] |
3175+
| tests.js:529:24:529:26 | src | tests.js:529:24:529:31 | src[key] |
3176+
| tests.js:529:24:529:26 | src | tests.js:529:24:529:31 | src[key] |
3177+
| tests.js:529:24:529:31 | src[key] | tests.js:529:24:529:31 | src[key] |
3178+
| tests.js:529:28:529:30 | key | tests.js:529:24:529:31 | src[key] |
3179+
| tests.js:529:28:529:30 | key | tests.js:529:24:529:31 | src[key] |
3180+
| tests.js:529:28:529:30 | key | tests.js:529:24:529:31 | src[key] |
3181+
| tests.js:529:28:529:30 | key | tests.js:529:24:529:31 | src[key] |
30783182
#select
30793183
| 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 |
30803184
| 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 |
@@ -3104,3 +3208,4 @@ edges
31043208
| 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 |
31053209
| 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 |
31063210
| 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 |
3211+
| tests.js:529:13:529:15 | dst | tests.js:525:14:525:16 | key | tests.js:529:13:529:15 | dst | Properties are copied from $@ to $@ without guarding against prototype pollution. | tests.js:525:21:525:23 | src | src | tests.js:529:13:529:15 | dst | dst |

‎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
@@ -518,3 +518,15 @@ function usingDefineProperty(dst, src) {
518518
}
519519
}
520520
}
521+
522+
function copyUsingForInAndRest(...args) {
523+
const dst = args[0];
524+
const src = args[1];
525+
for (let key in src) {
526+
if (dst[key]) {
527+
copyUsingForInAndRest(dst[key], src[key]);
528+
} else {
529+
dst[key] = src[key]; // NOT OK
530+
}
531+
}
532+
}

0 commit comments

Comments
 (0)