From 52d86471af33190a169cbafdc9334f76a44e09c9 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esben@semmle.com>
Date: Mon, 8 Apr 2019 09:50:27 +0200
Subject: [PATCH] JS: whitelist another emptiness check for the type-confusion
 query

---
 change-notes/1.21/analysis-javascript.md             |  1 +
 .../TypeConfusionThroughParameterTampering.qll       | 12 +++++++++++-
 .../ql/test/query-tests/Security/CWE-843/tst.js      |  2 ++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/change-notes/1.21/analysis-javascript.md b/change-notes/1.21/analysis-javascript.md
index 1cdfc75e4d7..276ef11d4e6 100644
--- a/change-notes/1.21/analysis-javascript.md
+++ b/change-notes/1.21/analysis-javascript.md
@@ -29,6 +29,7 @@
 | Incomplete string escaping or encoding | More results | This rule now considers the flow of regular expressions literals. |
 | Replacement of a substring with itself | More results | This rule now considers the flow of regular expressions literals. |
 | Server-side URL redirect       | Fewer false-positive results | This rule now treats URLs as safe in more cases where the hostname cannot be tampered with. |
+| Type confusion through parameter tampering | Fewer false-positive results | This rule now recognizes additional emptiness checks. |
 | Useless assignment to property | Fewer false-positive results | This rule now ignore reads of additional getters. |
 
 ## Changes to QL libraries
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTampering.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTampering.qll
index c21f07c46b6..50087cf6512 100644
--- a/javascript/ql/src/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTampering.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/TypeConfusionThroughParameterTampering.qll
@@ -83,15 +83,25 @@ module TypeConfusionThroughParameterTampering {
     LengthAccess() {
       exists(DataFlow::PropRead read |
         read.accesses(this, "length") and
-        // exclude truthiness checks on the length: an array/string confusion cannot control an emptiness check
+        // an array/string confusion cannot control an emptiness check
         not (
+          // `if (x.length) {...}`
           exists(ConditionGuardNode cond | read.asExpr() = cond.getTest())
           or
+          // `x.length == 0`, `x.length > 0`
           exists(Comparison cmp, Expr zero |
             zero.getIntValue() = 0 and
             cmp.hasOperands(read.asExpr(), zero)
           )
           or
+          // `x.length < 1`
+          exists(RelationalComparison cmp |
+            cmp.getLesserOperand() = read.asExpr() and
+            cmp.getGreaterOperand().getIntValue() = 1 and
+            not cmp.isInclusive()
+          )
+          or
+          // `!x.length`
           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 2e682544da2..45753b8d841 100644
--- a/javascript/ql/test/query-tests/Security/CWE-843/tst.js
+++ b/javascript/ql/test/query-tests/Security/CWE-843/tst.js
@@ -68,4 +68,6 @@ express().get('/some/path/:foo', function(req, res) {
     while (p.length) { // OK
       p = p.substr(1);
     }
+
+    p.length < 1; // OK
 });