Skip to content

Commit 72daa98

Browse files
asgerfasgerf
authored
Merge pull request github#17643 from asgerf/jss/cached-barriers
JS: Fix bug causing re-evaluation of cached barriers
2 parents 1cd00a1 + 5d2ce17 commit 72daa98

File tree

11 files changed

+65
-39
lines changed

11 files changed

+65
-39
lines changed

‎javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,9 @@ private predicate isBarrierGuardInternal(Configuration cfg, BarrierGuardNodeInte
287287
guard.(AdditionalBarrierGuardNode).appliesTo(cfg)
288288
or
289289
guard.(DerivedBarrierGuardNode).appliesTo(cfg)
290+
or
291+
cfg instanceof TaintTracking::Configuration and
292+
guard.(TaintTracking::AdditionalSanitizerGuardNode).appliesTo(cfg)
290293
}
291294

292295
/**
@@ -390,6 +393,12 @@ abstract private class DerivedBarrierGuardNode extends BarrierGuardNodeInternal
390393
abstract predicate blocks(boolean outcome, Expr e, string label);
391394
}
392395

396+
/**
397+
* Barrier guards derived from `AdditionalSanitizerGuard`
398+
*/
399+
private class BarrierGuardNodeFromAdditionalSanitizerGuard extends BarrierGuardNodeInternal instanceof TaintTracking::AdditionalSanitizerGuardNode
400+
{ }
401+
393402
/**
394403
* Holds if data flow node `guard` acts as a barrier for data flow.
395404
*
@@ -404,6 +413,10 @@ private predicate barrierGuardBlocksExpr(
404413
guard.(BarrierGuardNode).blocks(outcome, test, label)
405414
or
406415
guard.(DerivedBarrierGuardNode).blocks(outcome, test, label)
416+
or
417+
guard.(TaintTracking::AdditionalSanitizerGuardNode).sanitizes(outcome, test) and label = "taint"
418+
or
419+
guard.(TaintTracking::AdditionalSanitizerGuardNode).sanitizes(outcome, test, label)
407420
}
408421

409422
/**
@@ -534,7 +547,7 @@ private predicate isBarrierEdgeRaw(Configuration cfg, DataFlow::Node pred, DataF
534547
cfg.isBarrierEdge(pred, succ)
535548
or
536549
exists(BarrierGuardNodeInternal guard |
537-
cfg.isBarrierGuard(guard) and
550+
isBarrierGuardInternal(cfg, guard) and
538551
barrierGuardBlocksEdge(guard, pred, succ, "")
539552
)
540553
}
@@ -564,7 +577,7 @@ private predicate isLabeledBarrierEdgeRaw(
564577
cfg.isBarrierEdge(pred, succ, label)
565578
or
566579
exists(BarrierGuardNodeInternal guard |
567-
cfg.isBarrierGuard(guard) and
580+
isBarrierGuardInternal(cfg, guard) and
568581
barrierGuardBlocksEdge(guard, pred, succ, label)
569582
)
570583
}

‎javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,39 @@ module TaintTracking {
182182
* for analysis-specific taint sanitizer guards.
183183
*/
184184
cached
185-
abstract class AdditionalSanitizerGuardNode extends SanitizerGuardNode {
185+
abstract class AdditionalSanitizerGuardNode extends DataFlow::Node {
186+
// For backwards compatibility, this contains a copy of the SanitizerGuard interface,
187+
// but is does not inherit from it as that would cause re-evaluation of cached barriers.
188+
/**
189+
* Holds if this node blocks expression `e`, provided it evaluates to `outcome`.
190+
*/
191+
cached
192+
predicate blocks(boolean outcome, Expr e) { none() }
193+
194+
/**
195+
* Holds if this node sanitizes expression `e`, provided it evaluates
196+
* to `outcome`.
197+
*/
198+
cached
199+
abstract predicate sanitizes(boolean outcome, Expr e);
200+
201+
/**
202+
* Holds if this node blocks expression `e` from flow of type `label`, provided it evaluates to `outcome`.
203+
*/
204+
cached
205+
predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
206+
this.sanitizes(outcome, e) and label.isTaint()
207+
or
208+
this.sanitizes(outcome, e, label)
209+
}
210+
211+
/**
212+
* Holds if this node sanitizes expression `e`, provided it evaluates
213+
* to `outcome`.
214+
*/
215+
cached
216+
predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) { none() }
217+
186218
/**
187219
* Holds if this guard applies to the flow in `cfg`.
188220
*/

‎javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,9 @@ module DomBasedXssConfig implements DataFlow::StateConfigSig {
5555
label = prefixLabel()
5656
}
5757

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+
}
5961

6062
predicate isBarrier(DataFlow::Node node, DataFlow::FlowLabel lbl) {
6163
// copy all taint barrier guards to the TaintedUrlSuffix/PrefixLabel label

‎javascript/ql/lib/semmle/javascript/security/dataflow/ExceptionXssQuery.qll

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,9 @@ module ExceptionXssConfig implements DataFlow::StateConfigSig {
140140
sink instanceof XssShared::Sink and not label instanceof NotYetThrown
141141
}
142142

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+
}
144146

145147
predicate isAdditionalFlowStep(
146148
DataFlow::Node pred, DataFlow::FlowLabel inlbl, DataFlow::Node succ, DataFlow::FlowLabel outlbl

‎javascript/ql/lib/semmle/javascript/security/dataflow/StoredXssQuery.qll

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@ module StoredXssConfig implements DataFlow::ConfigSig {
1515

1616
predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
1717

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+
}
1921
}
2022

2123
/**

‎javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ module UnsafeHtmlConstructionConfig implements DataFlow::StateConfigSig {
3434
node instanceof UnsafeJQueryPlugin::Sanitizer
3535
or
3636
DomBasedXss::isOptionallySanitizedNode(node)
37+
or
38+
node = Shared::BarrierGuard::getABarrierNode()
3739
}
3840

3941
predicate isBarrier(DataFlow::Node node, DataFlow::FlowLabel label) {

‎javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@ module XssThroughDomConfig implements DataFlow::ConfigSig {
2222
node instanceof DomBasedXss::Sanitizer or
2323
DomBasedXss::isOptionallySanitizedNode(node) or
2424
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()
2627
}
2728

2829
predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {

‎javascript/ql/test/library-tests/TaintBarriers/tests.ql

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,10 @@ query predicate isLabeledBarrier(
1111

1212
query predicate isSanitizer(ExampleConfiguration cfg, DataFlow::Node n) { cfg.isSanitizer(n) }
1313

14-
query predicate sanitizingGuard(TaintTracking::SanitizerGuardNode g, Expr e, boolean b) {
15-
g.sanitizes(b, e)
14+
query predicate sanitizingGuard(DataFlow::Node g, Expr e, boolean b) {
15+
g.(TaintTracking::SanitizerGuardNode).sanitizes(b, e)
16+
or
17+
g.(TaintTracking::AdditionalSanitizerGuardNode).sanitizes(b, e)
1618
}
1719

1820
query predicate taintedSink(DataFlow::Node source, DataFlow::Node sink) {

‎javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/ConsistencyDomBasedXss.expected

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff 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 |

‎javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -284,16 +284,10 @@ nodes
284284
| sanitiser.js:16:17:16:27 | window.name | semmle.label | window.name |
285285
| sanitiser.js:23:21:23:44 | '<b>' + ... '</b>' | semmle.label | '<b>' + ... '</b>' |
286286
| sanitiser.js:23:29:23:35 | tainted | semmle.label | tainted |
287-
| sanitiser.js:25:21:25:44 | '<b>' + ... '</b>' | semmle.label | '<b>' + ... '</b>' |
288-
| sanitiser.js:25:29:25:35 | tainted | semmle.label | tainted |
289-
| sanitiser.js:28:21:28:44 | '<b>' + ... '</b>' | semmle.label | '<b>' + ... '</b>' |
290-
| sanitiser.js:28:29:28:35 | tainted | semmle.label | tainted |
291287
| sanitiser.js:30:21:30:44 | '<b>' + ... '</b>' | semmle.label | '<b>' + ... '</b>' |
292288
| sanitiser.js:30:29:30:35 | tainted | semmle.label | tainted |
293289
| sanitiser.js:33:21:33:44 | '<b>' + ... '</b>' | semmle.label | '<b>' + ... '</b>' |
294290
| sanitiser.js:33:29:33:35 | tainted | semmle.label | tainted |
295-
| sanitiser.js:35:21:35:44 | '<b>' + ... '</b>' | semmle.label | '<b>' + ... '</b>' |
296-
| sanitiser.js:35:29:35:35 | tainted | semmle.label | tainted |
297291
| sanitiser.js:38:21:38:44 | '<b>' + ... '</b>' | semmle.label | '<b>' + ... '</b>' |
298292
| sanitiser.js:38:29:38:35 | tainted | semmle.label | tainted |
299293
| sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' | semmle.label | '<b>' + ... '</b>' |
@@ -852,21 +846,15 @@ edges
852846
| react-use-state.js:22:14:22:17 | prev | react-use-state.js:23:35:23:38 | prev | provenance | |
853847
| react-use-state.js:25:20:25:30 | window.name | react-use-state.js:21:10:21:14 | state | provenance | |
854848
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:23:29:23:35 | tainted | provenance | |
855-
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:25:29:25:35 | tainted | provenance | |
856-
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:28:29:28:35 | tainted | provenance | |
857849
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:30:29:30:35 | tainted | provenance | |
858850
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:33:29:33:35 | tainted | provenance | |
859-
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:35:29:35:35 | tainted | provenance | |
860851
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:38:29:38:35 | tainted | provenance | |
861852
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:45:29:45:35 | tainted | provenance | |
862853
| sanitiser.js:16:7:16:27 | tainted | sanitiser.js:48:19:48:25 | tainted | provenance | |
863854
| sanitiser.js:16:17:16:27 | window.name | sanitiser.js:16:7:16:27 | tainted | provenance | |
864855
| sanitiser.js:23:29:23:35 | tainted | sanitiser.js:23:21:23:44 | '<b>' + ... '</b>' | provenance | |
865-
| sanitiser.js:25:29:25:35 | tainted | sanitiser.js:25:21:25:44 | '<b>' + ... '</b>' | provenance | |
866-
| sanitiser.js:28:29:28:35 | tainted | sanitiser.js:28:21:28:44 | '<b>' + ... '</b>' | provenance | |
867856
| sanitiser.js:30:29:30:35 | tainted | sanitiser.js:30:21:30:44 | '<b>' + ... '</b>' | provenance | |
868857
| sanitiser.js:33:29:33:35 | tainted | sanitiser.js:33:21:33:44 | '<b>' + ... '</b>' | provenance | |
869-
| sanitiser.js:35:29:35:35 | tainted | sanitiser.js:35:21:35:44 | '<b>' + ... '</b>' | provenance | |
870858
| sanitiser.js:38:29:38:35 | tainted | sanitiser.js:38:21:38:44 | '<b>' + ... '</b>' | provenance | |
871859
| sanitiser.js:45:29:45:35 | tainted | sanitiser.js:45:21:45:44 | '<b>' + ... '</b>' | provenance | |
872860
| sanitiser.js:48:19:48:25 | tainted | sanitiser.js:48:19:48:46 | tainted ... /g, '') | provenance | |
@@ -1265,11 +1253,8 @@ subpaths
12651253
| 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 |
12661254
| 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 |
12671255
| 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 |
1268-
| 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 |
1269-
| 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 |
12701256
| 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 |
12711257
| 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 |
1272-
| 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 |
12731258
| 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 |
12741259
| 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 |
12751260
| 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 |

0 commit comments

Comments
 (0)