From 00661b62dc489e08716e6c713ce2ca7dac66a07c Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 22 Apr 2025 12:00:02 +0200 Subject: [PATCH] JS: Add isMiddlewareSetup() hook to Routing model --- .../ql/lib/semmle/javascript/Routing.qll | 39 ++++++++++++++++++- .../semmle/javascript/frameworks/Fastify.qll | 8 +++- .../CodeInjection/CodeInjection.expected | 18 +++++++++ .../HeuristicSourceCodeInjection.expected | 12 ++++++ .../Security/CWE-094/CodeInjection/fastify.js | 6 +-- 5 files changed, 77 insertions(+), 6 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/Routing.qll b/javascript/ql/lib/semmle/javascript/Routing.qll index 29700a255d61..530322a2d2c4 100644 --- a/javascript/ql/lib/semmle/javascript/Routing.qll +++ b/javascript/ql/lib/semmle/javascript/Routing.qll @@ -139,6 +139,8 @@ module Routing { predicate mayResumeDispatch() { this.getLastChild().mayResumeDispatch() or + isInMiddlewareSetup(this) + or exists(this.(RouteHandler).getAContinuationInvocation()) or // Leaf nodes that aren't functions are assumed to invoke their continuation @@ -155,6 +157,8 @@ module Routing { predicate definitelyResumesDispatch() { this.getLastChild().definitelyResumesDispatch() or + isInMiddlewareSetup(this) + or exists(this.(RouteHandler).getAContinuationInvocation()) or this instanceof MkRouter @@ -325,6 +329,19 @@ module Routing { DataFlow::Node getValueImplicitlyStoredInAccessPath(int n, string path) { none() } } + /** + * Holds if `node` is installed at a route handler that is declared to be a middleware setup, + * and is therefore assume to resume dispatch. + */ + private predicate isInMiddlewareSetup(Node node) { + exists(RouteSetup::Range range | + node = getRouteSetupNode(range) and + range.isMiddlewareSetup() + ) + or + isInMiddlewareSetup(node.getParent()) + } + /** Holds if `pred` and `succ` are adjacent siblings and `succ` is installed after `pred`. */ private predicate areSiblings(Node pred, Node succ) { exists(ValueNode::Range base, int n | @@ -612,6 +629,20 @@ module Routing { * Holds if this route setup targets `router` and occurs at the given `cfgNode`. */ abstract predicate isInstalledAt(Router::Range router, ControlFlowNode cfgNode); + + /** + * Holds if this is a middleware setup, meaning dispatch will resume after the + * route handlers in this route setup have completed (usually meaning that they have returned a promise, which has resolved). + * + * This should only be overridden when the route setup itself determines whether subsequent + * route handlers are invoked afterwards. + * - For Express-like libraries, the route _handler_ determines whether to resume dispatch, + * based on whether the `next` callback is invoked. For such libraries, do not override `isMiddlewareSetup`. + * - For Fastify-like libraries, the route _setup_ determines whether to resume dispatch. + * For example, `.addHook()` will resume dispatch whereas `.get()` will not. `isMiddlewareSetup()` should thus + * hold for `.addHook()` but not for `.get()` calls. + */ + predicate isMiddlewareSetup() { none() } } /** @@ -892,10 +923,14 @@ module Routing { * based on `Node::Range::getValueAtAccessPath`. */ private DataFlow::Node getAnAccessPathRhs(Node base, int n, string path) { - // Assigned in the body of a route handler function, whi + // Assigned in the body of a route handler function, which is a middleware exists(RouteHandler handler | base = handler | result = AccessPath::getAnAssignmentTo(handler.getParameter(n).ref(), path) and - exists(handler.getAContinuationInvocation()) + ( + exists(handler.getAContinuationInvocation()) + or + isInMiddlewareSetup(handler) + ) ) or // Implicit assignment contributed by framework model diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Fastify.qll b/javascript/ql/lib/semmle/javascript/frameworks/Fastify.qll index 53c92e7f7b33..2c2f0f6fc114 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/Fastify.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/Fastify.qll @@ -164,13 +164,19 @@ module Fastify { private class ShorthandRoutingTreeSetup extends Routing::RouteSetup::MethodCall instanceof RouteSetup { - ShorthandRoutingTreeSetup() { not this.getMethodName() = "route" } + ShorthandRoutingTreeSetup() { not this.getMethodName() = ["route", "addHook"] } override string getRelativePath() { result = this.getArgument(0).getStringValue() } override Http::RequestMethodName getHttpMethod() { result = this.getMethodName().toUpperCase() } } + private class AddHookRouteSetup extends Routing::RouteSetup::MethodCall instanceof RouteSetup { + AddHookRouteSetup() { this.getMethodName() = "addHook" } + + override predicate isMiddlewareSetup() { any() } + } + /** Gets the name of the `n`th handler function that can be installed a route setup, in order of execution. */ private string getNthHandlerName(int n) { result = diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected index 9a81aa80a2c0..a63cc195ecbd 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected @@ -51,8 +51,14 @@ | fastify.js:58:44:58:52 | userInput | fastify.js:57:21:57:39 | request.query.input | fastify.js:58:44:58:52 | userInput | This code execution depends on a $@. | fastify.js:57:21:57:39 | request.query.input | user-provided value | | fastify.js:59:23:59:31 | userInput | fastify.js:57:21:57:33 | request.query | fastify.js:59:23:59:31 | userInput | This code execution depends on a $@. | fastify.js:57:21:57:33 | request.query | user-provided value | | fastify.js:59:23:59:31 | userInput | fastify.js:57:21:57:39 | request.query.input | fastify.js:59:23:59:31 | userInput | This code execution depends on a $@. | fastify.js:57:21:57:39 | request.query.input | user-provided value | +| fastify.js:71:34:71:51 | request.storedCode | fastify.js:66:24:66:36 | request.query | fastify.js:71:34:71:51 | request.storedCode | This code execution depends on a $@. | fastify.js:66:24:66:36 | request.query | user-provided value | +| fastify.js:71:34:71:51 | request.storedCode | fastify.js:66:24:66:47 | request ... redCode | fastify.js:71:34:71:51 | request.storedCode | This code execution depends on a $@. | fastify.js:66:24:66:47 | request ... redCode | user-provided value | | fastify.js:71:34:71:51 | request.storedCode | fastify.js:71:34:71:51 | request.storedCode | fastify.js:71:34:71:51 | request.storedCode | This code execution depends on a $@. | fastify.js:71:34:71:51 | request.storedCode | user-provided value | +| fastify.js:84:30:84:43 | reply.userCode | fastify.js:79:20:79:32 | request.query | fastify.js:84:30:84:43 | reply.userCode | This code execution depends on a $@. | fastify.js:79:20:79:32 | request.query | user-provided value | +| fastify.js:84:30:84:43 | reply.userCode | fastify.js:79:20:79:42 | request ... plyCode | fastify.js:84:30:84:43 | reply.userCode | This code execution depends on a $@. | fastify.js:79:20:79:42 | request ... plyCode | user-provided value | | fastify.js:84:30:84:43 | reply.userCode | fastify.js:84:30:84:43 | reply.userCode | fastify.js:84:30:84:43 | reply.userCode | This code execution depends on a $@. | fastify.js:84:30:84:43 | reply.userCode | user-provided value | +| fastify.js:99:30:99:52 | reply.l ... tedCode | fastify.js:94:29:94:41 | request.query | fastify.js:99:30:99:52 | reply.l ... tedCode | This code execution depends on a $@. | fastify.js:94:29:94:41 | request.query | user-provided value | +| fastify.js:99:30:99:52 | reply.l ... tedCode | fastify.js:94:29:94:51 | request ... plyCode | fastify.js:99:30:99:52 | reply.l ... tedCode | This code execution depends on a $@. | fastify.js:94:29:94:51 | request ... plyCode | user-provided value | | fastify.js:99:30:99:52 | reply.l ... tedCode | fastify.js:99:30:99:52 | reply.l ... tedCode | fastify.js:99:30:99:52 | reply.l ... tedCode | This code execution depends on a $@. | fastify.js:99:30:99:52 | reply.l ... tedCode | user-provided value | | module.js:9:16:9:29 | req.query.code | module.js:9:16:9:29 | req.query.code | module.js:9:16:9:29 | req.query.code | This code execution depends on a $@. | module.js:9:16:9:29 | req.query.code | user-provided value | | module.js:11:17:11:30 | req.query.code | module.js:11:17:11:30 | req.query.code | module.js:11:17:11:30 | req.query.code | This code execution depends on a $@. | module.js:11:17:11:30 | req.query.code | user-provided value | @@ -136,6 +142,12 @@ edges | fastify.js:57:9:57:39 | userInput | fastify.js:59:23:59:31 | userInput | provenance | | | fastify.js:57:21:57:33 | request.query | fastify.js:57:9:57:39 | userInput | provenance | | | fastify.js:57:21:57:39 | request.query.input | fastify.js:57:9:57:39 | userInput | provenance | | +| fastify.js:66:24:66:36 | request.query | fastify.js:66:24:66:47 | request ... redCode | provenance | | +| fastify.js:66:24:66:47 | request ... redCode | fastify.js:71:34:71:51 | request.storedCode | provenance | | +| fastify.js:79:20:79:32 | request.query | fastify.js:79:20:79:42 | request ... plyCode | provenance | | +| fastify.js:79:20:79:42 | request ... plyCode | fastify.js:84:30:84:43 | reply.userCode | provenance | | +| fastify.js:94:29:94:41 | request.query | fastify.js:94:29:94:51 | request ... plyCode | provenance | | +| fastify.js:94:29:94:51 | request ... plyCode | fastify.js:99:30:99:52 | reply.l ... tedCode | provenance | | | react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted | provenance | | | react-native.js:7:7:7:33 | tainted | react-native.js:10:23:10:29 | tainted | provenance | | | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | provenance | | @@ -250,8 +262,14 @@ nodes | fastify.js:57:21:57:39 | request.query.input | semmle.label | request.query.input | | fastify.js:58:44:58:52 | userInput | semmle.label | userInput | | fastify.js:59:23:59:31 | userInput | semmle.label | userInput | +| fastify.js:66:24:66:36 | request.query | semmle.label | request.query | +| fastify.js:66:24:66:47 | request ... redCode | semmle.label | request ... redCode | | fastify.js:71:34:71:51 | request.storedCode | semmle.label | request.storedCode | +| fastify.js:79:20:79:32 | request.query | semmle.label | request.query | +| fastify.js:79:20:79:42 | request ... plyCode | semmle.label | request ... plyCode | | fastify.js:84:30:84:43 | reply.userCode | semmle.label | reply.userCode | +| fastify.js:94:29:94:41 | request.query | semmle.label | request.query | +| fastify.js:94:29:94:51 | request ... plyCode | semmle.label | request ... plyCode | | fastify.js:99:30:99:52 | reply.l ... tedCode | semmle.label | reply.l ... tedCode | | module.js:9:16:9:29 | req.query.code | semmle.label | req.query.code | | module.js:11:17:11:30 | req.query.code | semmle.label | req.query.code | diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected index 0f52acfa6cde..aa23d7a6d5a1 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected @@ -45,6 +45,12 @@ edges | fastify.js:57:9:57:39 | userInput | fastify.js:59:23:59:31 | userInput | provenance | | | fastify.js:57:21:57:33 | request.query | fastify.js:57:9:57:39 | userInput | provenance | | | fastify.js:57:21:57:39 | request.query.input | fastify.js:57:9:57:39 | userInput | provenance | | +| fastify.js:66:24:66:36 | request.query | fastify.js:66:24:66:47 | request ... redCode | provenance | | +| fastify.js:66:24:66:47 | request ... redCode | fastify.js:71:34:71:51 | request.storedCode | provenance | | +| fastify.js:79:20:79:32 | request.query | fastify.js:79:20:79:42 | request ... plyCode | provenance | | +| fastify.js:79:20:79:42 | request ... plyCode | fastify.js:84:30:84:43 | reply.userCode | provenance | | +| fastify.js:94:29:94:41 | request.query | fastify.js:94:29:94:51 | request ... plyCode | provenance | | +| fastify.js:94:29:94:51 | request ... plyCode | fastify.js:99:30:99:52 | reply.l ... tedCode | provenance | | | react-native.js:7:7:7:33 | tainted | react-native.js:8:32:8:38 | tainted | provenance | | | react-native.js:7:7:7:33 | tainted | react-native.js:10:23:10:29 | tainted | provenance | | | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted | provenance | | @@ -161,8 +167,14 @@ nodes | fastify.js:57:21:57:39 | request.query.input | semmle.label | request.query.input | | fastify.js:58:44:58:52 | userInput | semmle.label | userInput | | fastify.js:59:23:59:31 | userInput | semmle.label | userInput | +| fastify.js:66:24:66:36 | request.query | semmle.label | request.query | +| fastify.js:66:24:66:47 | request ... redCode | semmle.label | request ... redCode | | fastify.js:71:34:71:51 | request.storedCode | semmle.label | request.storedCode | +| fastify.js:79:20:79:32 | request.query | semmle.label | request.query | +| fastify.js:79:20:79:42 | request ... plyCode | semmle.label | request ... plyCode | | fastify.js:84:30:84:43 | reply.userCode | semmle.label | reply.userCode | +| fastify.js:94:29:94:41 | request.query | semmle.label | request.query | +| fastify.js:94:29:94:51 | request ... plyCode | semmle.label | request ... plyCode | | fastify.js:99:30:99:52 | reply.l ... tedCode | semmle.label | reply.l ... tedCode | | module.js:9:16:9:29 | req.query.code | semmle.label | req.query.code | | module.js:11:17:11:30 | req.query.code | semmle.label | req.query.code | diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/fastify.js b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/fastify.js index bb5577c7acc1..e1cba0d277ca 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/fastify.js +++ b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/fastify.js @@ -63,7 +63,7 @@ fastify.get('/dangerous', async (request, reply) => { // Store user input in request object fastify.addHook('preHandler', async (request, reply) => { - request.storedCode = request.query.storedCode; + request.storedCode = request.query.storedCode; // $ Source[js/code-injection] }); fastify.get('/flow-through-request', async (request, reply) => { // Use the stored code from previous hook @@ -76,7 +76,7 @@ fastify.get('/flow-through-request', async (request, reply) => { // Store user input in reply object fastify.addHook('onRequest', async (request, reply) => { - reply.userCode = request.query.replyCode; + reply.userCode = request.query.replyCode; // $ Source[js/code-injection] }); fastify.get('/flow-through-reply', async (request, reply) => { // Use the code stored in reply object @@ -91,7 +91,7 @@ fastify.get('/flow-through-reply', async (request, reply) => { // Store user input in reply object fastify.addHook('onRequest', async (request, reply) => { reply.locals = reply.locals || {}; - reply.locals.nestedCode = request.query.replyCode; + reply.locals.nestedCode = request.query.replyCode; // $ Source[js/code-injection] }); fastify.get('/flow-through-reply', async (request, reply) => { // Use the code stored in reply object