From a72436f6f1b7900645b1c5aa700f762fc7551351 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Wed, 15 Mar 2023 10:14:31 +0100 Subject: [PATCH 1/2] recognize more express URL related sources --- .../lib/semmle/javascript/frameworks/Express.qll | 16 +++++++++++++++- .../ServerSideUrlRedirect.expected | 14 ++++++++++++++ .../CWE-601/ServerSideUrlRedirect/express.js | 5 +++++ 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Express.qll b/javascript/ql/lib/semmle/javascript/frameworks/Express.qll index 24ffde04cd9..24f80146019 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/Express.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/Express.qll @@ -677,7 +677,21 @@ module Express { RequestInputAccess() { kind = "parameter" and - this = [queryRef(request), paramsRef(request)].getAPropertyRead() + ( + // `req.query` / `req.params`. + // These are objects, so we prefer to use a property read if possible, otherwise we fall back to the object itself. + ( + if exists(queryRef(request).getAPropertyRead()) + then this = queryRef(request).getAPropertyRead() + else this = queryRef(request) + ) + or + ( + if exists(paramsRef(request).getAPropertyRead()) + then this = paramsRef(request).getAPropertyRead() + else this = paramsRef(request) + ) + ) or exists(DataFlow::SourceNode ref | ref = request.ref() | kind = "parameter" and diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected index 50f1ef72252..e0ef0bfaae8 100644 --- a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected +++ b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected @@ -57,6 +57,13 @@ nodes | express.js:155:18:155:23 | target | | express.js:160:18:160:23 | target | | express.js:160:18:160:23 | target | +| express.js:164:7:164:54 | myThing | +| express.js:164:17:164:41 | JSON.st ... .query) | +| express.js:164:17:164:54 | JSON.st ... (1, -1) | +| express.js:164:32:164:40 | req.query | +| express.js:164:32:164:40 | req.query | +| express.js:165:16:165:22 | myThing | +| express.js:165:16:165:22 | myThing | | koa.js:6:6:6:27 | url | | koa.js:6:12:6:27 | ctx.query.target | | koa.js:6:12:6:27 | ctx.query.target | @@ -153,6 +160,12 @@ edges | express.js:150:7:150:34 | target | express.js:160:18:160:23 | target | | express.js:150:16:150:34 | req.param("target") | express.js:150:7:150:34 | target | | express.js:150:16:150:34 | req.param("target") | express.js:150:7:150:34 | target | +| express.js:164:7:164:54 | myThing | express.js:165:16:165:22 | myThing | +| express.js:164:7:164:54 | myThing | express.js:165:16:165:22 | myThing | +| express.js:164:17:164:41 | JSON.st ... .query) | express.js:164:17:164:54 | JSON.st ... (1, -1) | +| express.js:164:17:164:54 | JSON.st ... (1, -1) | express.js:164:7:164:54 | myThing | +| express.js:164:32:164:40 | req.query | express.js:164:17:164:41 | JSON.st ... .query) | +| express.js:164:32:164:40 | req.query | express.js:164:17:164:41 | JSON.st ... .query) | | koa.js:6:6:6:27 | url | koa.js:7:15:7:17 | url | | koa.js:6:6:6:27 | url | koa.js:7:15:7:17 | url | | koa.js:6:6:6:27 | url | koa.js:8:18:8:20 | url | @@ -214,6 +227,7 @@ edges | express.js:146:16:146:24 | query.foo | express.js:146:16:146:24 | query.foo | express.js:146:16:146:24 | query.foo | Untrusted URL redirection depends on a $@. | express.js:146:16:146:24 | query.foo | user-provided value | | express.js:155:18:155:23 | target | express.js:150:16:150:34 | req.param("target") | express.js:155:18:155:23 | target | Untrusted URL redirection depends on a $@. | express.js:150:16:150:34 | req.param("target") | user-provided value | | express.js:160:18:160:23 | target | express.js:150:16:150:34 | req.param("target") | express.js:160:18:160:23 | target | Untrusted URL redirection depends on a $@. | express.js:150:16:150:34 | req.param("target") | user-provided value | +| express.js:165:16:165:22 | myThing | express.js:164:32:164:40 | req.query | express.js:165:16:165:22 | myThing | Untrusted URL redirection depends on a $@. | express.js:164:32:164:40 | req.query | user-provided value | | koa.js:7:15:7:17 | url | koa.js:6:12:6:27 | ctx.query.target | koa.js:7:15:7:17 | url | Untrusted URL redirection depends on a $@. | koa.js:6:12:6:27 | ctx.query.target | user-provided value | | koa.js:8:15:8:26 | `${url}${x}` | koa.js:6:12:6:27 | ctx.query.target | koa.js:8:15:8:26 | `${url}${x}` | Untrusted URL redirection depends on a $@. | koa.js:6:12:6:27 | ctx.query.target | user-provided value | | koa.js:14:16:14:18 | url | koa.js:6:12:6:27 | ctx.query.target | koa.js:14:16:14:18 | url | Untrusted URL redirection depends on a $@. | koa.js:6:12:6:27 | ctx.query.target | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/express.js b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/express.js index 667d6f89f05..b12ef66622b 100644 --- a/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/express.js +++ b/javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/express.js @@ -159,3 +159,8 @@ app.get('/some/path', function(req, res) { else res.redirect(target); // NOT OK }); + +app.get("/foo/:bar/:baz", (req, res) => { + let myThing = JSON.stringify(req.query).slice(1, -1); + res.redirect(myThing); // NOT OK +}); From f718d78a9a73fffd5ff34d7e9ad03b4862bb3ec2 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Thu, 16 Mar 2023 13:34:01 +0100 Subject: [PATCH 2/2] avoid redundant sources --- javascript/ql/lib/semmle/javascript/frameworks/Express.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Express.qll b/javascript/ql/lib/semmle/javascript/frameworks/Express.qll index 24f80146019..2391ef89a35 100644 --- a/javascript/ql/lib/semmle/javascript/frameworks/Express.qll +++ b/javascript/ql/lib/semmle/javascript/frameworks/Express.qll @@ -683,13 +683,13 @@ module Express { ( if exists(queryRef(request).getAPropertyRead()) then this = queryRef(request).getAPropertyRead() - else this = queryRef(request) + else this = request.ref().getAPropertyRead("query") ) or ( if exists(paramsRef(request).getAPropertyRead()) then this = paramsRef(request).getAPropertyRead() - else this = paramsRef(request) + else this = request.ref().getAPropertyRead("params") ) ) or