diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTamperingQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTamperingQuery.qll index 1615a69eda7..cd4c31146a8 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTamperingQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTamperingQuery.qll @@ -26,7 +26,11 @@ class Configuration extends DataFlow::Configuration { sink.analyze().getAType() = TTObject() } - override predicate isBarrier(DataFlow::Node node) { node instanceof Barrier } + override predicate isBarrier(DataFlow::Node node) { + super.isBarrier(node) + or + node instanceof Barrier + } override predicate isBarrierGuard(DataFlow::BarrierGuardNode guard) { guard instanceof TypeOfTestBarrier or @@ -50,7 +54,7 @@ private class TypeOfTestBarrier extends DataFlow::BarrierGuardNode, DataFlow::Va } private class IsArrayBarrier extends DataFlow::BarrierGuardNode, DataFlow::CallNode { - IsArrayBarrier() { this = DataFlow::globalVarRef("Array").getAMemberCall("isArray").getACall() } + IsArrayBarrier() { this = DataFlow::globalVarRef("Array").getAMemberCall("isArray") } override predicate blocks(boolean outcome, Expr e) { e = getArgument(0).asExpr() and diff --git a/javascript/ql/src/change-notes/2022-09-07-type-confusion-bugfix.md b/javascript/ql/src/change-notes/2022-09-07-type-confusion-bugfix.md new file mode 100644 index 00000000000..af351eae597 --- /dev/null +++ b/javascript/ql/src/change-notes/2022-09-07-type-confusion-bugfix.md @@ -0,0 +1,6 @@ +--- +category: fix +--- + +- Fixed a bug in the `js/type-confusion-through-parameter-tampering` query that would cause it to ignore + sanitizers in branching conditions. The query should now report fewer false positives. 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 997d8968fcc..13c97e3f327 100644 --- a/javascript/ql/test/query-tests/Security/CWE-843/TypeConfusionThroughParameterTampering.expected +++ b/javascript/ql/test/query-tests/Security/CWE-843/TypeConfusionThroughParameterTampering.expected @@ -16,10 +16,6 @@ nodes | tst.js:27:5:27:7 | foo | | tst.js:28:5:28:7 | foo | | tst.js:28:5:28:7 | foo | -| tst.js:36:9:36:11 | foo | -| tst.js:36:9:36:11 | foo | -| tst.js:41:5:41:7 | foo | -| tst.js:41:5:41:7 | foo | | tst.js:45:9:45:35 | foo | | tst.js:45:15:45:35 | ctx.req ... ery.foo | | tst.js:45:15:45:35 | ctx.req ... ery.foo | @@ -38,12 +34,14 @@ nodes | 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 | +| tst.js:103:9:103:29 | data | +| tst.js:103:16:103:29 | req.query.data | +| tst.js:103:16:103:29 | req.query.data | +| tst.js:104:5:104:8 | data | +| tst.js:104:5:104:8 | data | 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 | @@ -56,10 +54,6 @@ edges | tst.js:5:9:5:27 | foo | tst.js:27:5:27:7 | foo | | tst.js:5:9:5:27 | foo | tst.js:28:5:28:7 | foo | | tst.js:5:9:5:27 | foo | tst.js:28:5:28:7 | foo | -| tst.js:5:9:5:27 | foo | tst.js:36:9:36:11 | foo | -| tst.js:5:9:5:27 | foo | tst.js:36:9:36:11 | foo | -| tst.js:5:9:5:27 | foo | tst.js:41:5:41:7 | foo | -| tst.js:5:9:5:27 | foo | tst.js:41:5:41:7 | foo | | tst.js:5:15:5:27 | req.query.foo | tst.js:5:9:5:27 | foo | | tst.js:5:15:5:27 | req.query.foo | tst.js:5:9:5:27 | foo | | tst.js:14:16:14:18 | bar | tst.js:15:9:15:11 | bar | @@ -77,8 +71,11 @@ edges | 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 | +| tst.js:103:9:103:29 | data | tst.js:104:5:104:8 | data | +| tst.js:103:9:103:29 | data | tst.js:104:5:104:8 | data | +| tst.js:103:16:103:29 | req.query.data | tst.js:103:9:103:29 | data | +| tst.js:103:16:103:29 | req.query.data | tst.js:103:9:103:29 | data | #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 | @@ -86,12 +83,10 @@ edges | 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 | | 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 | | 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 | -| 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 | -| 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 | | 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 | +| 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 | 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 d49f7ce53d2..3f5840b9f08 100644 --- a/javascript/ql/test/query-tests/Security/CWE-843/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-843/tst.js @@ -100,7 +100,8 @@ express().get('/foo', function (req, res) { }); express().get('/foo', function (req, res) { - let data = req.query; + let data = req.query.data; + data.indexOf(); // NOT OK if (Array.isArray(data)) { data.indexOf(); // OK } else {