JS: Consistently use the shared XSS barrier guards in the XSS queries

asgerf · asgerf · commit 6cbe04dcb7dc · 2024-10-02T14:44:17.000+02:00
Previously only reflected XSS used shared barrier guards.
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll
@@ -55,7 +55,9 @@ module DomBasedXssConfig implements DataFlow::StateConfigSig {
     label = prefixLabel()
   }
 
-  predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
+  predicate isBarrier(DataFlow::Node node) {
+    node instanceof Sanitizer or node = Shared::BarrierGuard::getABarrierNode()
+  }
 
   predicate isBarrier(DataFlow::Node node, DataFlow::FlowLabel lbl) {
     // copy all taint barrier guards to the TaintedUrlSuffix/PrefixLabel label
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/ExceptionXssQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/ExceptionXssQuery.qll
@@ -140,7 +140,9 @@ module ExceptionXssConfig implements DataFlow::StateConfigSig {
     sink instanceof XssShared::Sink and not label instanceof NotYetThrown
   }
 
-  predicate isBarrier(DataFlow::Node node) { node instanceof XssShared::Sanitizer }
+  predicate isBarrier(DataFlow::Node node) {
+    node instanceof XssShared::Sanitizer or node = XssShared::BarrierGuard::getABarrierNode()
+  }
 
   predicate isAdditionalFlowStep(
     DataFlow::Node pred, DataFlow::FlowLabel inlbl, DataFlow::Node succ, DataFlow::FlowLabel outlbl
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/StoredXssQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/StoredXssQuery.qll
@@ -15,7 +15,9 @@ module StoredXssConfig implements DataFlow::ConfigSig {
 
   predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
-  predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
+  predicate isBarrier(DataFlow::Node node) {
+    node instanceof Sanitizer or node = Shared::BarrierGuard::getABarrierNode()
+  }
 }
 
 /**
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll
@@ -34,6 +34,8 @@ module UnsafeHtmlConstructionConfig implements DataFlow::StateConfigSig {
     node instanceof UnsafeJQueryPlugin::Sanitizer
     or
     DomBasedXss::isOptionallySanitizedNode(node)
+    or
+    node = Shared::BarrierGuard::getABarrierNode()
   }
 
   predicate isBarrier(DataFlow::Node node, DataFlow::FlowLabel label) {
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll
@@ -22,7 +22,8 @@ module XssThroughDomConfig implements DataFlow::ConfigSig {
     node instanceof DomBasedXss::Sanitizer or
     DomBasedXss::isOptionallySanitizedNode(node) or
     node = DataFlow::MakeBarrierGuard<BarrierGuard>::getABarrierNode() or
-    node = DataFlow::MakeBarrierGuard<UnsafeJQuery::BarrierGuard>::getABarrierNode()
+    node = DataFlow::MakeBarrierGuard<UnsafeJQuery::BarrierGuard>::getABarrierNode() or
+    node = Shared::BarrierGuard::getABarrierNode()
   }
 
   predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/ConsistencyDomBasedXss.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/ConsistencyDomBasedXss.expected
@@ -1,3 +0,0 @@
-| query-tests/Security/CWE-079/DomBasedXss/sanitiser.js:25 | did not expect an alert, but found an alert for HtmlInjection | OK | ConsistencyConfig |
-| query-tests/Security/CWE-079/DomBasedXss/sanitiser.js:28 | did not expect an alert, but found an alert for HtmlInjection | OK | ConsistencyConfig |
-| query-tests/Security/CWE-079/DomBasedXss/sanitiser.js:35 | did not expect an alert, but found an alert for HtmlInjection | OK | ConsistencyConfig |
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
@@ -284,16 +284,10 @@ nodes
 | sanitiser.js:16:17:16:27 | window.name | semmle.label | window.name |
 | sanitiser.js:23:21:23:44 | '<b>' + ...  '</b>' | semmle.label | '<b>' + ...  '</b>' |
 | sanitiser.js:23:29:23:35 | tainted | semmle.label | tainted |
-| sanitiser.js:25:21:25:44 | '<b>' + ...  '</b>' | semmle.label | '<b>' + ...  '</b>' |
-| sanitiser.js:25:29:25:35 | tainted | semmle.label | tainted |
-| sanitiser.js:28:21:28:44 | '<b>' + ...  '</b>' | semmle.label | '<b>' + ...  '</b>' |
-| sanitiser.js:28:29:28:35 | tainted | semmle.label | tainted |
 | sanitiser.js:30:21:30:44 | '<b>' + ...  '</b>' | semmle.label | '<b>' + ...  '</b>' |
 | sanitiser.js:30:29:30:35 | tainted | semmle.label | tainted |
 | sanitiser.js:33:21:33:44 | '<b>' + ...  '</b>' | semmle.label | '<b>' + ...  '</b>' |
 | sanitiser.js:33:29:33:35 | tainted | semmle.label | tainted |
-| sanitiser.js:35:21:35:44 | '<b>' + ...  '</b>' | semmle.label | '<b>' + ...  '</b>' |
-| sanitiser.js:35:29:35:35 | tainted | semmle.label | tainted |
 | sanitiser.js:38:21:38:44 | '<b>' + ...  '</b>' | semmle.label | '<b>' + ...  '</b>' |
 | sanitiser.js:38:29:38:35 | tainted | semmle.label | tainted |
 | sanitiser.js:45:21:45:44 | '<b>' + ...  '</b>' | semmle.label | '<b>' + ...  '</b>' |
@@ -852,21 +846,15 @@ edges
 | react-use-state.js:22:14:22:17 | prev | react-use-state.js:23:35:23:38 | prev | provenance |  |
 | react-use-state.js:25:20:25:30 | window.name | react-use-state.js:21:10:21:14 | state | provenance |  |
 | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:23:29:23:35 | tainted | provenance |  |
-| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:25:29:25:35 | tainted | provenance |  |
-| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:28:29:28:35 | tainted | provenance |  |
 | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:30:29:30:35 | tainted | provenance |  |
 | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:33:29:33:35 | tainted | provenance |  |
-| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:35:29:35:35 | tainted | provenance |  |
 | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:38:29:38:35 | tainted | provenance |  |
 | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:45:29:45:35 | tainted | provenance |  |
 | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:48:19:48:25 | tainted | provenance |  |
 | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:16:7:16:27 | tainted | provenance |  |
 | sanitiser.js:23:29:23:35 | tainted | sanitiser.js:23:21:23:44 | '<b>' + ...  '</b>' | provenance |  |
-| sanitiser.js:25:29:25:35 | tainted | sanitiser.js:25:21:25:44 | '<b>' + ...  '</b>' | provenance |  |
-| sanitiser.js:28:29:28:35 | tainted | sanitiser.js:28:21:28:44 | '<b>' + ...  '</b>' | provenance |  |
 | sanitiser.js:30:29:30:35 | tainted | sanitiser.js:30:21:30:44 | '<b>' + ...  '</b>' | provenance |  |
 | sanitiser.js:33:29:33:35 | tainted | sanitiser.js:33:21:33:44 | '<b>' + ...  '</b>' | provenance |  |
-| sanitiser.js:35:29:35:35 | tainted | sanitiser.js:35:21:35:44 | '<b>' + ...  '</b>' | provenance |  |
 | sanitiser.js:38:29:38:35 | tainted | sanitiser.js:38:21:38:44 | '<b>' + ...  '</b>' | provenance |  |
 | sanitiser.js:45:29:45:35 | tainted | sanitiser.js:45:21:45:44 | '<b>' + ...  '</b>' | provenance |  |
 | sanitiser.js:48:19:48:25 | tainted | sanitiser.js:48:19:48:46 | tainted ... /g, '') | provenance |  |
@@ -1265,11 +1253,8 @@ subpaths
 | react-use-state.js:17:51:17:55 | state | react-use-state.js:16:20:16:30 | window.name | react-use-state.js:17:51:17:55 | state | Cross-site scripting vulnerability due to $@. | react-use-state.js:16:20:16:30 | window.name | user-provided value |
 | react-use-state.js:23:35:23:38 | prev | react-use-state.js:25:20:25:30 | window.name | react-use-state.js:23:35:23:38 | prev | Cross-site scripting vulnerability due to $@. | react-use-state.js:25:20:25:30 | window.name | user-provided value |
 | sanitiser.js:23:21:23:44 | '<b>' + ...  '</b>' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:23:21:23:44 | '<b>' + ...  '</b>' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value |
-| sanitiser.js:25:21:25:44 | '<b>' + ...  '</b>' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:25:21:25:44 | '<b>' + ...  '</b>' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value |
-| sanitiser.js:28:21:28:44 | '<b>' + ...  '</b>' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:28:21:28:44 | '<b>' + ...  '</b>' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value |
 | sanitiser.js:30:21:30:44 | '<b>' + ...  '</b>' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:30:21:30:44 | '<b>' + ...  '</b>' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value |
 | sanitiser.js:33:21:33:44 | '<b>' + ...  '</b>' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:33:21:33:44 | '<b>' + ...  '</b>' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value |
-| sanitiser.js:35:21:35:44 | '<b>' + ...  '</b>' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:35:21:35:44 | '<b>' + ...  '</b>' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value |
 | sanitiser.js:38:21:38:44 | '<b>' + ...  '</b>' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:38:21:38:44 | '<b>' + ...  '</b>' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value |
 | sanitiser.js:45:21:45:44 | '<b>' + ...  '</b>' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:45:21:45:44 | '<b>' + ...  '</b>' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value |
 | sanitiser.js:48:19:48:46 | tainted ... /g, '') | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:48:19:48:46 | tainted ... /g, '') | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value |
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
@@ -289,16 +289,10 @@ nodes
 | sanitiser.js:16:17:16:27 | window.name | semmle.label | window.name |
 | sanitiser.js:23:21:23:44 | '<b>' + ...  '</b>' | semmle.label | '<b>' + ...  '</b>' |
 | sanitiser.js:23:29:23:35 | tainted | semmle.label | tainted |
-| sanitiser.js:25:21:25:44 | '<b>' + ...  '</b>' | semmle.label | '<b>' + ...  '</b>' |
-| sanitiser.js:25:29:25:35 | tainted | semmle.label | tainted |
-| sanitiser.js:28:21:28:44 | '<b>' + ...  '</b>' | semmle.label | '<b>' + ...  '</b>' |
-| sanitiser.js:28:29:28:35 | tainted | semmle.label | tainted |
 | sanitiser.js:30:21:30:44 | '<b>' + ...  '</b>' | semmle.label | '<b>' + ...  '</b>' |
 | sanitiser.js:30:29:30:35 | tainted | semmle.label | tainted |
 | sanitiser.js:33:21:33:44 | '<b>' + ...  '</b>' | semmle.label | '<b>' + ...  '</b>' |
 | sanitiser.js:33:29:33:35 | tainted | semmle.label | tainted |
-| sanitiser.js:35:21:35:44 | '<b>' + ...  '</b>' | semmle.label | '<b>' + ...  '</b>' |
-| sanitiser.js:35:29:35:35 | tainted | semmle.label | tainted |
 | sanitiser.js:38:21:38:44 | '<b>' + ...  '</b>' | semmle.label | '<b>' + ...  '</b>' |
 | sanitiser.js:38:29:38:35 | tainted | semmle.label | tainted |
 | sanitiser.js:45:21:45:44 | '<b>' + ...  '</b>' | semmle.label | '<b>' + ...  '</b>' |
@@ -876,21 +870,15 @@ edges
 | react-use-state.js:22:14:22:17 | prev | react-use-state.js:23:35:23:38 | prev | provenance |  |
 | react-use-state.js:25:20:25:30 | window.name | react-use-state.js:21:10:21:14 | state | provenance |  |
 | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:23:29:23:35 | tainted | provenance |  |
-| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:25:29:25:35 | tainted | provenance |  |
-| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:28:29:28:35 | tainted | provenance |  |
 | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:30:29:30:35 | tainted | provenance |  |
 | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:33:29:33:35 | tainted | provenance |  |
-| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:35:29:35:35 | tainted | provenance |  |
 | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:38:29:38:35 | tainted | provenance |  |
 | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:45:29:45:35 | tainted | provenance |  |
 | sanitiser.js:16:7:16:27 | tainted | sanitiser.js:48:19:48:25 | tainted | provenance |  |
 | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:16:7:16:27 | tainted | provenance |  |
 | sanitiser.js:23:29:23:35 | tainted | sanitiser.js:23:21:23:44 | '<b>' + ...  '</b>' | provenance |  |
-| sanitiser.js:25:29:25:35 | tainted | sanitiser.js:25:21:25:44 | '<b>' + ...  '</b>' | provenance |  |
-| sanitiser.js:28:29:28:35 | tainted | sanitiser.js:28:21:28:44 | '<b>' + ...  '</b>' | provenance |  |
 | sanitiser.js:30:29:30:35 | tainted | sanitiser.js:30:21:30:44 | '<b>' + ...  '</b>' | provenance |  |
 | sanitiser.js:33:29:33:35 | tainted | sanitiser.js:33:21:33:44 | '<b>' + ...  '</b>' | provenance |  |
-| sanitiser.js:35:29:35:35 | tainted | sanitiser.js:35:21:35:44 | '<b>' + ...  '</b>' | provenance |  |
 | sanitiser.js:38:29:38:35 | tainted | sanitiser.js:38:21:38:44 | '<b>' + ...  '</b>' | provenance |  |
 | sanitiser.js:45:29:45:35 | tainted | sanitiser.js:45:21:45:44 | '<b>' + ...  '</b>' | provenance |  |
 | sanitiser.js:48:19:48:25 | tainted | sanitiser.js:48:19:48:46 | tainted ... /g, '') | provenance |  |

Original file line number	Diff line number	Diff line change
`@@ -55,7 +55,9 @@ module DomBasedXssConfig implements DataFlow::StateConfigSig {`
`55`	`55`	`label = prefixLabel()`
`56`	`56`	`}`
`57`	`57`
`58`		`- predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }`
	`58`	`+ predicate isBarrier(DataFlow::Node node) {`
	`59`	`+ node instanceof Sanitizer or node = Shared::BarrierGuard::getABarrierNode()`
	`60`	`+ }`
`59`	`61`
`60`	`62`	`predicate isBarrier(DataFlow::Node node, DataFlow::FlowLabel lbl) {`
`61`	`63`	`// copy all taint barrier guards to the TaintedUrlSuffix/PrefixLabel label`
Original file line number	Diff line number	Diff line change
`@@ -140,7 +140,9 @@ module ExceptionXssConfig implements DataFlow::StateConfigSig {`
`140`	`140`	`sink instanceof XssShared::Sink and not label instanceof NotYetThrown`
`141`	`141`	`}`
`142`	`142`
`143`		`- predicate isBarrier(DataFlow::Node node) { node instanceof XssShared::Sanitizer }`
	`143`	`+ predicate isBarrier(DataFlow::Node node) {`
	`144`	`+ node instanceof XssShared::Sanitizer or node = XssShared::BarrierGuard::getABarrierNode()`
	`145`	`+ }`
`144`	`146`
`145`	`147`	`predicate isAdditionalFlowStep(`
`146`	`148`	`DataFlow::Node pred, DataFlow::FlowLabel inlbl, DataFlow::Node succ, DataFlow::FlowLabel outlbl`
Original file line number	Diff line number	Diff line change
`@@ -15,7 +15,9 @@ module StoredXssConfig implements DataFlow::ConfigSig {`
`15`	`15`
`16`	`16`	`predicate isSink(DataFlow::Node sink) { sink instanceof Sink }`
`17`	`17`
`18`		`- predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }`
	`18`	`+ predicate isBarrier(DataFlow::Node node) {`
	`19`	`+ node instanceof Sanitizer or node = Shared::BarrierGuard::getABarrierNode()`
	`20`	`+ }`
`19`	`21`	`}`
`20`	`22`
`21`	`23`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -34,6 +34,8 @@ module UnsafeHtmlConstructionConfig implements DataFlow::StateConfigSig {`
`34`	`34`	`node instanceof UnsafeJQueryPlugin::Sanitizer`
`35`	`35`	`or`
`36`	`36`	`DomBasedXss::isOptionallySanitizedNode(node)`
	`37`	`+ or`
	`38`	`+ node = Shared::BarrierGuard::getABarrierNode()`
`37`	`39`	`}`
`38`	`40`
`39`	`41`	`predicate isBarrier(DataFlow::Node node, DataFlow::FlowLabel label) {`
Original file line number	Diff line number	Diff line change
`@@ -22,7 +22,8 @@ module XssThroughDomConfig implements DataFlow::ConfigSig {`
`22`	`22`	`node instanceof DomBasedXss::Sanitizer or`
`23`	`23`	`DomBasedXss::isOptionallySanitizedNode(node) or`
`24`	`24`	`node = DataFlow::MakeBarrierGuard<BarrierGuard>::getABarrierNode() or`
`25`		`- node = DataFlow::MakeBarrierGuard<UnsafeJQuery::BarrierGuard>::getABarrierNode()`
	`25`	`+ node = DataFlow::MakeBarrierGuard<UnsafeJQuery::BarrierGuard>::getABarrierNode() or`
	`26`	`+ node = Shared::BarrierGuard::getABarrierNode()`
`26`	`27`	`}`
`27`	`28`
`28`	`29`	`predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {`
Original file line number	Diff line number	Diff line change
`@@ -1,3 +0,0 @@`
`1`		`-\| query-tests/Security/CWE-079/DomBasedXss/sanitiser.js:25 \| did not expect an alert, but found an alert for HtmlInjection \| OK \| ConsistencyConfig \|`
`2`		`-\| query-tests/Security/CWE-079/DomBasedXss/sanitiser.js:28 \| did not expect an alert, but found an alert for HtmlInjection \| OK \| ConsistencyConfig \|`
`3`		`-\| query-tests/Security/CWE-079/DomBasedXss/sanitiser.js:35 \| did not expect an alert, but found an alert for HtmlInjection \| OK \| ConsistencyConfig \|`