Merge pull request #5262 from max-schaefer/event-handler-receiver-is-dom-element

Approved by asgerf
2025-12-17 01:03:14 +01:00 · 2021-03-05 02:04:59 -08:00
parent 00983c8967 f93937f40a
commit d7b9251b0d
11 changed files with 102 additions and 0 deletions
--- a/javascript/change-notes/2021-02-25-event-handler-receiver-is-dom-element.md
+++ b/javascript/change-notes/2021-02-25-event-handler-receiver-is-dom-element.md
@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Modelling of DOM event handlers has been improved, enabling the `js/xss` query to flag additional alerts.
--- a/javascript/ql/src/semmle/javascript/DOM.qll
+++ b/javascript/ql/src/semmle/javascript/DOM.qll
@@ -353,6 +353,21 @@ module DOM {
          this = DataFlow::thisNode(eachCall.getCallback(0).getFunction()) or
          this = eachCall.getABoundCallbackParameter(0, 1)
        )
+        or
+        // A receiver node of an event handler on a DOM node
+        exists(DataFlow::SourceNode domNode, DataFlow::FunctionNode eventHandler |
+          // NOTE: we do not use `getABoundFunctionValue()`, since bound functions tend to have
+          // a different receiver anyway
+          eventHandler = domNode.getAPropertySource(any(string n | n.matches("on%")))
+          or
+          eventHandler =
+            domNode.getAMethodCall("addEventListener").getArgument(1).getAFunctionValue()
+        |
+          domNode = domValueRef() and
+          this = eventHandler.getReceiver()
+        )
+        or
+        this = DataFlow::thisNode(any(EventHandlerCode evt))
      }
    }
  }
--- a/javascript/ql/test/library-tests/DOM/Customizations.expected
+++ b/javascript/ql/test/library-tests/DOM/Customizations.expected
@@ -1,11 +1,22 @@
 test_documentRef
 | customization.js:2:13:2:31 | customGetDocument() |
+| event-handler-receiver.js:1:1:1:8 | document |
+| event-handler-receiver.js:5:1:5:8 | document |
 | nameditems.js:1:1:1:8 | document |
 test_locationRef
 | customization.js:3:3:3:14 | doc.location |
 test_domValueRef
 | customization.js:4:3:4:20 | doc.getElementById |
 | customization.js:4:3:4:28 | doc.get ... 'test') |
+| event-handler-receiver.html:4:20:4:19 | this |
+| event-handler-receiver.js:1:1:1:23 | documen ... entById |
+| event-handler-receiver.js:1:1:1:32 | documen ... my-id') |
+| event-handler-receiver.js:1:44:1:43 | this |
+| event-handler-receiver.js:2:3:2:17 | this.parentNode |
+| event-handler-receiver.js:5:1:5:23 | documen ... entById |
+| event-handler-receiver.js:5:1:5:32 | documen ... my-id') |
+| event-handler-receiver.js:5:60:5:59 | this |
+| event-handler-receiver.js:6:3:6:17 | this.parentNode |
 | nameditems.js:1:1:1:23 | documen ... entById |
 | nameditems.js:1:1:1:30 | documen ... ('foo') |
 | nameditems.js:1:1:2:19 | documen ... em('x') |
--- a/javascript/ql/test/library-tests/DOM/event-handler-receiver.html
+++ b/javascript/ql/test/library-tests/DOM/event-handler-receiver.html
@@ -0,0 +1,6 @@
+<html>
+<head></head>
+<body>
+  <button onclick="alert(this.tagName);">Click me</button>
+</body>
+</html>
--- a/javascript/ql/test/library-tests/DOM/event-handler-receiver.js
+++ b/javascript/ql/test/library-tests/DOM/event-handler-receiver.js
@@ -0,0 +1,7 @@
+document.getElementById('my-id').onclick = function () {
+  this.parentNode.innerHTML = '<b>hello</b>'; // `this` is a DOM element
+};
+
+document.getElementById('my-id').addEventListener("click", function (ev) {
+  this.parentNode.innerHTML = '<b>hello</b>'; // `this` is a DOM element
+});
--- a/javascript/ql/test/library-tests/DOM/externs/externs.js
+++ b/javascript/ql/test/library-tests/DOM/externs/externs.js
@@ -18,3 +18,14 @@ function WorkerGlobalScope() {}

 /** @type {WorkerLocation} */
 WorkerGlobalScope.prototype.location;
+
+/**
+ * @constructor
+ * @implements {EventTarget}
+ */
+function Node() {}
+
+/**
+ * @type {Node}
+ */
+Node.prototype.parentNode;
--- a/javascript/ql/test/library-tests/DOM/tests.expected
+++ b/javascript/ql/test/library-tests/DOM/tests.expected
@@ -1,4 +1,5 @@
 test_AttributeDefinition
+| event-handler-receiver.html:4:11:4:40 | onclick=alert(this.tagName); |
 | tst.html:3:6:3:30 | href=https://semmle.com |
 | tst.html:3:32:3:46 | target=_blank |
 | tst.js:2:22:2:37 | target: "_blank" |
@@ -13,12 +14,17 @@ test_AttributeDefinition
 | tst.jsx:4:40:4:48 | rel={rel} |
 | tst.jsx:4:50:4:64 | {...otherAttrs} |
 test_ElementDefinition_getAttribute
+| event-handler-receiver.html:4:3:4:58 | <button>...</> | 0 | event-handler-receiver.html:4:11:4:40 | onclick=alert(this.tagName); |
 | tst.html:3:3:3:57 | <a>...</> | 0 | tst.html:3:6:3:30 | href=https://semmle.com |
 | tst.html:3:3:3:57 | <a>...</> | 1 | tst.html:3:32:3:46 | target=_blank |
 | tst.jsx:4:11:4:75 | <a href ... mle</a> | 0 | tst.jsx:4:14:4:38 | href="h ... le.com" |
 | tst.jsx:4:11:4:75 | <a href ... mle</a> | 1 | tst.jsx:4:40:4:48 | rel={rel} |
 | tst.jsx:4:11:4:75 | <a href ... mle</a> | 2 | tst.jsx:4:50:4:64 | {...otherAttrs} |
 test_ElementDefinition_getRoot
+| event-handler-receiver.html:1:1:6:7 | <html>...</> | event-handler-receiver.html:1:1:6:7 | <html>...</> |
+| event-handler-receiver.html:2:1:2:13 | <head>...</> | event-handler-receiver.html:1:1:6:7 | <html>...</> |
+| event-handler-receiver.html:3:1:5:7 | <body>...</> | event-handler-receiver.html:1:1:6:7 | <html>...</> |
+| event-handler-receiver.html:4:3:4:58 | <button>...</> | event-handler-receiver.html:1:1:6:7 | <html>...</> |
 | tst.html:1:1:5:7 | <html>...</> | tst.html:1:1:5:7 | <html>...</> |
 | tst.html:2:1:4:7 | <body>...</> | tst.html:1:1:5:7 | <html>...</> |
 | tst.html:3:3:3:57 | <a>...</> | tst.html:1:1:5:7 | <html>...</> |
@@ -36,6 +42,7 @@ test_WebStorageWrite
 | tst.js:17:24:17:30 | "value" |
 | tst.js:18:33:18:39 | "value" |
 test_ElementDefinition_getAttributeByName
+| event-handler-receiver.html:4:3:4:58 | <button>...</> | onclick | event-handler-receiver.html:4:11:4:40 | onclick=alert(this.tagName); |
 | tst.html:3:3:3:57 | <a>...</> | href | tst.html:3:6:3:30 | href=https://semmle.com |
 | tst.html:3:3:3:57 | <a>...</> | target | tst.html:3:32:3:46 | target=_blank |
 | 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
 | tst.jsx:4:11:4:75 | <a href ... mle</a> | href | tst.jsx:4:14:4:38 | href="h ... le.com" |
 | tst.jsx:4:11:4:75 | <a href ... mle</a> | rel | tst.jsx:4:40:4:48 | rel={rel} |
 test_AttributeDefinition_getStringValue
+| event-handler-receiver.html:4:11:4:40 | onclick=alert(this.tagName); | alert(this.tagName); |
 | tst.html:3:6:3:30 | href=https://semmle.com | https://semmle.com |
 | tst.html:3:32:3:46 | target=_blank | _blank |
 | tst.js:2:22:2:37 | target: "_blank" | _blank |
@@ -61,6 +69,7 @@ test_AttributeDefinition_getStringValue
 | tst.js:13:3:13:28 | $.prop( ... d", "") |  |
 | tst.jsx:4:14:4:38 | href="h ... le.com" | https://semmle.com |
 test_AttributeDefinition_getName
+| event-handler-receiver.html:4:11:4:40 | onclick=alert(this.tagName); | onclick |
 | tst.html:3:6:3:30 | href=https://semmle.com | href |
 | tst.html:3:32:3:46 | target=_blank | target |
 | tst.js:2:22:2:37 | target: "_blank" | target |
@@ -74,6 +83,10 @@ test_AttributeDefinition_getName
 | tst.jsx:4:14:4:38 | href="h ... le.com" | href |
 | tst.jsx:4:40:4:48 | rel={rel} | rel |
 test_Element
+| event-handler-receiver.html:1:1:6:7 | <html>...</> | event-handler-receiver.html:1:1:6:7 | <html>...</> |
+| event-handler-receiver.html:2:1:2:13 | <head>...</> | event-handler-receiver.html:2:1:2:13 | <head>...</> |
+| event-handler-receiver.html:3:1:5:7 | <body>...</> | event-handler-receiver.html:3:1:5:7 | <body>...</> |
+| event-handler-receiver.html:4:3:4:58 | <button>...</> | event-handler-receiver.html:4:3:4:58 | <button>...</> |
 | tst.html:1:1:5:7 | <html>...</> | tst.html:1:1:5:7 | <html>...</> |
 | tst.html:2:1:4:7 | <body>...</> | tst.html:2:1:4:7 | <body>...</> |
 | tst.html:3:3:3:57 | <a>...</> | tst.html:3:3:3:57 | <a>...</> |
@@ -110,6 +123,10 @@ test_AttributeDefinition_getValueNode
 | tst.jsx:4:40:4:48 | rel={rel} | tst.jsx:4:45:4:47 | rel |
 | tst.jsx:4:50:4:64 | {...otherAttrs} | tst.jsx:4:50:4:64 | ...otherAttrs |
 test_ElementDefinition
+| event-handler-receiver.html:1:1:6:7 | <html>...</> | html |
+| event-handler-receiver.html:2:1:2:13 | <head>...</> | head |
+| event-handler-receiver.html:3:1:5:7 | <body>...</> | body |
+| event-handler-receiver.html:4:3:4:58 | <button>...</> | button |
 | tst.html:1:1:5:7 | <html>...</> | html |
 | tst.html:2:1:4:7 | <body>...</> | body |
 | tst.html:3:3:3:57 | <a>...</> | a |
--- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected
@@ -118,6 +118,11 @@ nodes
 | dates.js:18:31:18:66 | `Time i ... aint)}` |
 | dates.js:18:42:18:64 | datefor ...  taint) |
 | dates.js:18:59:18:63 | taint |
+| event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' |
+| 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:49:2:56 | location |
+| event-handler-receiver.js:2:49:2:61 | location.href |
 | 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") |
@@ -792,6 +797,10 @@ edges
 | dates.js:18:42:18:64 | datefor ...  taint) | dates.js:18:31:18:66 | `Time i ... aint)}` |
 | dates.js:18:42:18:64 | datefor ...  taint) | dates.js:18:31:18:66 | `Time i ... aint)}` |
 | dates.js:18:59:18:63 | taint | dates.js:18:42:18:64 | datefor ...  taint) |
+| event-handler-receiver.js:2:49:2:56 | location | event-handler-receiver.js:2:49:2:61 | location.href |
+| event-handler-receiver.js:2:49:2:56 | location | event-handler-receiver.js:2:49:2:61 | location.href |
+| event-handler-receiver.js:2:49:2:61 | location.href | event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' |
+| event-handler-receiver.js:2:49:2:61 | location.href | event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' |
 | express.js:7:15:7:33 | req.param("wobble") | express.js:7:15:7:33 | req.param("wobble") |
 | jquery.js:2:7:2:40 | tainted | jquery.js:7:20:7:26 | tainted |
 | jquery.js:2:7:2:40 | tainted | jquery.js:8:28:8:34 | tainted |
@@ -1334,6 +1343,7 @@ edges
 | 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 |
 | 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 |
 | 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 |
+| 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 |
 | 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 |
 | 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 |
 | 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 |
--- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected
@@ -118,6 +118,11 @@ nodes
 | dates.js:18:31:18:66 | `Time i ... aint)}` |
 | dates.js:18:42:18:64 | datefor ...  taint) |
 | dates.js:18:59:18:63 | taint |
+| event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' |
+| 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:49:2:56 | location |
+| event-handler-receiver.js:2:49:2:61 | location.href |
 | 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") |
@@ -803,6 +808,10 @@ edges
 | dates.js:18:42:18:64 | datefor ...  taint) | dates.js:18:31:18:66 | `Time i ... aint)}` |
 | dates.js:18:42:18:64 | datefor ...  taint) | dates.js:18:31:18:66 | `Time i ... aint)}` |
 | dates.js:18:59:18:63 | taint | dates.js:18:42:18:64 | datefor ...  taint) |
+| event-handler-receiver.js:2:49:2:56 | location | event-handler-receiver.js:2:49:2:61 | location.href |
+| event-handler-receiver.js:2:49:2:56 | location | event-handler-receiver.js:2:49:2:61 | location.href |
+| event-handler-receiver.js:2:49:2:61 | location.href | event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' |
+| event-handler-receiver.js:2:49:2:61 | location.href | event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' |
 | express.js:7:15:7:33 | req.param("wobble") | express.js:7:15:7:33 | req.param("wobble") |
 | jquery.js:2:7:2:40 | tainted | jquery.js:7:20:7:26 | tainted |
 | jquery.js:2:7:2:40 | tainted | jquery.js:8:28:8:34 | tainted |
--- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/event-handler-receiver.js
+++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/event-handler-receiver.js
@@ -0,0 +1,3 @@
+document.getElementById('my-id').onclick = function() {
+  this.parentNode.innerHTML = '<h2><a href="' + location.href + '">A link</a></h2>'; // NOT OK
+};
--- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/externs.js
+++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/externs.js
@@ -46,3 +46,14 @@ DomObjectStub.prototype.value;
 * @type {!DomObjectStub}
 */
 var document;
+
+/**
+ * @constructor
+ * @implements {EventTarget}
+ */
+function Node() {}
+
+/**
+ * @type {Node}
+ */
+Node.prototype.parentNode;