Skip to content

Commit be5b343

Browse files
authored
Merge pull request github#3564 from max-schaefer/js/reflective-argument-access
Approved by asgerf
2 parents 4b0354c + 5b0a3b9 commit be5b343

File tree

11 files changed

+158
-26
lines changed

11 files changed

+158
-26
lines changed

change-notes/1.25/analysis-javascript.md

Lines changed: 23 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
## General improvements
44

55
* Support for the following frameworks and libraries has been improved:
6+
- [Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise)
67
- [bluebird](http://bluebirdjs.com/)
78
- [express](https://www.npmjs.com/package/express)
89
- [fastify](https://www.npmjs.com/package/fastify)
@@ -14,12 +15,11 @@
1415
- [mssql](https://www.npmjs.com/package/mssql)
1516
- [mysql](https://www.npmjs.com/package/mysql)
1617
- [pg](https://www.npmjs.com/package/pg)
17-
- [Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise)
1818
- [sequelize](https://www.npmjs.com/package/sequelize)
1919
- [spanner](https://www.npmjs.com/package/spanner)
2020
- [sqlite](https://www.npmjs.com/package/sqlite)
21-
- [ssh2](https://www.npmjs.com/package/ssh2)
2221
- [ssh2-streams](https://www.npmjs.com/package/ssh2-streams)
22+
- [ssh2](https://www.npmjs.com/package/ssh2)
2323

2424
* TypeScript 3.9 is now supported.
2525

@@ -36,41 +36,42 @@
3636

3737
| **Query** | **Expected impact** | **Change** |
3838
|--------------------------------|------------------------------|---------------------------------------------------------------------------|
39-
| Misspelled variable name (`js/misspelled-variable-name`) | Message changed | The message for this query now correctly identifies the misspelled variable in additional cases. |
40-
| Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional file system calls. |
41-
| Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional command execution calls. |
42-
| Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | Less results | This query now recognizes additional safe patterns of doing URL redirects. |
43-
| Client-side cross-site scripting (`js/xss`) | Less results | This query now recognizes additional safe strings based on URLs. |
39+
| Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | Fewer results | This query now recognizes additional safe patterns of doing URL redirects. |
40+
| Client-side cross-site scripting (`js/xss`) | Fewer results | This query now recognizes additional safe strings based on URLs. |
41+
| Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving NoSQL code operators are now recognized. |
42+
| Expression has no effect (`js/useless-expression`) | Fewer results | This query no longer flags an expression when that expression is the only content of the containing file. |
4443
| Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | More results | This query now recognizes additional url scheme checks. |
44+
| Misspelled variable name (`js/misspelled-variable-name`) | Message changed | The message for this query now correctly identifies the misspelled variable in additional cases. |
4545
| Prototype pollution in utility function (`js/prototype-pollution-utility`) | More results | This query now recognizes additional utility functions as vulnerable to prototype polution. |
46-
| Expression has no effect (`js/useless-expression`) | Less results | This query no longer flags an expression when that expression is the only content of the containing file. |
47-
| Unknown directive (`js/unknown-directive`) | Less results | This query no longer flags directives generated by the Babel compiler. |
48-
| Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving NoSQL code operators are now recognized. |
46+
| Prototype pollution in utility function (`js/prototype-pollution-utility`) | More results | This query now recognizes more coding patterns that are vulnerable to prototype pollution. |
47+
| Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional command execution calls. |
48+
| Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional file system calls. |
49+
| Unknown directive (`js/unknown-directive`) | Fewer results | This query no longer flags directives generated by the Babel compiler. |
50+
| Unused property (`js/unused-property`) | Fewer results | This query no longer flags properties of objects that are operands of `yield` expressions. |
4951
| Zip Slip (`js/zipslip`) | More results | This query now recognizes additional vulnerabilities. |
50-
| Unused property (`js/unused-property`) | Less results | This query no longer flags properties of objects that are operands of `yield` expressions. |
5152

5253
The following low-precision queries are no longer run by default on LGTM (their results already were not displayed):
5354

5455
- `js/angular/dead-event-listener`
5556
- `js/angular/unused-dependency`
56-
- `js/conflicting-html-attribute`
57-
- `js/useless-assignment-to-global`
58-
- `js/too-many-parameters`
59-
- `js/unused-property`
6057
- `js/bitwise-sign-check`
6158
- `js/comparison-of-identical-expressions`
62-
- `js/misspelled-identifier`
59+
- `js/conflicting-html-attribute`
60+
- `js/ignored-setter-parameter`
6361
- `js/jsdoc/malformed-param-tag`
64-
- `js/jsdoc/unknown-parameter`
6562
- `js/jsdoc/missing-parameter`
66-
- `js/omitted-array-element`
67-
- `js/ignored-setter-parameter`
63+
- `js/jsdoc/unknown-parameter`
6864
- `js/json-in-javascript-file`
65+
- `js/misspelled-identifier`
66+
- `js/nested-loops-with-same-variable`
6967
- `js/node/cyclic-import`
7068
- `js/node/unused-npm-dependency`
71-
- `js/single-run-loop`
72-
- `js/nested-loops-with-same-variable`
69+
- `js/omitted-array-element`
7370
- `js/return-outside-function`
71+
- `js/single-run-loop`
72+
- `js/too-many-parameters`
73+
- `js/unused-property`
74+
- `js/useless-assignment-to-global`
7475

7576
## Changes to libraries
7677

@@ -80,3 +81,4 @@ The following low-precision queries are no longer run by default on LGTM (their
8081
- `Parameter.flow()` now gets the correct data flow node for a parameter. Previously this had a result, but the node was disconnected from the data flow graph.
8182
- `ParameterNode.asExpr()` and `.getAstNode()` now gets the parameter's AST node, whereas previously it had no result.
8283
- `Expr.flow()` now has a more meaningful result for destructuring patterns. Previously this node was disconnected from the data flow graph. Now it represents the values being destructured by the pattern.
84+
* The global data-flow and taint-tracking libraries now model indirect parameter accesses through the `arguments` object in some cases, which may lead to additional results from some of the security queries, particularly "Prototype pollution in utility function".

javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -151,11 +151,14 @@ private module CachedSteps {
151151
) {
152152
calls(invk, f) and
153153
(
154-
exists(int i, Parameter p |
155-
f.getParameter(i) = p and
156-
not p.isRestParameter() and
157-
arg = invk.getArgument(i) and
158-
parm = DataFlow::parameterNode(p)
154+
exists(int i | arg = invk.getArgument(i) |
155+
exists(Parameter p |
156+
f.getParameter(i) = p and
157+
not p.isRestParameter() and
158+
parm = DataFlow::parameterNode(p)
159+
)
160+
or
161+
parm = reflectiveParameterAccess(f, i)
159162
)
160163
or
161164
arg = invk.(DataFlow::CallNode).getReceiver() and
@@ -185,6 +188,22 @@ private module CachedSteps {
185188
)
186189
}
187190

191+
/**
192+
* Gets a data-flow node inside `f` that refers to the `arguments` object of `f`.
193+
*/
194+
private DataFlow::Node argumentsAccess(Function f) {
195+
result.getContainer().getEnclosingContainer*() = f and
196+
result.analyze().getAValue().(AbstractArguments).getFunction() = f
197+
}
198+
199+
/**
200+
* Gets a data-flow node that refers to the `i`th parameter of `f` through its `arguments`
201+
* object.
202+
*/
203+
private DataFlow::SourceNode reflectiveParameterAccess(Function f, int i) {
204+
result.(DataFlow::PropRead).accesses(argumentsAccess(f), any(string p | i = p.toInt()))
205+
}
206+
188207
/**
189208
* Holds if there is a flow step from `pred` to `succ` through parameter passing
190209
* to a function call.
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
| arguments.js:11:5:11:14 | f(1, 2, 3) | arguments.js:11:7:11:7 | 1 | arguments.js:2:5:10:5 | functio ... ;\\n } | arguments.js:2:16:2:16 | x |
2+
| arguments.js:11:5:11:14 | f(1, 2, 3) | arguments.js:11:7:11:7 | 1 | arguments.js:2:5:10:5 | functio ... ;\\n } | arguments.js:4:28:4:39 | arguments[0] |
3+
| arguments.js:11:5:11:14 | f(1, 2, 3) | arguments.js:11:10:11:10 | 2 | arguments.js:2:5:10:5 | functio ... ;\\n } | arguments.js:5:25:5:36 | arguments[1] |
4+
| arguments.js:11:5:11:14 | f(1, 2, 3) | arguments.js:11:13:11:13 | 3 | arguments.js:2:5:10:5 | functio ... ;\\n } | arguments.js:7:24:7:30 | args[2] |
5+
| sources.js:3:1:5:6 | (functi ... \\n})(23) | sources.js:5:4:5:5 | 23 | sources.js:3:2:5:1 | functio ... x+19;\\n} | sources.js:3:11:3:11 | x |
6+
| tst.js:16:1:20:9 | (functi ... ("arg") | tst.js:20:4:20:8 | "arg" | tst.js:16:2:20:1 | functio ... n "";\\n} | tst.js:16:13:16:13 | a |
7+
| tst.js:35:1:35:7 | g(true) | tst.js:35:3:35:6 | true | tst.js:32:1:34:1 | functio ... ables\\n} | tst.js:32:12:32:12 | b |
8+
| tst.js:44:1:44:5 | o.m() | tst.js:44:1:44:1 | o | tst.js:39:4:41:3 | () {\\n this;\\n } | tst.js:39:4:39:3 | this |
9+
| tst.js:87:1:96:2 | (functi ... r: 0\\n}) | tst.js:92:4:96:1 | {\\n p: ... r: 0\\n} | tst.js:87:2:92:1 | functio ... + z;\\n} | tst.js:87:11:87:24 | { p: x, ...o } |
10+
| tst.js:98:1:103:17 | (functi ... 3, 0 ]) | tst.js:103:4:103:16 | [ 19, 23, 0 ] | tst.js:98:2:103:1 | functio ... + z;\\n} | tst.js:98:11:98:24 | [ x, ...rest ] |
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
import javascript
2+
import semmle.javascript.dataflow.internal.FlowSteps as FlowSteps
3+
4+
from DataFlow::Node invk, DataFlow::Node arg, Function f, DataFlow::SourceNode parm
5+
where FlowSteps::argumentPassing(invk, arg, f, parm)
6+
select invk, arg, f, parm
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
(function() {
2+
function f(x) {
3+
let firstArg = x;
4+
let alsoFirstArg = arguments[0];
5+
let secondArg = arguments[1];
6+
let args = arguments;
7+
let thirdArg = args[2];
8+
arguments = {};
9+
let notFirstArg = arguments[0];
10+
}
11+
f(1, 2, 3);
12+
})();

javascript/ql/test/library-tests/DataFlow/enclosingExpr.expected

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,42 @@
1+
| arguments.js:1:1:12:2 | (functi ... 3);\\n}) | arguments.js:1:1:12:2 | (functi ... 3);\\n}) |
2+
| arguments.js:1:1:12:4 | (functi ... );\\n})() | arguments.js:1:1:12:4 | (functi ... );\\n})() |
3+
| arguments.js:1:2:12:1 | functio ... , 3);\\n} | arguments.js:1:2:12:1 | functio ... , 3);\\n} |
4+
| arguments.js:2:14:2:14 | f | arguments.js:2:14:2:14 | f |
5+
| arguments.js:2:16:2:16 | x | arguments.js:2:16:2:16 | x |
6+
| arguments.js:3:13:3:20 | firstArg | arguments.js:3:13:3:20 | firstArg |
7+
| arguments.js:3:13:3:24 | firstArg = x | arguments.js:3:13:3:24 | firstArg = x |
8+
| arguments.js:3:24:3:24 | x | arguments.js:3:24:3:24 | x |
9+
| arguments.js:4:13:4:24 | alsoFirstArg | arguments.js:4:13:4:24 | alsoFirstArg |
10+
| arguments.js:4:13:4:39 | alsoFir ... ents[0] | arguments.js:4:13:4:39 | alsoFir ... ents[0] |
11+
| arguments.js:4:28:4:36 | arguments | arguments.js:4:28:4:36 | arguments |
12+
| arguments.js:4:28:4:39 | arguments[0] | arguments.js:4:28:4:39 | arguments[0] |
13+
| arguments.js:4:38:4:38 | 0 | arguments.js:4:38:4:38 | 0 |
14+
| arguments.js:5:13:5:21 | secondArg | arguments.js:5:13:5:21 | secondArg |
15+
| arguments.js:5:13:5:36 | secondA ... ents[1] | arguments.js:5:13:5:36 | secondA ... ents[1] |
16+
| arguments.js:5:25:5:33 | arguments | arguments.js:5:25:5:33 | arguments |
17+
| arguments.js:5:25:5:36 | arguments[1] | arguments.js:5:25:5:36 | arguments[1] |
18+
| arguments.js:5:35:5:35 | 1 | arguments.js:5:35:5:35 | 1 |
19+
| arguments.js:6:13:6:16 | args | arguments.js:6:13:6:16 | args |
20+
| arguments.js:6:13:6:28 | args = arguments | arguments.js:6:13:6:28 | args = arguments |
21+
| arguments.js:6:20:6:28 | arguments | arguments.js:6:20:6:28 | arguments |
22+
| arguments.js:7:13:7:20 | thirdArg | arguments.js:7:13:7:20 | thirdArg |
23+
| arguments.js:7:13:7:30 | thirdArg = args[2] | arguments.js:7:13:7:30 | thirdArg = args[2] |
24+
| arguments.js:7:24:7:27 | args | arguments.js:7:24:7:27 | args |
25+
| arguments.js:7:24:7:30 | args[2] | arguments.js:7:24:7:30 | args[2] |
26+
| arguments.js:7:29:7:29 | 2 | arguments.js:7:29:7:29 | 2 |
27+
| arguments.js:8:9:8:17 | arguments | arguments.js:8:9:8:17 | arguments |
28+
| arguments.js:8:9:8:22 | arguments = {} | arguments.js:8:9:8:22 | arguments = {} |
29+
| arguments.js:8:21:8:22 | {} | arguments.js:8:21:8:22 | {} |
30+
| arguments.js:9:13:9:23 | notFirstArg | arguments.js:9:13:9:23 | notFirstArg |
31+
| arguments.js:9:13:9:38 | notFirs ... ents[0] | arguments.js:9:13:9:38 | notFirs ... ents[0] |
32+
| arguments.js:9:27:9:35 | arguments | arguments.js:9:27:9:35 | arguments |
33+
| arguments.js:9:27:9:38 | arguments[0] | arguments.js:9:27:9:38 | arguments[0] |
34+
| arguments.js:9:37:9:37 | 0 | arguments.js:9:37:9:37 | 0 |
35+
| arguments.js:11:5:11:5 | f | arguments.js:11:5:11:5 | f |
36+
| arguments.js:11:5:11:14 | f(1, 2, 3) | arguments.js:11:5:11:14 | f(1, 2, 3) |
37+
| arguments.js:11:7:11:7 | 1 | arguments.js:11:7:11:7 | 1 |
38+
| arguments.js:11:10:11:10 | 2 | arguments.js:11:10:11:10 | 2 |
39+
| arguments.js:11:13:11:13 | 3 | arguments.js:11:13:11:13 | 3 |
140
| eval.js:1:10:1:10 | k | eval.js:1:10:1:10 | k |
241
| eval.js:2:7:2:7 | x | eval.js:2:7:2:7 | x |
342
| eval.js:2:7:2:12 | x = 42 | eval.js:2:7:2:12 | x = 42 |

javascript/ql/test/library-tests/DataFlow/flowStep.expected

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,16 @@
1+
| arguments.js:1:2:12:1 | functio ... , 3);\\n} | arguments.js:1:1:12:2 | (functi ... 3);\\n}) |
2+
| arguments.js:2:5:2:5 | arguments | arguments.js:4:28:4:36 | arguments |
3+
| arguments.js:2:5:2:5 | arguments | arguments.js:5:25:5:33 | arguments |
4+
| arguments.js:2:5:2:5 | arguments | arguments.js:6:20:6:28 | arguments |
5+
| arguments.js:2:5:10:5 | functio ... ;\\n } | arguments.js:2:14:2:14 | f |
6+
| arguments.js:2:14:2:14 | f | arguments.js:11:5:11:5 | f |
7+
| arguments.js:2:16:2:16 | x | arguments.js:2:16:2:16 | x |
8+
| arguments.js:2:16:2:16 | x | arguments.js:3:24:3:24 | x |
9+
| arguments.js:6:13:6:28 | args | arguments.js:7:24:7:27 | args |
10+
| arguments.js:6:20:6:28 | arguments | arguments.js:6:13:6:28 | args |
11+
| arguments.js:8:9:8:22 | arguments | arguments.js:9:27:9:35 | arguments |
12+
| arguments.js:8:21:8:22 | {} | arguments.js:8:9:8:22 | arguments |
13+
| arguments.js:8:21:8:22 | {} | arguments.js:8:9:8:22 | arguments = {} |
114
| eval.js:2:7:2:12 | x | eval.js:4:3:4:3 | x |
215
| eval.js:2:11:2:12 | 42 | eval.js:2:7:2:12 | x |
316
| sources.js:1:6:1:6 | x | sources.js:1:6:1:6 | x |

javascript/ql/test/library-tests/DataFlow/getIntValue.expected

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,10 @@
1+
| arguments.js:4:38:4:38 | 0 | 0 |
2+
| arguments.js:5:35:5:35 | 1 | 1 |
3+
| arguments.js:7:29:7:29 | 2 | 2 |
4+
| arguments.js:9:37:9:37 | 0 | 0 |
5+
| arguments.js:11:7:11:7 | 1 | 1 |
6+
| arguments.js:11:10:11:10 | 2 | 2 |
7+
| arguments.js:11:13:11:13 | 3 | 3 |
18
| eval.js:2:11:2:12 | 42 | 42 |
29
| sources.js:4:12:4:13 | 19 | 19 |
310
| sources.js:5:4:5:5 | 23 | 23 |

javascript/ql/test/library-tests/DataFlow/incomplete.expected

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,13 @@
1+
| arguments.js:1:1:12:4 | exceptional return of (functi ... );\\n})() | call |
2+
| arguments.js:1:2:12:1 | exceptional return of anonymous function | call |
3+
| arguments.js:2:5:10:5 | exceptional return of function f | call |
4+
| arguments.js:2:16:2:16 | x | call |
5+
| arguments.js:4:28:4:39 | arguments[0] | heap |
6+
| arguments.js:5:25:5:36 | arguments[1] | heap |
7+
| arguments.js:7:24:7:30 | args[2] | heap |
8+
| arguments.js:9:27:9:38 | arguments[0] | heap |
9+
| arguments.js:11:5:11:14 | exceptional return of f(1, 2, 3) | call |
10+
| arguments.js:11:5:11:14 | f(1, 2, 3) | call |
111
| eval.js:1:1:5:1 | exceptional return of function k | call |
212
| eval.js:2:7:2:12 | x | eval |
313
| eval.js:3:3:3:6 | eval | global |

javascript/ql/test/library-tests/DataFlow/parameters.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
| arguments.js:2:16:2:16 | x |
12
| sources.js:1:6:1:6 | x |
23
| sources.js:3:11:3:11 | x |
34
| sources.js:9:14:9:18 | array |

0 commit comments

Comments
 (0)