Skip to content

Commit 1d39652

Browse files
esbenaesbena
committed
JS: add initial version of ServerCrash.ql
1 parent 8b3dd6d commit 1d39652

5 files changed

Lines changed: 226 additions & 0 deletions

File tree

‎javascript/ql/src/Security/CWE-730/ServerCrash.qhelp‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
8+
</overview>
9+
10+
<recommendation>
11+
12+
</recommendation>
13+
14+
<example>
15+
16+
</example>
17+
18+
<references>
19+
20+
</references>
21+
22+
</qhelp>

‎javascript/ql/src/Security/CWE-730/ServerCrash.ql‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
/**
2+
* @name Server crash
3+
* @description A server that can be forced to crash may be vulnerable to denial-of-service
4+
* attacks.
5+
* @kind problem
6+
* @problem.severity error
7+
* @precision high
8+
* @id js/server-crash
9+
* @tags security
10+
* external/cwe/cwe-730
11+
*/
12+
13+
import javascript
14+
15+
/**
16+
* Gets a function that `caller` invokes.
17+
*/
18+
Function getACallee(Function caller) {
19+
exists(DataFlow::InvokeNode invk |
20+
invk.getEnclosingFunction() = caller and result = invk.getACallee()
21+
)
22+
}
23+
24+
/**
25+
* Gets a function that `caller` invokes, excluding calls guarded in `try`-blocks.
26+
*/
27+
Function getAnUnguardedCallee(Function caller) {
28+
exists(DataFlow::InvokeNode invk |
29+
invk.getEnclosingFunction() = caller and
30+
result = invk.getACallee() and
31+
not exists(invk.asExpr().getEnclosingStmt().getEnclosingTryCatchStmt())
32+
)
33+
}
34+
35+
predicate isHeaderValue(HTTP::ExplicitHeaderDefinition def, DataFlow::Node node) {
36+
def.definesExplicitly(_, node.asExpr())
37+
}
38+
39+
predicate isDangerousPropertyRead(DataFlow::PropRead read) {
40+
// TODO use flow labels
41+
exists(DataFlow::PropRead base |
42+
base = read.getBase() and
43+
not read instanceof RemoteFlowSource and
44+
not exists(read.getACall())
45+
)
46+
}
47+
48+
class Configuration extends TaintTracking::Configuration {
49+
Configuration() { this = "Configuration" }
50+
51+
override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
52+
53+
override predicate isSink(DataFlow::Node node) {
54+
// using control characters in a header value will cause an exception
55+
isHeaderValue(_, node)
56+
or
57+
// accessing a property of undefined will cause an exception
58+
isDangerousPropertyRead(node)
59+
}
60+
}
61+
62+
predicate isLikelyToThrow(DataFlow::Node crash) {
63+
exists(Configuration cfg, DataFlow::Node sink | cfg.hasFlow(_, sink) |
64+
isHeaderValue(crash, sink)
65+
or
66+
isDangerousPropertyRead(sink) and sink = crash
67+
)
68+
}
69+
70+
/**
71+
* A call that looks like it is asynchronous.
72+
*/
73+
class AsyncCall extends DataFlow::CallNode {
74+
DataFlow::FunctionNode callback;
75+
76+
AsyncCall() {
77+
callback.flowsTo(getLastArgument()) and
78+
callback.getParameter(0).getName() = ["e", "err", "error"] and
79+
callback.getNumParameter() = 2 and
80+
not exists(callback.getAReturn())
81+
}
82+
83+
DataFlow::FunctionNode getCallback() { result = callback }
84+
}
85+
86+
/**
87+
* A node that will crash a server if it throws an exception.
88+
*
89+
* The crash happens because a route handler invokes an asynchronous function that in turn throws an exception produced by this node.
90+
*/
91+
class ServerCrasher extends ExprOrStmt {
92+
HTTP::RouteHandler routeHandler;
93+
DataFlow::FunctionNode asyncFunction;
94+
95+
ServerCrasher() {
96+
exists(AsyncCall asyncCall, Function throwingFunction |
97+
// the route handler transitively calls an async function
98+
asyncCall.getEnclosingFunction() =
99+
getACallee*(routeHandler.(DataFlow::FunctionNode).getFunction()) and
100+
asyncFunction = asyncCall.getCallback() and
101+
// the async function transitively calls a function that may throw an exception out of the the async function
102+
throwingFunction = getAnUnguardedCallee*(asyncFunction.getFunction()) and
103+
this.getContainer() = throwingFunction and
104+
not exists([this.(Expr).getEnclosingStmt(), this.(Stmt)].getEnclosingTryCatchStmt())
105+
)
106+
}
107+
108+
/**
109+
* Gets the asynchronous function from which a server-crashing exception escapes.
110+
*/
111+
DataFlow::FunctionNode getAsyncFunction() { result = asyncFunction }
112+
113+
/**
114+
* Gets the route handler that ultimately is responsible for the server crash.
115+
*/
116+
HTTP::RouteHandler getRouteHandler() { result = routeHandler }
117+
}
118+
119+
from ServerCrasher crasher
120+
where isLikelyToThrow(crasher.(Expr).flow()) or crasher instanceof ThrowStmt
121+
select crasher, "When an exception is thrown here and later exits $@, the server of $@ will crash.",
122+
crasher.getAsyncFunction(), "an asynchronous function", crasher.getRouteHandler(),
123+
"this route handler"

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
| 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 } | an asynchronous function | server-crash.js:31:25:73:1 | (req, r ... });\\n} | this route handler |
2+
| 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 } | an asynchronous function | server-crash.js:31:25:73:1 | (req, r ... });\\n} | this route handler |
3+
| 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 } | an asynchronous function | server-crash.js:31:25:73:1 | (req, r ... });\\n} | this route handler |
4+
| 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 } | an asynchronous function | server-crash.js:31:25:73:1 | (req, r ... });\\n} | this route handler |
5+
| 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 } | an asynchronous function | server-crash.js:31:25:73:1 | (req, r ... });\\n} | this route handler |
6+
| 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 } | an asynchronous function | server-crash.js:31:25:73:1 | (req, r ... });\\n} | this route handler |
7+
| server-crash.js:68:5:68:21 | req.query.foo.bar | When an exception is thrown here and later exits $@, the server of $@ will crash. | server-crash.js:67:28:69:3 | (err, x ... OK\\n } | an asynchronous function | server-crash.js:31:25:73:1 | (req, r ... });\\n} | this route handler |

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE-730/ServerCrash.ql

‎javascript/ql/test/query-tests/Security/CWE-730/server-crash.js‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
const express = require("express");
2+
const app = express();
3+
const fs = require("fs");
4+
5+
function indirection1() {
6+
fs.readFile("/WHATEVER", (err, x) => {
7+
throw err; // NOT OK
8+
});
9+
}
10+
function indirection2() {
11+
throw 42; // NOT OK
12+
}
13+
function indirection3() {
14+
try {
15+
fs.readFile("/WHATEVER", (err, x) => {
16+
throw err; // NOT OK
17+
});
18+
} catch (e) {}
19+
}
20+
function indirection4() {
21+
throw 42; // OK: guarded caller
22+
}
23+
function indirection5() {
24+
indirection6();
25+
}
26+
function indirection6() {
27+
fs.readFile("/WHATEVER", (err, x) => {
28+
throw err; // NOT OK
29+
});
30+
}
31+
app.get("/async-throw", (req, res) => {
32+
fs.readFile("/WHATEVER", (err, x) => {
33+
throw err; // NOT OK
34+
});
35+
fs.readFile("/WHATEVER", (err, x) => {
36+
try {
37+
throw err; // OK: guarded throw
38+
} catch (e) {}
39+
});
40+
fs.readFile("/WHATEVER", (err, x) => {
41+
res.setHeader("reflected", req.query.header); // NOT OK
42+
});
43+
fs.readFile("/WHATEVER", (err, x) => {
44+
try {
45+
res.setHeader("reflected", req.query.header); // OK: guarded call
46+
} catch (e) {}
47+
});
48+
49+
indirection1();
50+
fs.readFile("/WHATEVER", (err, x) => {
51+
indirection2();
52+
});
53+
54+
indirection3();
55+
try {
56+
indirection4();
57+
} catch (e) {}
58+
indirection5();
59+
60+
fs.readFile("/WHATEVER", (err, x) => {
61+
req.query.foo; // OK
62+
});
63+
fs.readFile("/WHATEVER", (err, x) => {
64+
req.query.foo.toString(); // OK
65+
});
66+
67+
fs.readFile("/WHATEVER", (err, x) => {
68+
req.query.foo.bar; // NOT OK
69+
});
70+
fs.readFile("/WHATEVER", (err, x) => {
71+
res.setHeader("reflected", unknown); // OK
72+
});
73+
});

0 commit comments

Comments
 (0)