Merge pull request #8509 from erik-krogh/fpXss

JS: filter away reads of .src that end in a URL sink for js/xss-through-dom
2026-05-03 12:45:27 +02:00 · 2022-03-22 14:51:17 +01:00
parent 5cdf0b5ee2 099d91ba6f
commit 8ae04e04d4
4 changed files with 88 additions and 30 deletions
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomCustomizations.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomCustomizations.qll
@@ -32,36 +32,58 @@ module XssThroughDom {
   */
  string unsafeDomPropertyName() { result = ["innerText", "textContent", "value", "name", "src"] }

-  /**
-   * A source for text from the DOM from a JQuery method call.
-   */
-  class JQueryTextSource extends Source, JQuery::MethodCall {
+  /** A read of a DOM property seen as a source for cross-site scripting vulnerabilities through the DOM. */
+  abstract class DomPropertySource extends Source {
+    /**
+     * Gets the name of the DOM property that the source originated from.
+     */
+    abstract string getPropertyName();
+  }
+
+  /* Gets a jQuery method where the receiver looks like `$("<p>" + ... )`, which is benign for this query. */
+  private JQuery::MethodCall benignJQueryMethod() {
+    exists(DataFlow::Node prefix |
+      DomBasedXss::isPrefixOfJQueryHtmlString(result
+            .getReceiver()
+            .(DataFlow::CallNode)
+            .getAnArgument(), prefix)
+    |
+      prefix.getStringValue().regexpMatch("\\s*<.*")
+    )
+  }
+
+  /** A source for text from the DOM from a JQuery method call. */
+  class JQueryTextSource extends Source instanceof JQuery::MethodCall {
    JQueryTextSource() {
-      (
-        this.getMethodName() = ["text", "val"] and this.getNumArgument() = 0
-        or
-        exists(string methodName, string value |
-          this.getMethodName() = methodName and
-          this.getNumArgument() = 1 and
-          forex(InferredType t | t = this.getArgument(0).analyze().getAType() | t = TTString()) and
-          this.getArgument(0).mayHaveStringValue(value)
-        |
-          methodName = "attr" and value = unsafeAttributeName()
-          or
-          methodName = "prop" and value = unsafeDomPropertyName()
-        )
-      ) and
-      // looks like a $("<p>" + ... ) source, which is benign for this query.
-      not exists(DataFlow::Node prefix |
-        DomBasedXss::isPrefixOfJQueryHtmlString(this.getReceiver()
-              .(DataFlow::CallNode)
-              .getAnArgument(), prefix)
-      |
-        prefix.getStringValue().regexpMatch("\\s*<.*")
-      )
+      this.getMethodName() = ["text", "val"] and
+      this.getNumArgument() = 0 and
+      not this = benignJQueryMethod()
    }
  }

+  /**
+   * A source for text from a DOM property read by jQuery.
+   */
+  class JQueryDOMPropertySource extends DomPropertySource instanceof JQuery::MethodCall {
+    string prop;
+
+    JQueryDOMPropertySource() {
+      exists(string methodName |
+        this.getMethodName() = methodName and
+        this.getNumArgument() = 1 and
+        forex(InferredType t | t = this.getArgument(0).analyze().getAType() | t = TTString()) and
+        this.getArgument(0).mayHaveStringValue(prop)
+      |
+        methodName = "attr" and prop = unsafeAttributeName()
+        or
+        methodName = "prop" and prop = unsafeDomPropertyName()
+      ) and
+      not this = benignJQueryMethod()
+    }
+
+    override string getPropertyName() { result = prop }
+  }
+
  /**
   * A source for text from the DOM from a `d3` method call.
   */
@@ -88,19 +110,25 @@ module XssThroughDom {
  /**
   * A source for text from the DOM from a DOM property read or call to `getAttribute()`.
   */
-  class DomTextSource extends Source {
+  class DomTextSource extends DomPropertySource {
+    string prop;
+
    DomTextSource() {
      exists(DataFlow::PropRead read | read = this |
        read.getBase().getALocalSource() = DOM::domValueRef() and
-        read.mayHavePropertyName(unsafeDomPropertyName())
+        prop = unsafeDomPropertyName() and
+        read.mayHavePropertyName(prop)
      )
      or
      exists(DataFlow::MethodCallNode mcn | mcn = this |
        mcn.getReceiver().getALocalSource() = DOM::domValueRef() and
        mcn.getMethodName() = "getAttribute" and
-        mcn.getArgument(0).mayHaveStringValue(unsafeAttributeName())
+        prop = unsafeAttributeName() and
+        mcn.getArgument(0).mayHaveStringValue(prop)
      )
    }
+
+    override string getPropertyName() { result = prop }
  }

  /** DEPRECATED: Alias for DomTextSource */
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll
@@ -35,4 +35,13 @@ class Configuration extends TaintTracking::Configuration {
  override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
    DomBasedXss::isOptionallySanitizedEdge(pred, succ)
  }
+
+  override predicate hasFlowPath(DataFlow::SourcePathNode src, DataFlow::SinkPathNode sink) {
+    super.hasFlowPath(src, sink) and
+    // filtering away readings of `src` that end in a URL sink.
+    not (
+      sink.getNode() instanceof DomBasedXss::WriteURLSink and
+      src.getNode().(DomPropertySource).getPropertyName() = "src"
+    )
+  }
 }
--- a/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/XssThroughDom.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/XssThroughDom.expected
@@ -122,6 +122,13 @@ nodes
 | xss-through-dom.js:109:31:109:70 | "<a src ... oo</a>" |
 | xss-through-dom.js:109:45:109:55 | this.el.src |
 | xss-through-dom.js:109:45:109:55 | this.el.src |
+| xss-through-dom.js:114:11:114:52 | src |
+| xss-through-dom.js:114:17:114:52 | documen ... k").src |
+| xss-through-dom.js:114:17:114:52 | documen ... k").src |
+| xss-through-dom.js:115:16:115:18 | src |
+| xss-through-dom.js:115:16:115:18 | src |
+| xss-through-dom.js:117:26:117:28 | src |
+| xss-through-dom.js:117:26:117:28 | src |
 edges
 | forms.js:8:23:8:28 | values | forms.js:9:31:9:36 | values |
 | forms.js:8:23:8:28 | values | forms.js:9:31:9:36 | values |
@@ -194,6 +201,12 @@ edges
 | xss-through-dom.js:109:45:109:55 | this.el.src | xss-through-dom.js:109:31:109:70 | "<a src ... oo</a>" |
 | xss-through-dom.js:109:45:109:55 | this.el.src | xss-through-dom.js:109:31:109:70 | "<a src ... oo</a>" |
 | xss-through-dom.js:109:45:109:55 | this.el.src | xss-through-dom.js:109:31:109:70 | "<a src ... oo</a>" |
+| xss-through-dom.js:114:11:114:52 | src | xss-through-dom.js:115:16:115:18 | src |
+| xss-through-dom.js:114:11:114:52 | src | xss-through-dom.js:115:16:115:18 | src |
+| xss-through-dom.js:114:11:114:52 | src | xss-through-dom.js:117:26:117:28 | src |
+| xss-through-dom.js:114:11:114:52 | src | xss-through-dom.js:117:26:117:28 | src |
+| xss-through-dom.js:114:17:114:52 | documen ... k").src | xss-through-dom.js:114:11:114:52 | src |
+| xss-through-dom.js:114:17:114:52 | documen ... k").src | xss-through-dom.js:114:11:114:52 | src |
 #select
 | forms.js:9:31:9:40 | values.foo | forms.js:8:23:8:28 | values | forms.js:9:31:9:40 | values.foo | $@ is reinterpreted as HTML without escaping meta-characters. | forms.js:8:23:8:28 | values | DOM text |
 | forms.js:12:31:12:40 | values.bar | forms.js:11:24:11:29 | values | forms.js:12:31:12:40 | values.bar | $@ is reinterpreted as HTML without escaping meta-characters. | forms.js:11:24:11:29 | values | DOM text |
@@ -228,3 +241,4 @@ edges
 | xss-through-dom.js:93:16:93:46 | $("#foo ... ].value | xss-through-dom.js:93:16:93:46 | $("#foo ... ].value | xss-through-dom.js:93:16:93:46 | $("#foo ... ].value | $@ is reinterpreted as HTML without escaping meta-characters. | xss-through-dom.js:93:16:93:46 | $("#foo ... ].value | DOM text |
 | xss-through-dom.js:96:17:96:47 | $("#foo ... ].value | xss-through-dom.js:96:17:96:47 | $("#foo ... ].value | xss-through-dom.js:96:17:96:47 | $("#foo ... ].value | $@ is reinterpreted as HTML without escaping meta-characters. | xss-through-dom.js:96:17:96:47 | $("#foo ... ].value | DOM text |
 | xss-through-dom.js:109:31:109:70 | "<a src ... oo</a>" | xss-through-dom.js:109:45:109:55 | this.el.src | xss-through-dom.js:109:31:109:70 | "<a src ... oo</a>" | $@ is reinterpreted as HTML without escaping meta-characters. | xss-through-dom.js:109:45:109:55 | this.el.src | DOM text |
+| xss-through-dom.js:115:16:115:18 | src | xss-through-dom.js:114:17:114:52 | documen ... k").src | xss-through-dom.js:115:16:115:18 | src | $@ is reinterpreted as HTML without escaping meta-characters. | xss-through-dom.js:114:17:114:52 | documen ... k").src | DOM text |
--- a/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/xss-through-dom.js
+++ b/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom/xss-through-dom.js
@@ -108,4 +108,11 @@ class Sub extends Super {
 		super();
 		$("#id").get(0).innerHTML = "<a src=\"" + this.el.src + "\">foo</a>"; // NOT OK. Attack: `<mytag id="id" src="x:&quot;&gt;&lt;img src=1 onerror=&quot;alert(1)&quot;&gt;" />`
 	}
-}
+}
+
+(function () {
+    const src = document.getElementById("#link").src;
+	$("#id").html(src); // NOT OK.
+
+    $("#id").attr("src", src); // OK
+})();