Skip to content

Commit 21494fb

Browse files
asgerfasgerf
committed
JS: Refactor BarrierGuardLegacy pattern to not depend on SanitizerGuardNode
Previously our barrier guard classes were direct descendents of SanitizerGuardNode which made it hard to deprecate that class. Now our barrier guards are not descending from any shared class. Instead they are contributed to SanitizerGuardNode via a private helper class we can remove in the future.
1 parent a574ff1 commit 21494fb

17 files changed

+184
-71
lines changed

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

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,14 +31,25 @@ module DomBasedXss {
3131
* Holds if this node acts as a barrier for `label`, blocking further flow from `e` if `this` evaluates to `outcome`.
3232
*/
3333
predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) { none() }
34+
35+
/** DEPRECATED. Use `blocksExpr` instead. */
36+
deprecated predicate sanitizes(boolean outcome, Expr e) { this.blocksExpr(outcome, e) }
37+
38+
/** DEPRECATED. Use `blocksExpr` instead. */
39+
deprecated predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
40+
this.blocksExpr(outcome, e, label)
41+
}
3442
}
3543

3644
/** A subclass of `BarrierGuard` that is used for backward compatibility with the old data flow library. */
37-
abstract class BarrierGuardLegacy extends BarrierGuard, TaintTracking::SanitizerGuardNode {
38-
override predicate sanitizes(boolean outcome, Expr e) { this.blocksExpr(outcome, e) }
45+
deprecated final private class BarrierGuardLegacy extends TaintTracking::SanitizerGuardNode instanceof BarrierGuard
46+
{
47+
override predicate sanitizes(boolean outcome, Expr e) {
48+
BarrierGuard.super.sanitizes(outcome, e)
49+
}
3950

4051
override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
41-
this.blocksExpr(outcome, e, label)
52+
BarrierGuard.super.sanitizes(outcome, e, label)
4253
}
4354
}
4455

@@ -378,7 +389,7 @@ module DomBasedXss {
378389
/**
379390
* A sanitizer that blocks the `PrefixString` label when the start of the string is being tested as being of a particular prefix.
380391
*/
381-
abstract class PrefixStringSanitizer extends BarrierGuardLegacy instanceof StringOps::StartsWith {
392+
abstract class PrefixStringSanitizer extends BarrierGuard instanceof StringOps::StartsWith {
382393
override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
383394
e = super.getBaseString().asExpr() and
384395
label = prefixLabel() and

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

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -179,14 +179,25 @@ module LoopBoundInjection {
179179
* Holds if this node acts as a barrier for `label`, blocking further flow from `e` if `this` evaluates to `outcome`.
180180
*/
181181
predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) { none() }
182+
183+
/** DEPRECATED. Use `blocksExpr` instead. */
184+
deprecated predicate sanitizes(boolean outcome, Expr e) { this.blocksExpr(outcome, e) }
185+
186+
/** DEPRECATED. Use `blocksExpr` instead. */
187+
deprecated predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
188+
this.blocksExpr(outcome, e, label)
189+
}
182190
}
183191

184192
/** A subclass of `BarrierGuard` that is used for backward compatibility with the old data flow library. */
185-
abstract class BarrierGuardLegacy extends BarrierGuard, TaintTracking::SanitizerGuardNode {
186-
override predicate sanitizes(boolean outcome, Expr e) { this.blocksExpr(outcome, e) }
193+
deprecated final private class BarrierGuardLegacy extends TaintTracking::SanitizerGuardNode instanceof BarrierGuard
194+
{
195+
override predicate sanitizes(boolean outcome, Expr e) {
196+
BarrierGuard.super.sanitizes(outcome, e)
197+
}
187198

188199
override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
189-
this.blocksExpr(outcome, e, label)
200+
BarrierGuard.super.sanitizes(outcome, e, label)
190201
}
191202
}
192203

@@ -198,7 +209,7 @@ module LoopBoundInjection {
198209
/**
199210
* A sanitizer that blocks taint flow if the array is checked to be an array using an `isArray` function.
200211
*/
201-
class IsArraySanitizerGuard extends BarrierGuardLegacy, DataFlow::ValueNode {
212+
class IsArraySanitizerGuard extends BarrierGuard, DataFlow::ValueNode {
202213
override CallExpr astNode;
203214

204215
IsArraySanitizerGuard() { astNode.getCalleeName() = "isArray" }
@@ -213,7 +224,7 @@ module LoopBoundInjection {
213224
/**
214225
* A sanitizer that blocks taint flow if the array is checked to be an array using an `X instanceof Array` check.
215226
*/
216-
class InstanceofArraySanitizerGuard extends BarrierGuardLegacy, DataFlow::ValueNode {
227+
class InstanceofArraySanitizerGuard extends BarrierGuard, DataFlow::ValueNode {
217228
override BinaryExpr astNode;
218229

219230
InstanceofArraySanitizerGuard() {
@@ -233,7 +244,7 @@ module LoopBoundInjection {
233244
*
234245
* Also implicitly makes sure that only the first DoS-prone loop is selected by the query (as the .length test has outcome=false when exiting the loop).
235246
*/
236-
class LengthCheckSanitizerGuard extends BarrierGuardLegacy, DataFlow::ValueNode {
247+
class LengthCheckSanitizerGuard extends BarrierGuard, DataFlow::ValueNode {
237248
override RelationalComparison astNode;
238249
DataFlow::PropRead propRead;
239250

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

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,14 +51,25 @@ module PrototypePollutingAssignment {
5151
* Holds if this node acts as a barrier for `label`, blocking further flow from `e` if `this` evaluates to `outcome`.
5252
*/
5353
predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) { none() }
54+
55+
/** DEPRECATED. Use `blocksExpr` instead. */
56+
deprecated predicate sanitizes(boolean outcome, Expr e) { this.blocksExpr(outcome, e) }
57+
58+
/** DEPRECATED. Use `blocksExpr` instead. */
59+
deprecated predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
60+
this.blocksExpr(outcome, e, label)
61+
}
5462
}
5563

5664
/** A subclass of `BarrierGuard` that is used for backward compatibility with the old data flow library. */
57-
abstract class BarrierGuardLegacy extends BarrierGuard, TaintTracking::SanitizerGuardNode {
58-
override predicate sanitizes(boolean outcome, Expr e) { this.blocksExpr(outcome, e) }
65+
deprecated final private class BarrierGuardLegacy extends TaintTracking::SanitizerGuardNode instanceof BarrierGuard
66+
{
67+
override predicate sanitizes(boolean outcome, Expr e) {
68+
BarrierGuard.super.sanitizes(outcome, e)
69+
}
5970

6071
override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
61-
this.blocksExpr(outcome, e, label)
72+
BarrierGuard.super.sanitizes(outcome, e, label)
6273
}
6374
}
6475

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

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,7 @@ private predicate isPropertyPresentOnObjectPrototype(string prop) {
256256
}
257257

258258
/** A check of form `e.prop` where `prop` is not present on `Object.prototype`. */
259-
private class PropertyPresenceCheck extends BarrierGuardLegacy, DataFlow::ValueNode {
259+
private class PropertyPresenceCheck extends BarrierGuard, DataFlow::ValueNode {
260260
override PropAccess astNode;
261261

262262
PropertyPresenceCheck() {
@@ -272,7 +272,7 @@ private class PropertyPresenceCheck extends BarrierGuardLegacy, DataFlow::ValueN
272272
}
273273

274274
/** A check of form `"prop" in e` where `prop` is not present on `Object.prototype`. */
275-
private class InExprCheck extends BarrierGuardLegacy, DataFlow::ValueNode {
275+
private class InExprCheck extends BarrierGuard, DataFlow::ValueNode {
276276
override InExpr astNode;
277277

278278
InExprCheck() {
@@ -287,7 +287,7 @@ private class InExprCheck extends BarrierGuardLegacy, DataFlow::ValueNode {
287287
}
288288

289289
/** A check of form `e instanceof X`, which is always false for `Object.prototype`. */
290-
private class InstanceofCheck extends BarrierGuardLegacy, DataFlow::ValueNode {
290+
private class InstanceofCheck extends BarrierGuard, DataFlow::ValueNode {
291291
override InstanceofExpr astNode;
292292

293293
override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
@@ -298,7 +298,7 @@ private class InstanceofCheck extends BarrierGuardLegacy, DataFlow::ValueNode {
298298
}
299299

300300
/** A check of form `typeof e === "string"`. */
301-
private class TypeofCheck extends BarrierGuardLegacy, DataFlow::ValueNode {
301+
private class TypeofCheck extends BarrierGuard, DataFlow::ValueNode {
302302
override EqualityTest astNode;
303303
Expr operand;
304304
boolean polarity;
@@ -319,7 +319,7 @@ private class TypeofCheck extends BarrierGuardLegacy, DataFlow::ValueNode {
319319
}
320320

321321
/** A guard that checks whether `x` is a number. */
322-
class NumberGuard extends BarrierGuardLegacy instanceof DataFlow::CallNode {
322+
class NumberGuard extends BarrierGuard instanceof DataFlow::CallNode {
323323
Expr x;
324324
boolean polarity;
325325

@@ -329,7 +329,7 @@ class NumberGuard extends BarrierGuardLegacy instanceof DataFlow::CallNode {
329329
}
330330

331331
/** A call to `Array.isArray`, which is false for `Object.prototype`. */
332-
private class IsArrayCheck extends BarrierGuardLegacy, DataFlow::CallNode {
332+
private class IsArrayCheck extends BarrierGuard, DataFlow::CallNode {
333333
IsArrayCheck() { this = DataFlow::globalVarRef("Array").getAMemberCall("isArray") }
334334

335335
override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
@@ -342,7 +342,7 @@ private class IsArrayCheck extends BarrierGuardLegacy, DataFlow::CallNode {
342342
/**
343343
* Sanitizer guard of form `x !== "__proto__"`.
344344
*/
345-
private class EqualityCheck extends BarrierGuardLegacy, DataFlow::ValueNode {
345+
private class EqualityCheck extends BarrierGuard, DataFlow::ValueNode {
346346
override EqualityTest astNode;
347347

348348
EqualityCheck() { astNode.getAnOperand().getStringValue() = "__proto__" }
@@ -356,7 +356,7 @@ private class EqualityCheck extends BarrierGuardLegacy, DataFlow::ValueNode {
356356
/**
357357
* Sanitizer guard of the form `x.includes("__proto__")`.
358358
*/
359-
private class IncludesCheck extends BarrierGuardLegacy, InclusionTest {
359+
private class IncludesCheck extends BarrierGuard, InclusionTest {
360360
IncludesCheck() { this.getContainedNode().mayHaveStringValue("__proto__") }
361361

362362
override predicate blocksExpr(boolean outcome, Expr e) {
@@ -368,7 +368,7 @@ private class IncludesCheck extends BarrierGuardLegacy, InclusionTest {
368368
/**
369369
* A sanitizer guard that checks tests whether `x` is included in a list like `["__proto__"].includes(x)`.
370370
*/
371-
private class DenyListInclusionGuard extends BarrierGuardLegacy, InclusionTest {
371+
private class DenyListInclusionGuard extends BarrierGuard, InclusionTest {
372372
DenyListInclusionGuard() {
373373
this.getContainerNode()
374374
.getALocalSource()

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,11 +39,17 @@ module ResourceExhaustion {
3939
* Holds if this node acts as a barrier for data flow, blocking further flow from `e` if `this` evaluates to `outcome`.
4040
*/
4141
predicate blocksExpr(boolean outcome, Expr e) { none() }
42+
43+
/** DEPRECATED. Use `blocksExpr` instead. */
44+
deprecated predicate sanitizes(boolean outcome, Expr e) { this.blocksExpr(outcome, e) }
4245
}
4346

4447
/** A subclass of `BarrierGuard` that is used for backward compatibility with the old data flow library. */
45-
abstract class BarrierGuardLegacy extends BarrierGuard, TaintTracking::SanitizerGuardNode {
46-
override predicate sanitizes(boolean outcome, Expr e) { this.blocksExpr(outcome, e) }
48+
deprecated final private class BarrierGuardLegacy extends TaintTracking::SanitizerGuardNode instanceof BarrierGuard
49+
{
50+
override predicate sanitizes(boolean outcome, Expr e) {
51+
BarrierGuard.super.sanitizes(outcome, e)
52+
}
4753
}
4854

4955
/**

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ predicate isNumericFlowStep(DataFlow::Node src, DataFlow::Node dst) {
7373
/**
7474
* A sanitizer that blocks taint flow if the size of a number is limited.
7575
*/
76-
class UpperBoundsCheckSanitizerGuard extends BarrierGuardLegacy, DataFlow::ValueNode {
76+
class UpperBoundsCheckSanitizerGuard extends BarrierGuard, DataFlow::ValueNode {
7777
override RelationalComparison astNode;
7878

7979
override predicate blocksExpr(boolean outcome, Expr e) {

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

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -96,14 +96,25 @@ module SecondOrderCommandInjection {
9696
* Holds if this node acts as a barrier for `label`, blocking further flow from `e` if `this` evaluates to `outcome`.
9797
*/
9898
predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) { none() }
99+
100+
/** DEPRECATED. Use `blocksExpr` instead. */
101+
deprecated predicate sanitizes(boolean outcome, Expr e) { this.blocksExpr(outcome, e) }
102+
103+
/** DEPRECATED. Use `blocksExpr` instead. */
104+
deprecated predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
105+
this.blocksExpr(outcome, e, label)
106+
}
99107
}
100108

101109
/** A subclass of `BarrierGuard` that is used for backward compatibility with the old data flow library. */
102-
abstract class BarrierGuardLegacy extends BarrierGuard, TaintTracking::SanitizerGuardNode {
103-
override predicate sanitizes(boolean outcome, Expr e) { this.blocksExpr(outcome, e) }
110+
deprecated final private class BarrierGuardLegacy extends TaintTracking::SanitizerGuardNode instanceof BarrierGuard
111+
{
112+
override predicate sanitizes(boolean outcome, Expr e) {
113+
BarrierGuard.super.sanitizes(outcome, e)
114+
}
104115

105116
override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
106-
this.blocksExpr(outcome, e, label)
117+
BarrierGuard.super.sanitizes(outcome, e, label)
107118
}
108119
}
109120

@@ -214,7 +225,7 @@ module SecondOrderCommandInjection {
214225
/**
215226
* A sanitizer that blocks flow when a string is tested to start with a certain prefix.
216227
*/
217-
class PrefixStringSanitizer extends BarrierGuardLegacy instanceof StringOps::StartsWith {
228+
class PrefixStringSanitizer extends BarrierGuard instanceof StringOps::StartsWith {
218229
override predicate blocksExpr(boolean outcome, Expr e) {
219230
e = super.getBaseString().asExpr() and
220231
outcome = super.getPolarity()
@@ -224,7 +235,7 @@ module SecondOrderCommandInjection {
224235
/**
225236
* A sanitizer that blocks flow when a string does not start with "--"
226237
*/
227-
class DoubleDashSanitizer extends BarrierGuardLegacy instanceof StringOps::StartsWith {
238+
class DoubleDashSanitizer extends BarrierGuard instanceof StringOps::StartsWith {
228239
DoubleDashSanitizer() { super.getSubstring().mayHaveStringValue("--") }
229240

230241
override predicate blocksExpr(boolean outcome, Expr e) {

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

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -41,14 +41,25 @@ module TaintedPath {
4141
* Holds if this node acts as a barrier for `label`, blocking further flow from `e` if `this` evaluates to `outcome`.
4242
*/
4343
predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) { none() }
44+
45+
/** DEPRECATED. Use `blocksExpr` instead. */
46+
deprecated predicate sanitizes(boolean outcome, Expr e) { this.blocksExpr(outcome, e) }
47+
48+
/** DEPRECATED. Use `blocksExpr` instead. */
49+
deprecated predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
50+
this.blocksExpr(outcome, e, label)
51+
}
4452
}
4553

4654
/** A subclass of `BarrierGuard` that is used for backward compatibility with the old data flow library. */
47-
abstract class BarrierGuardLegacy extends BarrierGuard, DataFlow::BarrierGuardNode {
48-
override predicate blocks(boolean outcome, Expr e) { this.blocksExpr(outcome, e) }
55+
deprecated final private class BarrierGuardLegacy extends TaintTracking::SanitizerGuardNode instanceof BarrierGuard
56+
{
57+
override predicate sanitizes(boolean outcome, Expr e) {
58+
BarrierGuard.super.sanitizes(outcome, e)
59+
}
4960

50-
override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
51-
this.blocksExpr(outcome, e, label)
61+
override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
62+
BarrierGuard.super.sanitizes(outcome, e, label)
5263
}
5364
}
5465

@@ -366,7 +377,7 @@ module TaintedPath {
366377
*
367378
* This is relevant for paths that are known to be normalized.
368379
*/
369-
class StartsWithDotDotSanitizer extends BarrierGuardLegacy instanceof StringOps::StartsWith {
380+
class StartsWithDotDotSanitizer extends BarrierGuard instanceof StringOps::StartsWith {
370381
StartsWithDotDotSanitizer() { isDotDotSlashPrefix(super.getSubstring()) }
371382

372383
override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
@@ -386,7 +397,7 @@ module TaintedPath {
386397
/**
387398
* A check of the form `whitelist.includes(x)` or equivalent, which sanitizes `x` in its "then" branch.
388399
*/
389-
class MembershipTestBarrierGuard extends BarrierGuardLegacy {
400+
class MembershipTestBarrierGuard extends BarrierGuard {
390401
MembershipCandidate candidate;
391402

392403
MembershipTestBarrierGuard() { this = candidate.getTest() }
@@ -401,7 +412,7 @@ module TaintedPath {
401412
* A check of form `x.startsWith(dir)` that sanitizes normalized absolute paths, since it is then
402413
* known to be in a subdirectory of `dir`.
403414
*/
404-
class StartsWithDirSanitizer extends BarrierGuardLegacy {
415+
class StartsWithDirSanitizer extends BarrierGuard {
405416
StringOps::StartsWith startsWith;
406417

407418
StartsWithDirSanitizer() {
@@ -425,7 +436,7 @@ module TaintedPath {
425436
* A call to `path.isAbsolute` as a sanitizer for relative paths in true branch,
426437
* and a sanitizer for absolute paths in the false branch.
427438
*/
428-
class IsAbsoluteSanitizer extends BarrierGuardLegacy {
439+
class IsAbsoluteSanitizer extends BarrierGuard {
429440
DataFlow::Node operand;
430441
boolean polarity;
431442
boolean negatable;
@@ -461,7 +472,7 @@ module TaintedPath {
461472
/**
462473
* An expression of form `x.includes("..")` or similar.
463474
*/
464-
class ContainsDotDotSanitizer extends BarrierGuardLegacy instanceof StringOps::Includes {
475+
class ContainsDotDotSanitizer extends BarrierGuard instanceof StringOps::Includes {
465476
ContainsDotDotSanitizer() { isDotDotSlashPrefix(super.getSubstring()) }
466477

467478
override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
@@ -474,7 +485,7 @@ module TaintedPath {
474485
/**
475486
* An expression of form `x.matches(/\.\./)` or similar.
476487
*/
477-
class ContainsDotDotRegExpSanitizer extends BarrierGuardLegacy instanceof StringOps::RegExpTest {
488+
class ContainsDotDotRegExpSanitizer extends BarrierGuard instanceof StringOps::RegExpTest {
478489
ContainsDotDotRegExpSanitizer() { super.getRegExp().getAMatchedString() = [".", "..", "../"] }
479490

480491
override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
@@ -505,7 +516,7 @@ module TaintedPath {
505516
* }
506517
* ```
507518
*/
508-
class RelativePathStartsWithSanitizer extends BarrierGuardLegacy {
519+
class RelativePathStartsWithSanitizer extends BarrierGuard {
509520
StringOps::StartsWith startsWith;
510521
DataFlow::CallNode pathCall;
511522
string member;
@@ -563,7 +574,7 @@ module TaintedPath {
563574
* An expression of form `isInside(x, y)` or similar, where `isInside` is
564575
* a library check for the relation between `x` and `y`.
565576
*/
566-
class IsInsideCheckSanitizer extends BarrierGuardLegacy {
577+
class IsInsideCheckSanitizer extends BarrierGuard {
567578
DataFlow::Node checked;
568579
boolean onlyNormalizedAbsolutePaths;
569580

0 commit comments

Comments
 (0)