Skip to content

Commit 1b13790

Browse files
aschackmullaschackmull
committed
Ruby: Deprecate and replace BarrierGuard class.
1 parent f473a0a commit 1b13790

21 files changed

Lines changed: 235 additions & 73 deletions

‎ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll‎

Lines changed: 74 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,23 @@ private import ruby
44
private import codeql.ruby.DataFlow
55
private import codeql.ruby.CFG
66

7+
private predicate stringConstCompare(CfgNodes::ExprCfgNode g, CfgNode e, boolean branch) {
8+
exists(CfgNodes::ExprNodes::ComparisonOperationCfgNode c |
9+
c = g and
10+
exists(CfgNodes::ExprNodes::StringLiteralCfgNode strLitNode |
11+
c.getExpr() instanceof EqExpr and branch = true
12+
or
13+
c.getExpr() instanceof CaseEqExpr and branch = true
14+
or
15+
c.getExpr() instanceof NEExpr and branch = false
16+
|
17+
c.getLeftOperand() = strLitNode and c.getRightOperand() = e
18+
or
19+
c.getLeftOperand() = e and c.getRightOperand() = strLitNode
20+
)
21+
)
22+
}
23+
724
/**
825
* A validation of value by comparing with a constant string value, for example
926
* in:
@@ -17,7 +34,28 @@ private import codeql.ruby.CFG
1734
* the equality operation guards against `dir` taking arbitrary values when used
1835
* in the `order` call.
1936
*/
20-
class StringConstCompare extends DataFlow::BarrierGuard,
37+
class StringConstCompareBarrier extends DataFlow::Node {
38+
StringConstCompareBarrier() {
39+
this = DataFlow::BarrierGuard<stringConstCompare/3>::getABarrierNode()
40+
}
41+
}
42+
43+
/**
44+
* DEPRECATED: Use `StringConstCompareBarrier` instead.
45+
*
46+
* A validation of value by comparing with a constant string value, for example
47+
* in:
48+
*
49+
* ```rb
50+
* dir = params[:order]
51+
* dir = "DESC" unless dir == "ASC"
52+
* User.order("name #{dir}")
53+
* ```
54+
*
55+
* the equality operation guards against `dir` taking arbitrary values when used
56+
* in the `order` call.
57+
*/
58+
deprecated class StringConstCompare extends DataFlow::BarrierGuard,
2159
CfgNodes::ExprNodes::ComparisonOperationCfgNode {
2260
private CfgNode checkedNode;
2361
// The value of the condition that results in the node being validated.
@@ -42,7 +80,41 @@ class StringConstCompare extends DataFlow::BarrierGuard,
4280
}
4381
}
4482

83+
private predicate stringConstArrayInclusionCall(CfgNodes::ExprCfgNode g, CfgNode e, boolean branch) {
84+
exists(CfgNodes::ExprNodes::MethodCallCfgNode mc, ArrayLiteral aLit |
85+
mc = g and
86+
mc.getExpr().getMethodName() = "include?" and
87+
[mc.getExpr().getReceiver(), mc.getExpr().getReceiver().(ConstantReadAccess).getValue()] = aLit
88+
|
89+
forall(Expr elem | elem = aLit.getAnElement() | elem instanceof StringLiteral) and
90+
mc.getArgument(0) = e
91+
) and
92+
branch = true
93+
}
94+
95+
/**
96+
* A validation of a value by checking for inclusion in an array of string
97+
* literal values, for example in:
98+
*
99+
* ```rb
100+
* name = params[:user_name]
101+
* if %w(alice bob charlie).include? name
102+
* User.find_by("username = #{name}")
103+
* end
104+
* ```
105+
*
106+
* the `include?` call guards against `name` taking arbitrary values when used
107+
* in the `find_by` call.
108+
*/
109+
class StringConstArrayInclusionCallBarrier extends DataFlow::Node {
110+
StringConstArrayInclusionCallBarrier() {
111+
this = DataFlow::BarrierGuard<stringConstArrayInclusionCall/3>::getABarrierNode()
112+
}
113+
}
114+
45115
/**
116+
* DEPRECATED: Use `StringConstArrayInclusionCallBarrier` instead.
117+
*
46118
* A validation of a value by checking for inclusion in an array of string
47119
* literal values, for example in:
48120
*
@@ -56,8 +128,7 @@ class StringConstCompare extends DataFlow::BarrierGuard,
56128
* the `include?` call guards against `name` taking arbitrary values when used
57129
* in the `find_by` call.
58130
*/
59-
//
60-
class StringConstArrayInclusionCall extends DataFlow::BarrierGuard,
131+
deprecated class StringConstArrayInclusionCall extends DataFlow::BarrierGuard,
61132
CfgNodes::ExprNodes::MethodCallCfgNode {
62133
private CfgNode checkedNode;
63134

‎ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll‎

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,66 @@ class ContentSet extends TContentSet {
334334
}
335335
}
336336

337+
/**
338+
* Holds if the guard `g` validates the expression `e` upon evaluating to `branch`.
339+
*
340+
* The expression `e` is expected to be a syntactic part of the guard `g`.
341+
* For example, the guard `g` might be a call `isSafe(x)` and the expression `e`
342+
* the argument `x`.
343+
*/
344+
signature predicate guardChecksSig(CfgNodes::ExprCfgNode g, CfgNode e, boolean branch);
345+
346+
/**
347+
* Provides a set of barrier nodes for a guard that validates an expression.
348+
*
349+
* This is expected to be used in `isBarrier`/`isSanitizer` definitions
350+
* in data flow and taint tracking.
351+
*/
352+
module BarrierGuard<guardChecksSig/3 guardChecks> {
353+
/** Gets a node that is safely guarded by the given guard check. */
354+
Node getABarrierNode() {
355+
exists(
356+
CfgNodes::ExprCfgNode g, boolean branch, CfgNodes::ExprCfgNode testedNode, Ssa::Definition def
357+
|
358+
def.getARead() = testedNode and
359+
def.getARead() = result.asExpr() and
360+
guardChecks(g, testedNode, branch) and
361+
guardControlsBlock(g, result.asExpr().getBasicBlock(), branch)
362+
)
363+
or
364+
result.asExpr() = getAMaybeGuardedCapturedDef().getARead()
365+
}
366+
367+
/**
368+
* Gets an implicit entry definition for a captured variable that
369+
* may be guarded, because a call to the capturing callable is guarded.
370+
*
371+
* This is restricted to calls where the variable is captured inside a
372+
* block.
373+
*/
374+
private Ssa::Definition getAMaybeGuardedCapturedDef() {
375+
exists(
376+
CfgNodes::ExprCfgNode g, boolean branch, CfgNodes::ExprCfgNode testedNode,
377+
Ssa::Definition def, CfgNodes::ExprNodes::CallCfgNode call
378+
|
379+
def.getARead() = testedNode and
380+
guardChecks(g, testedNode, branch) and
381+
SsaImpl::captureFlowIn(call, def, result) and
382+
guardControlsBlock(g, call.getBasicBlock(), branch) and
383+
result.getBasicBlock().getScope() = call.getExpr().(MethodCall).getBlock()
384+
)
385+
}
386+
}
387+
388+
/** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */
389+
private predicate guardControlsBlock(CfgNodes::ExprCfgNode guard, BasicBlock bb, boolean branch) {
390+
exists(ConditionBlock conditionBlock, SuccessorTypes::BooleanSuccessor s |
391+
guard = conditionBlock.getLastNode() and
392+
s.getValue() = branch and
393+
conditionBlock.controls(bb, s)
394+
)
395+
}
396+
337397
/**
338398
* A guard that validates some expression.
339399
*
@@ -343,7 +403,7 @@ class ContentSet extends TContentSet {
343403
*
344404
* It is important that all extending classes in scope are disjoint.
345405
*/
346-
abstract class BarrierGuard extends CfgNodes::ExprCfgNode {
406+
abstract deprecated class BarrierGuard extends CfgNodes::ExprCfgNode {
347407
private ConditionBlock conditionBlock;
348408

349409
BarrierGuard() { this = conditionBlock.getLastNode() }

‎ruby/ql/lib/codeql/ruby/dataflow/internal/TaintTrackingPrivate.qll‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,6 @@ private import FlowSummaryImpl as FlowSummaryImpl
1111
*/
1212
predicate defaultTaintSanitizer(DataFlow::Node node) { none() }
1313

14-
/**
15-
* Holds if `guard` should be a sanitizer guard in all global taint flow configurations
16-
* but not in local taint.
17-
*/
18-
predicate defaultTaintSanitizerGuard(DataFlow::BarrierGuard guard) { none() }
19-
2014
/**
2115
* Holds if default `TaintTracking::Configuration`s should allow implicit reads
2216
* of `c` at sinks and inputs to additional taint steps.

‎ruby/ql/lib/codeql/ruby/security/CodeInjectionCustomizations.qll‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,16 @@ module CodeInjection {
2222
abstract class Sink extends DataFlow::Node { }
2323

2424
/**
25+
* A sanitizer for "Code injection" vulnerabilities.
26+
*/
27+
abstract class Sanitizer extends DataFlow::Node { }
28+
29+
/**
30+
* DEPRECATED: Use `Sanitizer` instead.
31+
*
2532
* A sanitizer guard for "Code injection" vulnerabilities.
2633
*/
27-
abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
34+
abstract deprecated class SanitizerGuard extends DataFlow::BarrierGuard { }
2835

2936
/**
3037
* A source of remote user input, considered as a flow source.

‎ruby/ql/lib/codeql/ruby/security/CodeInjectionQuery.qll‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,13 @@ class Configuration extends TaintTracking::Configuration {
2020

2121
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
2222

23-
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
24-
guard instanceof SanitizerGuard or
25-
guard instanceof StringConstCompare or
26-
guard instanceof StringConstArrayInclusionCall
23+
override predicate isSanitizer(DataFlow::Node node) {
24+
node instanceof Sanitizer or
25+
node instanceof StringConstCompareBarrier or
26+
node instanceof StringConstArrayInclusionCallBarrier
27+
}
28+
29+
deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
30+
guard instanceof SanitizerGuard
2731
}
2832
}

‎ruby/ql/lib/codeql/ruby/security/CommandInjectionQuery.qll‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,9 @@ class Configuration extends TaintTracking::Configuration {
2323

2424
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
2525

26-
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
27-
28-
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
29-
guard instanceof StringConstCompare or
30-
guard instanceof StringConstArrayInclusionCall
26+
override predicate isSanitizer(DataFlow::Node node) {
27+
node instanceof Sanitizer or
28+
node instanceof StringConstCompareBarrier or
29+
node instanceof StringConstArrayInclusionCallBarrier
3130
}
3231
}

‎ruby/ql/lib/codeql/ruby/security/PathInjectionCustomizations.qll‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,16 @@ module PathInjection {
2424
abstract class Sink extends DataFlow::Node { }
2525

2626
/**
27+
* A sanitizer for path injection vulnerabilities.
28+
*/
29+
abstract class Sanitizer extends DataFlow::Node { }
30+
31+
/**
32+
* DEPRECATED: Use `Sanitizer` instead.
33+
*
2734
* A sanitizer guard for path injection vulnerabilities.
2835
*/
29-
abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
36+
abstract deprecated class SanitizerGuard extends DataFlow::BarrierGuard { }
3037

3138
/**
3239
* A source of remote user input, considered as a flow source.
@@ -43,12 +50,12 @@ module PathInjection {
4350
/**
4451
* A comparison with a constant string, considered as a sanitizer-guard.
4552
*/
46-
class StringConstCompareAsSanitizerGuard extends SanitizerGuard, StringConstCompare { }
53+
class StringConstCompareAsSanitizer extends Sanitizer, StringConstCompareBarrier { }
4754

4855
/**
4956
* An inclusion check against an array of constant strings, considered as a
5057
* sanitizer-guard.
5158
*/
52-
class StringConstArrayInclusionCallAsSanitizerGuard extends SanitizerGuard,
53-
StringConstArrayInclusionCall { }
59+
class StringConstArrayInclusionCallAsSanitizer extends Sanitizer,
60+
StringConstArrayInclusionCallBarrier { }
5461
}

‎ruby/ql/lib/codeql/ruby/security/PathInjectionQuery.qll‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,11 @@ class Configuration extends TaintTracking::Configuration {
2323

2424
override predicate isSink(DataFlow::Node sink) { sink instanceof PathInjection::Sink }
2525

26-
override predicate isSanitizer(DataFlow::Node node) { node instanceof Path::PathSanitization }
26+
override predicate isSanitizer(DataFlow::Node node) {
27+
node instanceof Path::PathSanitization or node instanceof PathInjection::Sanitizer
28+
}
2729

28-
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
30+
deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
2931
guard instanceof PathInjection::SanitizerGuard
3032
}
3133
}

‎ruby/ql/lib/codeql/ruby/security/ReflectedXSSQuery.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ module ReflectedXss {
2828

2929
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
3030

31-
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
31+
deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
3232
guard instanceof SanitizerGuard
3333
}
3434

‎ruby/ql/lib/codeql/ruby/security/ServerSideRequestForgeryCustomizations.qll‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,11 @@ module ServerSideRequestForgery {
3232
abstract class Sanitizer extends DataFlow::Node { }
3333

3434
/**
35+
* DEPRECATED: Use `Sanitizer` instead.
36+
*
3537
* A sanitizer guard for "URL redirection" vulnerabilities.
3638
*/
37-
abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
39+
abstract deprecated class SanitizerGuard extends DataFlow::BarrierGuard { }
3840

3941
/** A source of remote user input, considered as a flow source for server side request forgery. */
4042
class RemoteFlowSourceAsSource extends Source {

0 commit comments

Comments
 (0)