Skip to content

Commit ada72b8

Browse files
asgerfasgerf
authored
Merge pull request #10332 from asgerf/js/type-confusion-bugfix
JS: bugfixes in TypeThroughThroughParameterTampering
2 parents 30c9bea + 6806bc1 commit ada72b8

File tree

4 files changed

+24
-18
lines changed

4 files changed

+24
-18
lines changed

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,11 @@ class Configuration extends DataFlow::Configuration {
2626
sink.analyze().getAType() = TTObject()
2727
}
2828

29-
override predicate isBarrier(DataFlow::Node node) { node instanceof Barrier }
29+
override predicate isBarrier(DataFlow::Node node) {
30+
super.isBarrier(node)
31+
or
32+
node instanceof Barrier
33+
}
3034

3135
override predicate isBarrierGuard(DataFlow::BarrierGuardNode guard) {
3236
guard instanceof TypeOfTestBarrier or
@@ -50,7 +54,7 @@ private class TypeOfTestBarrier extends DataFlow::BarrierGuardNode, DataFlow::Va
5054
}
5155

5256
private class IsArrayBarrier extends DataFlow::BarrierGuardNode, DataFlow::CallNode {
53-
IsArrayBarrier() { this = DataFlow::globalVarRef("Array").getAMemberCall("isArray").getACall() }
57+
IsArrayBarrier() { this = DataFlow::globalVarRef("Array").getAMemberCall("isArray") }
5458

5559
override predicate blocks(boolean outcome, Expr e) {
5660
e = getArgument(0).asExpr() and

‎javascript/ql/src/change-notes/2022-09-07-type-confusion-bugfix.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
category: fix
3+
---
4+
5+
- Fixed a bug in the `js/type-confusion-through-parameter-tampering` query that would cause it to ignore
6+
sanitizers in branching conditions. The query should now report fewer false positives.

‎javascript/ql/test/query-tests/Security/CWE-843/TypeConfusionThroughParameterTampering.expected

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,6 @@ nodes
1616
| tst.js:27:5:27:7 | foo |
1717
| tst.js:28:5:28:7 | foo |
1818
| tst.js:28:5:28:7 | foo |
19-
| tst.js:36:9:36:11 | foo |
20-
| tst.js:36:9:36:11 | foo |
21-
| tst.js:41:5:41:7 | foo |
22-
| tst.js:41:5:41:7 | foo |
2319
| tst.js:45:9:45:35 | foo |
2420
| tst.js:45:15:45:35 | ctx.req ... ery.foo |
2521
| tst.js:45:15:45:35 | ctx.req ... ery.foo |
@@ -38,12 +34,14 @@ nodes
3834
| tst.js:92:9:92:16 | data.foo |
3935
| tst.js:92:9:92:16 | data.foo |
4036
| tst.js:92:9:92:16 | data.foo |
41-
| tst.js:95:9:95:16 | data.foo |
42-
| tst.js:95:9:95:16 | data.foo |
43-
| tst.js:95:9:95:16 | data.foo |
4437
| tst.js:98:9:98:16 | data.foo |
4538
| tst.js:98:9:98:16 | data.foo |
4639
| tst.js:98:9:98:16 | data.foo |
40+
| tst.js:103:9:103:29 | data |
41+
| tst.js:103:16:103:29 | req.query.data |
42+
| tst.js:103:16:103:29 | req.query.data |
43+
| tst.js:104:5:104:8 | data |
44+
| tst.js:104:5:104:8 | data |
4745
edges
4846
| tst.js:5:9:5:27 | foo | tst.js:6:5:6:7 | foo |
4947
| tst.js:5:9:5:27 | foo | tst.js:6:5:6:7 | foo |
@@ -56,10 +54,6 @@ edges
5654
| tst.js:5:9:5:27 | foo | tst.js:27:5:27:7 | foo |
5755
| tst.js:5:9:5:27 | foo | tst.js:28:5:28:7 | foo |
5856
| tst.js:5:9:5:27 | foo | tst.js:28:5:28:7 | foo |
59-
| tst.js:5:9:5:27 | foo | tst.js:36:9:36:11 | foo |
60-
| tst.js:5:9:5:27 | foo | tst.js:36:9:36:11 | foo |
61-
| tst.js:5:9:5:27 | foo | tst.js:41:5:41:7 | foo |
62-
| tst.js:5:9:5:27 | foo | tst.js:41:5:41:7 | foo |
6357
| tst.js:5:15:5:27 | req.query.foo | tst.js:5:9:5:27 | foo |
6458
| tst.js:5:15:5:27 | req.query.foo | tst.js:5:9:5:27 | foo |
6559
| tst.js:14:16:14:18 | bar | tst.js:15:9:15:11 | bar |
@@ -77,21 +71,22 @@ edges
7771
| tst.js:80:23:80:23 | p | tst.js:82:9:82:9 | p |
7872
| tst.js:90:5:90:12 | data.foo | tst.js:90:5:90:12 | data.foo |
7973
| tst.js:92:9:92:16 | data.foo | tst.js:92:9:92:16 | data.foo |
80-
| tst.js:95:9:95:16 | data.foo | tst.js:95:9:95:16 | data.foo |
8174
| tst.js:98:9:98:16 | data.foo | tst.js:98:9:98:16 | data.foo |
75+
| tst.js:103:9:103:29 | data | tst.js:104:5:104:8 | data |
76+
| tst.js:103:9:103:29 | data | tst.js:104:5:104:8 | data |
77+
| tst.js:103:16:103:29 | req.query.data | tst.js:103:9:103:29 | data |
78+
| tst.js:103:16:103:29 | req.query.data | tst.js:103:9:103:29 | data |
8279
#select
8380
| tst.js:6:5:6:7 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:6:5:6:7 | foo | Potential type confusion as $@ may be either an array or a string. | tst.js:5:15:5:27 | req.query.foo | this HTTP request parameter |
8481
| tst.js:8:5:8:7 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:8:5:8:7 | foo | Potential type confusion as $@ may be either an array or a string. | tst.js:5:15:5:27 | req.query.foo | this HTTP request parameter |
8582
| tst.js:11:9:11:11 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:11:9:11:11 | foo | Potential type confusion as $@ may be either an array or a string. | tst.js:5:15:5:27 | req.query.foo | this HTTP request parameter |
8683
| tst.js:15:9:15:11 | bar | tst.js:5:15:5:27 | req.query.foo | tst.js:15:9:15:11 | bar | Potential type confusion as $@ may be either an array or a string. | tst.js:5:15:5:27 | req.query.foo | this HTTP request parameter |
8784
| tst.js:27:5:27:7 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:27:5:27:7 | foo | Potential type confusion as $@ may be either an array or a string. | tst.js:5:15:5:27 | req.query.foo | this HTTP request parameter |
8885
| tst.js:28:5:28:7 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:28:5:28:7 | foo | Potential type confusion as $@ may be either an array or a string. | tst.js:5:15:5:27 | req.query.foo | this HTTP request parameter |
89-
| tst.js:36:9:36:11 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:36:9:36:11 | foo | Potential type confusion as $@ may be either an array or a string. | tst.js:5:15:5:27 | req.query.foo | this HTTP request parameter |
90-
| tst.js:41:5:41:7 | foo | tst.js:5:15:5:27 | req.query.foo | tst.js:41:5:41:7 | foo | Potential type confusion as $@ may be either an array or a string. | tst.js:5:15:5:27 | req.query.foo | this HTTP request parameter |
9186
| tst.js:46:5:46:7 | foo | tst.js:45:15:45:35 | ctx.req ... ery.foo | tst.js:46:5:46:7 | foo | Potential type confusion as $@ may be either an array or a string. | tst.js:45:15:45:35 | ctx.req ... ery.foo | this HTTP request parameter |
9287
| tst.js:81:9:81:9 | p | tst.js:77:25:77:38 | req.query.path | tst.js:81:9:81:9 | p | Potential type confusion as $@ may be either an array or a string. | tst.js:77:25:77:38 | req.query.path | this HTTP request parameter |
9388
| tst.js:82:9:82:9 | p | tst.js:77:25:77:38 | req.query.path | tst.js:82:9:82:9 | p | Potential type confusion as $@ may be either an array or a string. | tst.js:77:25:77:38 | req.query.path | this HTTP request parameter |
9489
| tst.js:90:5:90:12 | data.foo | tst.js:90:5:90:12 | data.foo | tst.js:90:5:90:12 | data.foo | Potential type confusion as $@ may be either an array or a string. | tst.js:90:5:90:12 | data.foo | this HTTP request parameter |
9590
| tst.js:92:9:92:16 | data.foo | tst.js:92:9:92:16 | data.foo | tst.js:92:9:92:16 | data.foo | Potential type confusion as $@ may be either an array or a string. | tst.js:92:9:92:16 | data.foo | this HTTP request parameter |
96-
| tst.js:95:9:95:16 | data.foo | tst.js:95:9:95:16 | data.foo | tst.js:95:9:95:16 | data.foo | Potential type confusion as $@ may be either an array or a string. | tst.js:95:9:95:16 | data.foo | this HTTP request parameter |
9791
| tst.js:98:9:98:16 | data.foo | tst.js:98:9:98:16 | data.foo | tst.js:98:9:98:16 | data.foo | Potential type confusion as $@ may be either an array or a string. | tst.js:98:9:98:16 | data.foo | this HTTP request parameter |
92+
| tst.js:104:5:104:8 | data | tst.js:103:16:103:29 | req.query.data | tst.js:104:5:104:8 | data | Potential type confusion as $@ may be either an array or a string. | tst.js:103:16:103:29 | req.query.data | this HTTP request parameter |

‎javascript/ql/test/query-tests/Security/CWE-843/tst.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,8 @@ express().get('/foo', function (req, res) {
100100
});
101101

102102
express().get('/foo', function (req, res) {
103-
let data = req.query;
103+
let data = req.query.data;
104+
data.indexOf(); // NOT OK
104105
if (Array.isArray(data)) {
105106
data.indexOf(); // OK
106107
} else {

0 commit comments

Comments
 (0)