Skip to content

Commit bc7f021

Browse files
esbenaesbena
committed
JS: replace class with two predicates (and improve alert message)
1 parent d6ae905 commit bc7f021

2 files changed

Lines changed: 31 additions & 38 deletions

File tree

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

Lines changed: 25 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -68,40 +68,33 @@ class AsyncCall extends DataFlow::CallNode {
6868
}
6969

7070
/**
71-
* A node that will crash a server if it throws an exception.
72-
*
73-
* The crash happens because a route handler invokes an asynchronous function that in turn throws an exception produced by this node.
71+
* Gets a function that is invoked by `asyncCallback` without any try-block wrapping, `asyncCallback` is in turn is called indirectly by `routeHandler`.
72+
*
73+
* If the result throws an excection, the server of `routeHandler` will crash.
7474
*/
75-
class ServerCrasher extends ExprOrStmt {
76-
HTTP::RouteHandler routeHandler;
77-
DataFlow::FunctionNode asyncFunction;
78-
79-
ServerCrasher() {
80-
exists(AsyncCall asyncCall, Function throwingFunction |
81-
// the route handler transitively calls an async function
82-
asyncCall.getEnclosingFunction() =
83-
getACallee*(routeHandler.(DataFlow::FunctionNode).getFunction()) and
84-
asyncFunction = asyncCall.getCallback() and
85-
// the async function transitively calls a function that may throw an exception out of the the async function
86-
throwingFunction = getAnUnguardedCallee*(asyncFunction.getFunction()) and
87-
this.getContainer() = throwingFunction and
88-
not exists([this.(Expr).getEnclosingStmt(), this.(Stmt)].getEnclosingTryCatchStmt())
89-
)
90-
}
91-
92-
/**
93-
* Gets the asynchronous function from which a server-crashing exception escapes.
94-
*/
95-
DataFlow::FunctionNode getAsyncFunction() { result = asyncFunction }
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+
}
9687

97-
/**
98-
* Gets the route handler that ultimately is responsible for the server crash.
99-
*/
100-
HTTP::RouteHandler getRouteHandler() { result = routeHandler }
88+
/**
89+
* Gets an AST node that is likely to throw an uncaught exception in `fun`.
90+
*/
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)
10195
}
10296

103-
from ServerCrasher crasher
104-
where isLikelyToThrow(crasher.(Expr).flow()) or crasher instanceof ThrowStmt
97+
from HTTP::RouteHandler routeHandler, DataFlow::FunctionNode asyncCallback, ExprOrStmt crasher
98+
where crasher = getALikelyExceptionThrower(getAPotentialServerCrasher(routeHandler, asyncCallback))
10599
select crasher, "When an exception is thrown here and later exits $@, the server of $@ will crash.",
106-
crasher.getAsyncFunction(), "an asynchronous function", crasher.getRouteHandler(),
107-
"this route handler"
100+
asyncCallback, "this asynchronous callback", routeHandler, "this route handler"

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
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 |
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 |

0 commit comments

Comments
 (0)