JS: add initial version of ServerCrash.ql

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>

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"

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 |

javascript/ql/test/query-tests/Security/CWE-730/ServerCrash.qlref

@@ -0,0 +1 @@
+Security/CWE-730/ServerCrash.ql

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
+  });
+});