From 48db2b9315f4eb2ab29f711881f52bfbbe4847ea Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 2 Apr 2025 10:12:16 +0200 Subject: [PATCH 1/5] JS: Add test --- .../ql/test/query-tests/Security/CWE-079/DomBasedXss/dom.js | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/dom.js diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/dom.js b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/dom.js new file mode 100644 index 000000000000..51adf27b6d3c --- /dev/null +++ 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; // $ MISSING: Alert +} From 46f88e7ce765db957f20a7396dca18797e094747 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 2 Apr 2025 09:50:09 +0200 Subject: [PATCH 2/5] JS: Updates to DOM model --- javascript/ql/lib/semmle/javascript/DOM.qll | 6 ++++-- .../query-tests/Security/CWE-079/DomBasedXss/Xss.expected | 2 ++ .../CWE-079/DomBasedXss/XssWithAdditionalSources.expected | 1 + .../ql/test/query-tests/Security/CWE-079/DomBasedXss/dom.js | 2 +- 4 files changed, 8 insertions(+), 3 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/DOM.qll b/javascript/ql/lib/semmle/javascript/DOM.qll index 50a529b4a53a..21854c1a9d0e 100644 --- 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 diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected index 7de1561f79e8..6ba8ab703bff 100644 --- 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 | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected index eb961fc83dbf..7780ae82dc4d 100644 --- 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 | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/dom.js b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/dom.js index 51adf27b6d3c..cf4f0bb3fbe2 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/dom.js +++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/dom.js @@ -1,5 +1,5 @@ function t1() { const elm = document.getElementById("foo"); const e2 = elm.getElementsByTagName("bar")[0]; - e2.innerHTML = window.name; // $ MISSING: Alert + e2.innerHTML = window.name; // $ Alert } From 78b25388ca8dcf3013b357b343ec6d365f45575f Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 2 Apr 2025 09:51:24 +0200 Subject: [PATCH 3/5] JS: Protect against bad join in BadRandomness This code resulted in bad join orders in response to certain library changes. The actual library changes have to be split into smaller pieces but I'd like to ensure I don't run into the bad join again. --- .../ql/src/Security/CWE-327/BadRandomness.ql | 32 ++++++++----------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/javascript/ql/src/Security/CWE-327/BadRandomness.ql b/javascript/ql/src/Security/CWE-327/BadRandomness.ql index dd6de6cc1c01..5de0f9b3dfd9 100644 --- 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() } /** From 30a9cd7c8a7cda2649996e680c525057adeba97e Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 2 Apr 2025 14:09:52 +0200 Subject: [PATCH 4/5] JS: Include document as a DOM value --- .../ql/test/library-tests/DOM/Customizations.expected | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/javascript/ql/test/library-tests/DOM/Customizations.expected b/javascript/ql/test/library-tests/DOM/Customizations.expected index 3fc5570c743c..df15c409be2d 100644 --- 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 | From 2c40359143e07e32c59fac49981b911429428671 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 2 Apr 2025 14:11:54 +0200 Subject: [PATCH 5/5] JS: Change note --- .../2025-04-02-name-resolution-independent-fixes.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 javascript/ql/src/change-notes/2025-04-02-name-resolution-independent-fixes.md diff --git 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 new file mode 100644 index 000000000000..4773744a984f --- /dev/null +++ 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.