Skip to content

Commit ab2d3a7

Browse files
erik-krogherik-krogh
authored
Merge pull request github#7828 from Naman-ntc/main
JS: Adding model for `.get` function of `Map` in Unvalidated Dynamic Method Call
2 parents f00d723 + 009c957 commit ab2d3a7

File tree

8 files changed

+112
-6
lines changed

8 files changed

+112
-6
lines changed

‎javascript/ql/lib/semmle/javascript/security/dataflow/UnvalidatedDynamicMethodCallQuery.qll

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,5 +58,12 @@ class Configuration extends TaintTracking::Configuration {
5858
// avoid overlapping results with unsafe dynamic method access query
5959
not PropertyInjection::hasUnsafeMethods(read.getBase().getALocalSource())
6060
)
61+
or
62+
exists(DataFlow::SourceNode base, DataFlow::CallNode get | get = base.getAMethodCall("get") |
63+
src = get.getArgument(0) and
64+
dst = get
65+
) and
66+
srclabel.isTaint() and
67+
dstlabel instanceof MaybeNonFunction
6168
}
6269
}

‎javascript/ql/src/Security/CWE-754/examples/UnvalidatedDynamicMethodCallGood.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,9 @@ actions.put("pause", function pause(data) {
1111

1212
app.get('/perform/:action/:payload', function(req, res) {
1313
if (actions.has(req.params.action)) {
14-
let action = actions.get(req.params.action);
14+
if (typeof actions.get(req.params.action) === 'function'){
15+
let action = actions.get(req.params.action);
16+
}
1517
// GOOD: `action` is either the `play` or the `pause` function from above
1618
res.end(action(req.params.payload));
1719
} else {

‎javascript/ql/test/query-tests/Security/CWE-754/UnvalidatedDynamicMethodCall.expected

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,12 @@ nodes
1010
| UnsafeDynamicMethodAccess.js:15:5:15:21 | obj[message.name] |
1111
| UnsafeDynamicMethodAccess.js:15:9:15:15 | message |
1212
| UnsafeDynamicMethodAccess.js:15:9:15:20 | message.name |
13+
| UnvalidatedDynamicMethodCall2.js:13:9:13:47 | action |
14+
| UnvalidatedDynamicMethodCall2.js:13:18:13:47 | actions ... action) |
15+
| UnvalidatedDynamicMethodCall2.js:13:30:13:46 | req.params.action |
16+
| UnvalidatedDynamicMethodCall2.js:13:30:13:46 | req.params.action |
17+
| UnvalidatedDynamicMethodCall2.js:14:13:14:18 | action |
18+
| UnvalidatedDynamicMethodCall2.js:14:13:14:18 | action |
1319
| UnvalidatedDynamicMethodCall.js:14:7:14:41 | action |
1420
| UnvalidatedDynamicMethodCall.js:14:7:14:41 | action |
1521
| UnvalidatedDynamicMethodCall.js:14:16:14:41 | actions ... action] |
@@ -19,6 +25,12 @@ nodes
1925
| UnvalidatedDynamicMethodCall.js:15:11:15:16 | action |
2026
| UnvalidatedDynamicMethodCall.js:15:11:15:16 | action |
2127
| UnvalidatedDynamicMethodCall.js:15:11:15:16 | action |
28+
| UnvalidatedDynamicMethodCallGood4.js:14:13:14:51 | action |
29+
| UnvalidatedDynamicMethodCallGood4.js:14:22:14:51 | actions ... action) |
30+
| UnvalidatedDynamicMethodCallGood4.js:14:34:14:50 | req.params.action |
31+
| UnvalidatedDynamicMethodCallGood4.js:14:34:14:50 | req.params.action |
32+
| UnvalidatedDynamicMethodCallGood4.js:15:17:15:22 | action |
33+
| UnvalidatedDynamicMethodCallGood4.js:15:17:15:22 | action |
2234
| tst.js:6:39:6:40 | ev |
2335
| tst.js:6:39:6:40 | ev |
2436
| tst.js:7:9:7:39 | name |
@@ -91,6 +103,11 @@ edges
91103
| UnsafeDynamicMethodAccess.js:15:9:15:20 | message.name | UnsafeDynamicMethodAccess.js:15:5:15:21 | obj[message.name] |
92104
| UnsafeDynamicMethodAccess.js:15:9:15:20 | message.name | UnsafeDynamicMethodAccess.js:15:5:15:21 | obj[message.name] |
93105
| UnsafeDynamicMethodAccess.js:15:9:15:20 | message.name | UnsafeDynamicMethodAccess.js:15:5:15:21 | obj[message.name] |
106+
| UnvalidatedDynamicMethodCall2.js:13:9:13:47 | action | UnvalidatedDynamicMethodCall2.js:14:13:14:18 | action |
107+
| UnvalidatedDynamicMethodCall2.js:13:9:13:47 | action | UnvalidatedDynamicMethodCall2.js:14:13:14:18 | action |
108+
| UnvalidatedDynamicMethodCall2.js:13:18:13:47 | actions ... action) | UnvalidatedDynamicMethodCall2.js:13:9:13:47 | action |
109+
| UnvalidatedDynamicMethodCall2.js:13:30:13:46 | req.params.action | UnvalidatedDynamicMethodCall2.js:13:18:13:47 | actions ... action) |
110+
| UnvalidatedDynamicMethodCall2.js:13:30:13:46 | req.params.action | UnvalidatedDynamicMethodCall2.js:13:18:13:47 | actions ... action) |
94111
| UnvalidatedDynamicMethodCall.js:14:7:14:41 | action | UnvalidatedDynamicMethodCall.js:15:11:15:16 | action |
95112
| UnvalidatedDynamicMethodCall.js:14:7:14:41 | action | UnvalidatedDynamicMethodCall.js:15:11:15:16 | action |
96113
| UnvalidatedDynamicMethodCall.js:14:7:14:41 | action | UnvalidatedDynamicMethodCall.js:15:11:15:16 | action |
@@ -101,6 +118,11 @@ edges
101118
| UnvalidatedDynamicMethodCall.js:14:24:14:40 | req.params.action | UnvalidatedDynamicMethodCall.js:14:16:14:41 | actions ... action] |
102119
| UnvalidatedDynamicMethodCall.js:14:24:14:40 | req.params.action | UnvalidatedDynamicMethodCall.js:14:16:14:41 | actions ... action] |
103120
| UnvalidatedDynamicMethodCall.js:14:24:14:40 | req.params.action | UnvalidatedDynamicMethodCall.js:14:16:14:41 | actions ... action] |
121+
| UnvalidatedDynamicMethodCallGood4.js:14:13:14:51 | action | UnvalidatedDynamicMethodCallGood4.js:15:17:15:22 | action |
122+
| UnvalidatedDynamicMethodCallGood4.js:14:13:14:51 | action | UnvalidatedDynamicMethodCallGood4.js:15:17:15:22 | action |
123+
| UnvalidatedDynamicMethodCallGood4.js:14:22:14:51 | actions ... action) | UnvalidatedDynamicMethodCallGood4.js:14:13:14:51 | action |
124+
| UnvalidatedDynamicMethodCallGood4.js:14:34:14:50 | req.params.action | UnvalidatedDynamicMethodCallGood4.js:14:22:14:51 | actions ... action) |
125+
| UnvalidatedDynamicMethodCallGood4.js:14:34:14:50 | req.params.action | UnvalidatedDynamicMethodCallGood4.js:14:22:14:51 | actions ... action) |
104126
| tst.js:6:39:6:40 | ev | tst.js:7:27:7:28 | ev |
105127
| tst.js:6:39:6:40 | ev | tst.js:7:27:7:28 | ev |
106128
| tst.js:6:39:6:40 | ev | tst.js:9:9:9:10 | ev |
@@ -164,7 +186,9 @@ edges
164186
| tst.js:49:19:49:22 | name | tst.js:49:14:49:23 | obj2[name] |
165187
#select
166188
| UnsafeDynamicMethodAccess.js:15:5:15:21 | obj[message.name] | UnsafeDynamicMethodAccess.js:5:37:5:38 | ev | UnsafeDynamicMethodAccess.js:15:5:15:21 | obj[message.name] | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | UnsafeDynamicMethodAccess.js:5:37:5:38 | ev | user-controlled |
189+
| UnvalidatedDynamicMethodCall2.js:14:13:14:18 | action | UnvalidatedDynamicMethodCall2.js:13:30:13:46 | req.params.action | UnvalidatedDynamicMethodCall2.js:14:13:14:18 | action | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | UnvalidatedDynamicMethodCall2.js:13:30:13:46 | req.params.action | user-controlled |
167190
| UnvalidatedDynamicMethodCall.js:15:11:15:16 | action | UnvalidatedDynamicMethodCall.js:14:24:14:40 | req.params.action | UnvalidatedDynamicMethodCall.js:15:11:15:16 | action | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | UnvalidatedDynamicMethodCall.js:14:24:14:40 | req.params.action | user-controlled |
191+
| UnvalidatedDynamicMethodCallGood4.js:15:17:15:22 | action | UnvalidatedDynamicMethodCallGood4.js:14:34:14:50 | req.params.action | UnvalidatedDynamicMethodCallGood4.js:15:17:15:22 | action | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | UnvalidatedDynamicMethodCallGood4.js:14:34:14:50 | req.params.action | user-controlled |
168192
| tst.js:9:5:9:16 | obj[ev.data] | tst.js:6:39:6:40 | ev | tst.js:9:5:9:16 | obj[ev.data] | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | tst.js:6:39:6:40 | ev | user-controlled |
169193
| tst.js:11:5:11:13 | obj[name] | tst.js:6:39:6:40 | ev | tst.js:11:5:11:13 | obj[name] | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | tst.js:6:39:6:40 | ev | user-controlled |
170194
| tst.js:18:5:18:6 | fn | tst.js:6:39:6:40 | ev | tst.js:18:5:18:6 | fn | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | tst.js:6:39:6:40 | ev | user-controlled |

‎javascript/ql/test/query-tests/Security/CWE-754/UnvalidatedDynamicMethodCall2.js

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
var express = require('express');
2+
var app = express();
3+
4+
var actions = new Map();
5+
actions.put("play", function play(data) {
6+
// ...
7+
});
8+
actions.put("pause", function pause(data) {
9+
// ...
10+
});
11+
12+
app.get('/perform/:action/:payload', function(req, res) {
13+
let action = actions.get(req.params.action);
14+
res.end(action(req.params.payload)); // NOT OK
15+
});

‎javascript/ql/test/query-tests/Security/CWE-754/UnvalidatedDynamicMethodCall3.js

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
var express = require('express');
2+
var app = express();
3+
4+
var actions = new Map();
5+
actions.put("play", function play(data) {
6+
// ...
7+
});
8+
actions.put("pause", function pause(data) {
9+
// ...
10+
});
11+
12+
app.get('/perform/:action/:payload', function(req, res) {
13+
if (actions.has(req.params.action)) {
14+
let action = actions.get(req.params.action);
15+
res.end(action(req.params.payload)); // NOT OK, but not flagged [INCONSISTENCY]
16+
}
17+
});

‎javascript/ql/test/query-tests/Security/CWE-754/UnvalidatedDynamicMethodCallGood.js

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,20 @@ var express = require('express');
22
var app = express();
33

44
var actions = new Map();
5-
actions.put("play", function (data) {
5+
actions.put("play", function play(data) {
66
// ...
77
});
8-
actions.put("pause", function(data) {
8+
actions.put("pause", function pause(data) {
99
// ...
1010
});
1111

12-
app.get('/perform/:action/:payload', function(req, res) {
12+
app.get('/perform/:action/:payload', function (req, res) {
1313
if (actions.has(req.params.action)) {
14-
let action = actions.get(req.params.action);
15-
res.end(action(req.params.payload));
14+
if (typeof actions.get(req.params.action) === 'function') {
15+
let action = actions.get(req.params.action);
16+
// GOOD: `action` is either the `play` or the `pause` function from above
17+
res.end(action(req.params.payload));
18+
}
1619
} else {
1720
res.end("Unsupported action.");
1821
}

‎javascript/ql/test/query-tests/Security/CWE-754/UnvalidatedDynamicMethodCallGood3.js

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
var express = require('express');
2+
var app = express();
3+
4+
var actions = new Map();
5+
actions.put("play", function play(data) {
6+
// ...
7+
});
8+
actions.put("pause", function pause(data) {
9+
// ...
10+
});
11+
12+
app.get('/perform/:action/:payload', function(req, res) {
13+
let action = actions.get(req.params.action);
14+
if (typeof action === 'function') {
15+
res.end(action(req.params.payload)); // GOOD: `action` is either the `play` or the `pause` function from above
16+
} else {
17+
res.end("Unsupported action.");
18+
}
19+
});

‎javascript/ql/test/query-tests/Security/CWE-754/UnvalidatedDynamicMethodCallGood4.js

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
var express = require('express');
2+
var app = express();
3+
4+
var actions = new Map();
5+
actions.put("play", function play(data) {
6+
// ...
7+
});
8+
actions.put("pause", function pause(data) {
9+
// ...
10+
});
11+
12+
app.get('/perform/:action/:payload', function(req, res) {
13+
if (typeof actions.get(req.params.action) === 'function') {
14+
let action = actions.get(req.params.action);
15+
res.end(action(req.params.payload)); // OK but flagged [INCONSISTENCY]. `action` is either the `play` or the `pause` function from above
16+
} else {
17+
res.end("Unsupported action.");
18+
}
19+
});

0 commit comments

Comments
 (0)