Skip to content

Commit 706d1d2

Browse files
committed
Ruby: Make StringArrayInclusion more sensitive
We now recognise the following pattern as a barrier guard for `x`: values = ["foo", "bar"] if values.include? x sink x end
1 parent 2aaedac commit 706d1d2

File tree

4 files changed

+103
-6
lines changed

4 files changed

+103
-6
lines changed

ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,9 @@ module ExprNodes {
374374
MethodCallCfgNode() { super.getExpr() instanceof MethodCall }
375375

376376
override MethodCall getExpr() { result = super.getExpr() }
377+
378+
/** Gets the name of this method call. */
379+
string getMethodName() { result = this.getExpr().getMethodName() }
377380
}
378381

379382
private class CaseExprChildMapping extends ExprChildMapping, CaseExpr {

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

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
private import ruby
44
private import codeql.ruby.DataFlow
55
private import codeql.ruby.CFG
6+
private import codeql.ruby.controlflow.CfgNodes
7+
private import codeql.ruby.dataflow.SSA
68

79
private predicate stringConstCompare(CfgNodes::ExprCfgNode g, CfgNode e, boolean branch) {
810
exists(CfgNodes::ExprNodes::ComparisonOperationCfgNode c |
@@ -133,13 +135,24 @@ deprecated class StringConstArrayInclusionCall extends DataFlow::BarrierGuard,
133135
private CfgNode checkedNode;
134136

135137
StringConstArrayInclusionCall() {
136-
exists(ArrayLiteral aLit |
137-
this.getExpr().getMethodName() = "include?" and
138-
[this.getExpr().getReceiver(), this.getExpr().getReceiver().(ConstantReadAccess).getValue()] =
139-
aLit
138+
this.getMethodName() = "include?" and
139+
this.getArgument(0) = checkedNode and
140+
exists(ExprNodes::ArrayLiteralCfgNode arr |
141+
// [...].include?
142+
this.getReceiver() = arr
143+
or
144+
// C = [...]
145+
// C.include?
146+
this.getReceiver().(ExprNodes::ConstantReadAccessCfgNode).getExpr().getValue().getDesugared() =
147+
arr.getExpr()
148+
or
149+
// x = [...]
150+
// x.include?
151+
exists(Ssa::WriteDefinition def | def.getARead() = this.getReceiver() and def.assigns(arr))
140152
|
141-
forall(Expr elem | elem = aLit.getAnElement() | elem instanceof StringLiteral) and
142-
this.getArgument(0) = checkedNode
153+
forall(ExprCfgNode elem | elem = arr.getAnArgument() |
154+
elem instanceof ExprNodes::StringLiteralCfgNode
155+
)
143156
)
144157
}
145158

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
| barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | barrier-guards.rb:3:4:3:6 | foo | true |
2+
| barrier-guards.rb:9:4:9:24 | call to include? | barrier-guards.rb:10:5:10:7 | foo | barrier-guards.rb:9:21:9:23 | foo | true |
3+
| barrier-guards.rb:15:4:15:15 | ... != ... | barrier-guards.rb:18:5:18:7 | foo | barrier-guards.rb:15:4:15:6 | foo | false |
4+
| barrier-guards.rb:21:8:21:19 | ... == ... | barrier-guards.rb:24:5:24:7 | foo | barrier-guards.rb:21:8:21:10 | foo | true |
5+
| barrier-guards.rb:27:8:27:19 | ... != ... | barrier-guards.rb:28:5:28:7 | foo | barrier-guards.rb:27:8:27:10 | foo | false |
6+
| barrier-guards.rb:37:4:37:20 | call to include? | barrier-guards.rb:38:5:38:7 | foo | barrier-guards.rb:37:17:37:19 | foo | true |
7+
| barrier-guards.rb:43:4:43:15 | ... == ... | barrier-guards.rb:45:9:45:11 | foo | barrier-guards.rb:43:4:43:6 | foo | true |
8+
| barrier-guards.rb:69:4:69:21 | call to include? | barrier-guards.rb:70:5:70:7 | foo | barrier-guards.rb:69:18:69:20 | foo | true |
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
foo = "foo"
2+
3+
if foo == "foo"
4+
foo
5+
else
6+
foo
7+
end
8+
9+
if ["foo"].include?(foo)
10+
foo
11+
else
12+
foo
13+
end
14+
15+
if foo != "foo"
16+
foo
17+
else
18+
foo
19+
end
20+
21+
unless foo == "foo"
22+
foo
23+
else
24+
foo
25+
end
26+
27+
unless foo != "foo"
28+
foo
29+
else
30+
foo
31+
end
32+
33+
foo
34+
35+
FOO = ["foo"]
36+
37+
if FOO.include?(foo)
38+
foo
39+
else
40+
foo
41+
end
42+
43+
if foo == "foo"
44+
capture {
45+
foo # guarded
46+
}
47+
end
48+
49+
if foo == "foo"
50+
capture {
51+
foo = "bar"
52+
foo # not guarded
53+
}
54+
end
55+
56+
if foo == "foo"
57+
my_lambda = -> () {
58+
foo # not guarded
59+
}
60+
61+
foo = "bar"
62+
63+
my_lambda()
64+
end
65+
66+
foos = ["foo"]
67+
bars = ["bar"]
68+
69+
if foos.include?(foo)
70+
foo
71+
else
72+
foo
73+
end

0 commit comments

Comments
 (0)