Skip to content

Commit d57276c

Browse files
asgerfasgerf
authored
Merge pull request #13719 from asgerf/js/barrier-inout
JS: Replace barrier edges with barrier nodes
2 parents cafc67e + c7abd4c commit d57276c

29 files changed

Lines changed: 216 additions & 107 deletions

‎javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,18 +23,15 @@ class DomBasedXssAtmConfig extends AtmConfig {
2323

2424
override predicate isSanitizer(DataFlow::Node node) {
2525
super.isSanitizer(node) or
26-
node instanceof DomBasedXss::Sanitizer
26+
node instanceof DomBasedXss::Sanitizer or
27+
DomBasedXss::isOptionallySanitizedNode(node)
2728
}
2829

2930
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
3031
guard instanceof PrefixStringSanitizerActivated or
3132
guard instanceof QuoteGuard or
3233
guard instanceof ContainsHtmlGuard
3334
}
34-
35-
override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
36-
DomBasedXss::isOptionallySanitizedEdge(pred, succ)
37-
}
3835
}
3936

4037
private import semmle.javascript.security.dataflow.Xss::Shared as Shared

‎javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssThroughDomATM.qll‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@ class XssThroughDomAtmConfig extends AtmConfig {
2323

2424
override predicate isSanitizer(DataFlow::Node node) {
2525
super.isSanitizer(node) or
26-
node instanceof DomBasedXss::Sanitizer
26+
node instanceof DomBasedXss::Sanitizer or
27+
DomBasedXss::isOptionallySanitizedNode(node)
2728
}
2829

2930
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
@@ -34,10 +35,6 @@ class XssThroughDomAtmConfig extends AtmConfig {
3435
guard instanceof QuoteGuard or
3536
guard instanceof ContainsHtmlGuard
3637
}
37-
38-
override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
39-
DomBasedXss::isOptionallySanitizedEdge(pred, succ)
40-
}
4138
}
4239

4340
/**

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

Lines changed: 52 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,26 @@ abstract class Configuration extends string {
166166
)
167167
}
168168

169+
/**
170+
* Holds if flow into `node` is prohibited.
171+
*/
172+
predicate isBarrierIn(DataFlow::Node node) { none() }
173+
174+
/**
175+
* Holds if flow out `node` is prohibited.
176+
*/
177+
predicate isBarrierOut(DataFlow::Node node) { none() }
178+
179+
/**
180+
* Holds if flow into `node` is prohibited for the flow label `lbl`.
181+
*/
182+
predicate isBarrierIn(DataFlow::Node node, FlowLabel lbl) { none() }
183+
184+
/**
185+
* Holds if flow out `node` is prohibited for the flow label `lbl`.
186+
*/
187+
predicate isBarrierOut(DataFlow::Node node, FlowLabel lbl) { none() }
188+
169189
/**
170190
* Holds if flow from `pred` to `succ` is prohibited.
171191
*/
@@ -494,7 +514,7 @@ private BasicBlock getADominatedBasicBlock(BarrierGuardNode guard, ConditionGuar
494514
*
495515
* Only holds for barriers that should apply to all flow labels.
496516
*/
497-
private predicate isBarrierEdge(Configuration cfg, DataFlow::Node pred, DataFlow::Node succ) {
517+
private predicate isBarrierEdgeRaw(Configuration cfg, DataFlow::Node pred, DataFlow::Node succ) {
498518
cfg.isBarrierEdge(pred, succ)
499519
or
500520
exists(DataFlow::BarrierGuardNode guard |
@@ -503,11 +523,26 @@ private predicate isBarrierEdge(Configuration cfg, DataFlow::Node pred, DataFlow
503523
)
504524
}
505525

526+
/**
527+
* Holds if there is a barrier edge `pred -> succ` in `cfg` either through an explicit barrier edge
528+
* or one implied by a barrier guard, or by an out/in barrier for `pred` or `succ`, respectively.
529+
*
530+
* Only holds for barriers that should apply to all flow labels.
531+
*/
532+
pragma[inline]
533+
private predicate isBarrierEdge(Configuration cfg, DataFlow::Node pred, DataFlow::Node succ) {
534+
isBarrierEdgeRaw(cfg, pred, succ)
535+
or
536+
cfg.isBarrierOut(pred)
537+
or
538+
cfg.isBarrierIn(succ)
539+
}
540+
506541
/**
507542
* Holds if there is a labeled barrier edge `pred -> succ` in `cfg` either through an explicit barrier edge
508543
* or one implied by a barrier guard.
509544
*/
510-
private predicate isLabeledBarrierEdge(
545+
private predicate isLabeledBarrierEdgeRaw(
511546
Configuration cfg, DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel label
512547
) {
513548
cfg.isBarrierEdge(pred, succ, label)
@@ -518,6 +553,21 @@ private predicate isLabeledBarrierEdge(
518553
)
519554
}
520555

556+
/**
557+
* Holds if there is a labeled barrier edge `pred -> succ` in `cfg` either through an explicit barrier edge
558+
* or one implied by a barrier guard, or by an out/in barrier for `pred` or `succ`, respectively.
559+
*/
560+
pragma[inline]
561+
private predicate isLabeledBarrierEdge(
562+
Configuration cfg, DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel label
563+
) {
564+
isLabeledBarrierEdgeRaw(cfg, pred, succ, label)
565+
or
566+
cfg.isBarrierOut(pred, label)
567+
or
568+
cfg.isBarrierIn(succ, label)
569+
}
570+
521571
/**
522572
* A guard node that only blocks specific labels.
523573
*/

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,26 @@ module TaintTracking {
6262
*/
6363
predicate isSanitizer(DataFlow::Node node) { none() }
6464

65+
/**
66+
* Holds if flow into `node` is prohibited.
67+
*/
68+
predicate isSanitizerIn(DataFlow::Node node) { none() }
69+
70+
/**
71+
* Holds if flow out `node` is prohibited.
72+
*/
73+
predicate isSanitizerOut(DataFlow::Node node) { none() }
74+
75+
/**
76+
* Holds if flow into `node` is prohibited for the flow label `lbl`.
77+
*/
78+
predicate isSanitizerIn(DataFlow::Node node, DataFlow::FlowLabel lbl) { none() }
79+
80+
/**
81+
* Holds if flow out `node` is prohibited for the flow label `lbl`.
82+
*/
83+
predicate isSanitizerOut(DataFlow::Node node, DataFlow::FlowLabel lbl) { none() }
84+
6585
/** Holds if the edge from `pred` to `succ` is a taint sanitizer. */
6686
predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) { none() }
6787

@@ -108,6 +128,22 @@ module TaintTracking {
108128
this.isSanitizerEdge(source, sink) and lbl.isTaint()
109129
}
110130

131+
final override predicate isBarrierIn(DataFlow::Node node) { none() }
132+
133+
final override predicate isBarrierOut(DataFlow::Node node) { none() }
134+
135+
final override predicate isBarrierIn(DataFlow::Node node, DataFlow::FlowLabel lbl) {
136+
this.isSanitizerIn(node, lbl)
137+
or
138+
this.isSanitizerIn(node) and lbl.isTaint()
139+
}
140+
141+
final override predicate isBarrierOut(DataFlow::Node node, DataFlow::FlowLabel lbl) {
142+
this.isSanitizerOut(node, lbl)
143+
or
144+
this.isSanitizerOut(node) and lbl.isTaint()
145+
}
146+
111147
final override predicate isBarrierGuard(DataFlow::BarrierGuardNode guard) {
112148
super.isBarrierGuard(guard) or
113149
guard.(AdditionalSanitizerGuardNode).appliesTo(this) or

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

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,6 @@ class Configuration extends TaintTracking::Configuration {
2727

2828
override predicate isSanitizer(DataFlow::Node node) { node instanceof CleartextLogging::Barrier }
2929

30-
override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
31-
CleartextLogging::isSanitizerEdge(pred, succ)
32-
}
33-
3430
override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node trg) {
3531
CleartextLogging::isAdditionalTaintStep(src, trg)
3632
}

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

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,12 +175,24 @@ module CleartextLogging {
175175
}
176176

177177
/**
178+
* DEPRECATED. Use `Barrier` instead, sanitized have been replaced by sanitized nodes.
179+
*
178180
* Holds if the edge `pred` -> `succ` should be sanitized for clear-text logging of sensitive information.
179181
*/
180-
predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
182+
deprecated predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
181183
succ.(DataFlow::PropRead).getBase() = pred
182184
}
183185

186+
private class PropReadAsBarrier extends Barrier {
187+
PropReadAsBarrier() {
188+
this = any(DataFlow::PropRead read).getBase() and
189+
// the 'foo' in 'foo.bar()' may have flow, we only want to suppress plain property reads
190+
not this = any(DataFlow::MethodCallNode call).getReceiver() and
191+
// do not block custom taint steps from this node
192+
not isAdditionalTaintStep(this, _)
193+
}
194+
}
195+
184196
/**
185197
* Holds if the edge `src` -> `trg` is an additional taint-step for clear-text logging of sensitive information.
186198
*/

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

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,6 @@ class Configuration extends TaintTracking::Configuration {
3333

3434
override predicate isSanitizer(DataFlow::Node node) { node instanceof Barrier }
3535

36-
override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
37-
CleartextLogging::isSanitizerEdge(pred, succ)
38-
}
39-
4036
override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node trg) {
4137
CleartextLogging::isAdditionalTaintStep(src, trg)
4238
}

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,7 @@ class Configuration extends TaintTracking::Configuration {
3131
node instanceof Sanitizer
3232
}
3333

34-
override predicate isSanitizerEdge(DataFlow::Node source, DataFlow::Node sink) {
35-
sanitizingPrefixEdge(source, sink)
36-
}
34+
override predicate isSanitizerOut(DataFlow::Node node) { sanitizingPrefixEdge(node, _) }
3735

3836
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
3937
isAdditionalRequestForgeryStep(pred, succ)

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,7 @@ class Configuration extends TaintTracking::Configuration {
3333
node instanceof Sanitizer
3434
}
3535

36-
override predicate isSanitizerEdge(DataFlow::Node source, DataFlow::Node sink) {
37-
hostnameSanitizingPrefixEdge(source, sink)
38-
}
36+
override predicate isSanitizerOut(DataFlow::Node node) { hostnameSanitizingPrefixEdge(node, _) }
3937

4038
override predicate isAdditionalFlowStep(
4139
DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel f, DataFlow::FlowLabel g

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

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -290,9 +290,13 @@ module DomBasedXss {
290290
private class HtmlSanitizerAsSanitizer extends Sanitizer instanceof HtmlSanitizerCall { }
291291

292292
/**
293+
* DEPRECATED. Use `isOptionallySanitizedNode` instead.
294+
*
293295
* Holds if there exists two dataflow edges to `succ`, where one edges is sanitized, and the other edge starts with `pred`.
294296
*/
295-
predicate isOptionallySanitizedEdge(DataFlow::Node pred, DataFlow::Node succ) {
297+
deprecated predicate isOptionallySanitizedEdge = isOptionallySanitizedEdgeInternal/2;
298+
299+
private predicate isOptionallySanitizedEdgeInternal(DataFlow::Node pred, DataFlow::Node succ) {
296300
exists(HtmlSanitizerCall sanitizer |
297301
// sanitized = sanitize ? sanitizer(source) : source;
298302
exists(ConditionalExpr branch, Variable var, VarAccess access |
@@ -319,6 +323,17 @@ module DomBasedXss {
319323
)
320324
}
321325

326+
/**
327+
* Holds if `node` should be considered optionally sanitized as it occurs in a branch
328+
* that controls whether sanitization is enabled.
329+
*
330+
* For example, in `sanitized = sanitize ? sanitizer(source) : source`, the right-hand `source` expression
331+
* is considered an optionally sanitized node.
332+
*/
333+
predicate isOptionallySanitizedNode(DataFlow::Node node) {
334+
isOptionallySanitizedEdgeInternal(_, node)
335+
}
336+
322337
/** A source of remote user input, considered as a flow source for DOM-based XSS. */
323338
class RemoteFlowSourceAsSource extends Source instanceof RemoteFlowSource { }
324339

0 commit comments

Comments
 (0)