diff --git a/change-notes/1.18/analysis-javascript.md b/change-notes/1.18/analysis-javascript.md index 6b5812a8023..18c40d9a515 100644 --- a/change-notes/1.18/analysis-javascript.md +++ b/change-notes/1.18/analysis-javascript.md @@ -95,6 +95,7 @@ | Reflected cross-site scripting | Fewer false-positive results | This rule now treats header names case-insensitively. | | Server-side URL redirect | More true-positive results | This rule now treats header names case-insensitively. | | Superfluous trailing arguments | Fewer false-positive results | This rule now ignores calls to some empty functions. | +| Type confusion through parameter tampering | Fewer false-positive results | This rule no longer flags emptiness checks. | | Uncontrolled command line | More true-positive results | This rule now recognizes indirect command injection through `sh -c` and similar. | | Unused variable | Fewer results | This rule no longer flags class expressions that could be made anonymous. While technically true, these results are not interesting. | | Unused variable | Renamed | This rule has been renamed to "Unused variable, import, function or class" to reflect the fact that it flags different kinds of unused program elements. | diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTampering.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTampering.qll index 2ead7914ffc..fa5fd0512af 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTampering.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTampering.qll @@ -101,7 +101,22 @@ module TypeConfusionThroughParameterTampering { LengthAccess() { exists (DataFlow::PropRead read | - read.accesses(this, "length") + read.accesses(this, "length") and + // exclude truthiness checks on the length: an array/string confusion can not control an emptiness check + not ( + exists (ConditionGuardNode cond | + read.asExpr() = cond.getTest() + ) + or + exists (EqualityTest eq, Expr zero | + zero.getIntValue() = 0 and + eq.hasOperands(read.asExpr(), zero) + ) + or + exists (LogNotExpr eq | + eq.getOperand() = read.asExpr() + ) + ) ) } 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 81161cc2c75..442a63277df 100644 --- a/javascript/ql/test/query-tests/Security/CWE-843/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-843/tst.js @@ -50,3 +50,21 @@ 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 + req.query.path.length == 0; // OK + !req.query.path.length == 0; // OK +}); + +express().get('/some/path/:foo', function(req, res) { + let p = req.query.path; + + if (typeof p !== 'string') { + return; + } + + while (p.length) { // OK + p = p.substr(1); + } +});