From 13d2453a457fed04064b796ab0e6face3bbdb750 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 27 Mar 2025 13:59:41 +0100 Subject: [PATCH 1/5] JS: Add GuardedRouteHandler access path component --- .../data/internal/ApiGraphModelsSpecific.qll | 18 ++++++++++++++-- .../frameworks/data/guardedRouteHandler.js | 21 +++++++++++++++++++ .../frameworks/data/test.expected | 4 ++++ .../frameworks/data/test.ext.yml | 2 ++ 4 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 javascript/ql/test/library-tests/frameworks/data/guardedRouteHandler.js diff --git a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsSpecific.qll b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsSpecific.qll index 29cd5da8da11..1f51af3efda0 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsSpecific.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsSpecific.qll @@ -184,6 +184,20 @@ API::Node getExtraSuccessorFromNode(API::Node node, AccessPathTokenBase token) { or token.getName() = "DecoratedParameter" and result = node.getADecoratedParameter() + or + token.getName() = "GuardedRouteHandler" and + result = getAGuardedRouteHandlerApprox(node) +} + +bindingset[node] +pragma[inline_late] +private API::Node getAGuardedRouteHandlerApprox(API::Node node) { + // For now just get any routing node with the same root (i.e. the same web app), as + // there are some known performance issues when checking if it is actually guarded by the given node. + exists(JS::Routing::Node root | + root = JS::Routing::getNode(node.getAValueReachableFromSource()).getRootNode() and + root = JS::Routing::getNode(result.asSink()).getRootNode() + ) } /** @@ -317,7 +331,7 @@ predicate isExtraValidTokenNameInIdentifyingAccessPath(string name) { [ "Member", "AnyMember", "Instance", "Awaited", "ArrayElement", "Element", "MapValue", "NewCall", "Call", "DecoratedClass", "DecoratedMember", "DecoratedParameter", - "WithStringArgument" + "WithStringArgument", "GuardedRouteHandler" ] } @@ -329,7 +343,7 @@ predicate isExtraValidNoArgumentTokenInIdentifyingAccessPath(string name) { name = [ "AnyMember", "Instance", "Awaited", "ArrayElement", "Element", "MapValue", "NewCall", "Call", - "DecoratedClass", "DecoratedMember", "DecoratedParameter" + "DecoratedClass", "DecoratedMember", "DecoratedParameter", "GuardedRouteHandler" ] } diff --git a/javascript/ql/test/library-tests/frameworks/data/guardedRouteHandler.js b/javascript/ql/test/library-tests/frameworks/data/guardedRouteHandler.js new file mode 100644 index 000000000000..972b8b9f1119 --- /dev/null +++ b/javascript/ql/test/library-tests/frameworks/data/guardedRouteHandler.js @@ -0,0 +1,21 @@ +const express = require('express'); +const app = express(); +const testlib = require('testlib'); + +app.get('/before', (req, res) => { + sink(req.injectedReqData); // OK [INCONSISTENCY] - happens before middleware + sink(req.injectedResData); // OK - wrong parameter + + sink(res.injectedReqData); // OK - wrong parameter + sink(res.injectedResData); // OK [INCONSISTENCY] - happens before middleware +}); + +app.use(testlib.middleware()); + +app.get('/after', (req, res) => { + sink(req.injectedReqData); // NOT OK + sink(req.injectedResData); // OK - wrong parameter + + sink(res.injectedReqData); // OK - wrong parameter + sink(res.injectedResData); // NOT OK +}); diff --git a/javascript/ql/test/library-tests/frameworks/data/test.expected b/javascript/ql/test/library-tests/frameworks/data/test.expected index 6586eaeff152..0bc1b6b6ee07 100644 --- a/javascript/ql/test/library-tests/frameworks/data/test.expected +++ b/javascript/ql/test/library-tests/frameworks/data/test.expected @@ -1,6 +1,10 @@ legacyDataFlowDifference consistencyIssue taintFlow +| guardedRouteHandler.js:6:10:6:28 | req.injectedReqData | guardedRouteHandler.js:6:10:6:28 | req.injectedReqData | +| guardedRouteHandler.js:10:10:10:28 | res.injectedResData | guardedRouteHandler.js:10:10:10:28 | res.injectedResData | +| guardedRouteHandler.js:16:10:16:28 | req.injectedReqData | guardedRouteHandler.js:16:10:16:28 | req.injectedReqData | +| guardedRouteHandler.js:20:10:20:28 | res.injectedResData | guardedRouteHandler.js:20:10:20:28 | res.injectedResData | | paramDecorator.ts:6:54:6:54 | x | paramDecorator.ts:7:10:7:10 | x | | test.js:5:30:5:37 | source() | test.js:5:8:5:38 | testlib ... urce()) | | test.js:6:22:6:29 | source() | test.js:6:8:6:30 | preserv ... urce()) | diff --git a/javascript/ql/test/library-tests/frameworks/data/test.ext.yml b/javascript/ql/test/library-tests/frameworks/data/test.ext.yml index b8e127397465..1ac621936a4a 100644 --- a/javascript/ql/test/library-tests/frameworks/data/test.ext.yml +++ b/javascript/ql/test/library-tests/frameworks/data/test.ext.yml @@ -13,6 +13,8 @@ extensions: - ['testlib', 'Member[getSourceArray].ReturnValue.ArrayElement', 'test-source'] - ['(testlib)', 'Member[parenthesizedPackageName].ReturnValue', 'test-source'] - ['danger-constant', 'Member[danger]', 'test-source'] + - ['testlib', 'Member[middleware].ReturnValue.GuardedRouteHandler.Parameter[0].Member[injectedReqData]', 'test-source'] + - ['testlib', 'Member[middleware].ReturnValue.GuardedRouteHandler.Parameter[1].Member[injectedResData]', 'test-source'] - addsTo: pack: codeql/javascript-all From 7de6a1e1c59341314589dc5fe1f564d236d9a192 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 27 Mar 2025 14:21:06 +0100 Subject: [PATCH 2/5] JS: Add documentation and example --- ...tomizing-library-models-for-javascript.rst | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/docs/codeql/codeql-language-guides/customizing-library-models-for-javascript.rst b/docs/codeql/codeql-language-guides/customizing-library-models-for-javascript.rst index b4a3446e942f..0693081abfd8 100644 --- a/docs/codeql/codeql-language-guides/customizing-library-models-for-javascript.rst +++ b/docs/codeql/codeql-language-guides/customizing-library-models-for-javascript.rst @@ -349,6 +349,48 @@ Note that this flow is already recognized by the CodeQL JS analysis, but for thi - The last column, **value**, indicates the kind of flow to add. The value **value** means the input value is unchanged as it flows to the output. + +Example: Modeling properties injected by a middleware function +-------------------------------------------------------------- + +In this example, we'll show how to model a hypothetical middleware function that adds a tainted value +on the incoming request objects: + +.. code-block:: js + + const express = require('express') + const app = express() + + app.use(require('@example/middleware').injectData()) + + app.get('/foo', (req, res) => { + req.data; // <-- mark 'req.data' as a taint source + }); + +This can be achieved with the following data extension: + +.. code-block:: yaml + + extensions: + - addsTo: + pack: codeql/javascript-all + extensible: sourceModel + data: + - [ + "@example/middleware", + "Member[injectData].ReturnValue.GuardedRouteHandler.Parameter[0].Member[data]", + "remote", + ] + +- Since we're adding a new taint source, we add a tuple to the **sourceModel** extensible predicate. +- The first column, **"@example/middleware"**, begins the search at imports of the hypothetical NPM package **@example/middleware**. +- **Member[injectData]** selects accesses to the **injectData** member. +- **ReturnValue** selects the return value of the call to **injectData**. +- **GuardedRouteHandler** interprets the current value as a middleware function and selects all route handlers guarded by that middleware. Since the current value is passd to **app.use()**, the callback subsequently passed to **app.get()** is seen as a guarded route handler. +- **Parameter[0]** selects the first parameter of the callback (the parameter named **req**). +- **Member[data]** selects accesses to the **data** property of the **req** object. +- Finally, the kind **remote** indicates that this is considered a source of remote flow. + Reference material ------------------ @@ -494,6 +536,11 @@ Components related to decorators: - **DecoratedParameter** selects a parameter that is decorated by the current value. - **DecoratedMember** selects a method, field, or accessor that is decorated by the current value. +Additionally there is a component related to middleware functions: + +- **GuardedRouteHandler** interprets the current value as a middleware function, and selects any route handler function that comes after it in the routing hierarchy. + This can be used to model properties injected onto request and response objects, such as **req.db** after a middleware that injects a database connection. + Additional notes about the syntax of operands: - Multiple operands may be given to a single component, as a shorthand for the union of the operands. For example, **Member[foo,bar]** matches the union of **Member[foo]** and **Member[bar]**. From e52bea630ac9987fa72e90d2d8174a7955f2c2d6 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 27 Mar 2025 14:27:00 +0100 Subject: [PATCH 3/5] JS: Add caveat about precision issue --- .../customizing-library-models-for-javascript.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/codeql/codeql-language-guides/customizing-library-models-for-javascript.rst b/docs/codeql/codeql-language-guides/customizing-library-models-for-javascript.rst index 0693081abfd8..fa2c1d4e8a82 100644 --- a/docs/codeql/codeql-language-guides/customizing-library-models-for-javascript.rst +++ b/docs/codeql/codeql-language-guides/customizing-library-models-for-javascript.rst @@ -540,6 +540,7 @@ Additionally there is a component related to middleware functions: - **GuardedRouteHandler** interprets the current value as a middleware function, and selects any route handler function that comes after it in the routing hierarchy. This can be used to model properties injected onto request and response objects, such as **req.db** after a middleware that injects a database connection. + Note that this currently over-approximates the set of route handlers but may be made more accurate in the future. Additional notes about the syntax of operands: From 2460874f4702e86fff1a55c524e0a1d48a4cb6ad Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 27 Mar 2025 20:13:27 +0100 Subject: [PATCH 4/5] JS: Add bogus model for testing --- .../ql/lib/ext/guarded-route-handler-stress-test.model.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 javascript/ql/lib/ext/guarded-route-handler-stress-test.model.yml diff --git a/javascript/ql/lib/ext/guarded-route-handler-stress-test.model.yml b/javascript/ql/lib/ext/guarded-route-handler-stress-test.model.yml new file mode 100644 index 000000000000..3c79088d1c11 --- /dev/null +++ b/javascript/ql/lib/ext/guarded-route-handler-stress-test.model.yml @@ -0,0 +1,6 @@ +extensions: + - addsTo: + pack: codeql/javascript-all + extensible: sourceModel + data: + - ["passport", "AnyMember.ReturnValue.GuardedRouteHandler.Parameter[0].Member[user]", "remote"] From 951b48adfe947510d9c600c9a6c34415c8c18a82 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 28 Mar 2025 09:24:49 +0100 Subject: [PATCH 5/5] Revert "JS: Add bogus model for testing" This reverts commit 2460874f4702e86fff1a55c524e0a1d48a4cb6ad. --- .../ql/lib/ext/guarded-route-handler-stress-test.model.yml | 6 ------ 1 file changed, 6 deletions(-) delete mode 100644 javascript/ql/lib/ext/guarded-route-handler-stress-test.model.yml diff --git a/javascript/ql/lib/ext/guarded-route-handler-stress-test.model.yml b/javascript/ql/lib/ext/guarded-route-handler-stress-test.model.yml deleted file mode 100644 index 3c79088d1c11..000000000000 --- a/javascript/ql/lib/ext/guarded-route-handler-stress-test.model.yml +++ /dev/null @@ -1,6 +0,0 @@ -extensions: - - addsTo: - pack: codeql/javascript-all - extensible: sourceModel - data: - - ["passport", "AnyMember.ReturnValue.GuardedRouteHandler.Parameter[0].Member[user]", "remote"]