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..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 @@ -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,12 @@ 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. + 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: - 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]**. 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