Skip to content

Commit 3d60637

Browse files
erik-krogherik-krogh
committed
split interpretsArgumentsAsURL out of interpretsArgumentsAsHTML, and use it to generalize AttributeUrlSink
1 parent 1dcfe89 commit 3d60637

File tree

7 files changed

+57
-98
lines changed

7 files changed

+57
-98
lines changed

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

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -168,15 +168,20 @@ module ClientSideUrlRedirect {
168168
}
169169

170170
/**
171-
* A script or iframe `src` attribute, viewed as a `ScriptUrlSink`.
171+
* A write to a `href` or similar attribute viewed as a `ScriptUrlSink`.
172172
*/
173-
class SrcAttributeUrlSink extends ScriptUrlSink, DataFlow::ValueNode {
174-
SrcAttributeUrlSink() {
173+
class AttributeUrlSink extends ScriptUrlSink {
174+
AttributeUrlSink() {
175+
// e.g. `$("<a>", {href: sink}).appendTo("body")`
175176
exists(DOM::AttributeDefinition attr |
176-
attr.getElement().getName() = ["script", "iframe"] and
177-
attr.getName() = "src" and
177+
not attr instanceof JSXAttribute and // handled more precisely in `ReactAttributeWriteUrlSink`.
178+
attr.getName() = DOM::getAPropertyNameInterpretedAsJavaScriptUrl()
179+
|
178180
this = attr.getValueNode()
179181
)
182+
or
183+
// e.g. node.setAttribute("href", sink)
184+
any(DomMethodCallExpr call).interpretsArgumentsAsURL(this.asExpr())
180185
}
181186

182187
override predicate isXSSSink() { any() }

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

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,17 @@ class DomMethodCallExpr extends MethodCallExpr {
8181
name = "createElement" and argPos = 0
8282
or
8383
name = "appendChild" and argPos = 0
84-
or
84+
)
85+
}
86+
87+
/**
88+
* Holds if `arg` is an argument that is used as an URL.
89+
*/
90+
predicate interpretsArgumentsAsURL(Expr arg) {
91+
exists(int argPos, string name |
92+
arg = this.getArgument(argPos) and
93+
name = this.getMethodName()
94+
|
8595
(
8696
name = "setAttribute" and argPos = 1
8797
or

‎javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected

Lines changed: 0 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -432,26 +432,6 @@ nodes
432432
| trusted-types.js:2:71:2:71 | x |
433433
| trusted-types.js:3:24:3:34 | window.name |
434434
| trusted-types.js:3:24:3:34 | window.name |
435-
| tst3.js:2:12:2:75 | JSON.pa ... tr(1))) |
436-
| tst3.js:2:23:2:74 | decodeU ... str(1)) |
437-
| tst3.js:2:42:2:63 | window. ... .search |
438-
| tst3.js:2:42:2:63 | window. ... .search |
439-
| tst3.js:2:42:2:73 | window. ... bstr(1) |
440-
| tst3.js:4:25:4:28 | data |
441-
| tst3.js:4:25:4:32 | data.src |
442-
| tst3.js:4:25:4:32 | data.src |
443-
| tst3.js:5:26:5:29 | data |
444-
| tst3.js:5:26:5:31 | data.p |
445-
| tst3.js:5:26:5:31 | data.p |
446-
| tst3.js:7:32:7:35 | data |
447-
| tst3.js:7:32:7:37 | data.p |
448-
| tst3.js:7:32:7:37 | data.p |
449-
| tst3.js:9:37:9:40 | data |
450-
| tst3.js:9:37:9:42 | data.p |
451-
| tst3.js:9:37:9:42 | data.p |
452-
| tst3.js:10:38:10:41 | data |
453-
| tst3.js:10:38:10:43 | data.p |
454-
| tst3.js:10:38:10:43 | data.p |
455435
| tst.js:2:7:2:39 | target |
456436
| tst.js:2:7:2:39 | target |
457437
| tst.js:2:16:2:39 | documen ... .search |
@@ -1192,25 +1172,6 @@ edges
11921172
| trusted-types.js:2:66:2:66 | x | trusted-types.js:2:71:2:71 | x |
11931173
| trusted-types.js:3:24:3:34 | window.name | trusted-types.js:2:66:2:66 | x |
11941174
| trusted-types.js:3:24:3:34 | window.name | trusted-types.js:2:66:2:66 | x |
1195-
| tst3.js:2:12:2:75 | JSON.pa ... tr(1))) | tst3.js:4:25:4:28 | data |
1196-
| tst3.js:2:12:2:75 | JSON.pa ... tr(1))) | tst3.js:5:26:5:29 | data |
1197-
| tst3.js:2:12:2:75 | JSON.pa ... tr(1))) | tst3.js:7:32:7:35 | data |
1198-
| tst3.js:2:12:2:75 | JSON.pa ... tr(1))) | tst3.js:9:37:9:40 | data |
1199-
| tst3.js:2:12:2:75 | JSON.pa ... tr(1))) | tst3.js:10:38:10:41 | data |
1200-
| tst3.js:2:23:2:74 | decodeU ... str(1)) | tst3.js:2:12:2:75 | JSON.pa ... tr(1))) |
1201-
| tst3.js:2:42:2:63 | window. ... .search | tst3.js:2:42:2:73 | window. ... bstr(1) |
1202-
| tst3.js:2:42:2:63 | window. ... .search | tst3.js:2:42:2:73 | window. ... bstr(1) |
1203-
| tst3.js:2:42:2:73 | window. ... bstr(1) | tst3.js:2:23:2:74 | decodeU ... str(1)) |
1204-
| tst3.js:4:25:4:28 | data | tst3.js:4:25:4:32 | data.src |
1205-
| tst3.js:4:25:4:28 | data | tst3.js:4:25:4:32 | data.src |
1206-
| tst3.js:5:26:5:29 | data | tst3.js:5:26:5:31 | data.p |
1207-
| tst3.js:5:26:5:29 | data | tst3.js:5:26:5:31 | data.p |
1208-
| tst3.js:7:32:7:35 | data | tst3.js:7:32:7:37 | data.p |
1209-
| tst3.js:7:32:7:35 | data | tst3.js:7:32:7:37 | data.p |
1210-
| tst3.js:9:37:9:40 | data | tst3.js:9:37:9:42 | data.p |
1211-
| tst3.js:9:37:9:40 | data | tst3.js:9:37:9:42 | data.p |
1212-
| tst3.js:10:38:10:41 | data | tst3.js:10:38:10:43 | data.p |
1213-
| tst3.js:10:38:10:41 | data | tst3.js:10:38:10:43 | data.p |
12141175
| tst.js:2:7:2:39 | target | tst.js:5:18:5:23 | target |
12151176
| tst.js:2:7:2:39 | target | tst.js:5:18:5:23 | target |
12161177
| tst.js:2:7:2:39 | target | tst.js:12:28:12:33 | target |
@@ -1622,11 +1583,6 @@ edges
16221583
| tooltip.jsx:11:25:11:30 | source | tooltip.jsx:6:20:6:30 | window.name | tooltip.jsx:11:25:11:30 | source | Cross-site scripting vulnerability due to $@. | tooltip.jsx:6:20:6:30 | window.name | user-provided value |
16231584
| translate.js:9:27:9:50 | searchP ... 'term') | translate.js:6:16:6:39 | documen ... .search | translate.js:9:27:9:50 | searchP ... 'term') | Cross-site scripting vulnerability due to $@. | translate.js:6:16:6:39 | documen ... .search | user-provided value |
16241585
| trusted-types.js:2:71:2:71 | x | trusted-types.js:3:24:3:34 | window.name | trusted-types.js:2:71:2:71 | x | Cross-site scripting vulnerability due to $@. | trusted-types.js:3:24:3:34 | window.name | user-provided value |
1625-
| tst3.js:4:25:4:32 | data.src | tst3.js:2:42:2:63 | window. ... .search | tst3.js:4:25:4:32 | data.src | Cross-site scripting vulnerability due to $@. | tst3.js:2:42:2:63 | window. ... .search | user-provided value |
1626-
| tst3.js:5:26:5:31 | data.p | tst3.js:2:42:2:63 | window. ... .search | tst3.js:5:26:5:31 | data.p | Cross-site scripting vulnerability due to $@. | tst3.js:2:42:2:63 | window. ... .search | user-provided value |
1627-
| tst3.js:7:32:7:37 | data.p | tst3.js:2:42:2:63 | window. ... .search | tst3.js:7:32:7:37 | data.p | Cross-site scripting vulnerability due to $@. | tst3.js:2:42:2:63 | window. ... .search | user-provided value |
1628-
| tst3.js:9:37:9:42 | data.p | tst3.js:2:42:2:63 | window. ... .search | tst3.js:9:37:9:42 | data.p | Cross-site scripting vulnerability due to $@. | tst3.js:2:42:2:63 | window. ... .search | user-provided value |
1629-
| tst3.js:10:38:10:43 | data.p | tst3.js:2:42:2:63 | window. ... .search | tst3.js:10:38:10:43 | data.p | Cross-site scripting vulnerability due to $@. | tst3.js:2:42:2:63 | window. ... .search | user-provided value |
16301586
| tst.js:5:18:5:23 | target | tst.js:2:16:2:39 | documen ... .search | tst.js:5:18:5:23 | target | Cross-site scripting vulnerability due to $@. | tst.js:2:16:2:39 | documen ... .search | user-provided value |
16311587
| tst.js:8:18:8:126 | "<OPTIO ... PTION>" | tst.js:8:37:8:58 | documen ... on.href | tst.js:8:18:8:126 | "<OPTIO ... PTION>" | Cross-site scripting vulnerability due to $@. | tst.js:8:37:8:58 | documen ... on.href | user-provided value |
16321588
| tst.js:12:5:12:42 | '<div s ... 'px">' | tst.js:2:16:2:39 | documen ... .search | tst.js:12:5:12:42 | '<div s ... 'px">' | Cross-site scripting vulnerability due to $@. | tst.js:2:16:2:39 | documen ... .search | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected

Lines changed: 0 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -439,26 +439,6 @@ nodes
439439
| trusted-types.js:2:71:2:71 | x |
440440
| trusted-types.js:3:24:3:34 | window.name |
441441
| trusted-types.js:3:24:3:34 | window.name |
442-
| tst3.js:2:12:2:75 | JSON.pa ... tr(1))) |
443-
| tst3.js:2:23:2:74 | decodeU ... str(1)) |
444-
| tst3.js:2:42:2:63 | window. ... .search |
445-
| tst3.js:2:42:2:63 | window. ... .search |
446-
| tst3.js:2:42:2:73 | window. ... bstr(1) |
447-
| tst3.js:4:25:4:28 | data |
448-
| tst3.js:4:25:4:32 | data.src |
449-
| tst3.js:4:25:4:32 | data.src |
450-
| tst3.js:5:26:5:29 | data |
451-
| tst3.js:5:26:5:31 | data.p |
452-
| tst3.js:5:26:5:31 | data.p |
453-
| tst3.js:7:32:7:35 | data |
454-
| tst3.js:7:32:7:37 | data.p |
455-
| tst3.js:7:32:7:37 | data.p |
456-
| tst3.js:9:37:9:40 | data |
457-
| tst3.js:9:37:9:42 | data.p |
458-
| tst3.js:9:37:9:42 | data.p |
459-
| tst3.js:10:38:10:41 | data |
460-
| tst3.js:10:38:10:43 | data.p |
461-
| tst3.js:10:38:10:43 | data.p |
462442
| tst.js:2:7:2:39 | target |
463443
| tst.js:2:7:2:39 | target |
464444
| tst.js:2:16:2:39 | documen ... .search |
@@ -1227,25 +1207,6 @@ edges
12271207
| trusted-types.js:2:66:2:66 | x | trusted-types.js:2:71:2:71 | x |
12281208
| trusted-types.js:3:24:3:34 | window.name | trusted-types.js:2:66:2:66 | x |
12291209
| trusted-types.js:3:24:3:34 | window.name | trusted-types.js:2:66:2:66 | x |
1230-
| tst3.js:2:12:2:75 | JSON.pa ... tr(1))) | tst3.js:4:25:4:28 | data |
1231-
| tst3.js:2:12:2:75 | JSON.pa ... tr(1))) | tst3.js:5:26:5:29 | data |
1232-
| tst3.js:2:12:2:75 | JSON.pa ... tr(1))) | tst3.js:7:32:7:35 | data |
1233-
| tst3.js:2:12:2:75 | JSON.pa ... tr(1))) | tst3.js:9:37:9:40 | data |
1234-
| tst3.js:2:12:2:75 | JSON.pa ... tr(1))) | tst3.js:10:38:10:41 | data |
1235-
| tst3.js:2:23:2:74 | decodeU ... str(1)) | tst3.js:2:12:2:75 | JSON.pa ... tr(1))) |
1236-
| tst3.js:2:42:2:63 | window. ... .search | tst3.js:2:42:2:73 | window. ... bstr(1) |
1237-
| tst3.js:2:42:2:63 | window. ... .search | tst3.js:2:42:2:73 | window. ... bstr(1) |
1238-
| tst3.js:2:42:2:73 | window. ... bstr(1) | tst3.js:2:23:2:74 | decodeU ... str(1)) |
1239-
| tst3.js:4:25:4:28 | data | tst3.js:4:25:4:32 | data.src |
1240-
| tst3.js:4:25:4:28 | data | tst3.js:4:25:4:32 | data.src |
1241-
| tst3.js:5:26:5:29 | data | tst3.js:5:26:5:31 | data.p |
1242-
| tst3.js:5:26:5:29 | data | tst3.js:5:26:5:31 | data.p |
1243-
| tst3.js:7:32:7:35 | data | tst3.js:7:32:7:37 | data.p |
1244-
| tst3.js:7:32:7:35 | data | tst3.js:7:32:7:37 | data.p |
1245-
| tst3.js:9:37:9:40 | data | tst3.js:9:37:9:42 | data.p |
1246-
| tst3.js:9:37:9:40 | data | tst3.js:9:37:9:42 | data.p |
1247-
| tst3.js:10:38:10:41 | data | tst3.js:10:38:10:43 | data.p |
1248-
| tst3.js:10:38:10:41 | data | tst3.js:10:38:10:43 | data.p |
12491210
| tst.js:2:7:2:39 | target | tst.js:5:18:5:23 | target |
12501211
| tst.js:2:7:2:39 | target | tst.js:5:18:5:23 | target |
12511212
| tst.js:2:7:2:39 | target | tst.js:12:28:12:33 | target |

‎javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/tst3.js

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
var foo = document.getElementById("foo");
22
var data = JSON.parse(decodeURIComponent(window.location.search.substr(1)));
33

4-
foo.setAttribute("src", data.src); // NOT OK
5-
foo.setAttribute("HREF", data.p); // NOT OK
4+
foo.setAttribute("src", data.src); // NOT OK - but not detected [INCONSISTENCY]
5+
foo.setAttribute("HREF", data.p); // NOT OK - but not detected [INCONSISTENCY]
66
foo.setAttribute("width", data.w); // OK
7-
foo.setAttribute("xlink:href", data.p) // NOT OK
7+
foo.setAttribute("xlink:href", data.p) // NOT OK - but not detected [INCONSISTENCY]
88

9-
foo.setAttributeNS('xlink', 'href', data.p); // NOT OK
10-
foo.setAttributeNS('foobar', 'href', data.p); // NOT OK
9+
foo.setAttributeNS('xlink', 'href', data.p); // NOT OK - but not detected [INCONSISTENCY]
10+
foo.setAttributeNS('foobar', 'href', data.p); // NOT OK - but not detected [INCONSISTENCY]
1111
foo.setAttributeNS('baz', 'width', data.w); // OK
1212

1313

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,18 @@ nodes
147147
| tst13.js:72:19:72:49 | history ... bstr(1) |
148148
| tst13.js:74:21:74:27 | payload |
149149
| tst13.js:74:21:74:27 | payload |
150+
| tst13.js:78:9:78:48 | url |
151+
| tst13.js:78:15:78:38 | documen ... .search |
152+
| tst13.js:78:15:78:38 | documen ... .search |
153+
| tst13.js:78:15:78:48 | documen ... bstr(1) |
154+
| tst13.js:80:21:80:23 | url |
155+
| tst13.js:80:21:80:23 | url |
156+
| tst13.js:81:28:81:30 | url |
157+
| tst13.js:81:28:81:30 | url |
158+
| tst13.js:82:27:82:29 | url |
159+
| tst13.js:82:27:82:29 | url |
160+
| tst13.js:83:22:83:24 | url |
161+
| tst13.js:83:22:83:24 | url |
150162
| tst.js:2:19:2:69 | /.*redi ... n.href) |
151163
| tst.js:2:19:2:72 | /.*redi ... ref)[1] |
152164
| tst.js:2:19:2:72 | /.*redi ... ref)[1] |
@@ -339,6 +351,17 @@ edges
339351
| tst13.js:72:19:72:39 | history ... on.hash | tst13.js:72:19:72:49 | history ... bstr(1) |
340352
| tst13.js:72:19:72:39 | history ... on.hash | tst13.js:72:19:72:49 | history ... bstr(1) |
341353
| tst13.js:72:19:72:49 | history ... bstr(1) | tst13.js:72:9:72:49 | payload |
354+
| tst13.js:78:9:78:48 | url | tst13.js:80:21:80:23 | url |
355+
| tst13.js:78:9:78:48 | url | tst13.js:80:21:80:23 | url |
356+
| tst13.js:78:9:78:48 | url | tst13.js:81:28:81:30 | url |
357+
| tst13.js:78:9:78:48 | url | tst13.js:81:28:81:30 | url |
358+
| tst13.js:78:9:78:48 | url | tst13.js:82:27:82:29 | url |
359+
| tst13.js:78:9:78:48 | url | tst13.js:82:27:82:29 | url |
360+
| tst13.js:78:9:78:48 | url | tst13.js:83:22:83:24 | url |
361+
| tst13.js:78:9:78:48 | url | tst13.js:83:22:83:24 | url |
362+
| tst13.js:78:15:78:38 | documen ... .search | tst13.js:78:15:78:48 | documen ... bstr(1) |
363+
| tst13.js:78:15:78:38 | documen ... .search | tst13.js:78:15:78:48 | documen ... bstr(1) |
364+
| tst13.js:78:15:78:48 | documen ... bstr(1) | tst13.js:78:9:78:48 | url |
342365
| tst.js:2:19:2:69 | /.*redi ... n.href) | tst.js:2:19:2:72 | /.*redi ... ref)[1] |
343366
| tst.js:2:19:2:69 | /.*redi ... n.href) | tst.js:2:19:2:72 | /.*redi ... ref)[1] |
344367
| tst.js:2:47:2:63 | document.location | tst.js:2:47:2:68 | documen ... on.href |
@@ -433,6 +456,10 @@ edges
433456
| tst13.js:61:18:61:24 | payload | tst13.js:59:19:59:42 | documen ... .search | tst13.js:61:18:61:24 | payload | Untrusted URL redirection due to $@. | tst13.js:59:19:59:42 | documen ... .search | user-provided value |
434457
| tst13.js:67:21:67:27 | payload | tst13.js:65:19:65:39 | history ... on.hash | tst13.js:67:21:67:27 | payload | Untrusted URL redirection due to $@. | tst13.js:65:19:65:39 | history ... on.hash | user-provided value |
435458
| tst13.js:74:21:74:27 | payload | tst13.js:72:19:72:39 | history ... on.hash | tst13.js:74:21:74:27 | payload | Untrusted URL redirection due to $@. | tst13.js:72:19:72:39 | history ... on.hash | user-provided value |
459+
| tst13.js:80:21:80:23 | url | tst13.js:78:15:78:38 | documen ... .search | tst13.js:80:21:80:23 | url | Untrusted URL redirection due to $@. | tst13.js:78:15:78:38 | documen ... .search | user-provided value |
460+
| tst13.js:81:28:81:30 | url | tst13.js:78:15:78:38 | documen ... .search | tst13.js:81:28:81:30 | url | Untrusted URL redirection due to $@. | tst13.js:78:15:78:38 | documen ... .search | user-provided value |
461+
| tst13.js:82:27:82:29 | url | tst13.js:78:15:78:38 | documen ... .search | tst13.js:82:27:82:29 | url | Untrusted URL redirection due to $@. | tst13.js:78:15:78:38 | documen ... .search | user-provided value |
462+
| tst13.js:83:22:83:24 | url | tst13.js:78:15:78:38 | documen ... .search | tst13.js:83:22:83:24 | url | Untrusted URL redirection due to $@. | tst13.js:78:15:78:38 | documen ... .search | user-provided value |
436463
| 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 |
437464
| tst.js:2:19:2:72 | /.*redi ... ref)[1] | tst.js:2:47:2:68 | documen ... on.href | tst.js:2:19:2:72 | /.*redi ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:2:47:2:68 | documen ... on.href | user-provided value |
438465
| 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: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,8 @@ function quz() {
7777
function bar() {
7878
var url = document.location.search.substr(1);
7979

80-
$("<a>", {href: url}).appendTo("body"); // NOT OK - but not detected [INCONSISTENCY]
81-
$("#foo").attr("href", url); // NOT OK - but not detected [INCONSISTENCY]
82-
$("#foo").attr({href: url}); // NOT OK - but not detected [INCONSISTENCY]
83-
$("<img>", {src: url}).appendTo("body"); // NOT OK - but not detected [INCONSISTENCY]
80+
$("<a>", {href: url}).appendTo("body"); // NOT OK
81+
$("#foo").attr("href", url); // NOT OK
82+
$("#foo").attr({href: url}); // NOT OK
83+
$("<img>", {src: url}).appendTo("body"); // NOT OK
8484
}

0 commit comments

Comments
 (0)