diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTamperingQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTamperingQuery.qll index da6b698d97f1..8e24b3e36d81 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTamperingQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTamperingQuery.qll @@ -27,4 +27,30 @@ class Configuration extends DataFlow::Configuration { } override predicate isBarrier(DataFlow::Node node) { node instanceof Barrier } + + override predicate isBarrierGuard(DataFlow::BarrierGuardNode guard) { + guard instanceof TypeOfTestBarrier or + guard instanceof IsArrayBarrier + } +} + +private class TypeOfTestBarrier extends DataFlow::BarrierGuardNode, DataFlow::ValueNode { + override EqualityTest astNode; + + TypeOfTestBarrier() { TaintTracking::isTypeofGuard(astNode, _, _) } + + override predicate blocks(boolean outcome, Expr e) { + if TaintTracking::isTypeofGuard(astNode, e, ["string", "object"]) + then outcome = [true, false] // separation between string/array removes type confusion in both branches + else outcome = astNode.getPolarity() // block flow to branch where value is neither string nor array + } +} + +private class IsArrayBarrier extends DataFlow::BarrierGuardNode, DataFlow::CallNode { + IsArrayBarrier() { this = DataFlow::globalVarRef("Array").getAMemberCall("isArray").getACall() } + + override predicate blocks(boolean outcome, Expr e) { + e = getArgument(0).asExpr() and + outcome = [true, false] // separation between string/array removes type confusion in both branches + } } diff --git a/javascript/ql/test/query-tests/Security/CWE-843/TypeConfusionThroughParameterTampering.expected b/javascript/ql/test/query-tests/Security/CWE-843/TypeConfusionThroughParameterTampering.expected index 5c560d8c4cae..997d8968fcca 100644 --- a/javascript/ql/test/query-tests/Security/CWE-843/TypeConfusionThroughParameterTampering.expected +++ b/javascript/ql/test/query-tests/Security/CWE-843/TypeConfusionThroughParameterTampering.expected @@ -32,6 +32,18 @@ nodes | tst.js:81:9:81:9 | p | | tst.js:82:9:82:9 | p | | tst.js:82:9:82:9 | p | +| tst.js:90:5:90:12 | data.foo | +| tst.js:90:5:90:12 | data.foo | +| tst.js:90:5:90:12 | data.foo | +| tst.js:92:9:92:16 | data.foo | +| tst.js:92:9:92:16 | data.foo | +| tst.js:92:9:92:16 | data.foo | +| tst.js:95:9:95:16 | data.foo | +| tst.js:95:9:95:16 | data.foo | +| tst.js:95:9:95:16 | data.foo | +| tst.js:98:9:98:16 | data.foo | +| tst.js:98:9:98:16 | data.foo | +| tst.js:98:9:98:16 | data.foo | edges | tst.js:5:9:5:27 | foo | tst.js:6:5:6:7 | foo | | tst.js:5:9:5:27 | foo | tst.js:6:5:6:7 | foo | @@ -63,6 +75,10 @@ edges | tst.js:80:23:80:23 | p | tst.js:81:9:81:9 | p | | tst.js:80:23:80:23 | p | tst.js:82:9:82:9 | p | | tst.js:80:23:80:23 | p | tst.js:82:9:82:9 | p | +| tst.js:90:5:90:12 | data.foo | tst.js:90:5:90:12 | data.foo | +| tst.js:92:9:92:16 | data.foo | tst.js:92:9:92:16 | data.foo | +| tst.js:95:9:95:16 | data.foo | tst.js:95:9:95:16 | data.foo | +| tst.js:98:9:98:16 | data.foo | tst.js:98:9:98:16 | data.foo | #select | 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 | | 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 | 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 | | 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 | | 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 | +| 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 | +| 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 | +| 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 | +| 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 | diff --git a/javascript/ql/test/query-tests/Security/CWE-843/tst.js b/javascript/ql/test/query-tests/Security/CWE-843/tst.js index 82650bcf0542..d49f7ce53d2b 100644 --- a/javascript/ql/test/query-tests/Security/CWE-843/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-843/tst.js @@ -1,7 +1,7 @@ var express = require('express'); var Koa = require('koa'); -express().get('/some/path', function(req, res) { +express().get('/some/path', function (req, res) { var foo = req.query.foo; foo.indexOf(); // NOT OK @@ -41,38 +41,38 @@ express().get('/some/path', function(req, res) { foo.length; // NOT OK }); -new Koa().use(function handler(ctx){ +new Koa().use(function handler(ctx) { var foo = ctx.request.query.foo; foo.indexOf(); // NOT OK }); -express().get('/some/path/:foo', function(req, res) { +express().get('/some/path/:foo', function (req, res) { var foo = req.params.foo; foo.indexOf(); // OK }); -express().get('/some/path/:foo', function(req, res) { - if (req.query.path.length) {} // OK +express().get('/some/path/:foo', function (req, res) { + if (req.query.path.length) { } // OK req.query.path.length == 0; // OK !req.query.path.length; // OK req.query.path.length > 0; // OK }); -express().get('/some/path/:foo', function(req, res) { +express().get('/some/path/:foo', function (req, res) { let p = req.query.path; if (typeof p !== 'string') { - return; + return; } while (p.length) { // OK - p = p.substr(1); + p = p.substr(1); } p.length < 1; // OK }); -express().get('/some/path/:foo', function(req, res) { +express().get('/some/path/:foo', function (req, res) { let someObject = {}; safeGet(someObject, req.query.path).bar = 'baz'; // prototype pollution here - but flagged in `safeGet` }); @@ -84,3 +84,26 @@ function safeGet(obj, p) { } return obj[p]; } + +express().get('/foo', function (req, res) { + let data = req.query; + data.foo.indexOf(); // NOT OK + if (typeof data.foo !== 'undefined') { + data.foo.indexOf(); // NOT OK + } + if (typeof data.foo !== 'string') { + data.foo.indexOf(); // OK + } + if (typeof data.foo !== 'undefined') { + data.foo.indexOf(); // NOT OK + } +}); + +express().get('/foo', function (req, res) { + let data = req.query; + if (Array.isArray(data)) { + data.indexOf(); // OK + } else { + data.indexOf(); // OK + } +});