Merge pull request #8687 from asgerf/js/missing-flow-fixes

Approved by erik-krogh
2025-12-17 01:03:14 +01:00 · 2022-04-11 14:08:15 +01:00
parent 626770aaab b85739cb7e
commit 9c8dee2a4d
9 changed files with 51 additions and 3 deletions
--- a/javascript/ql/lib/semmle/javascript/Arrays.qll
+++ b/javascript/ql/lib/semmle/javascript/Arrays.qll
@@ -45,9 +45,20 @@ module ArrayTaintTracking {
    )
    or
    // `array.reduce` with tainted value in callback
+    // The callback parameters are: (previousValue, currentValue, currentIndex, array)
    call.(DataFlow::MethodCallNode).getMethodName() = "reduce" and
-    pred = call.getArgument(0).(DataFlow::FunctionNode).getAReturn() and // Require the argument to be a closure to avoid spurious call/return flow
-    succ = call
+    exists(DataFlow::FunctionNode callback |
+      callback = call.getArgument(0) // Require the argument to be a closure to avoid spurious call/return flow
+    |
+      pred = callback.getAReturn() and
+      succ = call
+      or
+      pred = call.getReceiver() and
+      succ = callback.getParameter([1, 3]) // into currentValue or array
+      or
+      pred = [call.getArgument(1), callback.getAReturn()] and
+      succ = callback.getParameter(0) // into previousValue
+    )
    or
    // `array.push(e)`, `array.unshift(e)`: if `e` is tainted, then so is `array`.
    pred = call.getAnArgument() and
--- a/javascript/ql/lib/semmle/javascript/DOM.qll
+++ b/javascript/ql/lib/semmle/javascript/DOM.qll
@@ -430,6 +430,12 @@ module DOM {
    result.hasUnderlyingType("Element")
    or
    result.hasUnderlyingType(any(string s | s.matches("HTML%Element")))
+    or
+    exists(DataFlow::ClassNode cls |
+      cls.getASuperClassNode().getALocalSource() =
+        DataFlow::globalVarRef(any(string s | s.matches("HTML%Element"))) and
+      result = cls.getAnInstanceReference()
+    )
  }

  module LocationSource {
--- a/javascript/ql/src/change-notes/2022-04-07-missing-flow-fixes.md
+++ b/javascript/ql/src/change-notes/2022-04-07-missing-flow-fixes.md
@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* Improved handling of custom DOM elements, potentially leading to more alerts for the XSS queries.
+* Improved taint tracking through calls to the `Array.prototype.reduce` function.
--- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected
+++ b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected
@@ -16,6 +16,7 @@ typeInferenceMismatch
 | arrays.js:2:15:2:22 | source() | arrays.js:8:10:8:22 | arrayIfy(foo) |
 | arrays.js:2:15:2:22 | source() | arrays.js:11:10:11:28 | union(["bla"], foo) |
 | arrays.js:2:15:2:22 | source() | arrays.js:14:10:14:18 | flat(foo) |
+| arrays.js:2:15:2:22 | source() | arrays.js:19:10:19:12 | res |
 | booleanOps.js:2:11:2:18 | source() | booleanOps.js:4:8:4:8 | x |
 | booleanOps.js:2:11:2:18 | source() | booleanOps.js:13:10:13:10 | x |
 | booleanOps.js:2:11:2:18 | source() | booleanOps.js:19:10:19:10 | x |
--- a/javascript/ql/test/library-tests/TaintTracking/arrays.js
+++ b/javascript/ql/test/library-tests/TaintTracking/arrays.js
@@ -12,4 +12,9 @@ function test() {

    const flat = require("arr-flatten");
    sink(flat(foo)); // NOT OK
-}
+
+    let res = foo.reduce((prev, current) => {
+        return prev + '<b>' + current + '</b>';
+    }, '');
+    sink(res); // NOT OK
+}
--- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected
@@ -136,6 +136,10 @@ nodes
 | clipboard.ts:33:19:33:68 | e.origi ... /html') |
 | clipboard.ts:33:19:33:68 | e.origi ... /html') |
 | clipboard.ts:33:19:33:68 | e.origi ... /html') |
+| custom-element.js:5:26:5:36 | window.name |
+| custom-element.js:5:26:5:36 | window.name |
+| custom-element.js:5:26:5:36 | window.name |
+| custom-element.js:5:26:5:36 | window.name |
 | d3.js:4:12:4:22 | window.name |
 | d3.js:4:12:4:22 | window.name |
 | d3.js:4:12:4:22 | window.name |
@@ -1130,6 +1134,7 @@ edges
 | clipboard.ts:24:23:24:58 | e.clipb ... /html') | clipboard.ts:24:23:24:58 | e.clipb ... /html') |
 | clipboard.ts:29:19:29:54 | e.clipb ... /html') | clipboard.ts:29:19:29:54 | e.clipb ... /html') |
 | clipboard.ts:33:19:33:68 | e.origi ... /html') | clipboard.ts:33:19:33:68 | e.origi ... /html') |
+| custom-element.js:5:26:5:36 | window.name | custom-element.js:5:26:5:36 | window.name |
 | d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
 | d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
 | d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
@@ -2062,6 +2067,7 @@ edges
 | clipboard.ts:24:23:24:58 | e.clipb ... /html') | clipboard.ts:24:23:24:58 | e.clipb ... /html') | clipboard.ts:24:23:24:58 | e.clipb ... /html') | Cross-site scripting vulnerability due to $@. | clipboard.ts:24:23:24:58 | e.clipb ... /html') | user-provided value |
 | clipboard.ts:29:19:29:54 | e.clipb ... /html') | clipboard.ts:29:19:29:54 | e.clipb ... /html') | clipboard.ts:29:19:29:54 | e.clipb ... /html') | Cross-site scripting vulnerability due to $@. | clipboard.ts:29:19:29:54 | e.clipb ... /html') | user-provided value |
 | clipboard.ts:33:19:33:68 | e.origi ... /html') | clipboard.ts:33:19:33:68 | e.origi ... /html') | clipboard.ts:33:19:33:68 | e.origi ... /html') | Cross-site scripting vulnerability due to $@. | clipboard.ts:33:19:33:68 | e.origi ... /html') | user-provided value |
+| custom-element.js:5:26:5:36 | window.name | custom-element.js:5:26:5:36 | window.name | custom-element.js:5:26:5:36 | window.name | Cross-site scripting vulnerability due to $@. | custom-element.js:5:26:5:36 | window.name | user-provided value |
 | d3.js:11:15:11:24 | getTaint() | d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() | Cross-site scripting vulnerability due to $@. | d3.js:4:12:4:22 | window.name | user-provided value |
 | d3.js:12:20:12:29 | getTaint() | d3.js:4:12:4:22 | window.name | d3.js:12:20:12:29 | getTaint() | Cross-site scripting vulnerability due to $@. | d3.js:4:12:4:22 | window.name | user-provided value |
 | d3.js:14:20:14:29 | getTaint() | d3.js:4:12:4:22 | window.name | d3.js:14:20:14:29 | getTaint() | Cross-site scripting vulnerability due to $@. | d3.js:4:12:4:22 | window.name | 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
@@ -136,6 +136,10 @@ nodes
 | clipboard.ts:33:19:33:68 | e.origi ... /html') |
 | clipboard.ts:33:19:33:68 | e.origi ... /html') |
 | clipboard.ts:33:19:33:68 | e.origi ... /html') |
+| custom-element.js:5:26:5:36 | window.name |
+| custom-element.js:5:26:5:36 | window.name |
+| custom-element.js:5:26:5:36 | window.name |
+| custom-element.js:5:26:5:36 | window.name |
 | d3.js:4:12:4:22 | window.name |
 | d3.js:4:12:4:22 | window.name |
 | d3.js:4:12:4:22 | window.name |
@@ -1180,6 +1184,7 @@ edges
 | clipboard.ts:24:23:24:58 | e.clipb ... /html') | clipboard.ts:24:23:24:58 | e.clipb ... /html') |
 | clipboard.ts:29:19:29:54 | e.clipb ... /html') | clipboard.ts:29:19:29:54 | e.clipb ... /html') |
 | clipboard.ts:33:19:33:68 | e.origi ... /html') | clipboard.ts:33:19:33:68 | e.origi ... /html') |
+| custom-element.js:5:26:5:36 | window.name | custom-element.js:5:26:5:36 | window.name |
 | d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
 | d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
 | d3.js:4:12:4:22 | window.name | d3.js:11:15:11:24 | getTaint() |
--- a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/custom-element.js
+++ b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/custom-element.js
@@ -0,0 +1,7 @@
+import * as dummy from 'dummy';
+
+class CustomElm extends HTMLElement {
+    test() {
+        this.innerHTML = window.name; // NOT OK
+    }
+}
--- a/javascript/ql/test/query-tests/Security/CWE-312/BuildArtifactLeak.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-312/BuildArtifactLeak.expected
@@ -37,12 +37,14 @@ edges
 | build-leaks.js:15:24:15:34 | process.env | build-leaks.js:14:18:14:20 | env |
 | build-leaks.js:15:24:15:34 | process.env | build-leaks.js:14:18:14:20 | env |
 | build-leaks.js:16:20:16:22 | env | build-leaks.js:13:17:19:10 | Object. ...      }) |
+| build-leaks.js:16:20:16:22 | env | build-leaks.js:14:18:14:20 | env |
 | build-leaks.js:21:11:26:5 | stringifed | build-leaks.js:30:22:30:31 | stringifed |
 | build-leaks.js:21:24:26:5 | {\\n      ... )\\n    } | build-leaks.js:21:11:26:5 | stringifed |
 | build-leaks.js:22:24:25:14 | Object. ...  }, {}) | build-leaks.js:21:24:26:5 | {\\n      ... )\\n    } |
 | build-leaks.js:22:49:22:51 | env | build-leaks.js:24:20:24:22 | env |
 | build-leaks.js:23:39:23:41 | raw | build-leaks.js:22:49:22:51 | env |
 | build-leaks.js:24:20:24:22 | env | build-leaks.js:22:24:25:14 | Object. ...  }, {}) |
+| build-leaks.js:24:20:24:22 | env | build-leaks.js:22:49:22:51 | env |
 | build-leaks.js:30:22:30:31 | stringifed | build-leaks.js:34:26:34:57 | getEnv( ... ngified |
 | build-leaks.js:30:22:30:31 | stringifed | build-leaks.js:34:26:34:57 | getEnv( ... ngified |
 | build-leaks.js:40:9:40:60 | pw | build-leaks.js:41:82:41:83 | pw |