diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index ee3d05e7bb1b..63679cb0174c 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -49,6 +49,8 @@ | Unused variable, import, function or class | Fewer results | This rule now flags import statements with multiple unused imports once. | | Useless assignment to local variable | Fewer false-positive results | This rule now recognizes additional ways default values can be set. | | Whitespace contradicts operator precedence | Fewer false-positive results | This rule no longer flags operators with asymmetric whitespace. | +| Client-side URL redirect | Fewer false-positive results | This rule now recognizes safe redirects in more cases. | +| Server-side URL redirect | Fewer false-positive results | This rule now recognizes safe redirects in more cases. | ## Changes to QL libraries diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll index dc331b7d2a82..8cdc442edaea 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll @@ -58,7 +58,7 @@ module ClientSideUrlRedirect { } override predicate isSanitizer(DataFlow::Node source, DataFlow::Node sink) { - sanitizingPrefixEdge(source, sink) + hostnameSanitizingPrefixEdge(source, sink) } override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel f, DataFlow::FlowLabel g) { diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirect.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirect.qll index dcf3f0a81ca0..c2c4f21ad9e0 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirect.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirect.qll @@ -16,46 +16,8 @@ module ServerSideUrlRedirect { /** * A data flow sink for unvalidated URL redirect vulnerabilities. */ - abstract class Sink extends DataFlow::Node { - /** - * Holds if this sink may redirect to a non-local URL. - */ - predicate maybeNonLocal() { - exists (DataFlow::Node prefix | prefix = getAPrefix(this) | - not exists(prefix.asExpr().getStringValue()) - or - 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 - // so are localhost URLs - not prefixVal.regexpMatch("(\\w+:)?//localhost[:/].*") - ) - ) - } - } + abstract class Sink extends DataFlow::Node { } - /** - * Gets a node that is transitively reachable from `nd` along prefix predecessor edges. - */ - private DataFlow::Node prefixCandidate(Sink sink) { - result = sink or - result = prefixCandidate(sink).getAPredecessor() or - result = StringConcatenation::getFirstOperand(prefixCandidate(sink)) - } - - /** - * Gets an expression that may end up being a prefix of the string concatenation `nd`. - */ - 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 - ) - } - /** * A sanitizer for unvalidated URL redirect vulnerabilities. */ @@ -72,7 +34,7 @@ module ServerSideUrlRedirect { } override predicate isSink(DataFlow::Node sink) { - sink.(Sink).maybeNonLocal() + sink instanceof Sink } override predicate isSanitizer(DataFlow::Node node) { @@ -81,7 +43,7 @@ module ServerSideUrlRedirect { } override predicate isSanitizer(DataFlow::Node source, DataFlow::Node sink) { - sanitizingPrefixEdge(source, sink) + hostnameSanitizingPrefixEdge(source, sink) } override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) { diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/UrlConcatenation.qll b/javascript/ql/src/semmle/javascript/security/dataflow/UrlConcatenation.qll index 5d12531128ab..970e2ee22300 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/UrlConcatenation.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/UrlConcatenation.qll @@ -7,8 +7,10 @@ import javascript /** - * Holds if a string value containing `?` or `#` may flow into - * `nd` or one of its operands, assuming that it is a concatenation. + * Holds if the string value of `nd` prevents anything appended after it + * from affecting the hostname or path of a URL. + * + * Specifically, this holds if the string contains `?` or `#`. */ private predicate hasSanitizingSubstring(DataFlow::Node nd) { nd.asExpr().getStringValue().regexpMatch(".*[?#].*") @@ -21,8 +23,8 @@ private predicate hasSanitizingSubstring(DataFlow::Node nd) { } /** - * Holds if data that flows from `source` to `sink` may have a string - * containing the character `?` or `#` prepended to it. + * Holds if data that flows from `source` to `sink` cannot affect the + * path or earlier part of the resulting string when interpreted as a URL. * * This is considered as a sanitizing edge for the URL redirection queries. */ @@ -31,3 +33,37 @@ predicate sanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) { StringConcatenation::taintStep(source, sink, operator, n) and hasSanitizingSubstring(StringConcatenation::getOperand(operator, [0..n-1]))) } + +/** + * Holds if the string value of `nd` prevents anything appended after it + * from affecting the hostname of a URL. + * + * Specifically, this holds if the string contains any of the following: + * - `?` (any suffix becomes part of query) + * - `#` (any suffix becomes part of fragment) + * - `/` or `\`, immediately prefixed by a character other than `:`, `/`, or `\` (any suffix becomes part of the path) + * + * In the latter case, the additional prefix check is necessary to avoid a `/` that could be interpreted as + * the `//` separating the (optional) scheme from the hostname. + */ +private predicate hasHostnameSanitizingSubstring(DataFlow::Node nd) { + nd.asExpr().getStringValue().regexpMatch(".*([?#]|[^?#:/\\\\][/\\\\]).*") + or + hasHostnameSanitizingSubstring(StringConcatenation::getAnOperand(nd)) + or + hasHostnameSanitizingSubstring(nd.getAPredecessor()) + or + nd.isIncomplete(_) +} + +/** + * Holds if data that flows from `source` to `sink` cannot affect the + * hostname or scheme of the resulting string when interpreted as a URL. + * + * This is considered as a sanitizing edge for the URL redirection queries. + */ +predicate hostnameSanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) { + exists (DataFlow::Node operator, int n | + StringConcatenation::taintStep(source, sink, operator, n) and + hasHostnameSanitizingSubstring(StringConcatenation::getOperand(operator, [0..n-1]))) +} \ No newline at end of file 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 2d57e091ba27..a2711a2cdf04 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 @@ -7,14 +7,6 @@ nodes | express.js:35:16:35:21 | target | | express.js:40:16:40:108 | (req.pa ... ntacts" | | express.js:40:69:40:87 | req.param('action') | -| express.js:44:7:44:28 | handle | -| express.js:44:16:44:28 | req.params[0] | -| express.js:45:7:45:33 | url | -| express.js:45:13:45:27 | "/Me/" + handle | -| express.js:45:13:45:33 | "/Me/" ... e + "/" | -| express.js:45:22:45:27 | handle | -| express.js:49:3:49:3 | url | -| express.js:49:26:49:28 | url | | express.js:74:16:74:43 | `${req. ... )}/foo` | | express.js:74:19:74:37 | req.param("target") | | express.js:83:7:83:34 | target | @@ -24,6 +16,18 @@ nodes | express.js:118:16:118:63 | [req.qu ... ection] | | express.js:118:16:118:72 | [req.qu ... oin('') | | express.js:118:17:118:30 | req.query.page | +| express.js:134:16:134:36 | '/' + r ... ms.user | +| express.js:134:22:134:36 | req.params.user | +| express.js:135:16:135:37 | '//' + ... ms.user | +| express.js:135:23:135:37 | req.params.user | +| express.js:136:16:136:36 | 'u' + r ... ms.user | +| express.js:136:22:136:36 | req.params.user | +| express.js:138:16:138:45 | '/' + ( ... s.user) | +| express.js:138:22:138:45 | ('/u' + ... s.user) | +| express.js:138:23:138:44 | '/u' + ... ms.user | +| express.js:138:30:138:44 | req.params.user | +| express.js:139:16:139:37 | '/u' + ... ms.user | +| express.js:139:23:139:37 | req.params.user | | node.js:6:7:6:52 | target | | node.js:6:16:6:39 | url.par ... , true) | | node.js:6:16:6:45 | url.par ... ).query | @@ -54,19 +58,19 @@ edges | express.js:27:7:27:34 | target | express.js:35:16:35:21 | target | | express.js:27:16:27:34 | req.param("target") | express.js:27:7:27:34 | target | | express.js:40:69:40:87 | req.param('action') | express.js:40:16:40:108 | (req.pa ... ntacts" | -| express.js:44:7:44:28 | handle | express.js:45:22:45:27 | handle | -| express.js:44:16:44:28 | req.params[0] | express.js:44:7:44:28 | handle | -| express.js:45:7:45:33 | url | express.js:49:3:49:3 | url | -| express.js:45:13:45:27 | "/Me/" + handle | express.js:45:13:45:33 | "/Me/" ... e + "/" | -| express.js:45:13:45:33 | "/Me/" ... e + "/" | express.js:45:7:45:33 | url | -| express.js:45:22:45:27 | handle | express.js:45:13:45:27 | "/Me/" + handle | -| express.js:49:3:49:3 | url | express.js:49:26:49:28 | url | | express.js:74:19:74:37 | req.param("target") | express.js:74:16:74:43 | `${req. ... )}/foo` | | express.js:83:7:83:34 | target | express.js:90:18:90:23 | target | | express.js:83:7:83:34 | target | express.js:97:16:97:21 | target | | express.js:83:16:83:34 | req.param("target") | express.js:83:7:83:34 | target | | express.js:118:16:118:63 | [req.qu ... ection] | express.js:118:16:118:72 | [req.qu ... oin('') | | express.js:118:17:118:30 | req.query.page | express.js:118:16:118:63 | [req.qu ... ection] | +| express.js:134:22:134:36 | req.params.user | express.js:134:16:134:36 | '/' + r ... ms.user | +| express.js:135:23:135:37 | req.params.user | express.js:135:16:135:37 | '//' + ... ms.user | +| express.js:136:22:136:36 | req.params.user | express.js:136:16:136:36 | 'u' + r ... ms.user | +| express.js:138:22:138:45 | ('/u' + ... s.user) | express.js:138:16:138:45 | '/' + ( ... s.user) | +| express.js:138:23:138:44 | '/u' + ... ms.user | express.js:138:22:138:45 | ('/u' + ... s.user) | +| express.js:138:30:138:44 | req.params.user | express.js:138:23:138:44 | '/u' + ... ms.user | +| express.js:139:23:139:37 | req.params.user | express.js:139:16:139:37 | '/u' + ... ms.user | | node.js:6:7:6:52 | target | node.js:7:34:7:39 | target | | node.js:6:16:6:39 | url.par ... , true) | node.js:6:16:6:45 | url.par ... ).query | | node.js:6:16:6:45 | url.par ... ).query | node.js:6:16:6:52 | url.par ... .target | @@ -94,11 +98,15 @@ edges | express.js:33:18:33:23 | target | express.js:27:16:27:34 | req.param("target") | 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 | express.js:27:16:27:34 | req.param("target") | 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:40:16:40:108 | (req.pa ... ntacts" | express.js:40:69:40:87 | req.param('action') | express.js:40:16:40:108 | (req.pa ... ntacts" | Untrusted URL redirection due to $@. | express.js:40:69:40:87 | req.param('action') | user-provided value | -| express.js:49:26:49:28 | url | express.js:44:16:44:28 | req.params[0] | express.js:49:26:49:28 | url | Untrusted URL redirection due to $@. | express.js:44:16:44:28 | req.params[0] | user-provided value | | express.js:74:16:74:43 | `${req. ... )}/foo` | express.js:74:19:74:37 | req.param("target") | express.js:74:16:74:43 | `${req. ... )}/foo` | Untrusted URL redirection due to $@. | express.js:74:19:74:37 | req.param("target") | user-provided value | | express.js:90:18:90:23 | target | express.js:83:16:83:34 | req.param("target") | express.js:90:18:90:23 | target | Untrusted URL redirection due to $@. | express.js:83:16:83:34 | req.param("target") | user-provided value | | express.js:97:16:97:21 | target | express.js:83:16:83:34 | req.param("target") | express.js:97:16:97:21 | target | Untrusted URL redirection due to $@. | express.js:83:16:83:34 | req.param("target") | user-provided value | | express.js:118:16:118:72 | [req.qu ... oin('') | express.js:118:17:118:30 | req.query.page | express.js:118:16:118:72 | [req.qu ... oin('') | Untrusted URL redirection due to $@. | express.js:118:17:118:30 | req.query.page | user-provided value | +| express.js:134:16:134:36 | '/' + r ... ms.user | express.js:134:22:134:36 | req.params.user | express.js:134:16:134:36 | '/' + r ... ms.user | Untrusted URL redirection due to $@. | express.js:134:22:134:36 | req.params.user | user-provided value | +| express.js:135:16:135:37 | '//' + ... ms.user | express.js:135:23:135:37 | req.params.user | express.js:135:16:135:37 | '//' + ... ms.user | Untrusted URL redirection due to $@. | express.js:135:23:135:37 | req.params.user | user-provided value | +| express.js:136:16:136:36 | 'u' + r ... ms.user | express.js:136:22:136:36 | req.params.user | express.js:136:16:136:36 | 'u' + r ... ms.user | Untrusted URL redirection due to $@. | express.js:136:22:136:36 | req.params.user | user-provided value | +| express.js:138:16:138:45 | '/' + ( ... s.user) | express.js:138:30:138:44 | req.params.user | express.js:138:16:138:45 | '/' + ( ... s.user) | Untrusted URL redirection due to $@. | express.js:138:30:138:44 | req.params.user | user-provided value | +| express.js:139:16:139:37 | '/u' + ... ms.user | express.js:139:23:139:37 | req.params.user | express.js:139:16:139:37 | '/u' + ... ms.user | Untrusted URL redirection due to $@. | express.js:139:23:139:37 | req.params.user | user-provided value | | node.js:7:34:7:39 | target | node.js:6:26:6:32 | req.url | node.js:7:34:7:39 | target | Untrusted URL redirection due to $@. | node.js:6:26:6:32 | req.url | user-provided value | | node.js:15:34:15:45 | '/' + target | node.js:11:26:11:32 | req.url | node.js:15:34:15:45 | '/' + target | Untrusted URL redirection due to $@. | node.js:11:26:11:32 | req.url | user-provided value | | node.js:32:34:32:55 | target ... =" + me | node.js:29:26:29:32 | req.url | node.js:32:34:32:55 | target ... =" + me | Untrusted URL redirection due to $@. | node.js:29:26:29:32 | req.url | 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 37a1370698ce..36276d65c73b 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 @@ -126,3 +126,15 @@ function sendUserToUrl(res, nextUrl) { app.get('/call', function(req, res) { sendUserToUrl(res, req.query.nextUrl); }); + +app.get('/redirect/:user', function(req, res) { + res.redirect('/users/' + req.params.user); // GOOD + res.redirect('users/' + req.params.user); // GOOD + + res.redirect('/' + req.params.user); // BAD - could go to //evil.com + res.redirect('//' + req.params.user); // BAD - could go to //evil.com + res.redirect('u' + req.params.user); // BAD - could go to u.evil.com + + res.redirect('/' + ('/u' + req.params.user)); // BAD - could go to //u.evil.com + res.redirect('/u' + req.params.user); // GOOD - but flagged anyway +});