Merge pull request #19192 from asgerf/js/name-resolution-independent-fixes

JS: Some preliminary fixes from name resolution branch
2026-04-25 08:45:14 +02:00 · 2025-04-03 09:36:02 +02:00
parent 52660fa57d 6c3bc941c5
commit bb15f30ef6
7 changed files with 36 additions and 20 deletions
--- a/javascript/ql/lib/semmle/javascript/DOM.qll
+++ b/javascript/ql/lib/semmle/javascript/DOM.qll
@@ -247,7 +247,7 @@ module DOM {
        ]
    |
      (
-        result = documentRef().getAMethodCall(collectionName) or
+        result = domValueRef().getAMethodCall(collectionName) or
        result = DataFlow::globalVarRef(collectionName).getACall()
      )
    )
@@ -441,10 +441,12 @@ module DOM {
  DataFlow::SourceNode domValueRef() {
    result = domValueRef(DataFlow::TypeTracker::end())
    or
-    result.hasUnderlyingType("Element")
+    result.hasUnderlyingType(["Element", "HTMLCollection", "HTMLCollectionOf"])
    or
    result.hasUnderlyingType(any(string s | s.matches("HTML%Element")))
    or
+    result = documentRef()
+    or
    exists(DataFlow::ClassNode cls |
      cls.getASuperClassNode().getALocalSource() =
        DataFlow::globalVarRef(any(string s | s.matches("HTML%Element"))) and
--- a/javascript/ql/src/Security/CWE-327/BadRandomness.ql
+++ b/javascript/ql/src/Security/CWE-327/BadRandomness.ql
@@ -30,30 +30,26 @@ private int powerOfTwo() {
 * Gets a node that has value 2^n for some n.
 */
 private DataFlow::Node isPowerOfTwo() {
-  exists(DataFlow::Node prev |
-    prev.getIntValue() = powerOfTwo()
-    or
-    // Getting around the 32 bit ints in QL. These are some hex values of the form 0x10000000
-    prev.asExpr().(NumberLiteral).getValue() =
-      ["281474976710656", "17592186044416", "1099511627776", "68719476736", "4294967296"]
-  |
-    result = prev.getASuccessor*()
-  )
+  result.getIntValue() = powerOfTwo()
+  or
+  // Getting around the 32 bit ints in QL. These are some hex values of the form 0x10000000
+  result.asExpr().(NumberLiteral).getValue() =
+    ["281474976710656", "17592186044416", "1099511627776", "68719476736", "4294967296"]
+  or
+  result = isPowerOfTwo().getASuccessor()
 }

 /**
 * Gets a node that has value (2^n)-1 for some n.
 */
 private DataFlow::Node isPowerOfTwoMinusOne() {
-  exists(DataFlow::Node prev |
-    prev.getIntValue() = powerOfTwo() - 1
-    or
-    // Getting around the 32 bit ints in QL. These are some hex values of the form 0xfffffff
-    prev.asExpr().(NumberLiteral).getValue() =
-      ["281474976710655", "17592186044415", "1099511627775", "68719476735", "4294967295"]
-  |
-    result = prev.getASuccessor*()
-  )
+  result.getIntValue() = powerOfTwo() - 1
+  or
+  // Getting around the 32 bit ints in QL. These are some hex values of the form 0xfffffff
+  result.asExpr().(NumberLiteral).getValue() =
+    ["281474976710655", "17592186044415", "1099511627775", "68719476735", "4294967295"]
+  or
+  result = isPowerOfTwoMinusOne().getASuccessor()
 }

 /**
--- a/javascript/ql/src/change-notes/2025-04-02-name-resolution-independent-fixes.md
+++ b/javascript/ql/src/change-notes/2025-04-02-name-resolution-independent-fixes.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Slightly improved detection of DOM element references, leading to XSS results being detected in more cases.
--- a/javascript/ql/test/library-tests/DOM/Customizations.expected
+++ b/javascript/ql/test/library-tests/DOM/Customizations.expected
@@ -7,20 +7,26 @@ test_documentRef
 test_locationRef
 | customization.js:3:3:3:14 | doc.location |
 test_domValueRef
+| customization.js:2:13:2:31 | customGetDocument() |
+| customization.js:3:3:3:14 | doc.location |
 | 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:8 | document |
 | 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:8 | document |
 | 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:8 | document |
 | 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') |
+| querySelectorAll.js:2:5:2:12 | document |
 | querySelectorAll.js:2:5:2:29 | documen ... ctorAll |
 | querySelectorAll.js:2:5:2:36 | documen ... ('foo') |
 | querySelectorAll.js:2:46:2:48 | elm |
--- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected
@@ -53,6 +53,7 @@
 | dates.js:57:31:57:101 | `Time i ... aint)}` | dates.js:54:36:54:55 | window.location.hash | dates.js:57:31:57:101 | `Time i ... aint)}` | Cross-site scripting vulnerability due to $@. | dates.js:54:36:54:55 | window.location.hash | user-provided value |
 | dates.js:59:31:59:87 | `Time i ... aint)}` | dates.js:54:36:54:55 | window.location.hash | dates.js:59:31:59:87 | `Time i ... aint)}` | Cross-site scripting vulnerability due to $@. | dates.js:54:36:54:55 | window.location.hash | user-provided value |
 | dates.js:61:31:61:88 | `Time i ... aint)}` | dates.js:54:36:54:55 | window.location.hash | dates.js:61:31:61:88 | `Time i ... aint)}` | Cross-site scripting vulnerability due to $@. | dates.js:54:36:54:55 | window.location.hash | user-provided value |
+| dom.js:4:20:4:30 | window.name | dom.js:4:20:4:30 | window.name | dom.js:4:20:4:30 | window.name | Cross-site scripting vulnerability due to $@. | dom.js:4:20:4:30 | window.name | user-provided value |
 | dragAndDrop.ts:15:25:15:28 | html | dragAndDrop.ts:8:18:8:50 | dataTra ... /html') | dragAndDrop.ts:15:25:15:28 | html | Cross-site scripting vulnerability due to $@. | dragAndDrop.ts:8:18:8:50 | dataTra ... /html') | user-provided value |
 | dragAndDrop.ts:24:23:24:57 | e.dataT ... /html') | dragAndDrop.ts:24:23:24:57 | e.dataT ... /html') | dragAndDrop.ts:24:23:24:57 | e.dataT ... /html') | Cross-site scripting vulnerability due to $@. | dragAndDrop.ts:24:23:24:57 | e.dataT ... /html') | user-provided value |
 | dragAndDrop.ts:29:19:29:53 | e.dataT ... /html') | dragAndDrop.ts:29:19:29:53 | e.dataT ... /html') | dragAndDrop.ts:29:19:29:53 | e.dataT ... /html') | Cross-site scripting vulnerability due to $@. | dragAndDrop.ts:29:19:29:53 | e.dataT ... /html') | user-provided value |
@@ -937,6 +938,7 @@ nodes
 | dates.js:61:31:61:88 | `Time i ... aint)}` | semmle.label | `Time i ... aint)}` |
 | dates.js:61:42:61:86 | dayjs.s ... (taint) | semmle.label | dayjs.s ... (taint) |
 | dates.js:61:81:61:85 | taint | semmle.label | taint |
+| dom.js:4:20:4:30 | window.name | semmle.label | window.name |
 | dragAndDrop.ts:8:11:8:50 | html | semmle.label | html |
 | dragAndDrop.ts:8:18:8:50 | dataTra ... /html') | semmle.label | dataTra ... /html') |
 | dragAndDrop.ts:15:25:15:28 | html | semmle.label | html |
--- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected
@@ -138,6 +138,7 @@ nodes
 | dates.js:61:31:61:88 | `Time i ... aint)}` | semmle.label | `Time i ... aint)}` |
 | dates.js:61:42:61:86 | dayjs.s ... (taint) | semmle.label | dayjs.s ... (taint) |
 | dates.js:61:81:61:85 | taint | semmle.label | taint |
+| dom.js:4:20:4:30 | window.name | semmle.label | window.name |
 | dragAndDrop.ts:8:11:8:50 | html | semmle.label | html |
 | dragAndDrop.ts:8:18:8:50 | dataTra ... /html') | semmle.label | dataTra ... /html') |
 | dragAndDrop.ts:15:25:15:28 | html | semmle.label | html |
--- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/dom.js
+++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/dom.js
@@ -0,0 +1,5 @@
+function t1() {
+    const elm = document.getElementById("foo");
+    const e2 = elm.getElementsByTagName("bar")[0];
+    e2.innerHTML = window.name; // $ Alert
+}