From 2dbd762bd964650efe4cd409646490e3882fc267 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esbena@github.com>
Date: Mon, 11 Jan 2021 14:13:41 +0100
Subject: [PATCH 1/9] JS: reintroduce reverted js/server-crash

This reverts commit 0a8d15ccc4acad0ca63af6d39801aa90c7c41588.
---
 .../ql/src/Security/CWE-730/ServerCrash.qhelp |  22 ++++
 .../ql/src/Security/CWE-730/ServerCrash.ql    | 100 ++++++++++++++++++
 .../Security/CWE-730/ServerCrash.expected     |   6 ++
 .../Security/CWE-730/ServerCrash.qlref        |   1 +
 .../Security/CWE-730/server-crash.js          |  73 +++++++++++++
 5 files changed, 202 insertions(+)
 create mode 100644 javascript/ql/src/Security/CWE-730/ServerCrash.qhelp
 create mode 100644 javascript/ql/src/Security/CWE-730/ServerCrash.ql
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.expected
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.qlref
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-730/server-crash.js

diff --git a/javascript/ql/src/Security/CWE-730/ServerCrash.qhelp b/javascript/ql/src/Security/CWE-730/ServerCrash.qhelp
new file mode 100644
index 00000000000..c3258c4e5f1
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-730/ServerCrash.qhelp
@@ -0,0 +1,22 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+	<overview>
+
+	</overview>
+
+	<recommendation>
+
+	</recommendation>
+
+	<example>
+
+	</example>
+
+	<references>
+
+	</references>
+
+</qhelp>
diff --git a/javascript/ql/src/Security/CWE-730/ServerCrash.ql b/javascript/ql/src/Security/CWE-730/ServerCrash.ql
new file mode 100644
index 00000000000..8db7574d6c9
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-730/ServerCrash.ql
@@ -0,0 +1,100 @@
+/**
+ * @name Server crash
+ * @description A server that can be forced to crash may be vulnerable to denial-of-service
+ *              attacks.
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id js/server-crash
+ * @tags security
+ *       external/cwe/cwe-730
+ */
+
+import javascript
+
+/**
+ * Gets a function that `caller` invokes.
+ */
+Function getACallee(Function caller) {
+  exists(DataFlow::InvokeNode invk |
+    invk.getEnclosingFunction() = caller and result = invk.getACallee()
+  )
+}
+
+/**
+ * Gets a function that `caller` invokes, excluding calls guarded in `try`-blocks.
+ */
+Function getAnUnguardedCallee(Function caller) {
+  exists(DataFlow::InvokeNode invk |
+    invk.getEnclosingFunction() = caller and
+    result = invk.getACallee() and
+    not exists(invk.asExpr().getEnclosingStmt().getEnclosingTryCatchStmt())
+  )
+}
+
+predicate isHeaderValue(HTTP::ExplicitHeaderDefinition def, DataFlow::Node node) {
+  def.definesExplicitly(_, node.asExpr())
+}
+
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "Configuration" }
+
+  override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node node) {
+    // using control characters in a header value will cause an exception
+    isHeaderValue(_, node)
+  }
+}
+
+predicate isLikelyToThrow(DataFlow::Node crash) {
+  exists(Configuration cfg, DataFlow::Node sink | cfg.hasFlow(_, sink) | isHeaderValue(crash, sink))
+}
+
+/**
+ * A call that looks like it is asynchronous.
+ */
+class AsyncCall extends DataFlow::CallNode {
+  DataFlow::FunctionNode callback;
+
+  AsyncCall() {
+    callback.flowsTo(getLastArgument()) and
+    callback.getParameter(0).getName() = ["e", "err", "error"] and
+    callback.getNumParameter() = 2 and
+    not exists(callback.getAReturn())
+  }
+
+  DataFlow::FunctionNode getCallback() { result = callback }
+}
+
+/**
+ * Gets a function that is invoked by `asyncCallback` without any try-block wrapping, `asyncCallback` is in turn is called indirectly by `routeHandler`.
+ *
+ * If the result throws an excection, the server of `routeHandler` will crash.
+ */
+Function getAPotentialServerCrasher(
+  HTTP::RouteHandler routeHandler, DataFlow::FunctionNode asyncCallback
+) {
+  exists(AsyncCall asyncCall |
+    // the route handler transitively calls an async function
+    asyncCall.getEnclosingFunction() =
+      getACallee*(routeHandler.(DataFlow::FunctionNode).getFunction()) and
+    asyncCallback = asyncCall.getCallback() and
+    // the async function transitively calls a function that may throw an exception out of the the async function
+    result = getAnUnguardedCallee*(asyncCallback.getFunction())
+  )
+}
+
+/**
+ * Gets an AST node that is likely to throw an uncaught exception in `fun`.
+ */
+ExprOrStmt getALikelyExceptionThrower(Function fun) {
+  result.getContainer() = fun and
+  not exists([result.(Expr).getEnclosingStmt(), result.(Stmt)].getEnclosingTryCatchStmt()) and
+  (isLikelyToThrow(result.(Expr).flow()) or result instanceof ThrowStmt)
+}
+
+from HTTP::RouteHandler routeHandler, DataFlow::FunctionNode asyncCallback, ExprOrStmt crasher
+where crasher = getALikelyExceptionThrower(getAPotentialServerCrasher(routeHandler, asyncCallback))
+select crasher, "When an exception is thrown here and later exits $@, the server of $@ will crash.",
+  asyncCallback, "this asynchronous callback", routeHandler, "this route handler"
diff --git a/javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.expected b/javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.expected
new file mode 100644
index 00000000000..b4e8de623a6
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.expected
@@ -0,0 +1,6 @@
+| server-crash.js:7:5:7:14 | throw err; | When an exception is thrown here and later exits $@, the server of $@ will crash. | server-crash.js:6:28:8:3 | (err, x ...  OK\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
+| server-crash.js:11:3:11:11 | throw 42; | When an exception is thrown here and later exits $@, the server of $@ will crash. | server-crash.js:50:28:52:3 | (err, x ... ();\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
+| server-crash.js:16:7:16:16 | throw err; | When an exception is thrown here and later exits $@, the server of $@ will crash. | server-crash.js:15:30:17:5 | (err, x ... K\\n    } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
+| server-crash.js:28:5:28:14 | throw err; | When an exception is thrown here and later exits $@, the server of $@ will crash. | server-crash.js:27:28:29:3 | (err, x ...  OK\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
+| server-crash.js:33:5:33:14 | throw err; | When an exception is thrown here and later exits $@, the server of $@ will crash. | server-crash.js:32:28:34:3 | (err, x ...  OK\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
+| server-crash.js:41:5:41:48 | res.set ... header) | When an exception is thrown here and later exits $@, the server of $@ will crash. | server-crash.js:40:28:42:3 | (err, x ...  OK\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
diff --git a/javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.qlref b/javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.qlref
new file mode 100644
index 00000000000..294d08cdcbb
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.qlref
@@ -0,0 +1 @@
+Security/CWE-730/ServerCrash.ql
diff --git a/javascript/ql/test/query-tests/Security/CWE-730/server-crash.js b/javascript/ql/test/query-tests/Security/CWE-730/server-crash.js
new file mode 100644
index 00000000000..96b5fc71187
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-730/server-crash.js
@@ -0,0 +1,73 @@
+const express = require("express");
+const app = express();
+const fs = require("fs");
+
+function indirection1() {
+  fs.readFile("/WHATEVER", (err, x) => {
+    throw err; // NOT OK
+  });
+}
+function indirection2() {
+  throw 42; // NOT OK
+}
+function indirection3() {
+  try {
+    fs.readFile("/WHATEVER", (err, x) => {
+      throw err; // NOT OK
+    });
+  } catch (e) {}
+}
+function indirection4() {
+  throw 42; // OK: guarded caller
+}
+function indirection5() {
+  indirection6();
+}
+function indirection6() {
+  fs.readFile("/WHATEVER", (err, x) => {
+    throw err; // NOT OK
+  });
+}
+app.get("/async-throw", (req, res) => {
+  fs.readFile("/WHATEVER", (err, x) => {
+    throw err; // NOT OK
+  });
+  fs.readFile("/WHATEVER", (err, x) => {
+    try {
+      throw err; // OK: guarded throw
+    } catch (e) {}
+  });
+  fs.readFile("/WHATEVER", (err, x) => {
+    res.setHeader("reflected", req.query.header); // NOT OK
+  });
+  fs.readFile("/WHATEVER", (err, x) => {
+    try {
+      res.setHeader("reflected", req.query.header); // OK: guarded call
+    } catch (e) {}
+  });
+
+  indirection1();
+  fs.readFile("/WHATEVER", (err, x) => {
+    indirection2();
+  });
+
+  indirection3();
+  try {
+    indirection4();
+  } catch (e) {}
+  indirection5();
+
+  fs.readFile("/WHATEVER", (err, x) => {
+    req.query.foo; // OK
+  });
+  fs.readFile("/WHATEVER", (err, x) => {
+    req.query.foo.toString(); // OK
+  });
+
+  fs.readFile("/WHATEVER", (err, x) => {
+    req.query.foo.bar; // NOT OK [INCONSISTENCY]: need to add property reads as sinks
+  });
+  fs.readFile("/WHATEVER", (err, x) => {
+    res.setHeader("reflected", unknown); // OK
+  });
+});

From d591c519a8062053bbe588e8133b9297aa6cf4da Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esbena@github.com>
Date: Tue, 12 Jan 2021 15:11:52 +0100
Subject: [PATCH 2/9] JS: reformulate js/server-crash as a path problem

---
 .../ql/src/Security/CWE-730/ServerCrash.ql    | 194 ++++++++++++------
 .../Security/CWE-730/ServerCrash.expected     |  54 ++++-
 .../Security/CWE-730/server-crash.js          |   2 +-
 3 files changed, 185 insertions(+), 65 deletions(-)

diff --git a/javascript/ql/src/Security/CWE-730/ServerCrash.ql b/javascript/ql/src/Security/CWE-730/ServerCrash.ql
index 8db7574d6c9..4df31174150 100644
--- a/javascript/ql/src/Security/CWE-730/ServerCrash.ql
+++ b/javascript/ql/src/Security/CWE-730/ServerCrash.ql
@@ -2,7 +2,7 @@
  * @name Server crash
  * @description A server that can be forced to crash may be vulnerable to denial-of-service
  *              attacks.
- * @kind problem
+ * @kind path-problem
  * @problem.severity error
  * @precision high
  * @id js/server-crash
@@ -13,46 +13,7 @@
 import javascript
 
 /**
- * Gets a function that `caller` invokes.
- */
-Function getACallee(Function caller) {
-  exists(DataFlow::InvokeNode invk |
-    invk.getEnclosingFunction() = caller and result = invk.getACallee()
-  )
-}
-
-/**
- * Gets a function that `caller` invokes, excluding calls guarded in `try`-blocks.
- */
-Function getAnUnguardedCallee(Function caller) {
-  exists(DataFlow::InvokeNode invk |
-    invk.getEnclosingFunction() = caller and
-    result = invk.getACallee() and
-    not exists(invk.asExpr().getEnclosingStmt().getEnclosingTryCatchStmt())
-  )
-}
-
-predicate isHeaderValue(HTTP::ExplicitHeaderDefinition def, DataFlow::Node node) {
-  def.definesExplicitly(_, node.asExpr())
-}
-
-class Configuration extends TaintTracking::Configuration {
-  Configuration() { this = "Configuration" }
-
-  override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
-
-  override predicate isSink(DataFlow::Node node) {
-    // using control characters in a header value will cause an exception
-    isHeaderValue(_, node)
-  }
-}
-
-predicate isLikelyToThrow(DataFlow::Node crash) {
-  exists(Configuration cfg, DataFlow::Node sink | cfg.hasFlow(_, sink) | isHeaderValue(crash, sink))
-}
-
-/**
- * A call that looks like it is asynchronous.
+ * A call that appears to be asynchronous (heuristic).
  */
 class AsyncCall extends DataFlow::CallNode {
   DataFlow::FunctionNode callback;
@@ -64,37 +25,154 @@ class AsyncCall extends DataFlow::CallNode {
     not exists(callback.getAReturn())
   }
 
+  /**
+   * Gets the callback that is invoked asynchronously.
+   */
   DataFlow::FunctionNode getCallback() { result = callback }
 }
 
+/**
+ * Gets a function that is invoked as a consequence of invoking a route handler `rh`.
+ */
+Function invokedByRouteHandler(HTTP::RouteHandler rh) {
+  rh = result.flow()
+  or
+  // follow the immediate call graph
+  exists(DataFlow::InvokeNode invk |
+    result = invk.getACallee() and
+    invk.getEnclosingFunction() = invokedByRouteHandler(rh)
+  )
+  // if new edges are added here, the `edges` predicate should be updated accordingly
+}
+
+/**
+ * A callback provided to an asynchronous call.
+ */
+class AsyncCallback extends DataFlow::FunctionNode {
+  AsyncCallback() { this = any(AsyncCall c).getCallback() }
+}
+
+/**
+ * Gets a function that is in a call stack that starts at an asynchronous `callback`, calls in the call stack occur outside of `try` blocks.
+ */
+Function inUnguardedAsyncCallStack(AsyncCallback callback) {
+  callback = result.flow()
+  or
+  exists(DataFlow::InvokeNode invk |
+    result = invk.getACallee() and
+    not exists(invk.asExpr().getEnclosingStmt().getEnclosingTryCatchStmt()) and
+    invk.getEnclosingFunction() = inUnguardedAsyncCallStack(callback)
+  )
+}
+
 /**
  * Gets a function that is invoked by `asyncCallback` without any try-block wrapping, `asyncCallback` is in turn is called indirectly by `routeHandler`.
  *
  * If the result throws an excection, the server of `routeHandler` will crash.
  */
 Function getAPotentialServerCrasher(
-  HTTP::RouteHandler routeHandler, DataFlow::FunctionNode asyncCallback
+  HTTP::RouteHandler routeHandler, AsyncCall asyncCall, AsyncCallback asyncCallback
 ) {
-  exists(AsyncCall asyncCall |
-    // the route handler transitively calls an async function
-    asyncCall.getEnclosingFunction() =
-      getACallee*(routeHandler.(DataFlow::FunctionNode).getFunction()) and
-    asyncCallback = asyncCall.getCallback() and
-    // the async function transitively calls a function that may throw an exception out of the the async function
-    result = getAnUnguardedCallee*(asyncCallback.getFunction())
+  // the route handler transitively calls an async function
+  asyncCall.getEnclosingFunction() = invokedByRouteHandler(routeHandler) and
+  asyncCallback = asyncCall.getCallback() and
+  // the async function transitively calls a function that may throw an exception out of the the async function
+  result = inUnguardedAsyncCallStack(asyncCallback)
+}
+
+/**
+ * Gets a node that is likely to throw an uncaught exception in `fun`.
+ */
+LikelyExceptionThrower getALikelyUncaughtExceptionThrower(Function fun) {
+  result.getContainer() = fun and
+  not exists([result.(Expr).getEnclosingStmt(), result.(Stmt)].getEnclosingTryCatchStmt())
+}
+
+/**
+ * Edges that builds an explanatory graph that follows the mental model of how the the exception flows.
+ *
+ * - step 1. exception is thrown
+ * - step 2. exception exits the enclosing function
+ * - step 3. exception follows the call graph backwards until an async callee is encountered
+ * - step 4. (at this point, the program crashes)
+ * - step 5. if the program had not crashed, the exception would conceptually follow the call graph backwards to a route handler
+ */
+query predicate edges(ASTNode pred, ASTNode succ) {
+  nodes(pred) and
+  nodes(succ) and
+  (
+    // the first step from the alert location to the enclosing function
+    pred = getALikelyUncaughtExceptionThrower(_) and
+    succ = pred.getContainer()
+    or
+    // ordinary flow graph
+    exists(DataFlow::InvokeNode invoke, Function f |
+      invoke.getACallee() = f and
+      succ = invoke.getAstNode() and
+      pred = f
+      or
+      invoke.getContainer() = f and
+      succ = f and
+      pred = invoke.getAstNode()
+    )
+    or
+    // the async step
+    exists(DataFlow::Node predNode, DataFlow::Node succNode |
+      exists(getAPotentialServerCrasher(_, predNode, succNode)) and
+      predNode.getAstNode() = succ and
+      succNode.getAstNode() = pred
+    )
   )
 }
 
 /**
- * Gets an AST node that is likely to throw an uncaught exception in `fun`.
+ * Nodes for building an explanatory graph that follows the mental model of how the the exception flows.
  */
-ExprOrStmt getALikelyExceptionThrower(Function fun) {
-  result.getContainer() = fun and
-  not exists([result.(Expr).getEnclosingStmt(), result.(Stmt)].getEnclosingTryCatchStmt()) and
-  (isLikelyToThrow(result.(Expr).flow()) or result instanceof ThrowStmt)
+query predicate nodes(ASTNode node) {
+  exists(HTTP::RouteHandler rh, Function fun |
+    main(rh, _, _) and
+    fun = invokedByRouteHandler(rh)
+  |
+    node = any(DataFlow::InvokeNode invk | invk.getACallee() = fun).getAstNode() or
+    node = fun
+  )
+  or
+  exists(AsyncCallback cb, Function fun |
+    main(_, cb, _) and
+    fun = inUnguardedAsyncCallStack(cb)
+  |
+    node = any(DataFlow::InvokeNode invk | invk.getACallee() = fun).getAstNode() or
+    node = fun
+  )
+  or
+  main(_, _, node)
 }
 
-from HTTP::RouteHandler routeHandler, DataFlow::FunctionNode asyncCallback, ExprOrStmt crasher
-where crasher = getALikelyExceptionThrower(getAPotentialServerCrasher(routeHandler, asyncCallback))
-select crasher, "When an exception is thrown here and later exits $@, the server of $@ will crash.",
-  asyncCallback, "this asynchronous callback", routeHandler, "this route handler"
+predicate main(HTTP::RouteHandler rh, AsyncCallback asyncCallback, ExprOrStmt crasher) {
+  crasher = getALikelyUncaughtExceptionThrower(getAPotentialServerCrasher(rh, _, asyncCallback))
+}
+
+/**
+ * A node that is likely to throw an exception.
+ *
+ * This is the primary extension point for this query.
+ */
+abstract class LikelyExceptionThrower extends ASTNode { }
+
+/**
+ * A `throw` statement.
+ */
+class TrivialThrowStatement extends LikelyExceptionThrower, ThrowStmt { }
+
+/**
+ * Empty class for avoiding emptiness checks from the compiler when there are no Expr-typed instances of the LikelyExceptionThrower type.
+ */
+class CompilerConfusingExceptionThrower extends LikelyExceptionThrower {
+  CompilerConfusingExceptionThrower() { none() }
+}
+
+from HTTP::RouteHandler rh, AsyncCallback asyncCallback, ExprOrStmt crasher
+where main(rh, asyncCallback, crasher)
+select crasher, crasher, rh.getAstNode(),
+  "When an exception is thrown here and later escapes at $@, the server of $@ will crash.",
+  asyncCallback, "this asynchronous callback", rh, "this route handler"
diff --git a/javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.expected b/javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.expected
index b4e8de623a6..acee72436fc 100644
--- a/javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.expected
@@ -1,6 +1,48 @@
-| server-crash.js:7:5:7:14 | throw err; | When an exception is thrown here and later exits $@, the server of $@ will crash. | server-crash.js:6:28:8:3 | (err, x ...  OK\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
-| server-crash.js:11:3:11:11 | throw 42; | When an exception is thrown here and later exits $@, the server of $@ will crash. | server-crash.js:50:28:52:3 | (err, x ... ();\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
-| server-crash.js:16:7:16:16 | throw err; | When an exception is thrown here and later exits $@, the server of $@ will crash. | server-crash.js:15:30:17:5 | (err, x ... K\\n    } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
-| server-crash.js:28:5:28:14 | throw err; | When an exception is thrown here and later exits $@, the server of $@ will crash. | server-crash.js:27:28:29:3 | (err, x ...  OK\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
-| server-crash.js:33:5:33:14 | throw err; | When an exception is thrown here and later exits $@, the server of $@ will crash. | server-crash.js:32:28:34:3 | (err, x ...  OK\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
-| server-crash.js:41:5:41:48 | res.set ... header) | When an exception is thrown here and later exits $@, the server of $@ will crash. | server-crash.js:40:28:42:3 | (err, x ...  OK\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
+edges
+| server-crash.js:5:1:9:1 | functio ...   });\\n} | server-crash.js:49:3:49:16 | indirection1() |
+| server-crash.js:7:5:7:14 | throw err; | server-crash.js:6:28:8:3 | (err, x ...  OK\\n  } |
+| server-crash.js:10:1:12:1 | functio ... OT OK\\n} | server-crash.js:51:5:51:18 | indirection2() |
+| server-crash.js:11:3:11:11 | throw 42; | server-crash.js:10:1:12:1 | functio ... OT OK\\n} |
+| server-crash.js:13:1:19:1 | functio ... e) {}\\n} | server-crash.js:54:3:54:16 | indirection3() |
+| server-crash.js:16:7:16:16 | throw err; | server-crash.js:15:30:17:5 | (err, x ... K\\n    } |
+| server-crash.js:20:1:22:1 | functio ... aller\\n} | server-crash.js:56:5:56:18 | indirection4() |
+| server-crash.js:23:1:25:1 | functio ... n6();\\n} | server-crash.js:58:3:58:16 | indirection5() |
+| server-crash.js:24:3:24:16 | indirection6() | server-crash.js:23:1:25:1 | functio ... n6();\\n} |
+| server-crash.js:26:1:30:1 | functio ...   });\\n} | server-crash.js:24:3:24:16 | indirection6() |
+| server-crash.js:28:5:28:14 | throw err; | server-crash.js:27:28:29:3 | (err, x ...  OK\\n  } |
+| server-crash.js:33:5:33:14 | throw err; | server-crash.js:32:28:34:3 | (err, x ...  OK\\n  } |
+| server-crash.js:49:3:49:16 | indirection1() | server-crash.js:31:25:73:1 | (req, r ...   });\\n} |
+| server-crash.js:51:5:51:18 | indirection2() | server-crash.js:50:28:52:3 | (err, x ... ();\\n  } |
+| server-crash.js:54:3:54:16 | indirection3() | server-crash.js:31:25:73:1 | (req, r ...   });\\n} |
+| server-crash.js:56:5:56:18 | indirection4() | server-crash.js:31:25:73:1 | (req, r ...   });\\n} |
+| server-crash.js:58:3:58:16 | indirection5() | server-crash.js:31:25:73:1 | (req, r ...   });\\n} |
+nodes
+| server-crash.js:5:1:9:1 | functio ...   });\\n} |
+| server-crash.js:6:28:8:3 | (err, x ...  OK\\n  } |
+| server-crash.js:7:5:7:14 | throw err; |
+| server-crash.js:10:1:12:1 | functio ... OT OK\\n} |
+| server-crash.js:11:3:11:11 | throw 42; |
+| server-crash.js:13:1:19:1 | functio ... e) {}\\n} |
+| server-crash.js:15:30:17:5 | (err, x ... K\\n    } |
+| server-crash.js:16:7:16:16 | throw err; |
+| server-crash.js:20:1:22:1 | functio ... aller\\n} |
+| server-crash.js:23:1:25:1 | functio ... n6();\\n} |
+| server-crash.js:24:3:24:16 | indirection6() |
+| server-crash.js:26:1:30:1 | functio ...   });\\n} |
+| server-crash.js:27:28:29:3 | (err, x ...  OK\\n  } |
+| server-crash.js:28:5:28:14 | throw err; |
+| server-crash.js:31:25:73:1 | (req, r ...   });\\n} |
+| server-crash.js:32:28:34:3 | (err, x ...  OK\\n  } |
+| server-crash.js:33:5:33:14 | throw err; |
+| server-crash.js:49:3:49:16 | indirection1() |
+| server-crash.js:50:28:52:3 | (err, x ... ();\\n  } |
+| server-crash.js:51:5:51:18 | indirection2() |
+| server-crash.js:54:3:54:16 | indirection3() |
+| server-crash.js:56:5:56:18 | indirection4() |
+| server-crash.js:58:3:58:16 | indirection5() |
+#select
+| server-crash.js:7:5:7:14 | throw err; | server-crash.js:7:5:7:14 | throw err; | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | When an exception is thrown here and later escapes at $@, the server of $@ will crash. | server-crash.js:6:28:8:3 | (err, x ...  OK\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
+| server-crash.js:11:3:11:11 | throw 42; | server-crash.js:11:3:11:11 | throw 42; | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | When an exception is thrown here and later escapes at $@, the server of $@ will crash. | server-crash.js:50:28:52:3 | (err, x ... ();\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
+| server-crash.js:16:7:16:16 | throw err; | server-crash.js:16:7:16:16 | throw err; | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | When an exception is thrown here and later escapes at $@, the server of $@ will crash. | server-crash.js:15:30:17:5 | (err, x ... K\\n    } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
+| server-crash.js:28:5:28:14 | throw err; | server-crash.js:28:5:28:14 | throw err; | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | When an exception is thrown here and later escapes at $@, the server of $@ will crash. | server-crash.js:27:28:29:3 | (err, x ...  OK\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
+| server-crash.js:33:5:33:14 | throw err; | server-crash.js:33:5:33:14 | throw err; | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | When an exception is thrown here and later escapes at $@, the server of $@ will crash. | server-crash.js:32:28:34:3 | (err, x ...  OK\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
diff --git a/javascript/ql/test/query-tests/Security/CWE-730/server-crash.js b/javascript/ql/test/query-tests/Security/CWE-730/server-crash.js
index 96b5fc71187..7b61262ee92 100644
--- a/javascript/ql/test/query-tests/Security/CWE-730/server-crash.js
+++ b/javascript/ql/test/query-tests/Security/CWE-730/server-crash.js
@@ -38,7 +38,7 @@ app.get("/async-throw", (req, res) => {
     } catch (e) {}
   });
   fs.readFile("/WHATEVER", (err, x) => {
-    res.setHeader("reflected", req.query.header); // NOT OK
+    res.setHeader("reflected", req.query.header); // NOT OK [INCONSISTENCY]
   });
   fs.readFile("/WHATEVER", (err, x) => {
     try {

From 12b985be8748f5cd646639eb82aea560424f0bd0 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esbena@github.com>
Date: Wed, 13 Jan 2021 14:49:29 +0100
Subject: [PATCH 3/9] Update javascript/ql/src/Security/CWE-730/ServerCrash.ql

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
---
 javascript/ql/src/Security/CWE-730/ServerCrash.ql | 1 +
 1 file changed, 1 insertion(+)

diff --git a/javascript/ql/src/Security/CWE-730/ServerCrash.ql b/javascript/ql/src/Security/CWE-730/ServerCrash.ql
index 4df31174150..68607db011f 100644
--- a/javascript/ql/src/Security/CWE-730/ServerCrash.ql
+++ b/javascript/ql/src/Security/CWE-730/ServerCrash.ql
@@ -40,6 +40,7 @@ Function invokedByRouteHandler(HTTP::RouteHandler rh) {
   // follow the immediate call graph
   exists(DataFlow::InvokeNode invk |
     result = invk.getACallee() and
+    // purposely not checking for `getEnclosingTryCatchStmt`. An async callback called from inside a try-catch can still crash the server.
     invk.getEnclosingFunction() = invokedByRouteHandler(rh)
   )
   // if new edges are added here, the `edges` predicate should be updated accordingly

From 1bc7d68a50f402641a6fb5ba555ad4c55d37b763 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esbena@github.com>
Date: Wed, 13 Jan 2021 14:49:42 +0100
Subject: [PATCH 4/9] Update
 javascript/ql/test/query-tests/Security/CWE-730/server-crash.js

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
---
 .../ql/test/query-tests/Security/CWE-730/server-crash.js | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/javascript/ql/test/query-tests/Security/CWE-730/server-crash.js b/javascript/ql/test/query-tests/Security/CWE-730/server-crash.js
index 7b61262ee92..b1e796cbd7a 100644
--- a/javascript/ql/test/query-tests/Security/CWE-730/server-crash.js
+++ b/javascript/ql/test/query-tests/Security/CWE-730/server-crash.js
@@ -70,4 +70,13 @@ app.get("/async-throw", (req, res) => {
   fs.readFile("/WHATEVER", (err, x) => {
     res.setHeader("reflected", unknown); // OK
   });
+
+  try {
+    indirection7();
+  } catch (e) {}
 });
+function indirection7() {
+  fs.readFile("/foo", (err, x) => {
+    throw err; // NOT OK
+  });
+}

From 3015dcd3109563a0ffb415f57ca7fbcc183d5a6e Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esbena@github.com>
Date: Wed, 13 Jan 2021 15:46:34 +0100
Subject: [PATCH 5/9] JS: reformulate js/server-crash. Support promises and
 shorter paths.

---
 .../ql/src/Security/CWE-730/ServerCrash.ql    | 278 +++++++++---------
 .../Security/CWE-730/ServerCrash.expected     | 107 ++++---
 .../Security/CWE-730/server-crash.js          | 108 ++++++-
 3 files changed, 302 insertions(+), 191 deletions(-)

diff --git a/javascript/ql/src/Security/CWE-730/ServerCrash.ql b/javascript/ql/src/Security/CWE-730/ServerCrash.ql
index 68607db011f..b81126b33cd 100644
--- a/javascript/ql/src/Security/CWE-730/ServerCrash.ql
+++ b/javascript/ql/src/Security/CWE-730/ServerCrash.ql
@@ -3,7 +3,7 @@
  * @description A server that can be forced to crash may be vulnerable to denial-of-service
  *              attacks.
  * @kind path-problem
- * @problem.severity error
+ * @problem.severity warning
  * @precision high
  * @id js/server-crash
  * @tags security
@@ -13,144 +13,114 @@
 import javascript
 
 /**
- * A call that appears to be asynchronous (heuristic).
+ * Gets a function that indirectly invokes an asynchronous callback through `async`, where the callback throws an uncaught exception at `thrower`.
  */
-class AsyncCall extends DataFlow::CallNode {
-  DataFlow::FunctionNode callback;
+Function invokesCallbackThatThrowsUncaughtException(
+  AsyncSentinelCall async, LikelyExceptionThrower thrower
+) {
+  async.getAsyncCallee() = throwsUncaughtExceptionInAsyncContext(thrower) and
+  result = async.getEnclosingFunction()
+  or
+  exists(DataFlow::InvokeNode invk, Function fun |
+    fun = invokesCallbackThatThrowsUncaughtException(async, thrower) and
+    // purposely not checking for `getEnclosingTryCatchStmt`. An async callback called from inside a try-catch can still crash the server.
+    result = invk.getEnclosingFunction()
+  |
+    invk.getACallee() = fun
+    or
+    // traverse a slightly extended call graph to get additional TPs
+    invk.(AsyncSentinelCall).getAsyncCallee() = fun
+  )
+}
 
-  AsyncCall() {
-    callback.flowsTo(getLastArgument()) and
-    callback.getParameter(0).getName() = ["e", "err", "error"] and
-    callback.getNumParameter() = 2 and
-    not exists(callback.getAReturn())
+/**
+ * Gets a callee of an invocation `invk` that is not guarded by a try statement.
+ */
+Function getUncaughtExceptionRethrowerCallee(DataFlow::InvokeNode invk) {
+  not exists(invk.asExpr().getEnclosingStmt().getEnclosingTryCatchStmt()) and
+  result = invk.getACallee()
+}
+
+/**
+ * Holds if `thrower` is not guarded by a try statement.
+ */
+predicate isUncaughtExceptionThrower(LikelyExceptionThrower thrower) {
+  not exists([thrower.(Expr).getEnclosingStmt(), thrower.(Stmt)].getEnclosingTryCatchStmt())
+}
+
+/**
+ * Gets a function that may throw an uncaught exception originating at `thrower`, which then may escape in an asynchronous calling context.
+ */
+Function throwsUncaughtExceptionInAsyncContext(LikelyExceptionThrower thrower) {
+  (
+    isUncaughtExceptionThrower(thrower) and
+    result = thrower.getContainer()
+    or
+    exists(DataFlow::InvokeNode invk |
+      getUncaughtExceptionRethrowerCallee(invk) = throwsUncaughtExceptionInAsyncContext(thrower) and
+      result = invk.getEnclosingFunction()
+    )
+  ) and
+  // Anti-case:
+  // An exception from an `async` function results in a rejected promise.
+  // Unhandled promises requires `node --unhandled-rejections=strict ...` to terminate the process
+  // without that flag, the DEP0018 deprecation warning is printed instead (node.js version 14 and below)
+  not result.isAsync() and
+  // pruning optimization since this predicate always is related to `invokesCallbackThatThrowsUncaughtException`
+  result = reachableFromAsyncCallback()
+}
+
+/**
+ * Holds if `result` is reachable from a callback that is invoked asynchronously.
+ */
+Function reachableFromAsyncCallback() {
+  result instanceof AsyncCallback
+  or
+  exists(DataFlow::InvokeNode invk |
+    invk.getEnclosingFunction() = reachableFromAsyncCallback() and
+    result = invk.getACallee()
+  )
+}
+
+/**
+ * The main predicate of this query: used for both result display and path computation.
+ */
+predicate main(
+  HTTP::RouteHandler rh, AsyncSentinelCall async, AsyncCallback cb, LikelyExceptionThrower thrower
+) {
+  async.getAsyncCallee() = cb and
+  rh.getAstNode() = invokesCallbackThatThrowsUncaughtException(async, thrower)
+}
+
+/**
+ * A call that may cause a function to be invoked in an asynchronous context outside of the visible source code.
+ */
+class AsyncSentinelCall extends DataFlow::CallNode {
+  Function asyncCallee;
+
+  AsyncSentinelCall() {
+    exists(DataFlow::FunctionNode node | node.getAstNode() = asyncCallee |
+      // manual models
+      exists(string memberName |
+        not "Sync" = memberName.suffix(memberName.length() - 4) and
+        this = NodeJSLib::FS::moduleMember(memberName).getACall() and
+        node = this.getCallback([1 .. 2])
+      )
+      // (add additional cases here to improve the query)
+    )
   }
 
   /**
-   * Gets the callback that is invoked asynchronously.
+   * Gets the callee that is invoked in an asynchronous context.
    */
-  DataFlow::FunctionNode getCallback() { result = callback }
+  Function getAsyncCallee() { result = asyncCallee }
 }
 
 /**
- * Gets a function that is invoked as a consequence of invoking a route handler `rh`.
+ * A callback provided to an asynchronous call (heuristic).
  */
-Function invokedByRouteHandler(HTTP::RouteHandler rh) {
-  rh = result.flow()
-  or
-  // follow the immediate call graph
-  exists(DataFlow::InvokeNode invk |
-    result = invk.getACallee() and
-    // purposely not checking for `getEnclosingTryCatchStmt`. An async callback called from inside a try-catch can still crash the server.
-    invk.getEnclosingFunction() = invokedByRouteHandler(rh)
-  )
-  // if new edges are added here, the `edges` predicate should be updated accordingly
-}
-
-/**
- * A callback provided to an asynchronous call.
- */
-class AsyncCallback extends DataFlow::FunctionNode {
-  AsyncCallback() { this = any(AsyncCall c).getCallback() }
-}
-
-/**
- * Gets a function that is in a call stack that starts at an asynchronous `callback`, calls in the call stack occur outside of `try` blocks.
- */
-Function inUnguardedAsyncCallStack(AsyncCallback callback) {
-  callback = result.flow()
-  or
-  exists(DataFlow::InvokeNode invk |
-    result = invk.getACallee() and
-    not exists(invk.asExpr().getEnclosingStmt().getEnclosingTryCatchStmt()) and
-    invk.getEnclosingFunction() = inUnguardedAsyncCallStack(callback)
-  )
-}
-
-/**
- * Gets a function that is invoked by `asyncCallback` without any try-block wrapping, `asyncCallback` is in turn is called indirectly by `routeHandler`.
- *
- * If the result throws an excection, the server of `routeHandler` will crash.
- */
-Function getAPotentialServerCrasher(
-  HTTP::RouteHandler routeHandler, AsyncCall asyncCall, AsyncCallback asyncCallback
-) {
-  // the route handler transitively calls an async function
-  asyncCall.getEnclosingFunction() = invokedByRouteHandler(routeHandler) and
-  asyncCallback = asyncCall.getCallback() and
-  // the async function transitively calls a function that may throw an exception out of the the async function
-  result = inUnguardedAsyncCallStack(asyncCallback)
-}
-
-/**
- * Gets a node that is likely to throw an uncaught exception in `fun`.
- */
-LikelyExceptionThrower getALikelyUncaughtExceptionThrower(Function fun) {
-  result.getContainer() = fun and
-  not exists([result.(Expr).getEnclosingStmt(), result.(Stmt)].getEnclosingTryCatchStmt())
-}
-
-/**
- * Edges that builds an explanatory graph that follows the mental model of how the the exception flows.
- *
- * - step 1. exception is thrown
- * - step 2. exception exits the enclosing function
- * - step 3. exception follows the call graph backwards until an async callee is encountered
- * - step 4. (at this point, the program crashes)
- * - step 5. if the program had not crashed, the exception would conceptually follow the call graph backwards to a route handler
- */
-query predicate edges(ASTNode pred, ASTNode succ) {
-  nodes(pred) and
-  nodes(succ) and
-  (
-    // the first step from the alert location to the enclosing function
-    pred = getALikelyUncaughtExceptionThrower(_) and
-    succ = pred.getContainer()
-    or
-    // ordinary flow graph
-    exists(DataFlow::InvokeNode invoke, Function f |
-      invoke.getACallee() = f and
-      succ = invoke.getAstNode() and
-      pred = f
-      or
-      invoke.getContainer() = f and
-      succ = f and
-      pred = invoke.getAstNode()
-    )
-    or
-    // the async step
-    exists(DataFlow::Node predNode, DataFlow::Node succNode |
-      exists(getAPotentialServerCrasher(_, predNode, succNode)) and
-      predNode.getAstNode() = succ and
-      succNode.getAstNode() = pred
-    )
-  )
-}
-
-/**
- * Nodes for building an explanatory graph that follows the mental model of how the the exception flows.
- */
-query predicate nodes(ASTNode node) {
-  exists(HTTP::RouteHandler rh, Function fun |
-    main(rh, _, _) and
-    fun = invokedByRouteHandler(rh)
-  |
-    node = any(DataFlow::InvokeNode invk | invk.getACallee() = fun).getAstNode() or
-    node = fun
-  )
-  or
-  exists(AsyncCallback cb, Function fun |
-    main(_, cb, _) and
-    fun = inUnguardedAsyncCallStack(cb)
-  |
-    node = any(DataFlow::InvokeNode invk | invk.getACallee() = fun).getAstNode() or
-    node = fun
-  )
-  or
-  main(_, _, node)
-}
-
-predicate main(HTTP::RouteHandler rh, AsyncCallback asyncCallback, ExprOrStmt crasher) {
-  crasher = getALikelyUncaughtExceptionThrower(getAPotentialServerCrasher(rh, _, asyncCallback))
+class AsyncCallback extends Function {
+  AsyncCallback() { any(AsyncSentinelCall c).getAsyncCallee() = this }
 }
 
 /**
@@ -172,8 +142,48 @@ class CompilerConfusingExceptionThrower extends LikelyExceptionThrower {
   CompilerConfusingExceptionThrower() { none() }
 }
 
-from HTTP::RouteHandler rh, AsyncCallback asyncCallback, ExprOrStmt crasher
-where main(rh, asyncCallback, crasher)
-select crasher, crasher, rh.getAstNode(),
-  "When an exception is thrown here and later escapes at $@, the server of $@ will crash.",
-  asyncCallback, "this asynchronous callback", rh, "this route handler"
+/**
+ * Edges that builds an explanatory graph that follows the mental model of how the the exception flows.
+ *
+ * - step 1. exception is thrown
+ * - step 2. exception escapes the enclosing function
+ * - step 3. exception follows the call graph backwards until an async callee is encountered
+ * - step 4. (at this point, the program crashes)
+ */
+query predicate edges(ASTNode pred, ASTNode succ) {
+  exists(LikelyExceptionThrower thrower | main(_, _, _, thrower) |
+    pred = thrower and
+    succ = thrower.getContainer()
+    or
+    exists(DataFlow::InvokeNode invk, Function fun |
+      fun = throwsUncaughtExceptionInAsyncContext(thrower)
+    |
+      succ = invk.getAstNode() and
+      pred = invk.getACallee() and
+      pred = fun
+      or
+      succ = fun and
+      succ = invk.getContainer() and
+      pred = invk.getAstNode()
+    )
+  )
+}
+
+/**
+ * A node in the `edge/2` relation above.
+ */
+query predicate nodes(ASTNode node) {
+  edges(node, _) or
+  edges(_, node)
+}
+
+from
+  HTTP::RouteHandler rh, AsyncSentinelCall async, DataFlow::Node callbackArg, AsyncCallback cb,
+  ExprOrStmt crasher
+where
+  main(rh, async, cb, crasher) and
+  callbackArg.getALocalSource().getAstNode() = cb and
+  async.getAnArgument() = callbackArg
+select crasher, crasher, cb,
+  "The server of $@ will terminate when an uncaught exception from here escapes this $@", rh,
+  "this route handler", callbackArg, "asynchronous callback"
diff --git a/javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.expected b/javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.expected
index acee72436fc..ef20a62aa9d 100644
--- a/javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.expected
@@ -1,48 +1,65 @@
 edges
-| server-crash.js:5:1:9:1 | functio ...   });\\n} | server-crash.js:49:3:49:16 | indirection1() |
-| server-crash.js:7:5:7:14 | throw err; | server-crash.js:6:28:8:3 | (err, x ...  OK\\n  } |
-| server-crash.js:10:1:12:1 | functio ... OT OK\\n} | server-crash.js:51:5:51:18 | indirection2() |
-| server-crash.js:11:3:11:11 | throw 42; | server-crash.js:10:1:12:1 | functio ... OT OK\\n} |
-| server-crash.js:13:1:19:1 | functio ... e) {}\\n} | server-crash.js:54:3:54:16 | indirection3() |
-| server-crash.js:16:7:16:16 | throw err; | server-crash.js:15:30:17:5 | (err, x ... K\\n    } |
-| server-crash.js:20:1:22:1 | functio ... aller\\n} | server-crash.js:56:5:56:18 | indirection4() |
-| server-crash.js:23:1:25:1 | functio ... n6();\\n} | server-crash.js:58:3:58:16 | indirection5() |
-| server-crash.js:24:3:24:16 | indirection6() | server-crash.js:23:1:25:1 | functio ... n6();\\n} |
-| server-crash.js:26:1:30:1 | functio ...   });\\n} | server-crash.js:24:3:24:16 | indirection6() |
-| server-crash.js:28:5:28:14 | throw err; | server-crash.js:27:28:29:3 | (err, x ...  OK\\n  } |
-| server-crash.js:33:5:33:14 | throw err; | server-crash.js:32:28:34:3 | (err, x ...  OK\\n  } |
-| server-crash.js:49:3:49:16 | indirection1() | server-crash.js:31:25:73:1 | (req, r ...   });\\n} |
-| server-crash.js:51:5:51:18 | indirection2() | server-crash.js:50:28:52:3 | (err, x ... ();\\n  } |
-| server-crash.js:54:3:54:16 | indirection3() | server-crash.js:31:25:73:1 | (req, r ...   });\\n} |
-| server-crash.js:56:5:56:18 | indirection4() | server-crash.js:31:25:73:1 | (req, r ...   });\\n} |
-| server-crash.js:58:3:58:16 | indirection5() | server-crash.js:31:25:73:1 | (req, r ...   });\\n} |
+| server-crash.js:15:5:15:14 | throw err; | server-crash.js:14:23:16:3 | (err, x ...  OK\\n  } |
+| server-crash.js:18:1:20:1 | functio ... OT OK\\n} | server-crash.js:59:5:59:18 | indirection2() |
+| server-crash.js:19:3:19:11 | throw 42; | server-crash.js:18:1:20:1 | functio ... OT OK\\n} |
+| server-crash.js:24:7:24:16 | throw err; | server-crash.js:23:25:25:5 | (err, x ... K\\n    } |
+| server-crash.js:36:5:36:14 | throw err; | server-crash.js:35:23:37:3 | (err, x ...  OK\\n  } |
+| server-crash.js:41:5:41:14 | throw err; | server-crash.js:40:23:42:3 | (err, x ...  OK\\n  } |
+| server-crash.js:59:5:59:18 | indirection2() | server-crash.js:58:23:60:3 | (err, x ... ();\\n  } |
+| server-crash.js:88:5:88:14 | throw err; | server-crash.js:87:23:89:3 | (err, x ...  OK\\n  } |
+| server-crash.js:94:5:94:14 | throw "e"; | server-crash.js:93:22:95:3 | () => { ...  OK\\n  } |
+| server-crash.js:102:7:102:16 | throw "e"; | server-crash.js:101:24:103:5 | () => { ... K\\n    } |
+| server-crash.js:109:9:109:18 | throw "e"; | server-crash.js:108:26:110:7 | () => { ...       } |
+| server-crash.js:117:9:117:18 | throw "e"; | server-crash.js:116:26:118:7 | () => { ...       } |
+| server-crash.js:131:7:131:16 | throw err; | server-crash.js:130:25:132:5 | (err, x ... K\\n    } |
+| server-crash.js:152:3:154:3 | functio ...  OK\\n  } | server-crash.js:157:5:157:16 | throwError() |
+| server-crash.js:152:3:154:3 | functio ...  OK\\n  } | server-crash.js:160:5:160:16 | throwError() |
+| server-crash.js:152:3:154:3 | functio ...  OK\\n  } | server-crash.js:164:3:164:14 | throwError() |
+| server-crash.js:153:5:153:22 | throw new Error(); | server-crash.js:152:3:154:3 | functio ...  OK\\n  } |
+| server-crash.js:153:11:153:21 | new Error() | server-crash.js:152:3:154:3 | functio ...  OK\\n  } |
+| server-crash.js:157:5:157:16 | throwError() | server-crash.js:156:3:158:3 | functio ... ath\\n  } |
 nodes
-| server-crash.js:5:1:9:1 | functio ...   });\\n} |
-| server-crash.js:6:28:8:3 | (err, x ...  OK\\n  } |
-| server-crash.js:7:5:7:14 | throw err; |
-| server-crash.js:10:1:12:1 | functio ... OT OK\\n} |
-| server-crash.js:11:3:11:11 | throw 42; |
-| server-crash.js:13:1:19:1 | functio ... e) {}\\n} |
-| server-crash.js:15:30:17:5 | (err, x ... K\\n    } |
-| server-crash.js:16:7:16:16 | throw err; |
-| server-crash.js:20:1:22:1 | functio ... aller\\n} |
-| server-crash.js:23:1:25:1 | functio ... n6();\\n} |
-| server-crash.js:24:3:24:16 | indirection6() |
-| server-crash.js:26:1:30:1 | functio ...   });\\n} |
-| server-crash.js:27:28:29:3 | (err, x ...  OK\\n  } |
-| server-crash.js:28:5:28:14 | throw err; |
-| server-crash.js:31:25:73:1 | (req, r ...   });\\n} |
-| server-crash.js:32:28:34:3 | (err, x ...  OK\\n  } |
-| server-crash.js:33:5:33:14 | throw err; |
-| server-crash.js:49:3:49:16 | indirection1() |
-| server-crash.js:50:28:52:3 | (err, x ... ();\\n  } |
-| server-crash.js:51:5:51:18 | indirection2() |
-| server-crash.js:54:3:54:16 | indirection3() |
-| server-crash.js:56:5:56:18 | indirection4() |
-| server-crash.js:58:3:58:16 | indirection5() |
+| server-crash.js:14:23:16:3 | (err, x ...  OK\\n  } |
+| server-crash.js:15:5:15:14 | throw err; |
+| server-crash.js:18:1:20:1 | functio ... OT OK\\n} |
+| server-crash.js:19:3:19:11 | throw 42; |
+| server-crash.js:23:25:25:5 | (err, x ... K\\n    } |
+| server-crash.js:24:7:24:16 | throw err; |
+| server-crash.js:35:23:37:3 | (err, x ...  OK\\n  } |
+| server-crash.js:36:5:36:14 | throw err; |
+| server-crash.js:40:23:42:3 | (err, x ...  OK\\n  } |
+| server-crash.js:41:5:41:14 | throw err; |
+| server-crash.js:58:23:60:3 | (err, x ... ();\\n  } |
+| server-crash.js:59:5:59:18 | indirection2() |
+| server-crash.js:87:23:89:3 | (err, x ...  OK\\n  } |
+| server-crash.js:88:5:88:14 | throw err; |
+| server-crash.js:93:22:95:3 | () => { ...  OK\\n  } |
+| server-crash.js:94:5:94:14 | throw "e"; |
+| server-crash.js:101:24:103:5 | () => { ... K\\n    } |
+| server-crash.js:102:7:102:16 | throw "e"; |
+| server-crash.js:108:26:110:7 | () => { ...       } |
+| server-crash.js:109:9:109:18 | throw "e"; |
+| server-crash.js:116:26:118:7 | () => { ...       } |
+| server-crash.js:117:9:117:18 | throw "e"; |
+| server-crash.js:130:25:132:5 | (err, x ... K\\n    } |
+| server-crash.js:131:7:131:16 | throw err; |
+| server-crash.js:152:3:154:3 | functio ...  OK\\n  } |
+| server-crash.js:153:5:153:22 | throw new Error(); |
+| server-crash.js:153:11:153:21 | new Error() |
+| server-crash.js:156:3:158:3 | functio ... ath\\n  } |
+| server-crash.js:157:5:157:16 | throwError() |
+| server-crash.js:160:5:160:16 | throwError() |
+| server-crash.js:164:3:164:14 | throwError() |
 #select
-| server-crash.js:7:5:7:14 | throw err; | server-crash.js:7:5:7:14 | throw err; | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | When an exception is thrown here and later escapes at $@, the server of $@ will crash. | server-crash.js:6:28:8:3 | (err, x ...  OK\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
-| server-crash.js:11:3:11:11 | throw 42; | server-crash.js:11:3:11:11 | throw 42; | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | When an exception is thrown here and later escapes at $@, the server of $@ will crash. | server-crash.js:50:28:52:3 | (err, x ... ();\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
-| server-crash.js:16:7:16:16 | throw err; | server-crash.js:16:7:16:16 | throw err; | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | When an exception is thrown here and later escapes at $@, the server of $@ will crash. | server-crash.js:15:30:17:5 | (err, x ... K\\n    } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
-| server-crash.js:28:5:28:14 | throw err; | server-crash.js:28:5:28:14 | throw err; | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | When an exception is thrown here and later escapes at $@, the server of $@ will crash. | server-crash.js:27:28:29:3 | (err, x ...  OK\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
-| server-crash.js:33:5:33:14 | throw err; | server-crash.js:33:5:33:14 | throw err; | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | When an exception is thrown here and later escapes at $@, the server of $@ will crash. | server-crash.js:32:28:34:3 | (err, x ...  OK\\n  } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ...   });\\n} | this route handler |
+| server-crash.js:15:5:15:14 | throw err; | server-crash.js:15:5:15:14 | throw err; | server-crash.js:14:23:16:3 | (err, x ...  OK\\n  } | The server of $@ will terminate when an uncaught exception from here escapes this $@ | server-crash.js:39:25:85:1 | (req, r ... e) {}\\n} | this route handler | server-crash.js:14:23:16:3 | (err, x ...  OK\\n  } | asynchronous callback |
+| server-crash.js:19:3:19:11 | throw 42; | server-crash.js:19:3:19:11 | throw 42; | server-crash.js:58:23:60:3 | (err, x ... ();\\n  } | The server of $@ will terminate when an uncaught exception from here escapes this $@ | server-crash.js:39:25:85:1 | (req, r ... e) {}\\n} | this route handler | server-crash.js:58:23:60:3 | (err, x ... ();\\n  } | asynchronous callback |
+| server-crash.js:24:7:24:16 | throw err; | server-crash.js:24:7:24:16 | throw err; | server-crash.js:23:25:25:5 | (err, x ... K\\n    } | The server of $@ will terminate when an uncaught exception from here escapes this $@ | server-crash.js:39:25:85:1 | (req, r ... e) {}\\n} | this route handler | server-crash.js:23:25:25:5 | (err, x ... K\\n    } | asynchronous callback |
+| server-crash.js:36:5:36:14 | throw err; | server-crash.js:36:5:36:14 | throw err; | server-crash.js:35:23:37:3 | (err, x ...  OK\\n  } | The server of $@ will terminate when an uncaught exception from here escapes this $@ | server-crash.js:39:25:85:1 | (req, r ... e) {}\\n} | this route handler | server-crash.js:35:23:37:3 | (err, x ...  OK\\n  } | asynchronous callback |
+| server-crash.js:41:5:41:14 | throw err; | server-crash.js:41:5:41:14 | throw err; | server-crash.js:40:23:42:3 | (err, x ...  OK\\n  } | The server of $@ will terminate when an uncaught exception from here escapes this $@ | server-crash.js:39:25:85:1 | (req, r ... e) {}\\n} | this route handler | server-crash.js:40:23:42:3 | (err, x ...  OK\\n  } | asynchronous callback |
+| server-crash.js:88:5:88:14 | throw err; | server-crash.js:88:5:88:14 | throw err; | server-crash.js:87:23:89:3 | (err, x ...  OK\\n  } | The server of $@ will terminate when an uncaught exception from here escapes this $@ | server-crash.js:39:25:85:1 | (req, r ... e) {}\\n} | this route handler | server-crash.js:87:23:89:3 | (err, x ...  OK\\n  } | asynchronous callback |
+| server-crash.js:94:5:94:14 | throw "e"; | server-crash.js:94:5:94:14 | throw "e"; | server-crash.js:93:22:95:3 | () => { ...  OK\\n  } | The server of $@ will terminate when an uncaught exception from here escapes this $@ | server-crash.js:92:31:120:1 | (req, r ...   });\\n} | this route handler | server-crash.js:93:22:95:3 | () => { ...  OK\\n  } | asynchronous callback |
+| server-crash.js:102:7:102:16 | throw "e"; | server-crash.js:102:7:102:16 | throw "e"; | server-crash.js:101:24:103:5 | () => { ... K\\n    } | The server of $@ will terminate when an uncaught exception from here escapes this $@ | server-crash.js:92:31:120:1 | (req, r ...   });\\n} | this route handler | server-crash.js:101:24:103:5 | () => { ... K\\n    } | asynchronous callback |
+| server-crash.js:109:9:109:18 | throw "e"; | server-crash.js:109:9:109:18 | throw "e"; | server-crash.js:108:26:110:7 | () => { ...       } | The server of $@ will terminate when an uncaught exception from here escapes this $@ | server-crash.js:92:31:120:1 | (req, r ...   });\\n} | this route handler | server-crash.js:108:26:110:7 | () => { ...       } | asynchronous callback |
+| server-crash.js:117:9:117:18 | throw "e"; | server-crash.js:117:9:117:18 | throw "e"; | server-crash.js:116:26:118:7 | () => { ...       } | The server of $@ will terminate when an uncaught exception from here escapes this $@ | server-crash.js:92:31:120:1 | (req, r ...   });\\n} | this route handler | server-crash.js:116:26:118:7 | () => { ...       } | asynchronous callback |
+| server-crash.js:131:7:131:16 | throw err; | server-crash.js:131:7:131:16 | throw err; | server-crash.js:130:25:132:5 | (err, x ... K\\n    } | The server of $@ will terminate when an uncaught exception from here escapes this $@ | server-crash.js:128:32:135:1 | async ( ... un();\\n} | this route handler | server-crash.js:130:25:132:5 | (err, x ... K\\n    } | asynchronous callback |
+| server-crash.js:153:5:153:22 | throw new Error(); | server-crash.js:153:5:153:22 | throw new Error(); | server-crash.js:156:3:158:3 | functio ... ath\\n  } | The server of $@ will terminate when an uncaught exception from here escapes this $@ | server-crash.js:151:40:166:1 | (req, r ... nc();\\n} | this route handler | server-crash.js:161:16:161:17 | cb | asynchronous callback |
diff --git a/javascript/ql/test/query-tests/Security/CWE-730/server-crash.js b/javascript/ql/test/query-tests/Security/CWE-730/server-crash.js
index b1e796cbd7a..cb7bc4d9b8e 100644
--- a/javascript/ql/test/query-tests/Security/CWE-730/server-crash.js
+++ b/javascript/ql/test/query-tests/Security/CWE-730/server-crash.js
@@ -1,9 +1,17 @@
 const express = require("express");
 const app = express();
 const fs = require("fs");
+const EventEmitter = require("events");
+const http = require("http");
+
+const port = 12000;
+let server = app.listen(port, () =>
+  // for manual testing of the fickle node.js runtime
+  console.log(`Example app listening on port ${port}!`)
+);
 
 function indirection1() {
-  fs.readFile("/WHATEVER", (err, x) => {
+  fs.readFile("/foo", (err, x) => {
     throw err; // NOT OK
   });
 }
@@ -12,7 +20,7 @@ function indirection2() {
 }
 function indirection3() {
   try {
-    fs.readFile("/WHATEVER", (err, x) => {
+    fs.readFile("/foo", (err, x) => {
       throw err; // NOT OK
     });
   } catch (e) {}
@@ -24,30 +32,30 @@ function indirection5() {
   indirection6();
 }
 function indirection6() {
-  fs.readFile("/WHATEVER", (err, x) => {
+  fs.readFile("/foo", (err, x) => {
     throw err; // NOT OK
   });
 }
 app.get("/async-throw", (req, res) => {
-  fs.readFile("/WHATEVER", (err, x) => {
+  fs.readFile("/foo", (err, x) => {
     throw err; // NOT OK
   });
-  fs.readFile("/WHATEVER", (err, x) => {
+  fs.readFile("/foo", (err, x) => {
     try {
       throw err; // OK: guarded throw
     } catch (e) {}
   });
-  fs.readFile("/WHATEVER", (err, x) => {
+  fs.readFile("/foo", (err, x) => {
     res.setHeader("reflected", req.query.header); // NOT OK [INCONSISTENCY]
   });
-  fs.readFile("/WHATEVER", (err, x) => {
+  fs.readFile("/foo", (err, x) => {
     try {
       res.setHeader("reflected", req.query.header); // OK: guarded call
     } catch (e) {}
   });
 
   indirection1();
-  fs.readFile("/WHATEVER", (err, x) => {
+  fs.readFile("/foo", (err, x) => {
     indirection2();
   });
 
@@ -57,17 +65,17 @@ app.get("/async-throw", (req, res) => {
   } catch (e) {}
   indirection5();
 
-  fs.readFile("/WHATEVER", (err, x) => {
+  fs.readFile("/foo", (err, x) => {
     req.query.foo; // OK
   });
-  fs.readFile("/WHATEVER", (err, x) => {
+  fs.readFile("/foo", (err, x) => {
     req.query.foo.toString(); // OK
   });
 
-  fs.readFile("/WHATEVER", (err, x) => {
+  fs.readFile("/foo", (err, x) => {
     req.query.foo.bar; // NOT OK [INCONSISTENCY]: need to add property reads as sinks
   });
-  fs.readFile("/WHATEVER", (err, x) => {
+  fs.readFile("/foo", (err, x) => {
     res.setHeader("reflected", unknown); // OK
   });
 
@@ -80,3 +88,79 @@ function indirection7() {
     throw err; // NOT OK
   });
 }
+
+app.get("/async-throw-again", (req, res) => {
+  fs.readFile("foo", () => {
+    throw "e"; // NOT OK
+  });
+  fs.readFileSync("foo", () => {
+    throw "e"; // OK (does not take callbacks at all)
+  });
+  // can nest async calls (and only warns about the inner one)
+  fs.readFile("foo", () => {
+    fs.readFile("bar", () => {
+      throw "e"; // NOT OK
+    });
+  });
+  fs.readFile("foo", () => {
+    // can not catch async exceptions
+    try {
+      fs.readFile("bar", () => {
+        throw "e"; // NOT OK
+      });
+    } catch (e) {}
+  });
+  // can mix sync/async calls
+  fs.readFile("foo", () => {
+    (() =>
+      fs.readFile("bar", () => {
+        throw "e"; // NOT OK
+      }))();
+  });
+});
+
+app.get("/throw-in-promise-1", async (req, res) => {
+  async function fun() {
+    throw new Error(); // OK, requires `node --unhandled-rejections=strict ...` to terminate the process
+  }
+  await fun();
+});
+app.get("/throw-in-promise-2", async (req, res) => {
+  async function fun() {
+    fs.readFile("/foo", (err, x) => {
+      throw err; // NOT OK
+    });
+  }
+  await fun();
+});
+app.get("/throw-in-promise-3", async (req, res) => {
+  fs.readFile("/foo", async (err, x) => {
+    throw err; // OK, requires `node --unhandled-rejections=strict ...` to terminate the process
+  });
+});
+
+app.get("/throw-in-event-emitter", async (req, res) => {
+  class MyEmitter extends EventEmitter {}
+  const myEmitter = new MyEmitter();
+  myEmitter.on("event", () => {
+    throw new Error(); // OK, requires `node --unhandled-rejections=strict ...` to terminate the process
+  });
+  myEmitter.emit("event");
+});
+
+app.get("/throw-with-ambiguous-paths", (req, res) => {
+  function throwError() {
+    throw new Error(); // NOT OK
+  }
+
+  function cb() {
+    throwError(); // on path
+  }
+  function withAsync() {
+    throwError(); // not on path
+    fs.stat(X, cb);
+  }
+
+  throwError(); // not on path
+  withAsync();
+});

From 9e3cc3b1b2a3d1721acdde76cd7f916b01862112 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esbena@github.com>
Date: Tue, 19 Jan 2021 09:08:59 +0100
Subject: [PATCH 6/9] JS: add qhelp and changenotes for js/server-crash

---
 .../change-notes/2021-01-18-server-crash.md   |  2 +
 .../ql/src/Security/CWE-730/ServerCrash.qhelp | 84 ++++++++++++++++---
 .../CWE-730/examples/server-crash.BAD.js      | 21 +++++
 .../CWE-730/examples/server-crash.GOOD-A.js   | 14 ++++
 .../CWE-730/examples/server-crash.GOOD-B.js   | 13 +++
 5 files changed, 124 insertions(+), 10 deletions(-)
 create mode 100644 javascript/change-notes/2021-01-18-server-crash.md
 create mode 100644 javascript/ql/src/Security/CWE-730/examples/server-crash.BAD.js
 create mode 100644 javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-A.js
 create mode 100644 javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-B.js

diff --git a/javascript/change-notes/2021-01-18-server-crash.md b/javascript/change-notes/2021-01-18-server-crash.md
new file mode 100644
index 00000000000..bf5383c3bd8
--- /dev/null
+++ b/javascript/change-notes/2021-01-18-server-crash.md
@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The `js/server-crash` query has been added. It highlights servers may be terminated by a malicious user.
diff --git a/javascript/ql/src/Security/CWE-730/ServerCrash.qhelp b/javascript/ql/src/Security/CWE-730/ServerCrash.qhelp
index c3258c4e5f1..e46616ade47 100644
--- a/javascript/ql/src/Security/CWE-730/ServerCrash.qhelp
+++ b/javascript/ql/src/Security/CWE-730/ServerCrash.qhelp
@@ -1,22 +1,86 @@
 <!DOCTYPE qhelp PUBLIC
-  "-//Semmle//qhelp//EN"
-  "qhelp.dtd">
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
 <qhelp>
 
-	<overview>
+<overview>
 
-	</overview>
+	<p>
 
-	<recommendation>
+		Servers handle requests from clients until terminated
+		deliberately by a server administrator. A client request that results
+		in an uncaught server-side exception causes the current server
+		response generation to fail, and should not have an effect on
+		subsequent client requests.
 
-	</recommendation>
+	</p>
 
-	<example>
+	<p>
 
-	</example>
+		Under some circumstances, uncaught exceptions can however
+		cause the entire server to terminate abruptly. Such a behavior is
+		highly undesirable, especially if it gives malicious users the ability
+		to turn off the server at will, which is an efficient
+		denial-of-service attack.
 
-	<references>
+	</p>
 
-	</references>
+</overview>
+
+<recommendation>
+
+	<p>
+
+		Ensure that the processing of client requests can not cause
+		uncaught exceptions to terminate the entire server abruptly.
+
+	</p>
+
+</recommendation>
+
+<example>
+
+	<p>
+
+		The following server implementation checks if a client-provided
+		file path is valid and throws an exception if the check fails. It can
+		be seen that the exception is uncaught, and it is therefore reasonable to
+		expect the server to respond with an error response to client requests
+		that cause the check to fail.
+
+		But since the exception is uncaught in the context of an
+		asynchronous callback invocation (<code>fs.access(...)</code>), the
+		entire server will terminate instead.
+
+	</p>
+
+	<sample src="examples/server-crash.BAD.js"/>
+
+	<p>
+		To remedy this, the server can catch the exception explicitly with
+		a <code>try/catch</code> block, and generate an appropriate error
+		response instead:
+
+	</p>
+
+	<sample src="examples/server-crash.GOOD-A.js"/>
+
+	<p>
+
+		An alternative is to use an <code>async</code> and
+		<code>await</code> for the asynchronous behavior, since the server
+		will then print warning messages about uncaught exceptions instead of
+		terminating, unless the server was started with the commandline option
+		<code>--unhandled-rejections=strict</code>:
+
+	</p>
+
+	<sample src="examples/server-crash.GOOD-B.js"/>
+
+</example>
+
+<references>
+
+</references>
 
 </qhelp>
diff --git a/javascript/ql/src/Security/CWE-730/examples/server-crash.BAD.js b/javascript/ql/src/Security/CWE-730/examples/server-crash.BAD.js
new file mode 100644
index 00000000000..1b7001666ff
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-730/examples/server-crash.BAD.js
@@ -0,0 +1,21 @@
+const express = require("express"),
+      fs = require("fs");
+
+function save(rootDir, path, content){
+	if (!isValidPath(rootDir, req.query.filePath)) {
+		throw new Error(`Invalid filePath: ${req.query.filePath}`); // BAD crashes the server
+	}
+	// write content to disk
+}
+express().post("/save", (req, res) => {
+	fs.access(rootDir, (err) => {
+		if (err) {
+			console.error(`Server setup is corrupted, ${rootDir} does not exist!`);
+			res.status(500);
+			res.end();
+		}
+		save(rootDir, req.query.path, req.body);
+		res.status(200);
+		res.end();
+	});
+});
diff --git a/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-A.js b/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-A.js
new file mode 100644
index 00000000000..60314259dc0
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-A.js
@@ -0,0 +1,14 @@
+// ...
+express().post("/save", (req, res) => {
+  fs.access(rootDir, (err) => {
+    // ...
+    try {
+      save(rootDir, req.query.path, req.body); // GOOD no uncaught exception
+      res.status(200);
+      res.end();
+    } catch (e) {
+      res.status(500);
+      res.end();
+    }
+  });
+});
diff --git a/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-B.js b/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-B.js
new file mode 100644
index 00000000000..1d02956a341
--- /dev/null
+++ b/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-B.js
@@ -0,0 +1,13 @@
+// ...
+express().post("/save", async (req, res) => {
+  try {
+    await fs.access(rootDir);
+  } catch (e) {
+    console.error(`Server setup is corrupted, ${rootDir} does not exist!`);
+    res.status(500);
+    res.end();
+  }
+  save(rootDir, req.query.path, req.body); // MAYBE BAD, depends on the commandline options
+  res.status(200);
+  res.end();
+});

From 5a6e6928073603eda6cffbd0930f16c68197c198 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esbena@github.com>
Date: Tue, 19 Jan 2021 09:58:08 +0100
Subject: [PATCH 7/9] add js/server-crash to the security suite

---
 javascript/config/suites/javascript/security | 1 +
 1 file changed, 1 insertion(+)

diff --git a/javascript/config/suites/javascript/security b/javascript/config/suites/javascript/security
index 3e87d292d7b..9f826d04314 100644
--- a/javascript/config/suites/javascript/security
+++ b/javascript/config/suites/javascript/security
@@ -49,6 +49,7 @@
 + semmlecode-javascript-queries/Security/CWE-640/HostHeaderPoisoningInEmailGeneration.ql: /Security/CWE/CWE-640
 + semmlecode-javascript-queries/Security/CWE-643/XpathInjection.ql: /Security/CWE/CWE-643
 + semmlecode-javascript-queries/Security/CWE-730/RegExpInjection.ql: /Security/CWE/CWE-730
++ semmlecode-javascript-queries/Security/CWE-730/ServerCrash.ql: /Security/CWE/CWE-730
 + semmlecode-javascript-queries/Security/CWE-754/UnvalidatedDynamicMethodCall.ql: /Security/CWE/CWE-754
 + semmlecode-javascript-queries/Security/CWE-770/MissingRateLimiting.ql: /Security/CWE/CWE-770
 + semmlecode-javascript-queries/Security/CWE-776/XmlBomb.ql: /Security/CWE/CWE-776

From 718f6eb3fde9837192c282ad35357094f5f1c98a Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esbena@github.com>
Date: Fri, 22 Jan 2021 13:17:38 +0100
Subject: [PATCH 8/9] JS: update and prettify examples

---
 .../CWE-730/examples/server-crash.BAD.js      | 33 ++++++++++---------
 .../CWE-730/examples/server-crash.GOOD-A.js   |  2 +-
 .../CWE-730/examples/server-crash.GOOD-B.js   |  5 ++-
 3 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/javascript/ql/src/Security/CWE-730/examples/server-crash.BAD.js b/javascript/ql/src/Security/CWE-730/examples/server-crash.BAD.js
index 1b7001666ff..9642ced4c1a 100644
--- a/javascript/ql/src/Security/CWE-730/examples/server-crash.BAD.js
+++ b/javascript/ql/src/Security/CWE-730/examples/server-crash.BAD.js
@@ -1,21 +1,22 @@
 const express = require("express"),
-      fs = require("fs");
+  fs = require("fs");
 
-function save(rootDir, path, content){
-	if (!isValidPath(rootDir, req.query.filePath)) {
-		throw new Error(`Invalid filePath: ${req.query.filePath}`); // BAD crashes the server
-	}
-	// write content to disk
+function save(rootDir, path, content) {
+  if (!isValidPath(rootDir, req.query.filePath)) {
+    throw new Error(`Invalid filePath: ${req.query.filePath}`); // BAD crashes the server
+  }
+  // write content to disk
 }
 express().post("/save", (req, res) => {
-	fs.access(rootDir, (err) => {
-		if (err) {
-			console.error(`Server setup is corrupted, ${rootDir} does not exist!`);
-			res.status(500);
-			res.end();
-		}
-		save(rootDir, req.query.path, req.body);
-		res.status(200);
-		res.end();
-	});
+  fs.exists(rootDir, (exists) => {
+    if (!exists) {
+      console.error(`Server setup is corrupted, ${rootDir} does not exist!`);
+      res.status(500);
+      res.end();
+      return;
+    }
+    save(rootDir, req.query.path, req.body);
+    res.status(200);
+    res.end();
+  });
 });
diff --git a/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-A.js b/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-A.js
index 60314259dc0..b23071ea53c 100644
--- a/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-A.js
+++ b/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-A.js
@@ -1,6 +1,6 @@
 // ...
 express().post("/save", (req, res) => {
-  fs.access(rootDir, (err) => {
+  fs.exists(rootDir, (exists) => {
     // ...
     try {
       save(rootDir, req.query.path, req.body); // GOOD no uncaught exception
diff --git a/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-B.js b/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-B.js
index 1d02956a341..4e6e3c3bc72 100644
--- a/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-B.js
+++ b/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-B.js
@@ -1,11 +1,10 @@
 // ...
 express().post("/save", async (req, res) => {
-  try {
-    await fs.access(rootDir);
-  } catch (e) {
+  if (await fs.promises.exists(rootDir)) {
     console.error(`Server setup is corrupted, ${rootDir} does not exist!`);
     res.status(500);
     res.end();
+    return;
   }
   save(rootDir, req.query.path, req.body); // MAYBE BAD, depends on the commandline options
   res.status(200);

From 3f3962f7a9d3d4a252985e7dc6ce61c5fb71e2a3 Mon Sep 17 00:00:00 2001
From: Esben Sparre Andreasen <esbena@github.com>
Date: Fri, 22 Jan 2021 14:03:21 +0100
Subject: [PATCH 9/9] Update
 javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-B.js

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
---
 .../ql/src/Security/CWE-730/examples/server-crash.GOOD-B.js     | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-B.js b/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-B.js
index 4e6e3c3bc72..2a795ac001f 100644
--- a/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-B.js
+++ b/javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-B.js
@@ -1,6 +1,6 @@
 // ...
 express().post("/save", async (req, res) => {
-  if (await fs.promises.exists(rootDir)) {
+  if (!(await fs.promises.exists(rootDir))) {
     console.error(`Server setup is corrupted, ${rootDir} does not exist!`);
     res.status(500);
     res.end();