Skip to content

Commit d591c51

Browse files
esbenaesbena
committed
JS: reformulate js/server-crash as a path problem
1 parent 2dbd762 commit d591c51

3 files changed

Lines changed: 182 additions & 62 deletions

File tree

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

Lines changed: 133 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
* @name Server crash
33
* @description A server that can be forced to crash may be vulnerable to denial-of-service
44
* attacks.
5-
* @kind problem
5+
* @kind path-problem
66
* @problem.severity error
77
* @precision high
88
* @id js/server-crash
@@ -13,88 +13,166 @@
1313
import javascript
1414

1515
/**
16-
* Gets a function that `caller` invokes.
16+
* A call that appears to be asynchronous (heuristic).
1717
*/
18-
Function getACallee(Function caller) {
19-
exists(DataFlow::InvokeNode invk |
20-
invk.getEnclosingFunction() = caller and result = invk.getACallee()
21-
)
18+
class AsyncCall extends DataFlow::CallNode {
19+
DataFlow::FunctionNode callback;
20+
21+
AsyncCall() {
22+
callback.flowsTo(getLastArgument()) and
23+
callback.getParameter(0).getName() = ["e", "err", "error"] and
24+
callback.getNumParameter() = 2 and
25+
not exists(callback.getAReturn())
26+
}
27+
28+
/**
29+
* Gets the callback that is invoked asynchronously.
30+
*/
31+
DataFlow::FunctionNode getCallback() { result = callback }
2232
}
2333

2434
/**
25-
* Gets a function that `caller` invokes, excluding calls guarded in `try`-blocks.
35+
* Gets a function that is invoked as a consequence of invoking a route handler `rh`.
2636
*/
27-
Function getAnUnguardedCallee(Function caller) {
37+
Function invokedByRouteHandler(HTTP::RouteHandler rh) {
38+
rh = result.flow()
39+
or
40+
// follow the immediate call graph
2841
exists(DataFlow::InvokeNode invk |
29-
invk.getEnclosingFunction() = caller and
3042
result = invk.getACallee() and
31-
not exists(invk.asExpr().getEnclosingStmt().getEnclosingTryCatchStmt())
43+
invk.getEnclosingFunction() = invokedByRouteHandler(rh)
3244
)
45+
// if new edges are added here, the `edges` predicate should be updated accordingly
3346
}
3447

35-
predicate isHeaderValue(HTTP::ExplicitHeaderDefinition def, DataFlow::Node node) {
36-
def.definesExplicitly(_, node.asExpr())
48+
/**
49+
* A callback provided to an asynchronous call.
50+
*/
51+
class AsyncCallback extends DataFlow::FunctionNode {
52+
AsyncCallback() { this = any(AsyncCall c).getCallback() }
3753
}
3854

39-
class Configuration extends TaintTracking::Configuration {
40-
Configuration() { this = "Configuration" }
41-
42-
override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
55+
/**
56+
* 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.
57+
*/
58+
Function inUnguardedAsyncCallStack(AsyncCallback callback) {
59+
callback = result.flow()
60+
or
61+
exists(DataFlow::InvokeNode invk |
62+
result = invk.getACallee() and
63+
not exists(invk.asExpr().getEnclosingStmt().getEnclosingTryCatchStmt()) and
64+
invk.getEnclosingFunction() = inUnguardedAsyncCallStack(callback)
65+
)
66+
}
4367

44-
override predicate isSink(DataFlow::Node node) {
45-
// using control characters in a header value will cause an exception
46-
isHeaderValue(_, node)
47-
}
68+
/**
69+
* Gets a function that is invoked by `asyncCallback` without any try-block wrapping, `asyncCallback` is in turn is called indirectly by `routeHandler`.
70+
*
71+
* If the result throws an excection, the server of `routeHandler` will crash.
72+
*/
73+
Function getAPotentialServerCrasher(
74+
HTTP::RouteHandler routeHandler, AsyncCall asyncCall, AsyncCallback asyncCallback
75+
) {
76+
// the route handler transitively calls an async function
77+
asyncCall.getEnclosingFunction() = invokedByRouteHandler(routeHandler) and
78+
asyncCallback = asyncCall.getCallback() and
79+
// the async function transitively calls a function that may throw an exception out of the the async function
80+
result = inUnguardedAsyncCallStack(asyncCallback)
4881
}
4982

50-
predicate isLikelyToThrow(DataFlow::Node crash) {
51-
exists(Configuration cfg, DataFlow::Node sink | cfg.hasFlow(_, sink) | isHeaderValue(crash, sink))
83+
/**
84+
* Gets a node that is likely to throw an uncaught exception in `fun`.
85+
*/
86+
LikelyExceptionThrower getALikelyUncaughtExceptionThrower(Function fun) {
87+
result.getContainer() = fun and
88+
not exists([result.(Expr).getEnclosingStmt(), result.(Stmt)].getEnclosingTryCatchStmt())
5289
}
5390

5491
/**
55-
* A call that looks like it is asynchronous.
92+
* Edges that builds an explanatory graph that follows the mental model of how the the exception flows.
93+
*
94+
* - step 1. exception is thrown
95+
* - step 2. exception exits the enclosing function
96+
* - step 3. exception follows the call graph backwards until an async callee is encountered
97+
* - step 4. (at this point, the program crashes)
98+
* - step 5. if the program had not crashed, the exception would conceptually follow the call graph backwards to a route handler
5699
*/
57-
class AsyncCall extends DataFlow::CallNode {
58-
DataFlow::FunctionNode callback;
100+
query predicate edges(ASTNode pred, ASTNode succ) {
101+
nodes(pred) and
102+
nodes(succ) and
103+
(
104+
// the first step from the alert location to the enclosing function
105+
pred = getALikelyUncaughtExceptionThrower(_) and
106+
succ = pred.getContainer()
107+
or
108+
// ordinary flow graph
109+
exists(DataFlow::InvokeNode invoke, Function f |
110+
invoke.getACallee() = f and
111+
succ = invoke.getAstNode() and
112+
pred = f
113+
or
114+
invoke.getContainer() = f and
115+
succ = f and
116+
pred = invoke.getAstNode()
117+
)
118+
or
119+
// the async step
120+
exists(DataFlow::Node predNode, DataFlow::Node succNode |
121+
exists(getAPotentialServerCrasher(_, predNode, succNode)) and
122+
predNode.getAstNode() = succ and
123+
succNode.getAstNode() = pred
124+
)
125+
)
126+
}
59127

60-
AsyncCall() {
61-
callback.flowsTo(getLastArgument()) and
62-
callback.getParameter(0).getName() = ["e", "err", "error"] and
63-
callback.getNumParameter() = 2 and
64-
not exists(callback.getAReturn())
65-
}
128+
/**
129+
* Nodes for building an explanatory graph that follows the mental model of how the the exception flows.
130+
*/
131+
query predicate nodes(ASTNode node) {
132+
exists(HTTP::RouteHandler rh, Function fun |
133+
main(rh, _, _) and
134+
fun = invokedByRouteHandler(rh)
135+
|
136+
node = any(DataFlow::InvokeNode invk | invk.getACallee() = fun).getAstNode() or
137+
node = fun
138+
)
139+
or
140+
exists(AsyncCallback cb, Function fun |
141+
main(_, cb, _) and
142+
fun = inUnguardedAsyncCallStack(cb)
143+
|
144+
node = any(DataFlow::InvokeNode invk | invk.getACallee() = fun).getAstNode() or
145+
node = fun
146+
)
147+
or
148+
main(_, _, node)
149+
}
66150

67-
DataFlow::FunctionNode getCallback() { result = callback }
151+
predicate main(HTTP::RouteHandler rh, AsyncCallback asyncCallback, ExprOrStmt crasher) {
152+
crasher = getALikelyUncaughtExceptionThrower(getAPotentialServerCrasher(rh, _, asyncCallback))
68153
}
69154

70155
/**
71-
* Gets a function that is invoked by `asyncCallback` without any try-block wrapping, `asyncCallback` is in turn is called indirectly by `routeHandler`.
156+
* A node that is likely to throw an exception.
72157
*
73-
* If the result throws an excection, the server of `routeHandler` will crash.
158+
* This is the primary extension point for this query.
74159
*/
75-
Function getAPotentialServerCrasher(
76-
HTTP::RouteHandler routeHandler, DataFlow::FunctionNode asyncCallback
77-
) {
78-
exists(AsyncCall asyncCall |
79-
// the route handler transitively calls an async function
80-
asyncCall.getEnclosingFunction() =
81-
getACallee*(routeHandler.(DataFlow::FunctionNode).getFunction()) and
82-
asyncCallback = asyncCall.getCallback() and
83-
// the async function transitively calls a function that may throw an exception out of the the async function
84-
result = getAnUnguardedCallee*(asyncCallback.getFunction())
85-
)
86-
}
160+
abstract class LikelyExceptionThrower extends ASTNode { }
87161

88162
/**
89-
* Gets an AST node that is likely to throw an uncaught exception in `fun`.
163+
* A `throw` statement.
90164
*/
91-
ExprOrStmt getALikelyExceptionThrower(Function fun) {
92-
result.getContainer() = fun and
93-
not exists([result.(Expr).getEnclosingStmt(), result.(Stmt)].getEnclosingTryCatchStmt()) and
94-
(isLikelyToThrow(result.(Expr).flow()) or result instanceof ThrowStmt)
165+
class TrivialThrowStatement extends LikelyExceptionThrower, ThrowStmt { }
166+
167+
/**
168+
* Empty class for avoiding emptiness checks from the compiler when there are no Expr-typed instances of the LikelyExceptionThrower type.
169+
*/
170+
class CompilerConfusingExceptionThrower extends LikelyExceptionThrower {
171+
CompilerConfusingExceptionThrower() { none() }
95172
}
96173

97-
from HTTP::RouteHandler routeHandler, DataFlow::FunctionNode asyncCallback, ExprOrStmt crasher
98-
where crasher = getALikelyExceptionThrower(getAPotentialServerCrasher(routeHandler, asyncCallback))
99-
select crasher, "When an exception is thrown here and later exits $@, the server of $@ will crash.",
100-
asyncCallback, "this asynchronous callback", routeHandler, "this route handler"
174+
from HTTP::RouteHandler rh, AsyncCallback asyncCallback, ExprOrStmt crasher
175+
where main(rh, asyncCallback, crasher)
176+
select crasher, crasher, rh.getAstNode(),
177+
"When an exception is thrown here and later escapes at $@, the server of $@ will crash.",
178+
asyncCallback, "this asynchronous callback", rh, "this route handler"

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

Lines changed: 48 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,48 @@
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 } | this asynchronous callback | 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 } | this asynchronous callback | 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 } | this asynchronous callback | 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 } | this asynchronous callback | 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 } | this asynchronous callback | 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 } | this asynchronous callback | server-crash.js:31:25:73:1 | (req, r ... });\\n} | this route handler |
1+
edges
2+
| server-crash.js:5:1:9:1 | functio ... });\\n} | server-crash.js:49:3:49:16 | indirection1() |
3+
| server-crash.js:7:5:7:14 | throw err; | server-crash.js:6:28:8:3 | (err, x ... OK\\n } |
4+
| server-crash.js:10:1:12:1 | functio ... OT OK\\n} | server-crash.js:51:5:51:18 | indirection2() |
5+
| server-crash.js:11:3:11:11 | throw 42; | server-crash.js:10:1:12:1 | functio ... OT OK\\n} |
6+
| server-crash.js:13:1:19:1 | functio ... e) {}\\n} | server-crash.js:54:3:54:16 | indirection3() |
7+
| server-crash.js:16:7:16:16 | throw err; | server-crash.js:15:30:17:5 | (err, x ... K\\n } |
8+
| server-crash.js:20:1:22:1 | functio ... aller\\n} | server-crash.js:56:5:56:18 | indirection4() |
9+
| server-crash.js:23:1:25:1 | functio ... n6();\\n} | server-crash.js:58:3:58:16 | indirection5() |
10+
| server-crash.js:24:3:24:16 | indirection6() | server-crash.js:23:1:25:1 | functio ... n6();\\n} |
11+
| server-crash.js:26:1:30:1 | functio ... });\\n} | server-crash.js:24:3:24:16 | indirection6() |
12+
| server-crash.js:28:5:28:14 | throw err; | server-crash.js:27:28:29:3 | (err, x ... OK\\n } |
13+
| server-crash.js:33:5:33:14 | throw err; | server-crash.js:32:28:34:3 | (err, x ... OK\\n } |
14+
| server-crash.js:49:3:49:16 | indirection1() | server-crash.js:31:25:73:1 | (req, r ... });\\n} |
15+
| server-crash.js:51:5:51:18 | indirection2() | server-crash.js:50:28:52:3 | (err, x ... ();\\n } |
16+
| server-crash.js:54:3:54:16 | indirection3() | server-crash.js:31:25:73:1 | (req, r ... });\\n} |
17+
| server-crash.js:56:5:56:18 | indirection4() | server-crash.js:31:25:73:1 | (req, r ... });\\n} |
18+
| server-crash.js:58:3:58:16 | indirection5() | server-crash.js:31:25:73:1 | (req, r ... });\\n} |
19+
nodes
20+
| server-crash.js:5:1:9:1 | functio ... });\\n} |
21+
| server-crash.js:6:28:8:3 | (err, x ... OK\\n } |
22+
| server-crash.js:7:5:7:14 | throw err; |
23+
| server-crash.js:10:1:12:1 | functio ... OT OK\\n} |
24+
| server-crash.js:11:3:11:11 | throw 42; |
25+
| server-crash.js:13:1:19:1 | functio ... e) {}\\n} |
26+
| server-crash.js:15:30:17:5 | (err, x ... K\\n } |
27+
| server-crash.js:16:7:16:16 | throw err; |
28+
| server-crash.js:20:1:22:1 | functio ... aller\\n} |
29+
| server-crash.js:23:1:25:1 | functio ... n6();\\n} |
30+
| server-crash.js:24:3:24:16 | indirection6() |
31+
| server-crash.js:26:1:30:1 | functio ... });\\n} |
32+
| server-crash.js:27:28:29:3 | (err, x ... OK\\n } |
33+
| server-crash.js:28:5:28:14 | throw err; |
34+
| server-crash.js:31:25:73:1 | (req, r ... });\\n} |
35+
| server-crash.js:32:28:34:3 | (err, x ... OK\\n } |
36+
| server-crash.js:33:5:33:14 | throw err; |
37+
| server-crash.js:49:3:49:16 | indirection1() |
38+
| server-crash.js:50:28:52:3 | (err, x ... ();\\n } |
39+
| server-crash.js:51:5:51:18 | indirection2() |
40+
| server-crash.js:54:3:54:16 | indirection3() |
41+
| server-crash.js:56:5:56:18 | indirection4() |
42+
| server-crash.js:58:3:58:16 | indirection5() |
43+
#select
44+
| 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 |
45+
| 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 |
46+
| 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 |
47+
| 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 |
48+
| 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 |

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ app.get("/async-throw", (req, res) => {
3838
} catch (e) {}
3939
});
4040
fs.readFile("/WHATEVER", (err, x) => {
41-
res.setHeader("reflected", req.query.header); // NOT OK
41+
res.setHeader("reflected", req.query.header); // NOT OK [INCONSISTENCY]
4242
});
4343
fs.readFile("/WHATEVER", (err, x) => {
4444
try {

0 commit comments

Comments
 (0)