From 2dbd762bd964650efe4cd409646490e3882fc267 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Mon, 11 Jan 2021 14:13:41 +0100 Subject: [PATCH] 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 @@ + + + + + + + + + + + + + + + + + + + + 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 + }); +});