Skip to content

JS: Fix bug causing re-evaluation of cached barriers #17643

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
asgerf merged 3 commits into from
Oct 3, 2024
Merged

JS: Fix bug causing re-evaluation of cached barriers #17643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
341bacf
JS: Fix bug causing re-evaluation of cached barriers
asgerf Oct 1, 2024
6cbe04d
JS: Consistently use the shared XSS barrier guards in the XSS queries
asgerf Oct 2, 2024
5d2ce17
JS: Update a test to handle AdditionalSanitizerGuardNode
asgerf Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

/**
Expand Down Expand Up @@ -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.
*
Expand All @@ -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)
}

/**
Expand Down Expand Up @@ -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, "")
)
}
Expand Down Expand Up @@ -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)
)
}
Expand Down
34 changes: 33 additions & 1 deletion javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
*/
Expand Down
4 changes: 3 additions & 1 deletion javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion javascript/ql/lib/semmle/javascript/security/dataflow/ExceptionXssQuery.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion javascript/ql/lib/semmle/javascript/security/dataflow/StoredXssQuery.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}

/**
Expand Down
2 changes: 2 additions & 0 deletions javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
3 changes: 2 additions & 1 deletion javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
6 changes: 4 additions & 2 deletions javascript/ql/test/library-tests/TaintBarriers/tests.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
3 changes: 0 additions & 3 deletions javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/ConsistencyDomBasedXss.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 |
15 changes: 0 additions & 15 deletions javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>' |
Expand Down Expand Up @@ -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 | |
Expand Down Expand Up @@ -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 |
Expand Down
12 changes: 0 additions & 12 deletions ...script/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>' |
Expand Down Expand Up @@ -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 | |
Expand Down