Skip to content

Commit 9ee7599

Browse files
asgerfasgerf
committed
JS: Move AngularJSTemplateUrlSink to ClientSideUrlRedirection query
This is not perfect but at least we can be consistent about keeping URLs-that-lead-to-xss in the same query
1 parent 699d3a0 commit 9ee7599

File tree

7 files changed

+52
-43
lines changed

7 files changed

+52
-43
lines changed

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,15 @@ module ClientSideUrlRedirect {
237237
override predicate isXssSink() { any() }
238238
}
239239

240+
/**
241+
* A `templateUrl` member of an AngularJS directive.
242+
*/
243+
private class AngularJSTemplateUrlSink extends Sink {
244+
AngularJSTemplateUrlSink() { this = any(AngularJS::CustomDirective d).getMember("templateUrl") }
245+
246+
override predicate isXssSink() { any() }
247+
}
248+
240249
private class SinkFromModel extends Sink {
241250
SinkFromModel() { this = ModelOutput::getASinkNode("url-redirection").asSink() }
242251
}

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -653,10 +653,11 @@ module TaintedPath {
653653
}
654654

655655
/**
656-
* A `templateUrl` member of an AngularJS directive.
656+
* DEPRECATED. This is no longer seen as a path-injection sink. It is tentatively handled
657+
* by the client-side URL redirection query for now.
657658
*/
658-
class AngularJSTemplateUrlSink extends Sink, DataFlow::ValueNode {
659-
AngularJSTemplateUrlSink() { this = any(AngularJS::CustomDirective d).getMember("templateUrl") }
659+
deprecated class AngularJSTemplateUrlSink extends DataFlow::ValueNode instanceof Sink {
660+
AngularJSTemplateUrlSink() { none() }
660661
}
661662

662663
/**

‎javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected

Lines changed: 0 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -514,11 +514,6 @@ nodes
514514
| TaintedPath.js:60:57:60:60 | path |
515515
| TaintedPath.js:60:57:60:60 | path |
516516
| TaintedPath.js:60:57:60:60 | path |
517-
| TaintedPath.js:71:26:71:45 | Cookie.get("unsafe") |
518-
| TaintedPath.js:71:26:71:45 | Cookie.get("unsafe") |
519-
| TaintedPath.js:71:26:71:45 | Cookie.get("unsafe") |
520-
| TaintedPath.js:71:26:71:45 | Cookie.get("unsafe") |
521-
| TaintedPath.js:71:26:71:45 | Cookie.get("unsafe") |
522517
| TaintedPath.js:77:31:77:70 | require ... eq.url) |
523518
| TaintedPath.js:77:31:77:70 | require ... eq.url) |
524519
| TaintedPath.js:77:31:77:70 | require ... eq.url) |
@@ -639,19 +634,6 @@ nodes
639634
| TaintedPath.js:87:48:87:60 | req.params[0] |
640635
| TaintedPath.js:87:48:87:60 | req.params[0] |
641636
| TaintedPath.js:87:48:87:60 | req.params[0] |
642-
| TaintedPath.js:95:30:95:31 | ev |
643-
| TaintedPath.js:95:30:95:31 | ev |
644-
| TaintedPath.js:95:30:95:31 | ev |
645-
| TaintedPath.js:95:30:95:31 | ev |
646-
| TaintedPath.js:95:30:95:31 | ev |
647-
| TaintedPath.js:96:24:96:25 | ev |
648-
| TaintedPath.js:96:24:96:25 | ev |
649-
| TaintedPath.js:96:24:96:25 | ev |
650-
| TaintedPath.js:96:24:96:25 | ev |
651-
| TaintedPath.js:96:24:96:30 | ev.data |
652-
| TaintedPath.js:96:24:96:30 | ev.data |
653-
| TaintedPath.js:96:24:96:30 | ev.data |
654-
| TaintedPath.js:96:24:96:30 | ev.data |
655637
| TaintedPath.js:100:6:100:47 | path |
656638
| TaintedPath.js:100:6:100:47 | path |
657639
| TaintedPath.js:100:6:100:47 | path |
@@ -5373,26 +5355,6 @@ edges
53735355
| TaintedPath.js:79:60:79:66 | req.url | TaintedPath.js:79:31:79:67 | require ... eq.url) |
53745356
| TaintedPath.js:79:60:79:66 | req.url | TaintedPath.js:79:31:79:67 | require ... eq.url) |
53755357
| TaintedPath.js:87:48:87:60 | req.params[0] | TaintedPath.js:87:48:87:60 | req.params[0] |
5376-
| TaintedPath.js:95:30:95:31 | ev | TaintedPath.js:96:24:96:25 | ev |
5377-
| TaintedPath.js:95:30:95:31 | ev | TaintedPath.js:96:24:96:25 | ev |
5378-
| TaintedPath.js:95:30:95:31 | ev | TaintedPath.js:96:24:96:25 | ev |
5379-
| TaintedPath.js:95:30:95:31 | ev | TaintedPath.js:96:24:96:25 | ev |
5380-
| TaintedPath.js:95:30:95:31 | ev | TaintedPath.js:96:24:96:25 | ev |
5381-
| TaintedPath.js:95:30:95:31 | ev | TaintedPath.js:96:24:96:25 | ev |
5382-
| TaintedPath.js:95:30:95:31 | ev | TaintedPath.js:96:24:96:25 | ev |
5383-
| TaintedPath.js:95:30:95:31 | ev | TaintedPath.js:96:24:96:25 | ev |
5384-
| TaintedPath.js:96:24:96:25 | ev | TaintedPath.js:96:24:96:30 | ev.data |
5385-
| TaintedPath.js:96:24:96:25 | ev | TaintedPath.js:96:24:96:30 | ev.data |
5386-
| TaintedPath.js:96:24:96:25 | ev | TaintedPath.js:96:24:96:30 | ev.data |
5387-
| TaintedPath.js:96:24:96:25 | ev | TaintedPath.js:96:24:96:30 | ev.data |
5388-
| TaintedPath.js:96:24:96:30 | ev.data | TaintedPath.js:71:26:71:45 | Cookie.get("unsafe") |
5389-
| TaintedPath.js:96:24:96:30 | ev.data | TaintedPath.js:71:26:71:45 | Cookie.get("unsafe") |
5390-
| TaintedPath.js:96:24:96:30 | ev.data | TaintedPath.js:71:26:71:45 | Cookie.get("unsafe") |
5391-
| TaintedPath.js:96:24:96:30 | ev.data | TaintedPath.js:71:26:71:45 | Cookie.get("unsafe") |
5392-
| TaintedPath.js:96:24:96:30 | ev.data | TaintedPath.js:71:26:71:45 | Cookie.get("unsafe") |
5393-
| TaintedPath.js:96:24:96:30 | ev.data | TaintedPath.js:71:26:71:45 | Cookie.get("unsafe") |
5394-
| TaintedPath.js:96:24:96:30 | ev.data | TaintedPath.js:71:26:71:45 | Cookie.get("unsafe") |
5395-
| TaintedPath.js:96:24:96:30 | ev.data | TaintedPath.js:71:26:71:45 | Cookie.get("unsafe") |
53965358
| TaintedPath.js:100:6:100:47 | path | TaintedPath.js:102:44:102:47 | path |
53975359
| TaintedPath.js:100:6:100:47 | path | TaintedPath.js:102:44:102:47 | path |
53985360
| TaintedPath.js:100:6:100:47 | path | TaintedPath.js:102:44:102:47 | path |
@@ -10483,7 +10445,6 @@ edges
1048310445
| TaintedPath.js:56:29:56:52 | pathMod ... e(path) | TaintedPath.js:38:20:38:26 | req.url | TaintedPath.js:56:29:56:52 | pathMod ... e(path) | This path depends on a $@. | TaintedPath.js:38:20:38:26 | req.url | user-provided value |
1048410446
| TaintedPath.js:58:29:58:61 | pathMod ... ath, z) | TaintedPath.js:38:20:38:26 | req.url | TaintedPath.js:58:29:58:61 | pathMod ... ath, z) | This path depends on a $@. | TaintedPath.js:38:20:38:26 | req.url | user-provided value |
1048510447
| TaintedPath.js:60:29:60:61 | pathMod ... h(path) | TaintedPath.js:38:20:38:26 | req.url | TaintedPath.js:60:29:60:61 | pathMod ... h(path) | This path depends on a $@. | TaintedPath.js:38:20:38:26 | req.url | user-provided value |
10486-
| TaintedPath.js:71:26:71:45 | Cookie.get("unsafe") | TaintedPath.js:95:30:95:31 | ev | TaintedPath.js:71:26:71:45 | Cookie.get("unsafe") | This path depends on a $@. | TaintedPath.js:95:30:95:31 | ev | user-provided value |
1048710448
| TaintedPath.js:77:31:77:76 | require ... ).query | TaintedPath.js:77:63:77:69 | req.url | TaintedPath.js:77:31:77:76 | require ... ).query | This path depends on a $@. | TaintedPath.js:77:63:77:69 | req.url | user-provided value |
1048810449
| TaintedPath.js:78:31:78:74 | require ... ).query | TaintedPath.js:78:61:78:67 | req.url | TaintedPath.js:78:31:78:74 | require ... ).query | This path depends on a $@. | TaintedPath.js:78:61:78:67 | req.url | user-provided value |
1048910450
| TaintedPath.js:79:31:79:73 | require ... ).query | TaintedPath.js:79:60:79:66 | req.url | TaintedPath.js:79:31:79:73 | require ... ).query | This path depends on a $@. | TaintedPath.js:79:60:79:66 | req.url | user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ angular.module('myApp', [])
6868
})
6969
.directive('myCustomer', function() {
7070
return {
71-
templateUrl: Cookie.get("unsafe") // NOT OK
71+
templateUrl: Cookie.get("unsafe") // OK - (no longer flagged by this query)
7272
}
7373
})
7474

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,12 @@ nodes
8686
| angular2-client.ts:44:44:44:76 | routeSn ... ('foo') |
8787
| angular2-client.ts:44:44:44:76 | routeSn ... ('foo') |
8888
| angular2-client.ts:44:44:44:76 | routeSn ... ('foo') |
89+
| angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") |
90+
| angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") |
91+
| angular-tempate-url.js:13:30:13:31 | ev |
92+
| angular-tempate-url.js:13:30:13:31 | ev |
93+
| angular-tempate-url.js:14:26:14:27 | ev |
94+
| angular-tempate-url.js:14:26:14:32 | ev.data |
8995
| classnames.js:7:31:7:84 | `<span ... <span>` |
9096
| classnames.js:7:31:7:84 | `<span ... <span>` |
9197
| classnames.js:7:47:7:69 | classNa ... w.name) |
@@ -1275,6 +1281,11 @@ edges
12751281
| angular2-client.ts:38:44:38:58 | this.router.url | angular2-client.ts:38:44:38:58 | this.router.url |
12761282
| angular2-client.ts:40:45:40:59 | this.router.url | angular2-client.ts:40:45:40:59 | this.router.url |
12771283
| angular2-client.ts:44:44:44:76 | routeSn ... ('foo') | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') |
1284+
| angular-tempate-url.js:13:30:13:31 | ev | angular-tempate-url.js:14:26:14:27 | ev |
1285+
| angular-tempate-url.js:13:30:13:31 | ev | angular-tempate-url.js:14:26:14:27 | ev |
1286+
| angular-tempate-url.js:14:26:14:27 | ev | angular-tempate-url.js:14:26:14:32 | ev.data |
1287+
| angular-tempate-url.js:14:26:14:32 | ev.data | angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") |
1288+
| angular-tempate-url.js:14:26:14:32 | ev.data | angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") |
12781289
| classnames.js:7:47:7:69 | classNa ... w.name) | classnames.js:7:31:7:84 | `<span ... <span>` |
12791290
| classnames.js:7:47:7:69 | classNa ... w.name) | classnames.js:7:31:7:84 | `<span ... <span>` |
12801291
| classnames.js:7:58:7:68 | window.name | classnames.js:7:47:7:69 | classNa ... w.name) |
@@ -2407,6 +2418,7 @@ edges
24072418
| angular2-client.ts:38:44:38:58 | this.router.url | angular2-client.ts:38:44:38:58 | this.router.url | angular2-client.ts:38:44:38:58 | this.router.url | Cross-site scripting vulnerability due to $@. | angular2-client.ts:38:44:38:58 | this.router.url | user-provided value |
24082419
| angular2-client.ts:40:45:40:59 | this.router.url | angular2-client.ts:40:45:40:59 | this.router.url | angular2-client.ts:40:45:40:59 | this.router.url | Cross-site scripting vulnerability due to $@. | angular2-client.ts:40:45:40:59 | this.router.url | user-provided value |
24092420
| angular2-client.ts:44:44:44:76 | routeSn ... ('foo') | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') | Cross-site scripting vulnerability due to $@. | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') | user-provided value |
2421+
| angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") | angular-tempate-url.js:13:30:13:31 | ev | angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") | Cross-site scripting vulnerability due to $@. | angular-tempate-url.js:13:30:13:31 | ev | user-provided value |
24102422
| classnames.js:7:31:7:84 | `<span ... <span>` | classnames.js:7:58:7:68 | window.name | classnames.js:7:31:7:84 | `<span ... <span>` | Cross-site scripting vulnerability due to $@. | classnames.js:7:58:7:68 | window.name | user-provided value |
24112423
| classnames.js:8:31:8:85 | `<span ... <span>` | classnames.js:8:59:8:69 | window.name | classnames.js:8:31:8:85 | `<span ... <span>` | Cross-site scripting vulnerability due to $@. | classnames.js:8:59:8:69 | window.name | user-provided value |
24122424
| classnames.js:9:31:9:85 | `<span ... <span>` | classnames.js:9:59:9:69 | window.name | classnames.js:9:31:9:85 | `<span ... <span>` | Cross-site scripting vulnerability due to $@. | classnames.js:9:59:9:69 | window.name | user-provided value |

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,12 @@ nodes
8686
| angular2-client.ts:44:44:44:76 | routeSn ... ('foo') |
8787
| angular2-client.ts:44:44:44:76 | routeSn ... ('foo') |
8888
| angular2-client.ts:44:44:44:76 | routeSn ... ('foo') |
89+
| angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") |
90+
| angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") |
91+
| angular-tempate-url.js:13:30:13:31 | ev |
92+
| angular-tempate-url.js:13:30:13:31 | ev |
93+
| angular-tempate-url.js:14:26:14:27 | ev |
94+
| angular-tempate-url.js:14:26:14:32 | ev.data |
8995
| classnames.js:7:31:7:84 | `<span ... <span>` |
9096
| classnames.js:7:31:7:84 | `<span ... <span>` |
9197
| classnames.js:7:47:7:69 | classNa ... w.name) |
@@ -1325,6 +1331,11 @@ edges
13251331
| angular2-client.ts:38:44:38:58 | this.router.url | angular2-client.ts:38:44:38:58 | this.router.url |
13261332
| angular2-client.ts:40:45:40:59 | this.router.url | angular2-client.ts:40:45:40:59 | this.router.url |
13271333
| angular2-client.ts:44:44:44:76 | routeSn ... ('foo') | angular2-client.ts:44:44:44:76 | routeSn ... ('foo') |
1334+
| angular-tempate-url.js:13:30:13:31 | ev | angular-tempate-url.js:14:26:14:27 | ev |
1335+
| angular-tempate-url.js:13:30:13:31 | ev | angular-tempate-url.js:14:26:14:27 | ev |
1336+
| angular-tempate-url.js:14:26:14:27 | ev | angular-tempate-url.js:14:26:14:32 | ev.data |
1337+
| angular-tempate-url.js:14:26:14:32 | ev.data | angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") |
1338+
| angular-tempate-url.js:14:26:14:32 | ev.data | angular-tempate-url.js:9:26:9:45 | Cookie.get("unsafe") |
13281339
| classnames.js:7:47:7:69 | classNa ... w.name) | classnames.js:7:31:7:84 | `<span ... <span>` |
13291340
| classnames.js:7:47:7:69 | classNa ... w.name) | classnames.js:7:31:7:84 | `<span ... <span>` |
13301341
| classnames.js:7:58:7:68 | window.name | classnames.js:7:47:7:69 | classNa ... w.name) |

‎javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/angular-tempate-url.js

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
angular.module('myApp', [])
2+
.directive('myCustomer', function() {
3+
return {
4+
templateUrl: "SAFE" // OK
5+
}
6+
})
7+
.directive('myCustomer', function() {
8+
return {
9+
templateUrl: Cookie.get("unsafe") // NOT OK
10+
}
11+
});
12+
13+
addEventListener('message', (ev) => {
14+
Cookie.set("unsafe", ev.data);
15+
});

0 commit comments

Comments
 (0)