Skip to content

Commit 5f42866

Browse files
asgerfasgerf
authored
Merge pull request github#9318 from asgerf/js/type-confusion-parmaeter-tampering-barrier
JS: Fix FP in js/type-confusion-through-parameter-tampering
2 parents adb40f9 + 7e76e9a commit 5f42866

File tree

3 files changed

+78
-9
lines changed

3 files changed

+78
-9
lines changed

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,4 +27,30 @@ class Configuration extends DataFlow::Configuration {
2727
}
2828

2929
override predicate isBarrier(DataFlow::Node node) { node instanceof Barrier }
30+
31+
override predicate isBarrierGuard(DataFlow::BarrierGuardNode guard) {
32+
guard instanceof TypeOfTestBarrier or
33+
guard instanceof IsArrayBarrier
34+
}
35+
}
36+
37+
private class TypeOfTestBarrier extends DataFlow::BarrierGuardNode, DataFlow::ValueNode {
38+
override EqualityTest astNode;
39+
40+
TypeOfTestBarrier() { TaintTracking::isTypeofGuard(astNode, _, _) }
41+
42+
override predicate blocks(boolean outcome, Expr e) {
43+
if TaintTracking::isTypeofGuard(astNode, e, ["string", "object"])
44+
then outcome = [true, false] // separation between string/array removes type confusion in both branches
45+
else outcome = astNode.getPolarity() // block flow to branch where value is neither string nor array
46+
}
47+
}
48+
49+
private class IsArrayBarrier extends DataFlow::BarrierGuardNode, DataFlow::CallNode {
50+
IsArrayBarrier() { this = DataFlow::globalVarRef("Array").getAMemberCall("isArray").getACall() }
51+
52+
override predicate blocks(boolean outcome, Expr e) {
53+
e = getArgument(0).asExpr() and
54+
outcome = [true, false] // separation between string/array removes type confusion in both branches
55+
}
3056
}

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,18 @@ nodes
3232
| tst.js:81:9:81:9 | p |
3333
| tst.js:82:9:82:9 | p |
3434
| tst.js:82:9:82:9 | p |
35+
| tst.js:90:5:90:12 | data.foo |
36+
| tst.js:90:5:90:12 | data.foo |
37+
| tst.js:90:5:90:12 | data.foo |
38+
| tst.js:92:9:92:16 | data.foo |
39+
| tst.js:92:9:92:16 | data.foo |
40+
| 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 |
44+
| tst.js:98:9:98:16 | data.foo |
45+
| tst.js:98:9:98:16 | data.foo |
46+
| tst.js:98:9:98:16 | data.foo |
3547
edges
3648
| tst.js:5:9:5:27 | foo | tst.js:6:5:6:7 | foo |
3749
| tst.js:5:9:5:27 | foo | tst.js:6:5:6:7 | foo |
@@ -63,6 +75,10 @@ edges
6375
| tst.js:80:23:80:23 | p | tst.js:81:9:81:9 | p |
6476
| tst.js:80:23:80:23 | p | tst.js:82:9:82:9 | p |
6577
| tst.js:80:23:80:23 | p | tst.js:82:9:82:9 | p |
78+
| tst.js:90:5:90:12 | data.foo | tst.js:90:5:90:12 | data.foo |
79+
| 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 |
81+
| tst.js:98:9:98:16 | data.foo | tst.js:98:9:98:16 | data.foo |
6682
#select
6783
| 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 |
6884
| 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 |
@@ -75,3 +91,7 @@ edges
7591
| 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 |
7692
| 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 |
7793
| 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 |
94+
| 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 |
95+
| 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 |
97+
| 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 |

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

Lines changed: 32 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
var express = require('express');
22
var Koa = require('koa');
33

4-
express().get('/some/path', function(req, res) {
4+
express().get('/some/path', function (req, res) {
55
var foo = req.query.foo;
66
foo.indexOf(); // NOT OK
77

@@ -41,38 +41,38 @@ express().get('/some/path', function(req, res) {
4141
foo.length; // NOT OK
4242
});
4343

44-
new Koa().use(function handler(ctx){
44+
new Koa().use(function handler(ctx) {
4545
var foo = ctx.request.query.foo;
4646
foo.indexOf(); // NOT OK
4747
});
4848

49-
express().get('/some/path/:foo', function(req, res) {
49+
express().get('/some/path/:foo', function (req, res) {
5050
var foo = req.params.foo;
5151
foo.indexOf(); // OK
5252
});
5353

54-
express().get('/some/path/:foo', function(req, res) {
55-
if (req.query.path.length) {} // OK
54+
express().get('/some/path/:foo', function (req, res) {
55+
if (req.query.path.length) { } // OK
5656
req.query.path.length == 0; // OK
5757
!req.query.path.length; // OK
5858
req.query.path.length > 0; // OK
5959
});
6060

61-
express().get('/some/path/:foo', function(req, res) {
61+
express().get('/some/path/:foo', function (req, res) {
6262
let p = req.query.path;
6363

6464
if (typeof p !== 'string') {
65-
return;
65+
return;
6666
}
6767

6868
while (p.length) { // OK
69-
p = p.substr(1);
69+
p = p.substr(1);
7070
}
7171

7272
p.length < 1; // OK
7373
});
7474

75-
express().get('/some/path/:foo', function(req, res) {
75+
express().get('/some/path/:foo', function (req, res) {
7676
let someObject = {};
7777
safeGet(someObject, req.query.path).bar = 'baz'; // prototype pollution here - but flagged in `safeGet`
7878
});
@@ -84,3 +84,26 @@ function safeGet(obj, p) {
8484
}
8585
return obj[p];
8686
}
87+
88+
express().get('/foo', function (req, res) {
89+
let data = req.query;
90+
data.foo.indexOf(); // NOT OK
91+
if (typeof data.foo !== 'undefined') {
92+
data.foo.indexOf(); // NOT OK
93+
}
94+
if (typeof data.foo !== 'string') {
95+
data.foo.indexOf(); // OK
96+
}
97+
if (typeof data.foo !== 'undefined') {
98+
data.foo.indexOf(); // NOT OK
99+
}
100+
});
101+
102+
express().get('/foo', function (req, res) {
103+
let data = req.query;
104+
if (Array.isArray(data)) {
105+
data.indexOf(); // OK
106+
} else {
107+
data.indexOf(); // OK
108+
}
109+
});

0 commit comments

Comments
 (0)