js/xss-through-dom: filter away reads of .src that end in a URL sink

Author: erik-krogh

javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomCustomizations.qll

@@ -89,18 +89,25 @@ module XssThroughDom {
    * A source for text from the DOM from a DOM property read or call to `getAttribute()`.
    */
   class DomTextSource extends Source {
+    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)
       )
     }
+
+    /** Gets the name of the property read. */
+    string getPropertyName() { result = prop }
   }
 
   /** DEPRECATED: Alias for DomTextSource */

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().(DomTextSource).getPropertyName() = "src"
+    )
+  }
 }

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 |

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
+})();