From 9f07b1011df53af62c604bd0ba20a38c472dadbd Mon Sep 17 00:00:00 2001 From: Asger F Date: Mon, 1 Oct 2018 12:34:13 +0100 Subject: [PATCH] JS: bugfix in server-side redirect query --- change-notes/1.19/analysis-javascript.md | 1 + .../security/dataflow/ServerSideUrlRedirect.qll | 10 +++++----- .../ServerSideUrlRedirect.expected | 1 + .../Security/CWE-601/ServerSideUrlRedirect/express.js | 9 +++++++++ 4 files changed, 16 insertions(+), 5 deletions(-) diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index 47bf050789ba..fef21ffb2601 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -26,5 +26,6 @@ | Unbound event handler receiver | Fewer false-positive results | This rule now recognizes additional ways class methods can be bound. | | Remote property injection | Fewer results | The precision of this rule has been revised to "medium". Results are no longer shown on LGTM by default. | | Missing CSRF middleware | Fewer false-positive results | This rule now recognizes additional CSRF protection middlewares. | +| Server-side URL redirect | More results | This rule now recognizes redirection calls in more cases. | ## Changes to QL libraries diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirect.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirect.qll index 7836d2c594ea..bf7d352c4ba2 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirect.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirect.qll @@ -21,10 +21,10 @@ module ServerSideUrlRedirect { * Holds if this sink may redirect to a non-local URL. */ predicate maybeNonLocal() { - exists (Expr prefix | prefix = getAPrefix(this) | - not exists(prefix.getStringValue()) + exists (DataFlow::Node prefix | prefix = getAPrefix(this) | + not exists(prefix.asExpr().getStringValue()) or - exists (string prefixVal | prefixVal = prefix.getStringValue() | + exists (string prefixVal | prefixVal = prefix.asExpr().getStringValue() | // local URLs (i.e., URLs that start with `/` not followed by `\` or `/`, // or that start with `~/`) are unproblematic not prefixVal.regexpMatch("/[^\\\\/].*|~/.*") and @@ -47,12 +47,12 @@ module ServerSideUrlRedirect { /** * Gets an expression that may end up being a prefix of the string concatenation `nd`. */ - private Expr getAPrefix(Sink sink) { + private DataFlow::Node getAPrefix(Sink sink) { exists (DataFlow::Node prefix | prefix = prefixCandidate(sink) and not exists(StringConcatenation::getFirstOperand(prefix)) and not exists(prefix.getAPredecessor()) and - result = prefix.asExpr() + result = prefix ) } 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 fef8354768b5..56e5c781d631 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 @@ -3,6 +3,7 @@ | express.js:33:18:33:23 | target | Untrusted URL redirection due to $@. | express.js:27:16:27:34 | req.param("target") | user-provided value | | express.js:35:16:35:21 | target | Untrusted URL redirection due to $@. | express.js:27:16:27:34 | req.param("target") | user-provided value | | express.js:44:16:44:108 | (req.pa ... ntacts" | Untrusted URL redirection due to $@. | express.js:44:69:44:87 | req.param('action') | user-provided value | +| express.js:53:26:53:28 | url | Untrusted URL redirection due to $@. | express.js:48:16:48:28 | req.params[0] | user-provided value | | express.js:78:16:78:43 | `${req. ... )}/foo` | Untrusted URL redirection due to $@. | express.js:78:19:78:37 | req.param("target") | user-provided value | | express.js:94:18:94:23 | target | Untrusted URL redirection due to $@. | express.js:87:16:87:34 | req.param("target") | user-provided value | | express.js:101:16:101:21 | target | Untrusted URL redirection due to $@. | express.js:87:16:87:34 | req.param("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 2a62c7a9e5c0..1d61fd5f92b0 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 @@ -121,3 +121,12 @@ app.get('/array/join', function(req, res) { // BAD: request input becomes before query string res.redirect([req.query.page, '?section=', req.query.section].join('')); }); + +function sendUserToUrl(res, nextUrl) { + // BAD: value comes from query parameter + res.redrect(nextUrl); +} + +app.get('/call', function(req, res) { + sendUserToUrl(res, req.query.nextUrl); +});