Merge pull request #89 from esben-semmle/js/sharpen-type-confusion

JS: remove emptiness checks from the type confusion `x.length` sinks

Author: Max Schaefer

change-notes/1.18/analysis-javascript.md

@@ -106,6 +106,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. |

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 cannot control an emptiness check
+        not (
+          exists (ConditionGuardNode cond |
+            read.asExpr() = cond.getTest()
+          )
+          or
+          exists (Comparison cmp, Expr zero |
+            zero.getIntValue() = 0 and
+            cmp.hasOperands(read.asExpr(), zero)
+          )
+          or
+          exists (LogNotExpr neg |
+            neg.getOperand() = read.asExpr()
+          )
+        )
       )
     }
 

javascript/ql/test/query-tests/Security/CWE-843/tst.js

@@ -50,3 +50,22 @@ 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; // 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);
+    }
+});