remove flowpaths that has a returns without a matching call

Author: erik-krogh

javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll

@@ -28,5 +28,15 @@ module UnsafeHtmlConstruction {
     override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
       DomBasedXss::isOptionallySanitizedEdge(pred, succ)
     }
+
+    // override to require that there is a path without unmatched return steps
+    override predicate hasFlowPath(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
+      super.hasFlowPath(source, sink) and
+      requireMatchedReturn(source, sink)
+    }
+
+    override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
+      classFieldStep(pred, succ)
+    }
   }
 }

javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll

@@ -165,4 +165,28 @@ module UnsafeHtmlConstruction {
 
     override string describe() { result = "Markdown rendering" }
   }
+
+  /**
+   * A taint step from the write of a field in a constructor to a read of the same field in an instance method.
+   */
+  predicate classFieldStep(DataFlow::Node pred, DataFlow::Node succ) {
+    // flow-step from a property written in the constructor to a use in an instance method.
+    // "simulates" client usage of a class, and regains some flow-steps lost by `requireMatchedReturn` below.
+    exists(DataFlow::ClassNode clazz, string prop |
+      DataFlow::thisNode(clazz.getConstructor().getFunction()).getAPropertyWrite(prop).getRhs() =
+        pred and
+      DataFlow::thisNode(clazz.getAnInstanceMethod().getFunction()).getAPropertyRead(prop) = succ
+    )
+  }
+
+  /**
+   *  Holds if there is a path without unmatched return steps from `source` to `sink`.
+   */
+  predicate requireMatchedReturn(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
+    exists(DataFlow::MidPathNode mid |
+      source.getASuccessor*() = mid and
+      sink = mid.getASuccessor() and
+      mid.getPathSummary().hasReturn() = false
+    )
+  }
 }

javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected

@@ -15,6 +15,13 @@ nodes
 | main.js:21:47:21:47 | s |
 | main.js:22:34:22:34 | s |
 | main.js:22:34:22:34 | s |
+| main.js:46:17:46:17 | s |
+| main.js:47:21:47:21 | s |
+| main.js:52:65:52:73 | this.step |
+| main.js:52:65:52:73 | this.step |
+| main.js:57:41:57:41 | s |
+| main.js:57:41:57:41 | s |
+| main.js:58:20:58:20 | s |
 | typed.ts:1:39:1:39 | s |
 | typed.ts:1:39:1:39 | s |
 | typed.ts:2:29:2:29 | s |
@@ -47,6 +54,12 @@ edges
 | main.js:21:47:21:47 | s | main.js:22:34:22:34 | s |
 | main.js:21:47:21:47 | s | main.js:22:34:22:34 | s |
 | main.js:21:47:21:47 | s | main.js:22:34:22:34 | s |
+| main.js:46:17:46:17 | s | main.js:47:21:47:21 | s |
+| main.js:47:21:47:21 | s | main.js:52:65:52:73 | this.step |
+| main.js:47:21:47:21 | s | main.js:52:65:52:73 | this.step |
+| main.js:57:41:57:41 | s | main.js:58:20:58:20 | s |
+| main.js:57:41:57:41 | s | main.js:58:20:58:20 | s |
+| main.js:58:20:58:20 | s | main.js:46:17:46:17 | s |
 | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
 | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
 | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
@@ -67,6 +80,6 @@ edges
 | main.js:12:49:12:49 | s | main.js:11:60:11:60 | s | main.js:12:49:12:49 | s | $@ based on $@ might later cause $@. | main.js:12:49:12:49 | s | XML parsing | main.js:11:60:11:60 | s | library input | main.js:16:21:16:35 | xml.cloneNode() | cross-site scripting |
 | main.js:12:49:12:49 | s | main.js:11:60:11:60 | s | main.js:12:49:12:49 | s | $@ based on $@ might later cause $@. | main.js:12:49:12:49 | s | XML parsing | main.js:11:60:11:60 | s | library input | main.js:17:48:17:50 | tmp | cross-site scripting |
 | main.js:22:34:22:34 | s | main.js:21:47:21:47 | s | main.js:22:34:22:34 | s | $@ based on $@ might later cause $@. | main.js:22:34:22:34 | s | Markdown rendering | main.js:21:47:21:47 | s | library input | main.js:23:53:23:56 | html | cross-site scripting |
+| main.js:52:65:52:73 | this.step | main.js:57:41:57:41 | s | main.js:52:65:52:73 | this.step | $@ based on $@ might later cause $@. | main.js:52:65:52:73 | this.step | HTML construction | main.js:57:41:57:41 | s | library input | main.js:52:54:52:85 | "<span> ... /span>" | cross-site scripting |
 | typed.ts:2:29:2:29 | s | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s | $@ based on $@ might later cause $@. | typed.ts:2:29:2:29 | s | HTML construction | typed.ts:1:39:1:39 | s | library input | typed.ts:3:31:3:34 | html | cross-site scripting |
 | typed.ts:8:40:8:40 | s | typed.ts:6:43:6:43 | s | typed.ts:8:40:8:40 | s | $@ based on $@ might later cause $@. | typed.ts:8:40:8:40 | s | HTML construction | typed.ts:6:43:6:43 | s | library input | typed.ts:8:29:8:52 | "<span> ... /span>" | cross-site scripting |
-| typed.ts:17:29:17:29 | s | typed.ts:11:20:11:20 | s | typed.ts:17:29:17:29 | s | $@ based on $@ might later cause $@. | typed.ts:17:29:17:29 | s | HTML construction | typed.ts:11:20:11:20 | s | library input | typed.ts:18:31:18:34 | html | cross-site scripting |

javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js

@@ -44,7 +44,7 @@ class Foo {
 
     doXss() {
         // not called here, but still bad.
-        document.querySelector("#class").innerHTML = "<span>" + this.step + "</span>"; // NOT OK - but not flagged [INCONSISTENCY]
+        document.querySelector("#class").innerHTML = "<span>" + this.step + "</span>"; // NOT OK
     }
 
 }

javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/typed.ts

@@ -14,7 +14,7 @@ export function id(s: string) {
 
 export function notVulnerable() {
     const s = id("x");
-    const html = "<span>" + s + "</span>"; // OK - but flagged due to step with unmatched call [INCONSISTENCY]
+    const html = "<span>" + s + "</span>"; // OK
     document.body.innerHTML = html;
 }