Skip to content

Commit d7b9251

Browse files
codeql-cicodeql-ci
authored
Merge pull request github#5262 from max-schaefer/event-handler-receiver-is-dom-element
Approved by asgerf
2 parents 00983c8 + f93937f commit d7b9251

File tree

11 files changed

+102
-0
lines changed

11 files changed

+102
-0
lines changed

‎javascript/change-notes/2021-02-25-event-handler-receiver-is-dom-element.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* Modelling of DOM event handlers has been improved, enabling the `js/xss` query to flag additional alerts.

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,21 @@ module DOM {
353353
this = DataFlow::thisNode(eachCall.getCallback(0).getFunction()) or
354354
this = eachCall.getABoundCallbackParameter(0, 1)
355355
)
356+
or
357+
// A receiver node of an event handler on a DOM node
358+
exists(DataFlow::SourceNode domNode, DataFlow::FunctionNode eventHandler |
359+
// NOTE: we do not use `getABoundFunctionValue()`, since bound functions tend to have
360+
// a different receiver anyway
361+
eventHandler = domNode.getAPropertySource(any(string n | n.matches("on%")))
362+
or
363+
eventHandler =
364+
domNode.getAMethodCall("addEventListener").getArgument(1).getAFunctionValue()
365+
|
366+
domNode = domValueRef() and
367+
this = eventHandler.getReceiver()
368+
)
369+
or
370+
this = DataFlow::thisNode(any(EventHandlerCode evt))
356371
}
357372
}
358373
}

‎javascript/ql/test/library-tests/DOM/Customizations.expected

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,22 @@
11
test_documentRef
22
| customization.js:2:13:2:31 | customGetDocument() |
3+
| event-handler-receiver.js:1:1:1:8 | document |
4+
| event-handler-receiver.js:5:1:5:8 | document |
35
| nameditems.js:1:1:1:8 | document |
46
test_locationRef
57
| customization.js:3:3:3:14 | doc.location |
68
test_domValueRef
79
| customization.js:4:3:4:20 | doc.getElementById |
810
| customization.js:4:3:4:28 | doc.get ... 'test') |
11+
| event-handler-receiver.html:4:20:4:19 | this |
12+
| event-handler-receiver.js:1:1:1:23 | documen ... entById |
13+
| event-handler-receiver.js:1:1:1:32 | documen ... my-id') |
14+
| event-handler-receiver.js:1:44:1:43 | this |
15+
| event-handler-receiver.js:2:3:2:17 | this.parentNode |
16+
| event-handler-receiver.js:5:1:5:23 | documen ... entById |
17+
| event-handler-receiver.js:5:1:5:32 | documen ... my-id') |
18+
| event-handler-receiver.js:5:60:5:59 | this |
19+
| event-handler-receiver.js:6:3:6:17 | this.parentNode |
920
| nameditems.js:1:1:1:23 | documen ... entById |
1021
| nameditems.js:1:1:1:30 | documen ... ('foo') |
1122
| nameditems.js:1:1:2:19 | documen ... em('x') |

‎javascript/ql/test/library-tests/DOM/event-handler-receiver.html

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
<html>
2+
<head></head>
3+
<body>
4+
<button onclick="alert(this.tagName);">Click me</button>
5+
</body>
6+
</html>

‎javascript/ql/test/library-tests/DOM/event-handler-receiver.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
document.getElementById('my-id').onclick = function () {
2+
this.parentNode.innerHTML = '<b>hello</b>'; // `this` is a DOM element
3+
};
4+
5+
document.getElementById('my-id').addEventListener("click", function (ev) {
6+
this.parentNode.innerHTML = '<b>hello</b>'; // `this` is a DOM element
7+
});

‎javascript/ql/test/library-tests/DOM/externs/externs.js

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,3 +18,14 @@ function WorkerGlobalScope() {}
1818

1919
/** @type {WorkerLocation} */
2020
WorkerGlobalScope.prototype.location;
21+
22+
/**
23+
* @constructor
24+
* @implements {EventTarget}
25+
*/
26+
function Node() {}
27+
28+
/**
29+
* @type {Node}
30+
*/
31+
Node.prototype.parentNode;

‎javascript/ql/test/library-tests/DOM/tests.expected

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
test_AttributeDefinition
2+
| event-handler-receiver.html:4:11:4:40 | onclick=alert(this.tagName); |
23
| tst.html:3:6:3:30 | href=https://semmle.com |
34
| tst.html:3:32:3:46 | target=_blank |
45
| tst.js:2:22:2:37 | target: "_blank" |
@@ -13,12 +14,17 @@ test_AttributeDefinition
1314
| tst.jsx:4:40:4:48 | rel={rel} |
1415
| tst.jsx:4:50:4:64 | {...otherAttrs} |
1516
test_ElementDefinition_getAttribute
17+
| event-handler-receiver.html:4:3:4:58 | <button>...</> | 0 | event-handler-receiver.html:4:11:4:40 | onclick=alert(this.tagName); |
1618
| tst.html:3:3:3:57 | <a>...</> | 0 | tst.html:3:6:3:30 | href=https://semmle.com |
1719
| tst.html:3:3:3:57 | <a>...</> | 1 | tst.html:3:32:3:46 | target=_blank |
1820
| tst.jsx:4:11:4:75 | <a href ... mle</a> | 0 | tst.jsx:4:14:4:38 | href="h ... le.com" |
1921
| tst.jsx:4:11:4:75 | <a href ... mle</a> | 1 | tst.jsx:4:40:4:48 | rel={rel} |
2022
| tst.jsx:4:11:4:75 | <a href ... mle</a> | 2 | tst.jsx:4:50:4:64 | {...otherAttrs} |
2123
test_ElementDefinition_getRoot
24+
| event-handler-receiver.html:1:1:6:7 | <html>...</> | event-handler-receiver.html:1:1:6:7 | <html>...</> |
25+
| event-handler-receiver.html:2:1:2:13 | <head>...</> | event-handler-receiver.html:1:1:6:7 | <html>...</> |
26+
| event-handler-receiver.html:3:1:5:7 | <body>...</> | event-handler-receiver.html:1:1:6:7 | <html>...</> |
27+
| event-handler-receiver.html:4:3:4:58 | <button>...</> | event-handler-receiver.html:1:1:6:7 | <html>...</> |
2228
| tst.html:1:1:5:7 | <html>...</> | tst.html:1:1:5:7 | <html>...</> |
2329
| tst.html:2:1:4:7 | <body>...</> | tst.html:1:1:5:7 | <html>...</> |
2430
| tst.html:3:3:3:57 | <a>...</> | tst.html:1:1:5:7 | <html>...</> |
@@ -36,6 +42,7 @@ test_WebStorageWrite
3642
| tst.js:17:24:17:30 | "value" |
3743
| tst.js:18:33:18:39 | "value" |
3844
test_ElementDefinition_getAttributeByName
45+
| event-handler-receiver.html:4:3:4:58 | <button>...</> | onclick | event-handler-receiver.html:4:11:4:40 | onclick=alert(this.tagName); |
3946
| tst.html:3:3:3:57 | <a>...</> | href | tst.html:3:6:3:30 | href=https://semmle.com |
4047
| tst.html:3:3:3:57 | <a>...</> | target | tst.html:3:32:3:46 | target=_blank |
4148
| tst.js:3:11:3:31 | $("<a/> ... rAttrs) | data-bind | tst.js:6:5:6:24 | "data-bind": "stuff" |
@@ -49,6 +56,7 @@ test_ElementDefinition_getAttributeByName
4956
| tst.jsx:4:11:4:75 | <a href ... mle</a> | href | tst.jsx:4:14:4:38 | href="h ... le.com" |
5057
| tst.jsx:4:11:4:75 | <a href ... mle</a> | rel | tst.jsx:4:40:4:48 | rel={rel} |
5158
test_AttributeDefinition_getStringValue
59+
| event-handler-receiver.html:4:11:4:40 | onclick=alert(this.tagName); | alert(this.tagName); |
5260
| tst.html:3:6:3:30 | href=https://semmle.com | https://semmle.com |
5361
| tst.html:3:32:3:46 | target=_blank | _blank |
5462
| tst.js:2:22:2:37 | target: "_blank" | _blank |
@@ -61,6 +69,7 @@ test_AttributeDefinition_getStringValue
6169
| tst.js:13:3:13:28 | $.prop( ... d", "") | |
6270
| tst.jsx:4:14:4:38 | href="h ... le.com" | https://semmle.com |
6371
test_AttributeDefinition_getName
72+
| event-handler-receiver.html:4:11:4:40 | onclick=alert(this.tagName); | onclick |
6473
| tst.html:3:6:3:30 | href=https://semmle.com | href |
6574
| tst.html:3:32:3:46 | target=_blank | target |
6675
| tst.js:2:22:2:37 | target: "_blank" | target |
@@ -74,6 +83,10 @@ test_AttributeDefinition_getName
7483
| tst.jsx:4:14:4:38 | href="h ... le.com" | href |
7584
| tst.jsx:4:40:4:48 | rel={rel} | rel |
7685
test_Element
86+
| event-handler-receiver.html:1:1:6:7 | <html>...</> | event-handler-receiver.html:1:1:6:7 | <html>...</> |
87+
| event-handler-receiver.html:2:1:2:13 | <head>...</> | event-handler-receiver.html:2:1:2:13 | <head>...</> |
88+
| event-handler-receiver.html:3:1:5:7 | <body>...</> | event-handler-receiver.html:3:1:5:7 | <body>...</> |
89+
| event-handler-receiver.html:4:3:4:58 | <button>...</> | event-handler-receiver.html:4:3:4:58 | <button>...</> |
7790
| tst.html:1:1:5:7 | <html>...</> | tst.html:1:1:5:7 | <html>...</> |
7891
| tst.html:2:1:4:7 | <body>...</> | tst.html:2:1:4:7 | <body>...</> |
7992
| tst.html:3:3:3:57 | <a>...</> | tst.html:3:3:3:57 | <a>...</> |
@@ -110,6 +123,10 @@ test_AttributeDefinition_getValueNode
110123
| tst.jsx:4:40:4:48 | rel={rel} | tst.jsx:4:45:4:47 | rel |
111124
| tst.jsx:4:50:4:64 | {...otherAttrs} | tst.jsx:4:50:4:64 | ...otherAttrs |
112125
test_ElementDefinition
126+
| event-handler-receiver.html:1:1:6:7 | <html>...</> | html |
127+
| event-handler-receiver.html:2:1:2:13 | <head>...</> | head |
128+
| event-handler-receiver.html:3:1:5:7 | <body>...</> | body |
129+
| event-handler-receiver.html:4:3:4:58 | <button>...</> | button |
113130
| tst.html:1:1:5:7 | <html>...</> | html |
114131
| tst.html:2:1:4:7 | <body>...</> | body |
115132
| tst.html:3:3:3:57 | <a>...</> | a |

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,11 @@ nodes
118118
| dates.js:18:31:18:66 | `Time i ... aint)}` |
119119
| dates.js:18:42:18:64 | datefor ... taint) |
120120
| dates.js:18:59:18:63 | taint |
121+
| event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' |
122+
| event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' |
123+
| event-handler-receiver.js:2:49:2:56 | location |
124+
| event-handler-receiver.js:2:49:2:56 | location |
125+
| event-handler-receiver.js:2:49:2:61 | location.href |
121126
| express.js:7:15:7:33 | req.param("wobble") |
122127
| express.js:7:15:7:33 | req.param("wobble") |
123128
| express.js:7:15:7:33 | req.param("wobble") |
@@ -792,6 +797,10 @@ edges
792797
| dates.js:18:42:18:64 | datefor ... taint) | dates.js:18:31:18:66 | `Time i ... aint)}` |
793798
| dates.js:18:42:18:64 | datefor ... taint) | dates.js:18:31:18:66 | `Time i ... aint)}` |
794799
| dates.js:18:59:18:63 | taint | dates.js:18:42:18:64 | datefor ... taint) |
800+
| event-handler-receiver.js:2:49:2:56 | location | event-handler-receiver.js:2:49:2:61 | location.href |
801+
| event-handler-receiver.js:2:49:2:56 | location | event-handler-receiver.js:2:49:2:61 | location.href |
802+
| event-handler-receiver.js:2:49:2:61 | location.href | event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' |
803+
| event-handler-receiver.js:2:49:2:61 | location.href | event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' |
795804
| express.js:7:15:7:33 | req.param("wobble") | express.js:7:15:7:33 | req.param("wobble") |
796805
| jquery.js:2:7:2:40 | tainted | jquery.js:7:20:7:26 | tainted |
797806
| jquery.js:2:7:2:40 | tainted | jquery.js:8:28:8:34 | tainted |
@@ -1334,6 +1343,7 @@ edges
13341343
| dates.js:13:31:13:72 | `Time i ... time)}` | dates.js:9:36:9:50 | window.location | dates.js:13:31:13:72 | `Time i ... time)}` | Cross-site scripting vulnerability due to $@. | dates.js:9:36:9:50 | window.location | user-provided value |
13351344
| dates.js:16:31:16:69 | `Time i ... aint)}` | dates.js:9:36:9:50 | window.location | dates.js:16:31:16:69 | `Time i ... aint)}` | Cross-site scripting vulnerability due to $@. | dates.js:9:36:9:50 | window.location | user-provided value |
13361345
| dates.js:18:31:18:66 | `Time i ... aint)}` | dates.js:9:36:9:50 | window.location | dates.js:18:31:18:66 | `Time i ... aint)}` | Cross-site scripting vulnerability due to $@. | dates.js:9:36:9:50 | window.location | user-provided value |
1346+
| event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' | event-handler-receiver.js:2:49:2:56 | location | event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' | Cross-site scripting vulnerability due to $@. | event-handler-receiver.js:2:49:2:56 | location | user-provided value |
13371347
| express.js:7:15:7:33 | req.param("wobble") | express.js:7:15:7:33 | req.param("wobble") | express.js:7:15:7:33 | req.param("wobble") | Cross-site scripting vulnerability due to $@. | express.js:7:15:7:33 | req.param("wobble") | user-provided value |
13381348
| jquery.js:7:5:7:34 | "<div i ... + "\\">" | jquery.js:2:17:2:40 | documen ... .search | jquery.js:7:5:7:34 | "<div i ... + "\\">" | Cross-site scripting vulnerability due to $@. | jquery.js:2:17:2:40 | documen ... .search | user-provided value |
13391349
| jquery.js:8:18:8:34 | "XSS: " + tainted | jquery.js:2:17:2:33 | document.location | jquery.js:8:18:8:34 | "XSS: " + tainted | Cross-site scripting vulnerability due to $@. | jquery.js:2:17:2:33 | document.location | user-provided value |

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,11 @@ nodes
118118
| dates.js:18:31:18:66 | `Time i ... aint)}` |
119119
| dates.js:18:42:18:64 | datefor ... taint) |
120120
| dates.js:18:59:18:63 | taint |
121+
| event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' |
122+
| event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' |
123+
| event-handler-receiver.js:2:49:2:56 | location |
124+
| event-handler-receiver.js:2:49:2:56 | location |
125+
| event-handler-receiver.js:2:49:2:61 | location.href |
121126
| express.js:7:15:7:33 | req.param("wobble") |
122127
| express.js:7:15:7:33 | req.param("wobble") |
123128
| express.js:7:15:7:33 | req.param("wobble") |
@@ -803,6 +808,10 @@ edges
803808
| dates.js:18:42:18:64 | datefor ... taint) | dates.js:18:31:18:66 | `Time i ... aint)}` |
804809
| dates.js:18:42:18:64 | datefor ... taint) | dates.js:18:31:18:66 | `Time i ... aint)}` |
805810
| dates.js:18:59:18:63 | taint | dates.js:18:42:18:64 | datefor ... taint) |
811+
| event-handler-receiver.js:2:49:2:56 | location | event-handler-receiver.js:2:49:2:61 | location.href |
812+
| event-handler-receiver.js:2:49:2:56 | location | event-handler-receiver.js:2:49:2:61 | location.href |
813+
| event-handler-receiver.js:2:49:2:61 | location.href | event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' |
814+
| event-handler-receiver.js:2:49:2:61 | location.href | event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' |
806815
| express.js:7:15:7:33 | req.param("wobble") | express.js:7:15:7:33 | req.param("wobble") |
807816
| jquery.js:2:7:2:40 | tainted | jquery.js:7:20:7:26 | tainted |
808817
| jquery.js:2:7:2:40 | tainted | jquery.js:8:28:8:34 | tainted |

‎javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/event-handler-receiver.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
document.getElementById('my-id').onclick = function() {
2+
this.parentNode.innerHTML = '<h2><a href="' + location.href + '">A link</a></h2>'; // NOT OK
3+
};

0 commit comments

Comments
 (0)