hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-30 19:26:02 +02:00
Code Issues Packages Projects Releases Wiki Activity

JS: recognize sanitizing slashes in URL redirection queries

Browse Source
This commit is contained in:
Asger F
2018-11-08 11:02:46 +00:00
parent 0647743333
commit 6ec13feab4
4 changed files with 52 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files

42
javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirect.qll
View File

@@ -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) {

20
javascript/ql/src/semmle/javascript/security/dataflow/UrlConcatenation.qll
View File

@@ -7,11 +7,19 @@
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 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 hasSanitizingSubstring(DataFlow::Node nd) {
nd.asExpr().getStringValue().regexpMatch(".*[?#].*")
predicate hasSanitizingSubstring(DataFlow::Node nd) {
nd.asExpr().getStringValue().regexpMatch(".*([?#]|[^?#:/\\\\][/\\\\]).*")
or
hasSanitizingSubstring(StringConcatenation::getAnOperand(nd))
or
@@ -21,8 +29,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
* hostname of the resulting string when interpreted as a URL.
*
* This is considered as a sanitizing edge for the URL redirection queries.
*/

40
javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected
View File

@@ -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 |

12
javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/express.js
View File

@@ -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
});