diff --git a/javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll index 981155a5fee3..3d62ca33a665 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll @@ -287,6 +287,9 @@ private predicate isBarrierGuardInternal(Configuration cfg, BarrierGuardNodeInte guard.(AdditionalBarrierGuardNode).appliesTo(cfg) or guard.(DerivedBarrierGuardNode).appliesTo(cfg) + or + cfg instanceof TaintTracking::Configuration and + guard.(TaintTracking::AdditionalSanitizerGuardNode).appliesTo(cfg) } /** @@ -390,6 +393,12 @@ abstract private class DerivedBarrierGuardNode extends BarrierGuardNodeInternal abstract predicate blocks(boolean outcome, Expr e, string label); } +/** + * Barrier guards derived from `AdditionalSanitizerGuard` + */ +private class BarrierGuardNodeFromAdditionalSanitizerGuard extends BarrierGuardNodeInternal instanceof TaintTracking::AdditionalSanitizerGuardNode +{ } + /** * Holds if data flow node `guard` acts as a barrier for data flow. * @@ -404,6 +413,10 @@ private predicate barrierGuardBlocksExpr( guard.(BarrierGuardNode).blocks(outcome, test, label) or guard.(DerivedBarrierGuardNode).blocks(outcome, test, label) + or + guard.(TaintTracking::AdditionalSanitizerGuardNode).sanitizes(outcome, test) and label = "taint" + or + guard.(TaintTracking::AdditionalSanitizerGuardNode).sanitizes(outcome, test, label) } /** @@ -534,7 +547,7 @@ private predicate isBarrierEdgeRaw(Configuration cfg, DataFlow::Node pred, DataF cfg.isBarrierEdge(pred, succ) or exists(BarrierGuardNodeInternal guard | - cfg.isBarrierGuard(guard) and + isBarrierGuardInternal(cfg, guard) and barrierGuardBlocksEdge(guard, pred, succ, "") ) } @@ -564,7 +577,7 @@ private predicate isLabeledBarrierEdgeRaw( cfg.isBarrierEdge(pred, succ, label) or exists(BarrierGuardNodeInternal guard | - cfg.isBarrierGuard(guard) and + isBarrierGuardInternal(cfg, guard) and barrierGuardBlocksEdge(guard, pred, succ, label) ) } diff --git a/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll index ad42131ca4df..d95c27ad5d1d 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll @@ -182,7 +182,39 @@ module TaintTracking { * for analysis-specific taint sanitizer guards. */ cached - abstract class AdditionalSanitizerGuardNode extends SanitizerGuardNode { + abstract class AdditionalSanitizerGuardNode extends DataFlow::Node { + // For backwards compatibility, this contains a copy of the SanitizerGuard interface, + // but is does not inherit from it as that would cause re-evaluation of cached barriers. + /** + * Holds if this node blocks expression `e`, provided it evaluates to `outcome`. + */ + cached + predicate blocks(boolean outcome, Expr e) { none() } + + /** + * Holds if this node sanitizes expression `e`, provided it evaluates + * to `outcome`. + */ + cached + abstract predicate sanitizes(boolean outcome, Expr e); + + /** + * Holds if this node blocks expression `e` from flow of type `label`, provided it evaluates to `outcome`. + */ + cached + predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) { + this.sanitizes(outcome, e) and label.isTaint() + or + this.sanitizes(outcome, e, label) + } + + /** + * Holds if this node sanitizes expression `e`, provided it evaluates + * to `outcome`. + */ + cached + predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) { none() } + /** * Holds if this guard applies to the flow in `cfg`. */ diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll index 8a53e4f0790f..9c326c4f389d 100644 --- 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 index 9a748c0c301a..6908e50960d7 100644 --- 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 index b40b610b71e9..87a870abe35b 100644 --- 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 index 3101836334f1..dc3b7c96b990 100644 --- 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 index c9d8112ba5dd..22486167db3a 100644 --- 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::getABarrierNode() or - node = DataFlow::MakeBarrierGuard::getABarrierNode() + node = DataFlow::MakeBarrierGuard::getABarrierNode() or + node = Shared::BarrierGuard::getABarrierNode() } predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) { diff --git a/javascript/ql/test/library-tests/TaintBarriers/tests.ql b/javascript/ql/test/library-tests/TaintBarriers/tests.ql index 0feeae23a64d..d6f3b4ae845d 100644 --- a/javascript/ql/test/library-tests/TaintBarriers/tests.ql +++ b/javascript/ql/test/library-tests/TaintBarriers/tests.ql @@ -11,8 +11,10 @@ query predicate isLabeledBarrier( query predicate isSanitizer(ExampleConfiguration cfg, DataFlow::Node n) { cfg.isSanitizer(n) } -query predicate sanitizingGuard(TaintTracking::SanitizerGuardNode g, Expr e, boolean b) { - g.sanitizes(b, e) +query predicate sanitizingGuard(DataFlow::Node g, Expr e, boolean b) { + g.(TaintTracking::SanitizerGuardNode).sanitizes(b, e) + or + g.(TaintTracking::AdditionalSanitizerGuardNode).sanitizes(b, e) } query predicate taintedSink(DataFlow::Node source, DataFlow::Node sink) { 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 index 3ea47160e92b..e69de29bb2d1 100644 --- 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 index 922118ef84bc..0b29d370c6c0 100644 --- 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 | '' + ... '' | semmle.label | '' + ... '' | | sanitiser.js:23:29:23:35 | tainted | semmle.label | tainted | -| sanitiser.js:25:21:25:44 | '' + ... '' | semmle.label | '' + ... '' | -| sanitiser.js:25:29:25:35 | tainted | semmle.label | tainted | -| sanitiser.js:28:21:28:44 | '' + ... '' | semmle.label | '' + ... '' | -| sanitiser.js:28:29:28:35 | tainted | semmle.label | tainted | | sanitiser.js:30:21:30:44 | '' + ... '' | semmle.label | '' + ... '' | | sanitiser.js:30:29:30:35 | tainted | semmle.label | tainted | | sanitiser.js:33:21:33:44 | '' + ... '' | semmle.label | '' + ... '' | | sanitiser.js:33:29:33:35 | tainted | semmle.label | tainted | -| sanitiser.js:35:21:35:44 | '' + ... '' | semmle.label | '' + ... '' | -| sanitiser.js:35:29:35:35 | tainted | semmle.label | tainted | | sanitiser.js:38:21:38:44 | '' + ... '' | semmle.label | '' + ... '' | | sanitiser.js:38:29:38:35 | tainted | semmle.label | tainted | | sanitiser.js:45:21:45:44 | '' + ... '' | semmle.label | '' + ... '' | @@ -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 | '' + ... '' | provenance | | -| sanitiser.js:25:29:25:35 | tainted | sanitiser.js:25:21:25:44 | '' + ... '' | provenance | | -| sanitiser.js:28:29:28:35 | tainted | sanitiser.js:28:21:28:44 | '' + ... '' | provenance | | | sanitiser.js:30:29:30:35 | tainted | sanitiser.js:30:21:30:44 | '' + ... '' | provenance | | | sanitiser.js:33:29:33:35 | tainted | sanitiser.js:33:21:33:44 | '' + ... '' | provenance | | -| sanitiser.js:35:29:35:35 | tainted | sanitiser.js:35:21:35:44 | '' + ... '' | provenance | | | sanitiser.js:38:29:38:35 | tainted | sanitiser.js:38:21:38:44 | '' + ... '' | provenance | | | sanitiser.js:45:29:45:35 | tainted | sanitiser.js:45:21:45:44 | '' + ... '' | 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 | '' + ... '' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:23:21:23:44 | '' + ... '' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value | -| sanitiser.js:25:21:25:44 | '' + ... '' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:25:21:25:44 | '' + ... '' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value | -| sanitiser.js:28:21:28:44 | '' + ... '' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:28:21:28:44 | '' + ... '' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value | | sanitiser.js:30:21:30:44 | '' + ... '' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:30:21:30:44 | '' + ... '' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value | | sanitiser.js:33:21:33:44 | '' + ... '' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:33:21:33:44 | '' + ... '' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value | -| sanitiser.js:35:21:35:44 | '' + ... '' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:35:21:35:44 | '' + ... '' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value | | sanitiser.js:38:21:38:44 | '' + ... '' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:38:21:38:44 | '' + ... '' | Cross-site scripting vulnerability due to $@. | sanitiser.js:16:17:16:27 | window.name | user-provided value | | sanitiser.js:45:21:45:44 | '' + ... '' | sanitiser.js:16:17:16:27 | window.name | sanitiser.js:45:21:45:44 | '' + ... '' | 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 index b5be3dc820c3..c857ff42b092 100644 --- 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 | '' + ... '' | semmle.label | '' + ... '' | | sanitiser.js:23:29:23:35 | tainted | semmle.label | tainted | -| sanitiser.js:25:21:25:44 | '' + ... '' | semmle.label | '' + ... '' | -| sanitiser.js:25:29:25:35 | tainted | semmle.label | tainted | -| sanitiser.js:28:21:28:44 | '' + ... '' | semmle.label | '' + ... '' | -| sanitiser.js:28:29:28:35 | tainted | semmle.label | tainted | | sanitiser.js:30:21:30:44 | '' + ... '' | semmle.label | '' + ... '' | | sanitiser.js:30:29:30:35 | tainted | semmle.label | tainted | | sanitiser.js:33:21:33:44 | '' + ... '' | semmle.label | '' + ... '' | | sanitiser.js:33:29:33:35 | tainted | semmle.label | tainted | -| sanitiser.js:35:21:35:44 | '' + ... '' | semmle.label | '' + ... '' | -| sanitiser.js:35:29:35:35 | tainted | semmle.label | tainted | | sanitiser.js:38:21:38:44 | '' + ... '' | semmle.label | '' + ... '' | | sanitiser.js:38:29:38:35 | tainted | semmle.label | tainted | | sanitiser.js:45:21:45:44 | '' + ... '' | semmle.label | '' + ... '' | @@ -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 | '' + ... '' | provenance | | -| sanitiser.js:25:29:25:35 | tainted | sanitiser.js:25:21:25:44 | '' + ... '' | provenance | | -| sanitiser.js:28:29:28:35 | tainted | sanitiser.js:28:21:28:44 | '' + ... '' | provenance | | | sanitiser.js:30:29:30:35 | tainted | sanitiser.js:30:21:30:44 | '' + ... '' | provenance | | | sanitiser.js:33:29:33:35 | tainted | sanitiser.js:33:21:33:44 | '' + ... '' | provenance | | -| sanitiser.js:35:29:35:35 | tainted | sanitiser.js:35:21:35:44 | '' + ... '' | provenance | | | sanitiser.js:38:29:38:35 | tainted | sanitiser.js:38:21:38:44 | '' + ... '' | provenance | | | sanitiser.js:45:29:45:35 | tainted | sanitiser.js:45:21:45:44 | '' + ... '' | provenance | | | sanitiser.js:48:19:48:25 | tainted | sanitiser.js:48:19:48:46 | tainted ... /g, '') | provenance | |