Skip to content

Commit 8855ab8

Browse files
codeql-cicodeql-ci
authored
Merge pull request github#3835 from Raz0r/js/xss-protocol-sinks
Approved by erik-krogh
2 parents a4f8b19 + 3487ec1 commit 8855ab8

File tree

5 files changed

+146
-1
lines changed

5 files changed

+146
-1
lines changed

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,4 +145,17 @@ module ClientSideUrlRedirect {
145145
)
146146
}
147147
}
148+
149+
/**
150+
* A write of an attribute which may execute JavaScript code or
151+
* exfiltrate data to an attacker controlled site.
152+
*/
153+
class AttributeWriteUrlSink extends ScriptUrlSink, DataFlow::ValueNode {
154+
AttributeWriteUrlSink() {
155+
exists(DomPropWriteNode pw |
156+
pw.interpretsValueAsJavaScriptUrl() and
157+
this = DataFlow::valueNode(pw.getRhs())
158+
)
159+
}
160+
}
148161
}

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

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,8 @@ class DomMethodCallExpr extends MethodCallExpr {
9090
attr = "formaction" or
9191
attr = "href" or
9292
attr = "src" or
93-
attr = "xlink:href"
93+
attr = "xlink:href" or
94+
attr = "data"
9495
|
9596
getArgument(argPos - 1).getStringValue().toLowerCase() = attr
9697
)
@@ -116,6 +117,17 @@ class DomPropWriteNode extends Assignment {
116117
lhs.getPropertyName() = "innerHTML" or
117118
lhs.getPropertyName() = "outerHTML"
118119
}
120+
121+
/**
122+
* Holds if the assigned value is interpreted as JavaScript via javascript: protocol.
123+
*/
124+
predicate interpretsValueAsJavaScriptUrl() {
125+
lhs.getPropertyName() = "action" or
126+
lhs.getPropertyName() = "formaction" or
127+
lhs.getPropertyName() = "href" or
128+
lhs.getPropertyName() = "src" or
129+
lhs.getPropertyName() = "data"
130+
}
119131
}
120132

121133
/**

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

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,33 @@ nodes
8686
| tst12.js:4:15:4:45 | urlPart ... s.value |
8787
| tst12.js:5:23:5:25 | loc |
8888
| tst12.js:5:23:5:25 | loc |
89+
| tst13.js:2:9:2:52 | payload |
90+
| tst13.js:2:19:2:35 | document.location |
91+
| tst13.js:2:19:2:35 | document.location |
92+
| tst13.js:2:19:2:42 | documen ... .search |
93+
| tst13.js:2:19:2:52 | documen ... bstr(1) |
94+
| tst13.js:4:15:4:21 | payload |
95+
| tst13.js:4:15:4:21 | payload |
96+
| tst13.js:8:21:8:27 | payload |
97+
| tst13.js:8:21:8:27 | payload |
98+
| tst13.js:12:14:12:20 | payload |
99+
| tst13.js:12:14:12:20 | payload |
100+
| tst13.js:16:17:16:23 | payload |
101+
| tst13.js:16:17:16:23 | payload |
102+
| tst13.js:20:14:20:20 | payload |
103+
| tst13.js:20:14:20:20 | payload |
104+
| tst13.js:24:14:24:20 | payload |
105+
| tst13.js:24:14:24:20 | payload |
106+
| tst13.js:28:21:28:27 | payload |
107+
| tst13.js:28:21:28:27 | payload |
108+
| tst13.js:32:17:32:23 | payload |
109+
| tst13.js:32:17:32:23 | payload |
110+
| tst13.js:36:21:36:27 | payload |
111+
| tst13.js:36:21:36:27 | payload |
112+
| tst13.js:40:15:40:21 | payload |
113+
| tst13.js:40:15:40:21 | payload |
114+
| tst13.js:44:14:44:20 | payload |
115+
| tst13.js:44:14:44:20 | payload |
89116
| tst.js:2:19:2:69 | /.*redi ... n.href) |
90117
| tst.js:2:19:2:72 | /.*redi ... ref)[1] |
91118
| tst.js:2:19:2:72 | /.*redi ... ref)[1] |
@@ -181,6 +208,32 @@ edges
181208
| tst12.js:4:15:4:25 | urlParts[0] | tst12.js:4:15:4:45 | urlPart ... s.value |
182209
| tst12.js:4:15:4:45 | urlPart ... s.value | tst12.js:4:9:4:45 | loc |
183210
| tst12.js:5:23:5:25 | loc | tst12.js:3:20:3:34 | window.location |
211+
| tst13.js:2:9:2:52 | payload | tst13.js:4:15:4:21 | payload |
212+
| tst13.js:2:9:2:52 | payload | tst13.js:4:15:4:21 | payload |
213+
| tst13.js:2:9:2:52 | payload | tst13.js:8:21:8:27 | payload |
214+
| tst13.js:2:9:2:52 | payload | tst13.js:8:21:8:27 | payload |
215+
| tst13.js:2:9:2:52 | payload | tst13.js:12:14:12:20 | payload |
216+
| tst13.js:2:9:2:52 | payload | tst13.js:12:14:12:20 | payload |
217+
| tst13.js:2:9:2:52 | payload | tst13.js:16:17:16:23 | payload |
218+
| tst13.js:2:9:2:52 | payload | tst13.js:16:17:16:23 | payload |
219+
| tst13.js:2:9:2:52 | payload | tst13.js:20:14:20:20 | payload |
220+
| tst13.js:2:9:2:52 | payload | tst13.js:20:14:20:20 | payload |
221+
| tst13.js:2:9:2:52 | payload | tst13.js:24:14:24:20 | payload |
222+
| tst13.js:2:9:2:52 | payload | tst13.js:24:14:24:20 | payload |
223+
| tst13.js:2:9:2:52 | payload | tst13.js:28:21:28:27 | payload |
224+
| tst13.js:2:9:2:52 | payload | tst13.js:28:21:28:27 | payload |
225+
| tst13.js:2:9:2:52 | payload | tst13.js:32:17:32:23 | payload |
226+
| tst13.js:2:9:2:52 | payload | tst13.js:32:17:32:23 | payload |
227+
| tst13.js:2:9:2:52 | payload | tst13.js:36:21:36:27 | payload |
228+
| tst13.js:2:9:2:52 | payload | tst13.js:36:21:36:27 | payload |
229+
| tst13.js:2:9:2:52 | payload | tst13.js:40:15:40:21 | payload |
230+
| tst13.js:2:9:2:52 | payload | tst13.js:40:15:40:21 | payload |
231+
| tst13.js:2:9:2:52 | payload | tst13.js:44:14:44:20 | payload |
232+
| tst13.js:2:9:2:52 | payload | tst13.js:44:14:44:20 | payload |
233+
| tst13.js:2:19:2:35 | document.location | tst13.js:2:19:2:42 | documen ... .search |
234+
| tst13.js:2:19:2:35 | document.location | tst13.js:2:19:2:42 | documen ... .search |
235+
| tst13.js:2:19:2:42 | documen ... .search | tst13.js:2:19:2:52 | documen ... bstr(1) |
236+
| tst13.js:2:19:2:52 | documen ... bstr(1) | tst13.js:2:9:2:52 | payload |
184237
| tst.js:2:19:2:69 | /.*redi ... n.href) | tst.js:2:19:2:72 | /.*redi ... ref)[1] |
185238
| tst.js:2:19:2:69 | /.*redi ... n.href) | tst.js:2:19:2:72 | /.*redi ... ref)[1] |
186239
| tst.js:2:47:2:63 | document.location | tst.js:2:47:2:68 | documen ... on.href |
@@ -212,5 +265,16 @@ edges
212265
| tst10.js:11:17:11:50 | '//foo' ... .search | tst10.js:11:27:11:43 | document.location | tst10.js:11:17:11:50 | '//foo' ... .search | Untrusted URL redirection due to $@. | tst10.js:11:27:11:43 | document.location | user-provided value |
213266
| tst10.js:14:17:14:56 | 'https: ... .search | tst10.js:14:33:14:49 | document.location | tst10.js:14:17:14:56 | 'https: ... .search | Untrusted URL redirection due to $@. | tst10.js:14:33:14:49 | document.location | user-provided value |
214267
| tst12.js:5:23:5:25 | loc | tst12.js:3:20:3:34 | window.location | tst12.js:5:23:5:25 | loc | Untrusted URL redirection due to $@. | tst12.js:3:20:3:34 | window.location | user-provided value |
268+
| tst13.js:4:15:4:21 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:4:15:4:21 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value |
269+
| tst13.js:8:21:8:27 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:8:21:8:27 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value |
270+
| tst13.js:12:14:12:20 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:12:14:12:20 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value |
271+
| tst13.js:16:17:16:23 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:16:17:16:23 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value |
272+
| tst13.js:20:14:20:20 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:20:14:20:20 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value |
273+
| tst13.js:24:14:24:20 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:24:14:24:20 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value |
274+
| tst13.js:28:21:28:27 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:28:21:28:27 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value |
275+
| tst13.js:32:17:32:23 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:32:17:32:23 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value |
276+
| tst13.js:36:21:36:27 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:36:21:36:27 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value |
277+
| tst13.js:40:15:40:21 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:40:15:40:21 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value |
278+
| tst13.js:44:14:44:20 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:44:14:44:20 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value |
215279
| tst.js:2:19:2:72 | /.*redi ... ref)[1] | tst.js:2:47:2:63 | document.location | tst.js:2:19:2:72 | /.*redi ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:2:47:2:63 | document.location | user-provided value |
216280
| tst.js:6:20:6:59 | indirec ... ref)[1] | tst.js:6:34:6:50 | document.location | tst.js:6:20:6:59 | indirec ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:6:34:6:50 | document.location | user-provided value |

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

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
function foo() {
2+
var payload = document.location.search.substr(1);
3+
var el = document.createElement("a");
4+
el.href = payload;
5+
document.body.appendChild(el); // NOT OK
6+
7+
var el = document.createElement("button");
8+
el.formaction = payload;
9+
document.body.appendChild(el); // NOT OK
10+
11+
var el = document.createElement("embed");
12+
el.src = payload;
13+
document.body.appendChild(el); // NOT OK
14+
15+
var el = document.createElement("form");
16+
el.action = payload;
17+
document.body.appendChild(el); // NOT OK
18+
19+
var el = document.createElement("frame");
20+
el.src = payload;
21+
document.body.appendChild(el); // NOT OK
22+
23+
var el = document.createElement("iframe");
24+
el.src = payload;
25+
document.body.appendChild(el); // NOT OK
26+
27+
var el = document.createElement("input");
28+
el.formaction = payload;
29+
document.body.appendChild(el); // NOT OK
30+
31+
var el = document.createElement("isindex");
32+
el.action = payload;
33+
document.body.appendChild(el); // NOT OK
34+
35+
var el = document.createElement("isindex");
36+
el.formaction = payload;
37+
document.body.appendChild(el); // NOT OK
38+
39+
var el = document.createElement("object");
40+
el.data = payload;
41+
document.body.appendChild(el); // NOT OK
42+
43+
var el = document.createElement("script");
44+
el.src = payload;
45+
document.body.appendChild(el); // NOT OK
46+
}

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
function foo() {
2+
var url = document.location.search.substr(1);
3+
var el = document.createElement("script");
4+
el.src = "https://github.com/" + url;
5+
document.body.appendChild(el); // OK
6+
7+
if (url.startsWith('https://github.com/')) {
8+
window.location = url; // OK
9+
}
10+
}

0 commit comments

Comments
 (0)