Skip to content

Commit 9e4aeb3

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #436 from asger-semmle/url-concat
Approved by xiemaisi
2 parents 328c86c + c06c9a0 commit 9e4aeb3

File tree

6 files changed

+82
-62
lines changed

6 files changed

+82
-62
lines changed

‎change-notes/1.19/analysis-javascript.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,8 @@
4949
| Unused variable, import, function or class | Fewer results | This rule now flags import statements with multiple unused imports once. |
5050
| Useless assignment to local variable | Fewer false-positive results | This rule now recognizes additional ways default values can be set. |
5151
| Whitespace contradicts operator precedence | Fewer false-positive results | This rule no longer flags operators with asymmetric whitespace. |
52+
| Client-side URL redirect | Fewer false-positive results | This rule now recognizes safe redirects in more cases. |
53+
| Server-side URL redirect | Fewer false-positive results | This rule now recognizes safe redirects in more cases. |
5254

5355
## Changes to QL libraries
5456

‎javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ module ClientSideUrlRedirect {
5858
}
5959

6060
override predicate isSanitizer(DataFlow::Node source, DataFlow::Node sink) {
61-
sanitizingPrefixEdge(source, sink)
61+
hostnameSanitizingPrefixEdge(source, sink)
6262
}
6363

6464
override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel f, DataFlow::FlowLabel g) {

‎javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirect.qll‎

Lines changed: 3 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -16,46 +16,8 @@ module ServerSideUrlRedirect {
1616
/**
1717
* A data flow sink for unvalidated URL redirect vulnerabilities.
1818
*/
19-
abstract class Sink extends DataFlow::Node {
20-
/**
21-
* Holds if this sink may redirect to a non-local URL.
22-
*/
23-
predicate maybeNonLocal() {
24-
exists (DataFlow::Node prefix | prefix = getAPrefix(this) |
25-
not exists(prefix.asExpr().getStringValue())
26-
or
27-
exists (string prefixVal | prefixVal = prefix.asExpr().getStringValue() |
28-
// local URLs (i.e., URLs that start with `/` not followed by `\` or `/`,
29-
// or that start with `~/`) are unproblematic
30-
not prefixVal.regexpMatch("/[^\\\\/].*|~/.*") and
31-
// so are localhost URLs
32-
not prefixVal.regexpMatch("(\\w+:)?//localhost[:/].*")
33-
)
34-
)
35-
}
36-
}
19+
abstract class Sink extends DataFlow::Node { }
3720

38-
/**
39-
* Gets a node that is transitively reachable from `nd` along prefix predecessor edges.
40-
*/
41-
private DataFlow::Node prefixCandidate(Sink sink) {
42-
result = sink or
43-
result = prefixCandidate(sink).getAPredecessor() or
44-
result = StringConcatenation::getFirstOperand(prefixCandidate(sink))
45-
}
46-
47-
/**
48-
* Gets an expression that may end up being a prefix of the string concatenation `nd`.
49-
*/
50-
private DataFlow::Node getAPrefix(Sink sink) {
51-
exists (DataFlow::Node prefix |
52-
prefix = prefixCandidate(sink) and
53-
not exists(StringConcatenation::getFirstOperand(prefix)) and
54-
not exists(prefix.getAPredecessor()) and
55-
result = prefix
56-
)
57-
}
58-
5921
/**
6022
* A sanitizer for unvalidated URL redirect vulnerabilities.
6123
*/
@@ -72,7 +34,7 @@ module ServerSideUrlRedirect {
7234
}
7335

7436
override predicate isSink(DataFlow::Node sink) {
75-
sink.(Sink).maybeNonLocal()
37+
sink instanceof Sink
7638
}
7739

7840
override predicate isSanitizer(DataFlow::Node node) {
@@ -81,7 +43,7 @@ module ServerSideUrlRedirect {
8143
}
8244

8345
override predicate isSanitizer(DataFlow::Node source, DataFlow::Node sink) {
84-
sanitizingPrefixEdge(source, sink)
46+
hostnameSanitizingPrefixEdge(source, sink)
8547
}
8648

8749
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {

‎javascript/ql/src/semmle/javascript/security/dataflow/UrlConcatenation.qll‎

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,10 @@
77
import javascript
88

99
/**
10-
* Holds if a string value containing `?` or `#` may flow into
11-
* `nd` or one of its operands, assuming that it is a concatenation.
10+
* Holds if the string value of `nd` prevents anything appended after it
11+
* from affecting the hostname or path of a URL.
12+
*
13+
* Specifically, this holds if the string contains `?` or `#`.
1214
*/
1315
private predicate hasSanitizingSubstring(DataFlow::Node nd) {
1416
nd.asExpr().getStringValue().regexpMatch(".*[?#].*")
@@ -21,8 +23,8 @@ private predicate hasSanitizingSubstring(DataFlow::Node nd) {
2123
}
2224

2325
/**
24-
* Holds if data that flows from `source` to `sink` may have a string
25-
* containing the character `?` or `#` prepended to it.
26+
* Holds if data that flows from `source` to `sink` cannot affect the
27+
* path or earlier part of the resulting string when interpreted as a URL.
2628
*
2729
* This is considered as a sanitizing edge for the URL redirection queries.
2830
*/
@@ -31,3 +33,37 @@ predicate sanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) {
3133
StringConcatenation::taintStep(source, sink, operator, n) and
3234
hasSanitizingSubstring(StringConcatenation::getOperand(operator, [0..n-1])))
3335
}
36+
37+
/**
38+
* Holds if the string value of `nd` prevents anything appended after it
39+
* from affecting the hostname of a URL.
40+
*
41+
* Specifically, this holds if the string contains any of the following:
42+
* - `?` (any suffix becomes part of query)
43+
* - `#` (any suffix becomes part of fragment)
44+
* - `/` or `\`, immediately prefixed by a character other than `:`, `/`, or `\` (any suffix becomes part of the path)
45+
*
46+
* In the latter case, the additional prefix check is necessary to avoid a `/` that could be interpreted as
47+
* the `//` separating the (optional) scheme from the hostname.
48+
*/
49+
private predicate hasHostnameSanitizingSubstring(DataFlow::Node nd) {
50+
nd.asExpr().getStringValue().regexpMatch(".*([?#]|[^?#:/\\\\][/\\\\]).*")
51+
or
52+
hasHostnameSanitizingSubstring(StringConcatenation::getAnOperand(nd))
53+
or
54+
hasHostnameSanitizingSubstring(nd.getAPredecessor())
55+
or
56+
nd.isIncomplete(_)
57+
}
58+
59+
/**
60+
* Holds if data that flows from `source` to `sink` cannot affect the
61+
* hostname or scheme of the resulting string when interpreted as a URL.
62+
*
63+
* This is considered as a sanitizing edge for the URL redirection queries.
64+
*/
65+
predicate hostnameSanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) {
66+
exists (DataFlow::Node operator, int n |
67+
StringConcatenation::taintStep(source, sink, operator, n) and
68+
hasHostnameSanitizingSubstring(StringConcatenation::getOperand(operator, [0..n-1])))
69+
}

‎javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/ServerSideUrlRedirect.expected‎

Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,6 @@ nodes
77
| express.js:35:16:35:21 | target |
88
| express.js:40:16:40:108 | (req.pa ... ntacts" |
99
| express.js:40:69:40:87 | req.param('action') |
10-
| express.js:44:7:44:28 | handle |
11-
| express.js:44:16:44:28 | req.params[0] |
12-
| express.js:45:7:45:33 | url |
13-
| express.js:45:13:45:27 | "/Me/" + handle |
14-
| express.js:45:13:45:33 | "/Me/" ... e + "/" |
15-
| express.js:45:22:45:27 | handle |
16-
| express.js:49:3:49:3 | url |
17-
| express.js:49:26:49:28 | url |
1810
| express.js:74:16:74:43 | `${req. ... )}/foo` |
1911
| express.js:74:19:74:37 | req.param("target") |
2012
| express.js:83:7:83:34 | target |
@@ -24,6 +16,18 @@ nodes
2416
| express.js:118:16:118:63 | [req.qu ... ection] |
2517
| express.js:118:16:118:72 | [req.qu ... oin('') |
2618
| express.js:118:17:118:30 | req.query.page |
19+
| express.js:134:16:134:36 | '/' + r ... ms.user |
20+
| express.js:134:22:134:36 | req.params.user |
21+
| express.js:135:16:135:37 | '//' + ... ms.user |
22+
| express.js:135:23:135:37 | req.params.user |
23+
| express.js:136:16:136:36 | 'u' + r ... ms.user |
24+
| express.js:136:22:136:36 | req.params.user |
25+
| express.js:138:16:138:45 | '/' + ( ... s.user) |
26+
| express.js:138:22:138:45 | ('/u' + ... s.user) |
27+
| express.js:138:23:138:44 | '/u' + ... ms.user |
28+
| express.js:138:30:138:44 | req.params.user |
29+
| express.js:139:16:139:37 | '/u' + ... ms.user |
30+
| express.js:139:23:139:37 | req.params.user |
2731
| node.js:6:7:6:52 | target |
2832
| node.js:6:16:6:39 | url.par ... , true) |
2933
| node.js:6:16:6:45 | url.par ... ).query |
@@ -54,19 +58,19 @@ edges
5458
| express.js:27:7:27:34 | target | express.js:35:16:35:21 | target |
5559
| express.js:27:16:27:34 | req.param("target") | express.js:27:7:27:34 | target |
5660
| express.js:40:69:40:87 | req.param('action') | express.js:40:16:40:108 | (req.pa ... ntacts" |
57-
| express.js:44:7:44:28 | handle | express.js:45:22:45:27 | handle |
58-
| express.js:44:16:44:28 | req.params[0] | express.js:44:7:44:28 | handle |
59-
| express.js:45:7:45:33 | url | express.js:49:3:49:3 | url |
60-
| express.js:45:13:45:27 | "/Me/" + handle | express.js:45:13:45:33 | "/Me/" ... e + "/" |
61-
| express.js:45:13:45:33 | "/Me/" ... e + "/" | express.js:45:7:45:33 | url |
62-
| express.js:45:22:45:27 | handle | express.js:45:13:45:27 | "/Me/" + handle |
63-
| express.js:49:3:49:3 | url | express.js:49:26:49:28 | url |
6461
| express.js:74:19:74:37 | req.param("target") | express.js:74:16:74:43 | `${req. ... )}/foo` |
6562
| express.js:83:7:83:34 | target | express.js:90:18:90:23 | target |
6663
| express.js:83:7:83:34 | target | express.js:97:16:97:21 | target |
6764
| express.js:83:16:83:34 | req.param("target") | express.js:83:7:83:34 | target |
6865
| express.js:118:16:118:63 | [req.qu ... ection] | express.js:118:16:118:72 | [req.qu ... oin('') |
6966
| express.js:118:17:118:30 | req.query.page | express.js:118:16:118:63 | [req.qu ... ection] |
67+
| express.js:134:22:134:36 | req.params.user | express.js:134:16:134:36 | '/' + r ... ms.user |
68+
| express.js:135:23:135:37 | req.params.user | express.js:135:16:135:37 | '//' + ... ms.user |
69+
| express.js:136:22:136:36 | req.params.user | express.js:136:16:136:36 | 'u' + r ... ms.user |
70+
| express.js:138:22:138:45 | ('/u' + ... s.user) | express.js:138:16:138:45 | '/' + ( ... s.user) |
71+
| express.js:138:23:138:44 | '/u' + ... ms.user | express.js:138:22:138:45 | ('/u' + ... s.user) |
72+
| express.js:138:30:138:44 | req.params.user | express.js:138:23:138:44 | '/u' + ... ms.user |
73+
| express.js:139:23:139:37 | req.params.user | express.js:139:16:139:37 | '/u' + ... ms.user |
7074
| node.js:6:7:6:52 | target | node.js:7:34:7:39 | target |
7175
| node.js:6:16:6:39 | url.par ... , true) | node.js:6:16:6:45 | url.par ... ).query |
7276
| node.js:6:16:6:45 | url.par ... ).query | node.js:6:16:6:52 | url.par ... .target |
@@ -94,11 +98,15 @@ edges
9498
| 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 |
9599
| 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 |
96100
| 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 |
97-
| 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 |
98101
| 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 |
99102
| 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 |
100103
| 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 |
101104
| 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 |
105+
| 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 |
106+
| 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 |
107+
| 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 |
108+
| 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 |
109+
| 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 |
102110
| 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 |
103111
| 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 |
104112
| 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 |

‎javascript/ql/test/query-tests/Security/CWE-601/ServerSideUrlRedirect/express.js‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,3 +126,15 @@ function sendUserToUrl(res, nextUrl) {
126126
app.get('/call', function(req, res) {
127127
sendUserToUrl(res, req.query.nextUrl);
128128
});
129+
130+
app.get('/redirect/:user', function(req, res) {
131+
res.redirect('/users/' + req.params.user); // GOOD
132+
res.redirect('users/' + req.params.user); // GOOD
133+
134+
res.redirect('/' + req.params.user); // BAD - could go to //evil.com
135+
res.redirect('//' + req.params.user); // BAD - could go to //evil.com
136+
res.redirect('u' + req.params.user); // BAD - could go to u.evil.com
137+
138+
res.redirect('/' + ('/u' + req.params.user)); // BAD - could go to //u.evil.com
139+
res.redirect('/u' + req.params.user); // GOOD - but flagged anyway
140+
});

0 commit comments

Comments
 (0)