Skip to content

Commit fe0c5a9

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#3892 from asger-semmle/js/redirect-starts-with-sanitizer
Approved by esbena
2 parents 6d80445 + 9a94462 commit fe0c5a9

File tree

7 files changed

+108
-3
lines changed

7 files changed

+108
-3
lines changed

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,5 +50,9 @@ module ClientSideUrlRedirect {
5050
g instanceof DocumentUrl and
5151
succ.(DataFlow::PropRead).accesses(pred, "href")
5252
}
53+
54+
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
55+
guard instanceof HostnameSanitizerGuard
56+
}
5357
}
5458
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
import javascript
88
import semmle.javascript.security.dataflow.RemoteFlowSources
9-
import UrlConcatenation
9+
private import UrlConcatenation
1010

1111
module ClientSideUrlRedirect {
1212
private import Xss::DomBasedXss as DomBasedXss

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,8 @@ module ServerSideUrlRedirect {
3434
}
3535

3636
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
37-
guard instanceof LocalUrlSanitizingGuard
37+
guard instanceof LocalUrlSanitizingGuard or
38+
guard instanceof HostnameSanitizerGuard
3839
}
3940
}
4041
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
import javascript
88
import RemoteFlowSources
9-
import UrlConcatenation
9+
private import UrlConcatenation
1010

1111
module ServerSideUrlRedirect {
1212
/**

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,3 +96,15 @@ predicate hostnameSanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sin
9696
hasHostnameSanitizingSubstring(StringConcatenation::getOperand(operator, [0 .. n - 1]))
9797
)
9898
}
99+
100+
/**
101+
* A check that sanitizes the hostname of a URL.
102+
*/
103+
class HostnameSanitizerGuard extends TaintTracking::SanitizerGuardNode, StringOps::StartsWith {
104+
HostnameSanitizerGuard() { hasHostnameSanitizingSubstring(getSubstring()) }
105+
106+
override predicate sanitizes(boolean outcome, Expr e) {
107+
outcome = getPolarity() and
108+
e = getBaseString().asExpr()
109+
}
110+
}

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

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,23 @@
11
nodes
2+
| sanitizer.js:2:9:2:25 | url |
3+
| sanitizer.js:2:15:2:25 | window.name |
4+
| sanitizer.js:2:15:2:25 | window.name |
5+
| sanitizer.js:4:27:4:29 | url |
6+
| sanitizer.js:4:27:4:29 | url |
7+
| sanitizer.js:16:27:16:29 | url |
8+
| sanitizer.js:16:27:16:29 | url |
9+
| sanitizer.js:19:27:19:29 | url |
10+
| sanitizer.js:19:27:19:29 | url |
11+
| sanitizer.js:22:27:22:29 | url |
12+
| sanitizer.js:22:27:22:29 | url |
13+
| sanitizer.js:25:27:25:29 | url |
14+
| sanitizer.js:25:27:25:29 | url |
15+
| sanitizer.js:28:27:28:29 | url |
16+
| sanitizer.js:28:27:28:29 | url |
17+
| sanitizer.js:31:27:31:29 | url |
18+
| sanitizer.js:31:27:31:29 | url |
19+
| sanitizer.js:37:27:37:29 | url |
20+
| sanitizer.js:37:27:37:29 | url |
221
| tst2.js:2:7:2:33 | href |
322
| tst2.js:2:7:2:33 | href |
423
| tst2.js:2:14:2:28 | window.location |
@@ -80,6 +99,24 @@ nodes
8099
| tst.js:6:34:6:50 | document.location |
81100
| tst.js:6:34:6:55 | documen ... on.href |
82101
edges
102+
| sanitizer.js:2:9:2:25 | url | sanitizer.js:4:27:4:29 | url |
103+
| sanitizer.js:2:9:2:25 | url | sanitizer.js:4:27:4:29 | url |
104+
| sanitizer.js:2:9:2:25 | url | sanitizer.js:16:27:16:29 | url |
105+
| sanitizer.js:2:9:2:25 | url | sanitizer.js:16:27:16:29 | url |
106+
| sanitizer.js:2:9:2:25 | url | sanitizer.js:19:27:19:29 | url |
107+
| sanitizer.js:2:9:2:25 | url | sanitizer.js:19:27:19:29 | url |
108+
| sanitizer.js:2:9:2:25 | url | sanitizer.js:22:27:22:29 | url |
109+
| sanitizer.js:2:9:2:25 | url | sanitizer.js:22:27:22:29 | url |
110+
| sanitizer.js:2:9:2:25 | url | sanitizer.js:25:27:25:29 | url |
111+
| sanitizer.js:2:9:2:25 | url | sanitizer.js:25:27:25:29 | url |
112+
| sanitizer.js:2:9:2:25 | url | sanitizer.js:28:27:28:29 | url |
113+
| sanitizer.js:2:9:2:25 | url | sanitizer.js:28:27:28:29 | url |
114+
| sanitizer.js:2:9:2:25 | url | sanitizer.js:31:27:31:29 | url |
115+
| sanitizer.js:2:9:2:25 | url | sanitizer.js:31:27:31:29 | url |
116+
| sanitizer.js:2:9:2:25 | url | sanitizer.js:37:27:37:29 | url |
117+
| sanitizer.js:2:9:2:25 | url | sanitizer.js:37:27:37:29 | url |
118+
| sanitizer.js:2:15:2:25 | window.name | sanitizer.js:2:9:2:25 | url |
119+
| sanitizer.js:2:15:2:25 | window.name | sanitizer.js:2:9:2:25 | url |
83120
| tst2.js:2:7:2:33 | href | tst2.js:4:21:4:24 | href |
84121
| tst2.js:2:7:2:33 | href | tst2.js:4:21:4:24 | href |
85122
| tst2.js:2:14:2:28 | window.location | tst2.js:2:14:2:33 | window.location.href |
@@ -155,6 +192,14 @@ edges
155192
| tst.js:6:34:6:50 | document.location | tst.js:6:34:6:55 | documen ... on.href |
156193
| tst.js:6:34:6:55 | documen ... on.href | tst.js:6:20:6:56 | indirec ... n.href) |
157194
#select
195+
| sanitizer.js:4:27:4:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:4:27:4:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
196+
| sanitizer.js:16:27:16:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:16:27:16:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
197+
| sanitizer.js:19:27:19:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:19:27:19:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
198+
| sanitizer.js:22:27:22:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:22:27:22:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
199+
| sanitizer.js:25:27:25:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:25:27:25:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
200+
| sanitizer.js:28:27:28:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:28:27:28:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
201+
| sanitizer.js:31:27:31:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:31:27:31:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
202+
| sanitizer.js:37:27:37:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:37:27:37:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
158203
| tst2.js:4:21:4:55 | href.su ... '?')+1) | tst2.js:2:14:2:28 | window.location | tst2.js:4:21:4:55 | href.su ... '?')+1) | Untrusted URL redirection due to $@. | tst2.js:2:14:2:28 | window.location | user-provided value |
159204
| tst6.js:4:21:4:28 | redirect | tst6.js:2:18:2:45 | $locati ... irect') | tst6.js:4:21:4:28 | redirect | Untrusted URL redirection due to $@. | tst6.js:2:18:2:45 | $locati ... irect') | user-provided value |
160205
| tst6.js:6:17:6:24 | redirect | tst6.js:2:18:2:45 | $locati ... irect') | tst6.js:6:17:6:24 | redirect | Untrusted URL redirection due to $@. | tst6.js:2:18:2:45 | $locati ... irect') | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/sanitizer.js

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
function f() {
2+
let url = window.name;
3+
if (url.startsWith('https://example.com')) {
4+
window.location = url; // NOT OK - can be example.com.evil.com
5+
}
6+
if (url.startsWith('https://example.com/')) {
7+
window.location = url; // OK
8+
}
9+
if (url.startsWith('https://example.com//')) {
10+
window.location = url; // OK
11+
}
12+
if (url.startsWith('https://example.com/foo')) {
13+
window.location = url; // OK
14+
}
15+
if (url.startsWith('https://')) {
16+
window.location = url; // NOT OK - does not restrict hostname
17+
}
18+
if (url.startsWith('https:/')) {
19+
window.location = url; // NOT OK - does not restrict hostname
20+
}
21+
if (url.startsWith('https:')) {
22+
window.location = url; // NOT OK - does not restrict hostname
23+
}
24+
if (url.startsWith('/')) {
25+
window.location = url; // NOT OK - can be //evil.com
26+
}
27+
if (url.startsWith('//')) {
28+
window.location = url; // NOT OK - can be //evil.com
29+
}
30+
if (url.startsWith('//example.com')) {
31+
window.location = url; // NOT OK - can be //example.com.evil.com
32+
}
33+
if (url.startsWith('//example.com/')) {
34+
window.location = url; // OK
35+
}
36+
if (url.endsWith('https://example.com/')) {
37+
window.location = url; // NOT OK - could be evil.com?x=https://example.com/
38+
}
39+
let basedir = whatever() ? 'foo' : 'bar';
40+
if (url.startsWith('https://example.com/' + basedir)) {
41+
window.location = url; // OK - the whole prefix is not known, but enough to restrict hostname
42+
}
43+
}

0 commit comments

Comments
 (0)