Skip to content

Commit 527c415

Browse files
authored
Merge pull request #4951 from esbena/js/reintroduce-server-crash
Approved by erik-krogh
2 parents 87b738d + 3f3962f commit 527c415

File tree

10 files changed

+558
-0
lines changed

10 files changed

+558
-0
lines changed
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* The `js/server-crash` query has been added. It highlights servers may be terminated by a malicious user.

javascript/config/suites/javascript/security

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
+ semmlecode-javascript-queries/Security/CWE-640/HostHeaderPoisoningInEmailGeneration.ql: /Security/CWE/CWE-640
5050
+ semmlecode-javascript-queries/Security/CWE-643/XpathInjection.ql: /Security/CWE/CWE-643
5151
+ semmlecode-javascript-queries/Security/CWE-730/RegExpInjection.ql: /Security/CWE/CWE-730
52+
+ semmlecode-javascript-queries/Security/CWE-730/ServerCrash.ql: /Security/CWE/CWE-730
5253
+ semmlecode-javascript-queries/Security/CWE-754/UnvalidatedDynamicMethodCall.ql: /Security/CWE/CWE-754
5354
+ semmlecode-javascript-queries/Security/CWE-770/MissingRateLimiting.ql: /Security/CWE/CWE-770
5455
+ semmlecode-javascript-queries/Security/CWE-776/XmlBomb.ql: /Security/CWE/CWE-776
Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
8+
<p>
9+
10+
Servers handle requests from clients until terminated
11+
deliberately by a server administrator. A client request that results
12+
in an uncaught server-side exception causes the current server
13+
response generation to fail, and should not have an effect on
14+
subsequent client requests.
15+
16+
</p>
17+
18+
<p>
19+
20+
Under some circumstances, uncaught exceptions can however
21+
cause the entire server to terminate abruptly. Such a behavior is
22+
highly undesirable, especially if it gives malicious users the ability
23+
to turn off the server at will, which is an efficient
24+
denial-of-service attack.
25+
26+
</p>
27+
28+
</overview>
29+
30+
<recommendation>
31+
32+
<p>
33+
34+
Ensure that the processing of client requests can not cause
35+
uncaught exceptions to terminate the entire server abruptly.
36+
37+
</p>
38+
39+
</recommendation>
40+
41+
<example>
42+
43+
<p>
44+
45+
The following server implementation checks if a client-provided
46+
file path is valid and throws an exception if the check fails. It can
47+
be seen that the exception is uncaught, and it is therefore reasonable to
48+
expect the server to respond with an error response to client requests
49+
that cause the check to fail.
50+
51+
But since the exception is uncaught in the context of an
52+
asynchronous callback invocation (<code>fs.access(...)</code>), the
53+
entire server will terminate instead.
54+
55+
</p>
56+
57+
<sample src="examples/server-crash.BAD.js"/>
58+
59+
<p>
60+
To remedy this, the server can catch the exception explicitly with
61+
a <code>try/catch</code> block, and generate an appropriate error
62+
response instead:
63+
64+
</p>
65+
66+
<sample src="examples/server-crash.GOOD-A.js"/>
67+
68+
<p>
69+
70+
An alternative is to use an <code>async</code> and
71+
<code>await</code> for the asynchronous behavior, since the server
72+
will then print warning messages about uncaught exceptions instead of
73+
terminating, unless the server was started with the commandline option
74+
<code>--unhandled-rejections=strict</code>:
75+
76+
</p>
77+
78+
<sample src="examples/server-crash.GOOD-B.js"/>
79+
80+
</example>
81+
82+
<references>
83+
84+
</references>
85+
86+
</qhelp>
Lines changed: 189 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,189 @@
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 path-problem
6+
* @problem.severity warning
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 indirectly invokes an asynchronous callback through `async`, where the callback throws an uncaught exception at `thrower`.
17+
*/
18+
Function invokesCallbackThatThrowsUncaughtException(
19+
AsyncSentinelCall async, LikelyExceptionThrower thrower
20+
) {
21+
async.getAsyncCallee() = throwsUncaughtExceptionInAsyncContext(thrower) and
22+
result = async.getEnclosingFunction()
23+
or
24+
exists(DataFlow::InvokeNode invk, Function fun |
25+
fun = invokesCallbackThatThrowsUncaughtException(async, thrower) and
26+
// purposely not checking for `getEnclosingTryCatchStmt`. An async callback called from inside a try-catch can still crash the server.
27+
result = invk.getEnclosingFunction()
28+
|
29+
invk.getACallee() = fun
30+
or
31+
// traverse a slightly extended call graph to get additional TPs
32+
invk.(AsyncSentinelCall).getAsyncCallee() = fun
33+
)
34+
}
35+
36+
/**
37+
* Gets a callee of an invocation `invk` that is not guarded by a try statement.
38+
*/
39+
Function getUncaughtExceptionRethrowerCallee(DataFlow::InvokeNode invk) {
40+
not exists(invk.asExpr().getEnclosingStmt().getEnclosingTryCatchStmt()) and
41+
result = invk.getACallee()
42+
}
43+
44+
/**
45+
* Holds if `thrower` is not guarded by a try statement.
46+
*/
47+
predicate isUncaughtExceptionThrower(LikelyExceptionThrower thrower) {
48+
not exists([thrower.(Expr).getEnclosingStmt(), thrower.(Stmt)].getEnclosingTryCatchStmt())
49+
}
50+
51+
/**
52+
* Gets a function that may throw an uncaught exception originating at `thrower`, which then may escape in an asynchronous calling context.
53+
*/
54+
Function throwsUncaughtExceptionInAsyncContext(LikelyExceptionThrower thrower) {
55+
(
56+
isUncaughtExceptionThrower(thrower) and
57+
result = thrower.getContainer()
58+
or
59+
exists(DataFlow::InvokeNode invk |
60+
getUncaughtExceptionRethrowerCallee(invk) = throwsUncaughtExceptionInAsyncContext(thrower) and
61+
result = invk.getEnclosingFunction()
62+
)
63+
) and
64+
// Anti-case:
65+
// An exception from an `async` function results in a rejected promise.
66+
// Unhandled promises requires `node --unhandled-rejections=strict ...` to terminate the process
67+
// without that flag, the DEP0018 deprecation warning is printed instead (node.js version 14 and below)
68+
not result.isAsync() and
69+
// pruning optimization since this predicate always is related to `invokesCallbackThatThrowsUncaughtException`
70+
result = reachableFromAsyncCallback()
71+
}
72+
73+
/**
74+
* Holds if `result` is reachable from a callback that is invoked asynchronously.
75+
*/
76+
Function reachableFromAsyncCallback() {
77+
result instanceof AsyncCallback
78+
or
79+
exists(DataFlow::InvokeNode invk |
80+
invk.getEnclosingFunction() = reachableFromAsyncCallback() and
81+
result = invk.getACallee()
82+
)
83+
}
84+
85+
/**
86+
* The main predicate of this query: used for both result display and path computation.
87+
*/
88+
predicate main(
89+
HTTP::RouteHandler rh, AsyncSentinelCall async, AsyncCallback cb, LikelyExceptionThrower thrower
90+
) {
91+
async.getAsyncCallee() = cb and
92+
rh.getAstNode() = invokesCallbackThatThrowsUncaughtException(async, thrower)
93+
}
94+
95+
/**
96+
* A call that may cause a function to be invoked in an asynchronous context outside of the visible source code.
97+
*/
98+
class AsyncSentinelCall extends DataFlow::CallNode {
99+
Function asyncCallee;
100+
101+
AsyncSentinelCall() {
102+
exists(DataFlow::FunctionNode node | node.getAstNode() = asyncCallee |
103+
// manual models
104+
exists(string memberName |
105+
not "Sync" = memberName.suffix(memberName.length() - 4) and
106+
this = NodeJSLib::FS::moduleMember(memberName).getACall() and
107+
node = this.getCallback([1 .. 2])
108+
)
109+
// (add additional cases here to improve the query)
110+
)
111+
}
112+
113+
/**
114+
* Gets the callee that is invoked in an asynchronous context.
115+
*/
116+
Function getAsyncCallee() { result = asyncCallee }
117+
}
118+
119+
/**
120+
* A callback provided to an asynchronous call (heuristic).
121+
*/
122+
class AsyncCallback extends Function {
123+
AsyncCallback() { any(AsyncSentinelCall c).getAsyncCallee() = this }
124+
}
125+
126+
/**
127+
* A node that is likely to throw an exception.
128+
*
129+
* This is the primary extension point for this query.
130+
*/
131+
abstract class LikelyExceptionThrower extends ASTNode { }
132+
133+
/**
134+
* A `throw` statement.
135+
*/
136+
class TrivialThrowStatement extends LikelyExceptionThrower, ThrowStmt { }
137+
138+
/**
139+
* Empty class for avoiding emptiness checks from the compiler when there are no Expr-typed instances of the LikelyExceptionThrower type.
140+
*/
141+
class CompilerConfusingExceptionThrower extends LikelyExceptionThrower {
142+
CompilerConfusingExceptionThrower() { none() }
143+
}
144+
145+
/**
146+
* Edges that builds an explanatory graph that follows the mental model of how the the exception flows.
147+
*
148+
* - step 1. exception is thrown
149+
* - step 2. exception escapes the enclosing function
150+
* - step 3. exception follows the call graph backwards until an async callee is encountered
151+
* - step 4. (at this point, the program crashes)
152+
*/
153+
query predicate edges(ASTNode pred, ASTNode succ) {
154+
exists(LikelyExceptionThrower thrower | main(_, _, _, thrower) |
155+
pred = thrower and
156+
succ = thrower.getContainer()
157+
or
158+
exists(DataFlow::InvokeNode invk, Function fun |
159+
fun = throwsUncaughtExceptionInAsyncContext(thrower)
160+
|
161+
succ = invk.getAstNode() and
162+
pred = invk.getACallee() and
163+
pred = fun
164+
or
165+
succ = fun and
166+
succ = invk.getContainer() and
167+
pred = invk.getAstNode()
168+
)
169+
)
170+
}
171+
172+
/**
173+
* A node in the `edge/2` relation above.
174+
*/
175+
query predicate nodes(ASTNode node) {
176+
edges(node, _) or
177+
edges(_, node)
178+
}
179+
180+
from
181+
HTTP::RouteHandler rh, AsyncSentinelCall async, DataFlow::Node callbackArg, AsyncCallback cb,
182+
ExprOrStmt crasher
183+
where
184+
main(rh, async, cb, crasher) and
185+
callbackArg.getALocalSource().getAstNode() = cb and
186+
async.getAnArgument() = callbackArg
187+
select crasher, crasher, cb,
188+
"The server of $@ will terminate when an uncaught exception from here escapes this $@", rh,
189+
"this route handler", callbackArg, "asynchronous callback"
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
const express = require("express"),
2+
fs = require("fs");
3+
4+
function save(rootDir, path, content) {
5+
if (!isValidPath(rootDir, req.query.filePath)) {
6+
throw new Error(`Invalid filePath: ${req.query.filePath}`); // BAD crashes the server
7+
}
8+
// write content to disk
9+
}
10+
express().post("/save", (req, res) => {
11+
fs.exists(rootDir, (exists) => {
12+
if (!exists) {
13+
console.error(`Server setup is corrupted, ${rootDir} does not exist!`);
14+
res.status(500);
15+
res.end();
16+
return;
17+
}
18+
save(rootDir, req.query.path, req.body);
19+
res.status(200);
20+
res.end();
21+
});
22+
});
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// ...
2+
express().post("/save", (req, res) => {
3+
fs.exists(rootDir, (exists) => {
4+
// ...
5+
try {
6+
save(rootDir, req.query.path, req.body); // GOOD no uncaught exception
7+
res.status(200);
8+
res.end();
9+
} catch (e) {
10+
res.status(500);
11+
res.end();
12+
}
13+
});
14+
});
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// ...
2+
express().post("/save", async (req, res) => {
3+
if (!(await fs.promises.exists(rootDir))) {
4+
console.error(`Server setup is corrupted, ${rootDir} does not exist!`);
5+
res.status(500);
6+
res.end();
7+
return;
8+
}
9+
save(rootDir, req.query.path, req.body); // MAYBE BAD, depends on the commandline options
10+
res.status(200);
11+
res.end();
12+
});

0 commit comments

Comments
 (0)