Skip to content

Commit 2d4cf55

Browse files
hvitvedhvitved
authored
Merge pull request github#15985 from hvitved/ruby/phi-barrier-guards
Ruby: Extend barrier guards to handle phi inputs
2 parents 9be2b9c + 90779f4 commit 2d4cf55

File tree

11 files changed

+1675
-453
lines changed

11 files changed

+1675
-453
lines changed

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

Lines changed: 85 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -72,47 +72,51 @@ CfgNodes::ExprCfgNode getAPostUpdateNodeForArg(Argument arg) {
7272
not exists(getALastEvalNode(result))
7373
}
7474

75-
/** Provides predicates related to local data flow. */
76-
module LocalFlow {
77-
private import codeql.ruby.dataflow.internal.SsaImpl
75+
/** An SSA definition into which another SSA definition may flow. */
76+
class SsaInputDefinitionExt extends SsaImpl::DefinitionExt {
77+
SsaInputDefinitionExt() {
78+
this instanceof Ssa::PhiNode
79+
or
80+
this instanceof SsaImpl::PhiReadNode
81+
}
7882

79-
/** An SSA definition into which another SSA definition may flow. */
80-
private class SsaInputDefinitionExtNode extends SsaDefinitionExtNode {
81-
SsaInputDefinitionExtNode() {
82-
def instanceof Ssa::PhiNode
83-
or
84-
def instanceof SsaImpl::PhiReadNode
85-
}
83+
predicate hasInputFromBlock(SsaImpl::DefinitionExt def, BasicBlock bb, int i, BasicBlock input) {
84+
SsaImpl::lastRefBeforeRedefExt(def, bb, i, input, this)
8685
}
86+
}
8787

88+
/** Provides predicates related to local data flow. */
89+
module LocalFlow {
8890
/**
8991
* Holds if `nodeFrom` is a node for SSA definition `def`, which can reach `next`.
9092
*/
9193
pragma[nomagic]
9294
private predicate localFlowSsaInputFromDef(
93-
SsaDefinitionExtNode nodeFrom, SsaImpl::DefinitionExt def, SsaInputDefinitionExtNode next
95+
SsaDefinitionExtNode nodeFrom, SsaImpl::DefinitionExt def, SsaInputNode nodeTo
9496
) {
95-
exists(BasicBlock bb, int i |
96-
lastRefBeforeRedefExt(def, bb, i, next.getDefinitionExt()) and
97+
exists(BasicBlock bb, int i, BasicBlock input, SsaInputDefinitionExt next |
98+
next.hasInputFromBlock(def, bb, i, input) and
9799
def = nodeFrom.getDefinitionExt() and
98100
def.definesAt(_, bb, i, _) and
99-
nodeFrom != next
101+
nodeTo = TSsaInputNode(next, input)
100102
)
101103
}
102104

103105
/**
104106
* Holds if `nodeFrom` is a last read of SSA definition `def`, which
105-
* can reach `next`.
107+
* can reach `nodeTo`.
106108
*/
107109
pragma[nomagic]
108-
predicate localFlowSsaInputFromRead(
109-
SsaImpl::DefinitionExt def, Node nodeFrom, SsaInputDefinitionExtNode next
110-
) {
111-
exists(BasicBlock bb, int i, CfgNodes::ExprCfgNode exprFrom |
112-
SsaImpl::lastRefBeforeRedefExt(def, bb, i, next.getDefinitionExt()) and
110+
predicate localFlowSsaInputFromRead(SsaImpl::DefinitionExt def, Node nodeFrom, SsaInputNode nodeTo) {
111+
exists(
112+
BasicBlock bb, int i, CfgNodes::ExprCfgNode exprFrom, BasicBlock input,
113+
SsaInputDefinitionExt next
114+
|
115+
next.hasInputFromBlock(def, bb, i, input) and
113116
exprFrom = bb.getNode(i) and
114117
exprFrom.getExpr() instanceof VariableReadAccess and
115-
exprFrom = [nodeFrom.asExpr(), nodeFrom.(PostUpdateNodeImpl).getPreUpdateNode().asExpr()]
118+
exprFrom = [nodeFrom.asExpr(), nodeFrom.(PostUpdateNodeImpl).getPreUpdateNode().asExpr()] and
119+
nodeTo = TSsaInputNode(next, input)
116120
)
117121
}
118122

@@ -181,14 +185,17 @@ module LocalFlow {
181185
or
182186
// Flow from SSA definition to first read
183187
def = nodeFrom.(SsaDefinitionExtNode).getDefinitionExt() and
184-
firstReadExt(def, nodeTo.asExpr())
188+
SsaImpl::firstReadExt(def, nodeTo.asExpr())
185189
or
186190
// Flow from post-update read to next read
187191
localSsaFlowStepUseUse(def, nodeFrom.(PostUpdateNodeImpl).getPreUpdateNode(), nodeTo)
188192
or
189193
// Flow into phi (read) SSA definition node from def
190194
localFlowSsaInputFromDef(nodeFrom, def, nodeTo)
191195
or
196+
nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = def and
197+
def = nodeFrom.(SsaInputNode).getDefinitionExt()
198+
or
192199
localFlowSsaParamInput(nodeFrom, nodeTo) and
193200
def = nodeTo.(SsaDefinitionExtNode).getDefinitionExt()
194201
}
@@ -530,6 +537,9 @@ private module Cached {
530537
TExprNode(CfgNodes::ExprCfgNode n) or
531538
TReturningNode(CfgNodes::ReturningCfgNode n) { exists(n.getReturnedValueNode()) } or
532539
TSsaDefinitionExtNode(SsaImpl::DefinitionExt def) or
540+
TSsaInputNode(SsaInputDefinitionExt def, BasicBlock input) {
541+
def.hasInputFromBlock(_, _, _, input)
542+
} or
533543
TCapturedVariableNode(VariableCapture::CapturedVariable v) or
534544
TNormalParameterNode(Parameter p) {
535545
p instanceof SimpleParameter or
@@ -802,6 +812,8 @@ import Cached
802812
predicate nodeIsHidden(Node n) {
803813
n.(SsaDefinitionExtNode).isHidden()
804814
or
815+
n instanceof SsaInputNode
816+
or
805817
n = LocalFlow::getParameterDefNode(_)
806818
or
807819
exists(AstNode desug |
@@ -863,6 +875,57 @@ class SsaDefinitionExtNode extends NodeImpl, TSsaDefinitionExtNode {
863875
override string toStringImpl() { result = def.toString() }
864876
}
865877

878+
/**
879+
* A node that represents an input to an SSA phi (read) definition.
880+
*
881+
* This allows for barrier guards to filter input to phi nodes. For example, in
882+
*
883+
* ```rb
884+
* x = taint
885+
* if x != "safe" then
886+
* x = "safe"
887+
* end
888+
* sink x
889+
* ```
890+
*
891+
* the `false` edge out of `x != "safe"` guards the input from `x = taint` into the
892+
* `phi` node after the condition.
893+
*
894+
* It is also relevant to filter input into phi read nodes:
895+
*
896+
* ```rb
897+
* x = taint
898+
* if b then
899+
* if x != "safe1" then
900+
* return
901+
* end
902+
* else
903+
* if x != "safe2" then
904+
* return
905+
* end
906+
* end
907+
*
908+
* sink x
909+
* ```
910+
*
911+
* both inputs into the phi read node after the outer condition are guarded.
912+
*/
913+
class SsaInputNode extends NodeImpl, TSsaInputNode {
914+
SsaImpl::DefinitionExt def;
915+
BasicBlock input;
916+
917+
SsaInputNode() { this = TSsaInputNode(def, input) }
918+
919+
/** Gets the underlying SSA definition. */
920+
SsaImpl::DefinitionExt getDefinitionExt() { result = def }
921+
922+
override CfgScope getCfgScope() { result = input.getScope() }
923+
924+
override Location getLocationImpl() { result = input.getLastNode().getLocation() }
925+
926+
override string toStringImpl() { result = "[input] " + def }
927+
}
928+
866929
/** An SSA definition for a `self` variable. */
867930
class SsaSelfDefinitionNode extends SsaDefinitionExtNode {
868931
private SelfVariable self;

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

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -856,24 +856,52 @@ private predicate sameSourceVariable(Ssa::Definition def1, Ssa::Definition def2)
856856
* in data flow and taint tracking.
857857
*/
858858
module BarrierGuard<guardChecksSig/3 guardChecks> {
859+
private import SsaImpl as SsaImpl
860+
859861
pragma[nomagic]
860862
private predicate guardChecksSsaDef(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def) {
861863
guardChecks(g, def.getARead(), branch)
862864
}
863865

864866
pragma[nomagic]
865-
private predicate guardControlsSsaDef(
867+
private predicate guardControlsSsaRead(
866868
CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def, Node n
867869
) {
868870
def.getARead() = n.asExpr() and
869871
guardControlsBlock(g, n.asExpr().getBasicBlock(), branch)
870872
}
871873

874+
pragma[nomagic]
875+
private predicate guardControlsPhiInput(
876+
CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def, BasicBlock input,
877+
SsaInputDefinitionExt phi
878+
) {
879+
phi.hasInputFromBlock(def, _, _, input) and
880+
(
881+
guardControlsBlock(g, input, branch)
882+
or
883+
exists(SuccessorTypes::ConditionalSuccessor s |
884+
g = input.getLastNode() and
885+
s.getValue() = branch and
886+
input.getASuccessor(s) = phi.getBasicBlock()
887+
)
888+
)
889+
}
890+
872891
/** Gets a node that is safely guarded by the given guard check. */
873892
Node getABarrierNode() {
874893
exists(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def |
875894
guardChecksSsaDef(g, branch, def) and
876-
guardControlsSsaDef(g, branch, def, result)
895+
guardControlsSsaRead(g, branch, def, result)
896+
)
897+
or
898+
exists(
899+
CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def, BasicBlock input,
900+
SsaInputDefinitionExt phi
901+
|
902+
guardChecksSsaDef(g, branch, def) and
903+
guardControlsPhiInput(g, branch, def, input, phi) and
904+
result = TSsaInputNode(phi, input)
877905
)
878906
or
879907
result.asExpr() = getAMaybeGuardedCapturedDef().getARead()

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -459,14 +459,16 @@ private module Cached {
459459
* The reference is either a read of `def` or `def` itself.
460460
*/
461461
cached
462-
predicate lastRefBeforeRedefExt(DefinitionExt def, Cfg::BasicBlock bb, int i, DefinitionExt next) {
462+
predicate lastRefBeforeRedefExt(
463+
DefinitionExt def, Cfg::BasicBlock bb, int i, Cfg::BasicBlock input, DefinitionExt next
464+
) {
463465
exists(LocalVariable v |
464-
Impl::lastRefRedefExt(def, v, bb, i, next) and
466+
Impl::lastRefRedefExt(def, v, bb, i, input, next) and
465467
not SsaInput::variableRead(bb, i, v, false)
466468
)
467469
or
468470
exists(SsaInput::BasicBlock bb0, int i0 |
469-
Impl::lastRefRedefExt(def, _, bb0, i0, next) and
471+
Impl::lastRefRedefExt(def, _, bb0, i0, input, next) and
470472
adjacentDefReachesUncertainReadExt(def, bb, i, bb0, i0)
471473
)
472474
}

‎ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.expected

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
testFailures
2+
edges
3+
| barrier_flow.rb:2:5:2:5 | x | barrier_flow.rb:4:10:4:10 | x | provenance | |
4+
| barrier_flow.rb:2:9:2:17 | call to source | barrier_flow.rb:2:5:2:5 | x | provenance | |
5+
| barrier_flow.rb:8:5:8:5 | x | barrier_flow.rb:11:14:11:14 | x | provenance | |
6+
| barrier_flow.rb:8:9:8:17 | call to source | barrier_flow.rb:8:5:8:5 | x | provenance | |
7+
| barrier_flow.rb:24:5:24:5 | x | barrier_flow.rb:26:10:26:10 | x | provenance | |
8+
| barrier_flow.rb:24:9:24:17 | call to source | barrier_flow.rb:24:5:24:5 | x | provenance | |
9+
nodes
10+
| barrier_flow.rb:2:5:2:5 | x | semmle.label | x |
11+
| barrier_flow.rb:2:9:2:17 | call to source | semmle.label | call to source |
12+
| barrier_flow.rb:4:10:4:10 | x | semmle.label | x |
13+
| barrier_flow.rb:8:5:8:5 | x | semmle.label | x |
14+
| barrier_flow.rb:8:9:8:17 | call to source | semmle.label | call to source |
15+
| barrier_flow.rb:11:14:11:14 | x | semmle.label | x |
16+
| barrier_flow.rb:24:5:24:5 | x | semmle.label | x |
17+
| barrier_flow.rb:24:9:24:17 | call to source | semmle.label | call to source |
18+
| barrier_flow.rb:26:10:26:10 | x | semmle.label | x |
19+
subpaths
20+
#select
21+
| barrier_flow.rb:4:10:4:10 | x | barrier_flow.rb:2:9:2:17 | call to source | barrier_flow.rb:4:10:4:10 | x | $@ | barrier_flow.rb:2:9:2:17 | call to source | call to source |
22+
| barrier_flow.rb:11:14:11:14 | x | barrier_flow.rb:8:9:8:17 | call to source | barrier_flow.rb:11:14:11:14 | x | $@ | barrier_flow.rb:8:9:8:17 | call to source | call to source |
23+
| barrier_flow.rb:26:10:26:10 | x | barrier_flow.rb:24:9:24:17 | call to source | barrier_flow.rb:26:10:26:10 | x | $@ | barrier_flow.rb:24:9:24:17 | call to source | call to source |

‎ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.ql

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* @kind path-problem
3+
*/
4+
5+
import codeql.ruby.AST
6+
import codeql.ruby.CFG
7+
import TestUtilities.InlineFlowTest
8+
import codeql.ruby.dataflow.BarrierGuards
9+
import PathGraph
10+
11+
module FlowConfig implements DataFlow::ConfigSig {
12+
predicate isSource = DefaultFlowConfig::isSource/1;
13+
14+
predicate isSink = DefaultFlowConfig::isSink/1;
15+
16+
predicate isBarrier(DataFlow::Node n) { n instanceof StringConstCompareBarrier }
17+
}
18+
19+
import ValueFlowTest<FlowConfig>
20+
21+
from PathNode source, PathNode sink
22+
where flowPath(source, sink)
23+
select sink, source, sink, "$@", source, source.toString()

‎ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,15 @@
11
testFailures
22
failures
33
newStyleBarrierGuards
4+
| barrier-guards.rb:3:16:4:19 | [input] SSA phi read(foo) |
45
| barrier-guards.rb:4:5:4:7 | foo |
56
| barrier-guards.rb:10:5:10:7 | foo |
67
| barrier-guards.rb:18:5:18:7 | foo |
78
| barrier-guards.rb:24:5:24:7 | foo |
89
| barrier-guards.rb:28:5:28:7 | foo |
910
| barrier-guards.rb:38:5:38:7 | foo |
1011
| barrier-guards.rb:45:9:45:11 | foo |
12+
| barrier-guards.rb:70:22:71:19 | [input] SSA phi read(foo) |
1113
| barrier-guards.rb:71:5:71:7 | foo |
1214
| barrier-guards.rb:83:5:83:7 | foo |
1315
| barrier-guards.rb:91:5:91:7 | foo |
@@ -36,6 +38,14 @@ newStyleBarrierGuards
3638
| barrier-guards.rb:276:5:276:7 | foo |
3739
| barrier-guards.rb:282:5:282:7 | foo |
3840
| barrier-guards.rb:292:5:292:7 | foo |
41+
| barrier_flow.rb:19:14:19:14 | x |
42+
| barrier_flow.rb:32:10:32:10 | x |
43+
| barrier_flow.rb:38:8:38:18 | [input] phi |
44+
| barrier_flow.rb:48:23:48:33 | [input] phi |
45+
| barrier_flow.rb:56:10:57:34 | [input] SSA phi read(x) |
46+
| barrier_flow.rb:58:5:59:34 | [input] SSA phi read(x) |
47+
| barrier_flow.rb:68:10:71:11 | [input] SSA phi read(x) |
48+
| barrier_flow.rb:72:5:75:11 | [input] SSA phi read(x) |
3949
controls
4050
| barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | true |
4151
| barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:6:5:6:7 | foo | false |
@@ -331,3 +341,29 @@ controls
331341
| barrier-guards.rb:291:6:291:6 | g | barrier-guards.rb:291:1:292:19 | [no-match] when ... | no-match |
332342
| barrier-guards.rb:291:6:291:6 | g | barrier-guards.rb:292:5:292:7 | foo | match |
333343
| barrier-guards.rb:291:6:291:6 | g | barrier-guards.rb:294:5:294:7 | foo | no-match |
344+
| barrier_flow.rb:10:8:10:18 | ... != ... | barrier_flow.rb:11:9:11:14 | self | true |
345+
| barrier_flow.rb:18:8:18:18 | ... == ... | barrier_flow.rb:19:9:19:14 | self | true |
346+
| barrier_flow.rb:26:19:26:29 | ... == ... | barrier_flow.rb:26:5:26:10 | self | false |
347+
| barrier_flow.rb:32:19:32:29 | ... != ... | barrier_flow.rb:32:5:32:10 | self | false |
348+
| barrier_flow.rb:38:8:38:18 | ... != ... | barrier_flow.rb:39:9:39:9 | x | true |
349+
| barrier_flow.rb:48:23:48:33 | ... == ... | barrier_flow.rb:48:5:48:5 | x | false |
350+
| barrier_flow.rb:56:8:56:8 | b | barrier_flow.rb:57:9:57:14 | return | true |
351+
| barrier_flow.rb:56:8:56:8 | b | barrier_flow.rb:57:9:57:34 | ... unless ... | true |
352+
| barrier_flow.rb:56:8:56:8 | b | barrier_flow.rb:57:23:57:23 | x | true |
353+
| barrier_flow.rb:56:8:56:8 | b | barrier_flow.rb:59:9:59:14 | return | false |
354+
| barrier_flow.rb:56:8:56:8 | b | barrier_flow.rb:59:9:59:34 | ... unless ... | false |
355+
| barrier_flow.rb:56:8:56:8 | b | barrier_flow.rb:59:23:59:23 | x | false |
356+
| barrier_flow.rb:57:23:57:34 | ... == ... | barrier_flow.rb:57:9:57:14 | return | false |
357+
| barrier_flow.rb:57:23:57:34 | ... == ... | barrier_flow.rb:57:9:57:34 | ... unless ... | true |
358+
| barrier_flow.rb:59:23:59:34 | ... == ... | barrier_flow.rb:59:9:59:14 | return | false |
359+
| barrier_flow.rb:59:23:59:34 | ... == ... | barrier_flow.rb:59:9:59:34 | ... unless ... | true |
360+
| barrier_flow.rb:68:8:68:8 | b | barrier_flow.rb:69:9:71:11 | if ... | true |
361+
| barrier_flow.rb:68:8:68:8 | b | barrier_flow.rb:69:12:69:12 | x | true |
362+
| barrier_flow.rb:68:8:68:8 | b | barrier_flow.rb:70:13:70:18 | return | true |
363+
| barrier_flow.rb:68:8:68:8 | b | barrier_flow.rb:73:9:75:11 | if ... | false |
364+
| barrier_flow.rb:68:8:68:8 | b | barrier_flow.rb:73:12:73:12 | x | false |
365+
| barrier_flow.rb:68:8:68:8 | b | barrier_flow.rb:74:13:74:18 | return | false |
366+
| barrier_flow.rb:69:12:69:23 | ... != ... | barrier_flow.rb:69:9:71:11 | if ... | false |
367+
| barrier_flow.rb:69:12:69:23 | ... != ... | barrier_flow.rb:70:13:70:18 | return | true |
368+
| barrier_flow.rb:73:12:73:23 | ... != ... | barrier_flow.rb:73:9:75:11 | if ... | false |
369+
| barrier_flow.rb:73:12:73:23 | ... != ... | barrier_flow.rb:74:13:74:18 | return | true |

‎ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import codeql.ruby.dataflow.internal.DataFlowPrivate
12
import codeql.ruby.dataflow.internal.DataFlowPublic
23
import codeql.ruby.dataflow.BarrierGuards
34
import codeql.ruby.controlflow.CfgNodes
@@ -25,6 +26,7 @@ module BarrierGuardTest implements TestSig {
2526
tag = "guarded" and
2627
exists(DataFlow::Node n |
2728
newStyleBarrierGuards(n) and
29+
not n instanceof SsaInputNode and
2830
location = n.getLocation() and
2931
element = n.toString() and
3032
value = ""

0 commit comments

Comments
 (0)