Add query for detecting potential DOS form a tainted .length property

javascript/ql/src/Security/CWE-834/TaintedLength.qhelp

@@ -0,0 +1,67 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+
+<overview>
+    <p>
+        Iterating the elements of an untrusted object using the 
+        <code>.length</code> property can lead to a server looping 
+        indefinitely or crashing. Thereby creating a 
+        denial-of-service or DOS.
+        This happens when an attacker creates a JSON object with an
+        absurdly large number in the .length property that the server then 
+        loops through.
+        <br />
+        The problem can also happen when using utility methods from Lodash or 
+        Underscore that operate on array-like values. 
+        As a simple example of how a DOS can happen, this code will crash most
+        servers with an out-of-memory exception 
+        <code>_.map({length:999999999})</code>. 
+    </p>
+</overview>
+
+<recommendation>
+    <p>
+        The attack is easy to prevent and there are multiple ways of doing so.
+        Forcing the user controlled object to be an array or preventing the 
+        <code>.length</code> property from being too large can limit the 
+        impact of the attack. 
+        <br/>
+        Alternatively the loop can exit early if the currently iterated element
+        is seen to be <code>undefined</code>, as the attacker cannot create an
+        array-like object with non-<code>undefined</code> values for an 
+        unlimited amount of array elements. 
+        <br />
+        Accessing a property of the currently iterated element will also
+        prevent the attack, as a null-pointer exception will occur in the first
+        iteration where the element is <code>undefined</code>.
+    </p> 
+</recommendation>
+
+<example>
+    <p>
+        In the example below, a server iterates over a user controlled object
+        <code>obj</code> using the <code>obj.length</code> property in order 
+        to copy the elements from <code>obj</code> to an array. 
+    </p>
+
+    <sample src="examples/TaintedLength.js"/>
+
+    <p>
+        This is not secure since an attacker can control the value of 
+        <code>obj.length</code>, and thereby cause the loop to loop 
+        indefinitely. 
+        Here the potential DOS is fixed by enforcing that the user controlled 
+        object is an array. 
+    </p>
+
+    <sample src="examples/TaintedLength_fixed.js"/>
+</example>
+
+<references>
+    <li>CWE entry:
+        <a href="https://cwe.mitre.org/data/definitions/834.html">CWE-834</a>
+    </li>
+</references>
+</qhelp>

javascript/ql/src/Security/CWE-834/TaintedLength.ql

@@ -0,0 +1,22 @@
+/**
+ * @name Tainted .length looping
+ * @description If server-side code iterates over a user-controlled object with
+ *              an arbitary .length value, then an attacker can trick the server
+ *              to loop infinitely. 
+ * @kind path-problem
+ * @problem.severity warning
+ * @id js/tainted-length-looping
+ * @tags security
+ * @precision low
+ */
+
+import javascript
+
+import semmle.javascript.security.dataflow.TaintedLength::TaintedLength
+
+from
+  Configuration dataflow, DataFlow::PathNode source, DataFlow::PathNode sink
+where dataflow.hasFlowPath(source, sink)
+select sink, source, sink,
+  "Iterating over user controlled object with an unbounded .length property $@.",
+  source, "here"

javascript/ql/src/Security/CWE-834/examples/TaintedLength.js

@@ -0,0 +1,22 @@
+'use strict';
+
+var express = require('express');
+var router = new express.Router();
+var rootRoute = router.route('foobar');
+
+rootRoute.post(function(req, res) {
+    var obj = req.body;
+
+    calcStuff(obj);
+});
+
+function calcStuff(obj) {
+    var ret = [];
+
+    // potential DOS if obj.length is large.
+    for (var i = 0; i < obj.length; i++) {
+        ret.push(obj[i]);
+    }
+
+    return ret;
+}

javascript/ql/src/Security/CWE-834/examples/TaintedLength_fixed.js

@@ -0,0 +1,24 @@
+'use strict';
+
+var express = require('express');
+var router = new express.Router();
+var rootRoute = router.route('foobar');
+
+rootRoute.post(function(req, res) {
+    var obj = req.body;
+
+    calcStuff(obj);
+});
+
+function calcStuff(obj) {
+    if (!(obj instanceof Array)) { // prevents DOS
+        return [];
+    }
+
+    var ret = [];
+    for (var i = 0; i < obj.length; i++) {
+        ret.push(obj[i]);
+    }
+
+    return ret;
+}

javascript/ql/src/semmle/javascript/security/dataflow/TaintedLength.qll

@@ -0,0 +1,43 @@
+/**
+ * Provides a taint tracking configuration for reasoning about DOS attacks
+ * using a user controlled object with an unbounded .length property. 
+ *
+ * Note, for performance reasons: only import this file if
+ * `TaintedLength::Configuration` is needed, otherwise
+ * `TaintedLengthCustomizations` should be imported instead.
+ */
+
+import javascript
+import semmle.javascript.security.TaintedObject
+
+module TaintedLength {
+  import TaintedLengthCustomizations::TaintedLength
+
+  /**
+   * A taint-tracking configuration for reasoning about looping on tainted objects with unbounded length.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "LabeledLoopTaintedObject" }
+
+    override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
+      source instanceof Source and label = TaintedObject::label()
+    }
+
+    override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
+      sink instanceof Sink and label = TaintedObject::label()
+    }
+
+    override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
+      guard instanceof TaintedObject::SanitizerGuard or
+      guard instanceof IsArraySanitizerGuard or
+      guard instanceof InstanceofArraySanitizerGuard or
+      guard instanceof LengthCheckSanitizerGuard
+    }
+
+    override predicate isAdditionalFlowStep(
+      DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl
+    ) {
+      TaintedObject::step(src, trg, inlbl, outlbl)
+    }
+  }
+}

javascript/ql/src/semmle/javascript/security/dataflow/TaintedLengthCustomizations.qll

@@ -0,0 +1,256 @@
+/**
+ * Provides default sources, sinks and sanitisers for reasoning about
+ * DOS attacks using objects with unbounded length object. 
+ * As well as extension points for adding your own.
+ */
+
+import javascript
+
+module TaintedLength {
+  import semmle.javascript.security.dataflow.RemoteFlowSources
+  import semmle.javascript.security.TaintedObject
+  import DataFlow::PathGraph
+
+  // Heavily inspired by Expr::inNullSensitiveContext
+  predicate isCrashingWithNullValues(Expr e) {
+    exists(ExprOrStmt ctx |
+      e = ctx.(PropAccess).getBase()
+      or
+      e = ctx.(InvokeExpr).getCallee()
+      or
+      e = ctx.(AssignExpr).getRhs() and
+      ctx.(AssignExpr).getLhs() instanceof DestructuringPattern
+      or
+      e = ctx.(SpreadElement).getOperand()
+      or
+      e = ctx.(ForOfStmt).getIterationDomain()
+    )
+  }
+
+  // Inspired by LoopIterationSkippedDueToShifting::ArrayIterationLoop
+  // Added some Dataflow to the .length access.
+  // Added support for while/dowhile loops.
+  class ArrayIterationLoop extends LoopStmt {
+    LocalVariable indexVariable;
+
+    ArrayIterationLoop() {
+      exists(RelationalComparison compare, DataFlow::PropRead lengthRead |
+        compare = this.getTest() and
+        compare.getLesserOperand() = indexVariable.getAnAccess() and
+        lengthRead.accesses(_, "length") and
+        lengthRead.flowsToExpr(compare.getGreaterOperand())
+      ) and
+      (
+        this.(ForStmt).getUpdate().(IncExpr).getOperand() = indexVariable.getAnAccess() or
+        this.getBody().getAChild*().(IncExpr).getOperand() = indexVariable.getAnAccess()
+      )
+    }
+
+    override Stmt getBody() {
+      result = this.(ForStmt).getBody() or
+      result = this.(WhileStmt).getBody() or
+      result = this.(DoWhileStmt).getBody()
+    }
+
+    override Expr getTest() {
+      result = this.(ForStmt).getTest() or
+      result = this.(WhileStmt).getTest() or
+      result = this.(DoWhileStmt).getTest()
+    }
+
+    /**
+     * Gets the variable holding the loop variable and current array index.
+     */
+    LocalVariable getIndexVariable() { result = indexVariable }
+  }
+
+  abstract class Sink extends DataFlow::Node { }
+
+  // for (..; .. sink.length; ...) ...
+  private class LoopSink extends Sink {
+    LoopSink() {
+      exists(ArrayIterationLoop loop, Expr lengthAccess, DataFlow::PropRead lengthRead |
+        loop.getTest().getAChild*() = lengthAccess and
+        lengthRead.flowsToExpr(lengthAccess) and
+        lengthRead.accesses(this, "length") and
+        // In the DOS we are looking for arrayRead will evaluate to undefined.
+        // If an obvious nullpointer happens on this undefined, then the DOS cannot happen.
+        not exists(DataFlow::PropRead arrayRead, Expr throws |
+          // It doesn't only happen inside the for-loop, outside is also a sink (assuming it happens before).
+          loop.getBody().getAChild*() = arrayRead.asExpr() and
+          loop.getBody().getAChild*() = throws and
+          arrayRead.getPropertyNameExpr() = loop.getIndexVariable().getAnAccess() and
+          arrayRead.flowsToExpr(throws) and
+          isCrashingWithNullValues(throws)
+        ) and
+        // The existance of some kind of early-exit usually indicates that the loop will stop early and no DOS happens.
+        not exists(BreakStmt br | br.getTarget() = loop) and
+        not exists(ReturnStmt ret |
+          loop.getBody().getAChild*() = ret and
+          ret.getContainer() = loop.getContainer()
+        ) and
+        not exists(ThrowStmt throw |
+          loop.getBody().getAChild*() = throw and
+          not loop.getBody().getAChild*() = throw.getTarget()
+        )
+      )
+    }
+  }
+
+  predicate loopableLodashMethod(string name) {
+    name = "chunk" or
+    name = "compact" or
+    name = "difference" or
+    name = "differenceBy" or
+    name = "differenceWith" or
+    name = "drop" or
+    name = "dropRight" or
+    name = "dropRightWhile" or
+    name = "dropWhile" or
+    name = "fill" or
+    name = "findIndex" or
+    name = "findLastIndex" or
+    name = "flatten" or
+    name = "flattenDeep" or
+    name = "flattenDepth" or
+    name = "initial" or
+    name = "intersection" or
+    name = "intersectionBy" or
+    name = "intersectionWith" or
+    name = "join" or
+    name = "remove" or
+    name = "reverse" or
+    name = "slice" or
+    name = "sortedUniq" or
+    name = "sortedUniqBy" or
+    name = "tail" or
+    name = "union" or
+    name = "unionBy" or
+    name = "unionWith" or
+    name = "uniqBy" or
+    name = "unzip" or
+    name = "unzipWith" or
+    name = "without" or
+    name = "zip" or
+    name = "zipObject" or
+    name = "zipObjectDeep" or
+    name = "zipWith" or
+    name = "countBy" or
+    name = "each" or
+    name = "forEach" or
+    name = "eachRight" or
+    name = "forEachRight" or
+    name = "filter" or
+    name = "find" or
+    name = "findLast" or
+    name = "flatMap" or
+    name = "flatMapDeep" or
+    name = "flatMapDepth" or
+    name = "forEach" or
+    name = "forEachRight" or
+    name = "groupBy" or
+    name = "invokeMap" or
+    name = "keyBy" or
+    name = "map" or
+    name = "orderBy" or
+    name = "partition" or
+    name = "reduce" or
+    name = "reduceRight" or
+    name = "reject" or
+    name = "sortBy"
+  }
+
+  // _.each(sink);
+  private class LodashIterationSink extends Sink {
+    DataFlow::CallNode call;
+
+    LodashIterationSink() {
+      exists(string name |
+        loopableLodashMethod(name) and
+        call = any(LodashUnderscore::member(name)).getACall() and
+        call.getArgument(0) = this and
+        
+        // Here it is just assumed that the array elements is the first parameter in the callback function.
+        not exists(DataFlow::FunctionNode func, DataFlow::ParameterNode e |
+          func.flowsTo(call.getAnArgument()) and
+          e = func.getParameter(0) and
+          (
+            // Looking for obvious null-pointers happening on the array elements in the iteration.
+            // Similar to what is done in the loop iteration sink.
+            exists(Expr throws |
+              throws = func.asExpr().(Function).getBody().getAChild*() and
+              e.flowsToExpr(throws) and
+              isCrashingWithNullValues(throws)
+            )
+            or
+            // similar to the loop sink - the existance of an early-exit usually means that no DOS can happen.
+            exists(ThrowStmt throw |
+              throw = func.asExpr().(Function).getBody().getAChild*() and
+              throw.getTarget() = func.asExpr()
+            )
+          )
+        )
+      )
+    }
+  }
+  
+  abstract class Source extends DataFlow::Node { }
+  
+  /**
+   * A source of remote user input objects.
+   */
+  class TaintedObjectSource extends Source {
+    TaintedObjectSource() { this instanceof TaintedObject::Source }
+  }
+  
+
+  class IsArraySanitizerGuard extends TaintTracking::LabeledSanitizerGuardNode,
+    DataFlow::ValueNode {
+    override CallExpr astNode;
+
+    IsArraySanitizerGuard() { astNode.getCalleeName() = "isArray" }
+
+    override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+      true = outcome and
+      e = astNode.getAnArgument() and
+      label = TaintedObject::label()
+    }
+  }
+
+  class InstanceofArraySanitizerGuard extends TaintTracking::LabeledSanitizerGuardNode,
+    DataFlow::ValueNode {
+    override BinaryExpr astNode;
+
+    InstanceofArraySanitizerGuard() {
+      astNode.getOperator() = "instanceof" and
+      astNode.getRightOperand().(Identifier).getName() = "Array"
+    }
+
+    override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+      true = outcome and
+      e = astNode.getLeftOperand() and
+      label = TaintedObject::label()
+    }
+  }
+
+  // Does two things:
+  // 1) Detects any length-check that limits the size of the .length property.
+  // 2) Makes sure that only the first loop that is DOS-prone is selected by the query. (due to the .length test having outcome=false when exiting the loop).
+  class LengthCheckSanitizerGuard extends TaintTracking::LabeledSanitizerGuardNode,
+    DataFlow::ValueNode {
+    override RelationalComparison astNode;
+
+    PropAccess propAccess;
+
+    LengthCheckSanitizerGuard() {
+      propAccess = astNode.getGreaterOperand().getAChild*() and
+      propAccess.getPropertyName() = "length"
+    }
+
+    override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+      false = outcome and
+      astNode.getAChild*() = e and
+      label = TaintedObject::label()
+    }
+  }
+}

javascript/ql/test/query-tests/Security/CWE-834/TaintedLength.expected

@@ -0,0 +1,57 @@
+nodes
+| TaintedLengthBad.js:8:10:8:17 | req.body |
+| TaintedLengthBad.js:10:12:10:19 | req.body |
+| TaintedLengthBad.js:12:22:12:29 | req.body |
+| TaintedLengthBad.js:14:16:14:23 | req.body |
+| TaintedLengthBad.js:17:18:17:20 | val |
+| TaintedLengthBad.js:21:22:21:24 | val |
+| TaintedLengthBad.js:26:20:26:22 | val |
+| TaintedLengthBad.js:30:13:30:15 | val |
+| TaintedLengthBad.js:37:30:37:32 | val |
+| TaintedLengthBad.js:40:12:40:14 | val |
+| TaintedLengthBad.js:49:24:49:26 | val |
+| TaintedLengthBad.js:54:22:54:24 | val |
+| TaintedLengthExitBad.js:8:9:8:16 | req.body |
+| TaintedLengthExitBad.js:10:9:10:16 | req.body |
+| TaintedLengthExitBad.js:12:10:12:17 | req.body |
+| TaintedLengthExitBad.js:14:14:14:21 | req.body |
+| TaintedLengthExitBad.js:17:17:17:19 | val |
+| TaintedLengthExitBad.js:20:22:20:24 | val |
+| TaintedLengthExitBad.js:30:17:30:19 | val |
+| TaintedLengthExitBad.js:33:22:33:24 | val |
+| TaintedLengthExitBad.js:46:18:46:20 | val |
+| TaintedLengthExitBad.js:49:22:49:24 | val |
+| TaintedLengthExitBad.js:59:22:59:24 | val |
+| TaintedLengthExitBad.js:60:8:60:10 | val |
+| TaintedLengthLodash.js:9:10:9:17 | req.body |
+| TaintedLengthLodash.js:14:18:14:20 | val |
+| TaintedLengthLodash.js:15:10:15:12 | val |
+edges
+| TaintedLengthBad.js:8:10:8:17 | req.body | TaintedLengthBad.js:17:18:17:20 | val |
+| TaintedLengthBad.js:10:12:10:19 | req.body | TaintedLengthBad.js:26:20:26:22 | val |
+| TaintedLengthBad.js:12:22:12:29 | req.body | TaintedLengthBad.js:37:30:37:32 | val |
+| TaintedLengthBad.js:14:16:14:23 | req.body | TaintedLengthBad.js:49:24:49:26 | val |
+| TaintedLengthBad.js:17:18:17:20 | val | TaintedLengthBad.js:21:22:21:24 | val |
+| TaintedLengthBad.js:26:20:26:22 | val | TaintedLengthBad.js:30:13:30:15 | val |
+| TaintedLengthBad.js:37:30:37:32 | val | TaintedLengthBad.js:40:12:40:14 | val |
+| TaintedLengthBad.js:49:24:49:26 | val | TaintedLengthBad.js:54:22:54:24 | val |
+| TaintedLengthExitBad.js:8:9:8:16 | req.body | TaintedLengthExitBad.js:17:17:17:19 | val |
+| TaintedLengthExitBad.js:10:9:10:16 | req.body | TaintedLengthExitBad.js:30:17:30:19 | val |
+| TaintedLengthExitBad.js:12:10:12:17 | req.body | TaintedLengthExitBad.js:46:18:46:20 | val |
+| TaintedLengthExitBad.js:14:14:14:21 | req.body | TaintedLengthExitBad.js:59:22:59:24 | val |
+| TaintedLengthExitBad.js:17:17:17:19 | val | TaintedLengthExitBad.js:20:22:20:24 | val |
+| TaintedLengthExitBad.js:30:17:30:19 | val | TaintedLengthExitBad.js:33:22:33:24 | val |
+| TaintedLengthExitBad.js:46:18:46:20 | val | TaintedLengthExitBad.js:49:22:49:24 | val |
+| TaintedLengthExitBad.js:59:22:59:24 | val | TaintedLengthExitBad.js:60:8:60:10 | val |
+| TaintedLengthLodash.js:9:10:9:17 | req.body | TaintedLengthLodash.js:14:18:14:20 | val |
+| TaintedLengthLodash.js:14:18:14:20 | val | TaintedLengthLodash.js:15:10:15:12 | val |
+#select
+| TaintedLengthBad.js:21:22:21:24 | val | TaintedLengthBad.js:8:10:8:17 | req.body | TaintedLengthBad.js:21:22:21:24 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthBad.js:8:10:8:17 | req.body | here |
+| TaintedLengthBad.js:30:13:30:15 | val | TaintedLengthBad.js:10:12:10:19 | req.body | TaintedLengthBad.js:30:13:30:15 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthBad.js:10:12:10:19 | req.body | here |
+| TaintedLengthBad.js:40:12:40:14 | val | TaintedLengthBad.js:12:22:12:29 | req.body | TaintedLengthBad.js:40:12:40:14 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthBad.js:12:22:12:29 | req.body | here |
+| TaintedLengthBad.js:54:22:54:24 | val | TaintedLengthBad.js:14:16:14:23 | req.body | TaintedLengthBad.js:54:22:54:24 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthBad.js:14:16:14:23 | req.body | here |
+| TaintedLengthExitBad.js:20:22:20:24 | val | TaintedLengthExitBad.js:8:9:8:16 | req.body | TaintedLengthExitBad.js:20:22:20:24 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthExitBad.js:8:9:8:16 | req.body | here |
+| TaintedLengthExitBad.js:33:22:33:24 | val | TaintedLengthExitBad.js:10:9:10:16 | req.body | TaintedLengthExitBad.js:33:22:33:24 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthExitBad.js:10:9:10:16 | req.body | here |
+| TaintedLengthExitBad.js:49:22:49:24 | val | TaintedLengthExitBad.js:12:10:12:17 | req.body | TaintedLengthExitBad.js:49:22:49:24 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthExitBad.js:12:10:12:17 | req.body | here |
+| TaintedLengthExitBad.js:60:8:60:10 | val | TaintedLengthExitBad.js:14:14:14:21 | req.body | TaintedLengthExitBad.js:60:8:60:10 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthExitBad.js:14:14:14:21 | req.body | here |
+| TaintedLengthLodash.js:15:10:15:12 | val | TaintedLengthLodash.js:9:10:9:17 | req.body | TaintedLengthLodash.js:15:10:15:12 | val | Iterating over user controlled object with an unbounded .length property $@. | TaintedLengthLodash.js:9:10:9:17 | req.body | here |

javascript/ql/test/query-tests/Security/CWE-834/TaintedLength.qlref

@@ -0,0 +1 @@
+Security/CWE-834/TaintedLength.ql

javascript/ql/test/query-tests/Security/CWE-834/TaintedLengthBad.js

@@ -0,0 +1,59 @@
+'use strict';
+
+var express = require('express');
+var router = new express.Router();
+var rootRoute = router.route('foobar');
+
+rootRoute.post(function(req, res) {
+    problem(req.body);
+
+    whileLoop(req.body);
+
+    useLengthIndirectly(req.body);
+
+    noNullPointer(req.body);
+});
+
+function problem(val) {
+    var ret = [];
+
+    // Potential DOS! .length property could have been set to an arbitrary
+    // value!
+    for (var i = 0; i < val.length; i++) {
+        ret.push(val[i]);
+    }
+}
+
+function whileLoop(val) {
+    var ret = [];
+    var i = 0;
+    // Potential DOS! .length property could have been set to an arbitrary
+    // value!
+    while (i < val.length) {
+        ret.push(val[i]);
+        i++;
+    }
+}
+
+function useLengthIndirectly(val) {
+    var ret = [];
+
+    var len = val.length;
+
+    // Same as above, but the .length access happens outside the loop.
+    for (var i = 0; i < len; i++) {
+        ret.push(val[i]);
+    }
+}
+
+// the obvious null-pointer detection should not hit this one.
+function noNullPointer(val) {
+    var ret = [];
+
+    const c = 0;
+
+    for (var i = 0; i < val.length; i++) {
+        ret.push(val[c].foo); // constantly accessing element 0, therefore not
+                                // guaranteed null-pointer.
+    }
+}

javascript/ql/test/query-tests/Security/CWE-834/TaintedLengthExitBad.js

@@ -0,0 +1,70 @@
+var express = require('express');
+var router = new express.Router();
+var rootRoute = router.route('foobar');
+
+var _ = require("lodash");
+
+rootRoute.post(function (req, res) {
+	breaks(req.body);
+	
+	throws(req.body);
+	
+	returns(req.body);
+	
+	lodashThrow(req.body);
+});
+
+function breaks(val) {
+	var ret = [];
+	
+	for (var i = 0; i < val.length; i++) {
+	  for (var k = 0; k < 2; k++) {
+		  if (k == 3) {
+		      // Does not prevent DOS, because this is inside an inner loop.
+			  break; 
+		  }
+	  }
+      ret.push(val[i]);
+    }
+}
+
+function throws(val) {
+	var ret = [];
+	
+	for (var i = 0; i < val.length; i++) {
+	  if (val[i] == null) {
+		  try {
+			  throw 2; // Is catched, and therefore the DOS is not prevented.
+		  } catch(e) {
+			  // ignored
+		  }
+	  }
+      ret.push(val[i]);
+    }
+}
+
+// the obvious null-pointer detection should not hit this one.
+function returns(val) {
+	var ret = [];
+	
+	for (var i = 0; i < val.length; i++) {
+	  if (val[i] == null) {
+		  (function (i) {
+			  return i+2; // Does not prevent DOS.
+		  })(i);
+	  }
+      ret.push(val[i]);
+    }
+}
+
+function lodashThrow(val) {
+	_.map(val, function (e) {
+		if (!e) {
+			try {
+				throw new Error(); // Does not prevent DOS
+			} catch(e) {
+				// ignored.
+			}
+		}
+	})
+}

javascript/ql/test/query-tests/Security/CWE-834/TaintedLengthExitGood.js

@@ -0,0 +1,57 @@
+var express = require('express');
+var router = new express.Router();
+var rootRoute = router.route('foobar');
+
+var _ = require("lodash");
+
+rootRoute.post(function (req, res) {
+	breaks(req.body);
+	
+	throws(req.body);
+	
+	returns(req.body);
+	
+	lodashThrow(req.body);
+});
+
+function breaks(val) {
+	var ret = [];
+	
+	for (var i = 0; i < val.length; i++) {
+	  if (val[i] == null) {
+		  break; // prevents DOS.
+	  }
+      ret.push(val[i]);
+    }
+}
+
+function throws(val) {
+	var ret = [];
+	
+	for (var i = 0; i < val.length; i++) {
+	  if (val[i] == null) {
+		  throw 2; // prevents DOS.
+	  }
+      ret.push(val[i]);
+    }
+}
+
+// the obvious null-pointer detection should not hit this one.
+function returns(val) {
+	var ret = [];
+	
+	for (var i = 0; i < val.length; i++) {
+	  if (val[i] == null) {
+		  return 2; // prevents DOS.
+	  }
+      ret.push(val[i]);
+    }
+}
+
+function lodashThrow(val) {
+	_.map(val, function (e) {
+		if (!e) {
+			throw new Error(); // prevents DOS.
+		}
+	})
+}

javascript/ql/test/query-tests/Security/CWE-834/TaintedLengthGood.js

@@ -0,0 +1,73 @@
+'use strict';
+
+var express = require('express');
+var router = new express.Router();
+var rootRoute = router.route('foobar');
+
+rootRoute.post(function(req, res) {
+    sanitized(req.body);
+
+    sanitized2(req.body);
+
+    sanitized3(req.body);
+
+    sanitized4(req.body);
+});
+
+function sanitized(val) {
+    var ret = [];
+
+    if (!Array.isArray(val)) {
+        return [];
+    }
+    // At this point we know that val must be an Array, and an attacked is
+    // therefore not able to send a cheap request that spends a lot of time
+    // inside the loop.
+    for (var i = 0; i < val.length; i++) {
+        ret.push(val[i] + 42);
+    }
+}
+
+function sanitized2(val) {
+    var ret = [];
+
+    if (typeof val === "object") {
+        return [];
+    }
+    // Val can only be a primitive. Therefore no issue!
+    for (var i = 0; i < val.length; i++) {
+        ret.push(val[i] + 42);
+    }
+}
+
+function isArray(foo) {
+    return foo instanceof Array;
+}
+
+function sanitized3(val) {
+    var ret = [];
+
+    if (!isArray(val)) {
+        return [];
+    }
+    // At this point we know that val must be an Array, and an attacked is
+    // therefore not able to send a cheap request that spends a lot of time
+    // inside the loop.
+    for (var i = 0; i < val.length; i++) {
+        ret.push(val[i] + 42);
+    }
+}
+
+function sanitized4(val) {
+    var ret = [];
+
+    if (!(val instanceof Array)) {
+        return [];
+    }
+    // At this point we know that val must be an Array, and an attacked is
+    // therefore not able to send a cheap request that spends a lot of time
+    // inside the loop.
+    for (var i = 0; i < val.length; i++) {
+        ret.push(val[i] + 42);
+    }
+}

javascript/ql/test/query-tests/Security/CWE-834/TaintedLengthLodash.js

@@ -0,0 +1,17 @@
+'use strict';
+
+var _ = require('lodash');
+var express = require('express');
+var router = new express.Router();
+var rootRoute = router.route('foobar');
+
+rootRoute.post(function(req, res) {
+    problem(req.body);
+
+    useLengthIndirectly(req.body);
+});
+
+function problem(val) {
+    // can take an arbitrary amount of time with a tainted .length property
+    _.chunk(val, 2);
+}

javascript/ql/test/query-tests/Security/CWE-834/TaintedLengthNested.js

@@ -0,0 +1,13 @@
+'use strict';
+
+var express = require('express');
+var router = new express.Router();
+var rootRoute = router.route('foobar');
+
+var global;
+
+rootRoute.post(function(req, res) {
+    for (i = 0; i < req.body.personas.length; i++) {
+        req.body.personas[i].parentesco.id;
+    }
+});

javascript/ql/test/query-tests/Security/CWE-834/TaintedLengthObviousLengthCheck.js

@@ -0,0 +1,22 @@
+'use strict';
+
+var express = require('express');
+var router = new express.Router();
+var rootRoute = router.route('foobar');
+
+rootRoute.post(function(req, res) {
+    problem(req.body);
+});
+
+function problem(val) {
+    var ret = [];
+
+    // Prevents DOS
+    if (val.length > 100) {
+        return [];
+    }
+
+    for (var i = 0; i < val.length; i++) {
+        ret.push(val[i]);
+    }
+}

javascript/ql/test/query-tests/Security/CWE-834/TaintedLengthObviousNullPointer.js

@@ -0,0 +1,58 @@
+'use strict';
+
+var express = require('express');
+var router = new express.Router();
+var rootRoute = router.route('foobar');
+
+var _ = require("lodash");
+
+rootRoute.post(function(req, res) {
+    nullPointer(req.body);
+    
+    nullPointer2(req.body);
+    
+    nullPointer3(req.body);
+    
+    lodashPointer(req.body);
+    
+    lodashArrowFunc(req.body);
+});
+
+function nullPointer(val) {
+    var ret = [];
+
+    for (var i = 0; i < val.length; i++) {
+        ret.push(val[i].foo + 42);
+    }
+}
+
+
+function nullPointer2(val) {
+    var ret = [];
+
+    for (var i = 0; i < val.length; i++) {
+    	var element = val[i];
+        ret.push(element.foo + 42);
+    }
+}
+
+function nullPointer3(val) {
+	let arr = val.messaging
+	for (let i = 0; i < arr.length; i++) {
+		let event = val.messaging[i]
+		let sender = event.sender.id
+	}
+}
+
+
+function lodashPointer(val) {
+    return _.map(val, function(e) {
+        return e.foo;
+    })
+}
+
+function lodashArrowFunc(val) {
+	return _.map(val, (e) => {
+        return e.foo;
+    });
+}

javascript/ql/test/query-tests/Security/CWE-834/TaintedLengthObviousNullPointerInPreviousLoop.js

@@ -0,0 +1,24 @@
+'use strict';
+
+var express = require('express');
+var router = new express.Router();
+var rootRoute = router.route('foobar');
+
+var _ = require("lodash");
+
+rootRoute.post(function(req, res) {
+    nullPointer(req.body);
+});
+
+function nullPointer(val) {
+    var ret = [];
+
+    // Has obvious null-pointer. And guards the next loop.
+    for (var i = 0; i < val.length; i++) {
+        ret.push(val[i].foo);
+    }
+
+    for (var i = 0; i < val.length; i++) {
+        ret.push(val[i]);
+    }
+}