From fef257b1ecbc832220b65db49291a08cb306b26d Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Wed, 22 Aug 2018 13:20:52 +0200 Subject: [PATCH 1/3] JS: remove emptiness checks from the type confusion `x.length` sinks --- change-notes/1.18/analysis-javascript.md | 1 + .../TypeConfusionThroughParameterTampering.qll | 17 ++++++++++++++++- .../test/query-tests/Security/CWE-843/tst.js | 18 ++++++++++++++++++ 3 files changed, 35 insertions(+), 1 deletion(-) 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); + } +}); From 218c0cb51a6cc2213fbd1c3132f7ae24c6e320bf Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Wed, 22 Aug 2018 13:54:07 +0200 Subject: [PATCH 2/3] JS: address review comments --- .../dataflow/TypeConfusionThroughParameterTampering.qll | 8 ++++---- javascript/ql/test/query-tests/Security/CWE-843/tst.js | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTampering.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTampering.qll index fa5fd0512af..bbc1e714046 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTampering.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTampering.qll @@ -108,13 +108,13 @@ module TypeConfusionThroughParameterTampering { read.asExpr() = cond.getTest() ) or - exists (EqualityTest eq, Expr zero | + exists (Comparison cmp, Expr zero | zero.getIntValue() = 0 and - eq.hasOperands(read.asExpr(), zero) + cmp.hasOperands(read.asExpr(), zero) ) or - exists (LogNotExpr eq | - eq.getOperand() = read.asExpr() + exists (LogNotExpr neg | + neg.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 442a63277df..2e682544da2 100644 --- a/javascript/ql/test/query-tests/Security/CWE-843/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-843/tst.js @@ -54,7 +54,8 @@ express().get('/some/path/:foo', function(req, res) { 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 + !req.query.path.length; // OK + req.query.path.length > 0; // OK }); express().get('/some/path/:foo', function(req, res) { From b4c77b8344f4445cebd9ffcce8aefc707e52177b Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Wed, 22 Aug 2018 14:07:57 +0200 Subject: [PATCH 3/3] JS: s/can not/cannot/ --- .../dataflow/TypeConfusionThroughParameterTampering.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTampering.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTampering.qll index bbc1e714046..fa72fea1c0c 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTampering.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTampering.qll @@ -102,7 +102,7 @@ module TypeConfusionThroughParameterTampering { LengthAccess() { exists (DataFlow::PropRead read | read.accesses(this, "length") and - // exclude truthiness checks on the length: an array/string confusion can not control an emptiness check + // exclude truthiness checks on the length: an array/string confusion cannot control an emptiness check not ( exists (ConditionGuardNode cond | read.asExpr() = cond.getTest()