Merge pull request #2165 from erik-krogh/dosHigh

Approved by asger-semmle

Author: semmle-qlci

change-notes/1.23/analysis-javascript.md

@@ -20,7 +20,7 @@
 | **Query**                                                                 | **Tags**                                                          | **Purpose**                                                                                                                                                                            |
 |---------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | Unused index variable (`js/unused-index-variable`)                        | correctness                                                       | Highlights loops that iterate over an array, but do not use the index variable to access array elements, indicating a possible typo or logic error. Results are shown on LGTM by default. |
-| Loop bound injection (`js/loop-bound-injection`)                          | security, external/cwe/cwe-834                                      | Highlights loops where a user-controlled object with an arbitrary .length value can trick the server to loop indefinitely. Results are not shown on LGTM by default. |
+| Loop bound injection (`js/loop-bound-injection`)                          | security, external/cwe/cwe-834                                      | Highlights loops where a user-controlled object with an arbitrary .length value can trick the server to loop indefinitely. Results are shown on LGTM by default. |
 | Suspicious method name (`js/suspicious-method-name-declaration`)          | correctness, typescript, methods                                  | Highlights suspiciously named methods where the developer likely meant to write a constructor or function. Results are shown on LGTM by default. |
 | Shell command built from environment values (`js/shell-command-injection-from-environment`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-088 | Highlights shell commands that may change behavior inadvertently depending on the execution environment, indicating a possible violation of [CWE-78](https://cwe.mitre.org/data/definitions/78.html). Results are shown on LGTM by default.|
 | Use of returnless function (`js/use-of-returnless-function`)              | maintainability, correctness                                      | Highlights calls where the return value is used, but the callee never returns a value. Results are shown on LGTM by default. |

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

@@ -7,7 +7,7 @@
  * @id js/loop-bound-injection
  * @tags security
  *       external/cwe/cwe-834
- * @precision medium
+ * @precision high
  */
 
 import javascript

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

@@ -79,21 +79,26 @@ module LoopBoundInjection {
    */
   abstract class Sink extends DataFlow::Node { }
 
+  /**
+   * Holds if there exists an array access indexed by the variable `var` where it is likely that
+   * the array access will cause a crash if `var` grows unbounded.
+   */
+  predicate hasCrashingArrayAccess(Variable var) {
+    exists(DataFlow::PropRead arrayRead, Expr throws |
+      arrayRead.getPropertyNameExpr() = var.getAnAccess() and
+      arrayRead.flowsToExpr(throws) and
+      isCrashingWithNullValues(throws)
+    )
+  }
+
   /**
    * An object that that is being iterated in a `for` loop, such as `for (..; .. sink.length; ...) ...`
    */
   private class LoopSink extends Sink {
     LoopSink() {
       exists(ArrayIterationLoop loop |
         this = loop.getLengthRead().getBase() and
-        // In the DoS we are looking for arrayRead will evaluate to undefined,
-        // this may cause an exception to be thrown, thus bailing out of the loop.
-        // A DoS cannot happen if such an exception is thrown.
-        not exists(DataFlow::PropRead arrayRead, Expr throws |
-          arrayRead.getPropertyNameExpr() = loop.getIndexVariable().getAnAccess() and
-          arrayRead.flowsToExpr(throws) and
-          isCrashingWithNullValues(throws)
-        ) and
+        not hasCrashingArrayAccess(loop.getIndexVariable()) and
         // The existence 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 |
@@ -187,21 +192,23 @@ module LoopBoundInjection {
         call = LodashUnderscore::member(name).getACall() and
         call.getArgument(0) = this and
         // Here it is just assumed that the array element is the first parameter in the callback function.
-        not exists(DataFlow::FunctionNode func, DataFlow::ParameterNode e |
+        not exists(DataFlow::FunctionNode func |
           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 |
-              e.flowsToExpr(throws) and
+              func.getParameter(0).flowsToExpr(throws) and
               isCrashingWithNullValues(throws)
             )
             or
             // Similar to the loop sink - the existence of an early-exit usually means that no DoS can happen.
             exists(ThrowStmt throw |
               throw.getTarget() = func.asExpr()
             )
+            or
+            // A crash happens looking up the index variable.
+            hasCrashingArrayAccess(func.getParameter(1).getParameter().getVariable())
           )
         )
       )