From e17c067dcce8683e657f7a88b65acb31f1a91a78 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 3 Jul 2024 11:35:42 +0200 Subject: [PATCH] Ruby: Adopt shared SSA data-flow integration --- .../ruby/controlflow/internal/Guards.qll | 11 + ruby/ql/lib/codeql/ruby/dataflow/SSA.qll | 21 +- .../dataflow/internal/DataFlowDispatch.qll | 3 +- .../dataflow/internal/DataFlowPrivate.qll | 314 +++++++----------- .../ruby/dataflow/internal/DataFlowPublic.qll | 78 +---- .../codeql/ruby/dataflow/internal/SsaImpl.qll | 189 +++++++---- .../internal/TaintTrackingPrivate.qll | 3 +- 7 files changed, 285 insertions(+), 334 deletions(-) create mode 100644 ruby/ql/lib/codeql/ruby/controlflow/internal/Guards.qll diff --git a/ruby/ql/lib/codeql/ruby/controlflow/internal/Guards.qll b/ruby/ql/lib/codeql/ruby/controlflow/internal/Guards.qll new file mode 100644 index 0000000000000..c240595161c77 --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/controlflow/internal/Guards.qll @@ -0,0 +1,11 @@ +private import codeql.ruby.CFG + +/** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */ +pragma[nomagic] +predicate guardControlsBlock(CfgNodes::AstCfgNode guard, BasicBlock bb, boolean branch) { + exists(ConditionBlock conditionBlock, SuccessorTypes::ConditionalSuccessor s | + guard = conditionBlock.getLastNode() and + s.getValue() = branch and + conditionBlock.controls(bb, s) + ) +} diff --git a/ruby/ql/lib/codeql/ruby/dataflow/SSA.qll b/ruby/ql/lib/codeql/ruby/dataflow/SSA.qll index f21dadb7c5abe..07ce826d966a2 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/SSA.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/SSA.qll @@ -202,8 +202,17 @@ module Ssa { final VariableWriteAccessCfgNode getWriteAccess() { result = write } /** - * Holds if this SSA definition represents a direct assignment of `value` - * to the underlying variable. + * Holds if this SSA definition assigns `value` to the underlying variable. + * + * This is either a direct assignment, `x = value`, or an assignment via + * simple pattern matching + * + * ```rb + * case value + * in Foo => x then ... + * in y => then ... + * end + * ``` */ predicate assigns(CfgNodes::ExprCfgNode value) { exists(CfgNodes::ExprNodes::AssignExprCfgNode a, BasicBlock bb, int i | @@ -211,6 +220,14 @@ module Ssa { a = bb.getNode(i) and value = a.getRhs() ) + or + exists(CfgNodes::ExprNodes::CaseExprCfgNode case, CfgNodes::AstCfgNode pattern | + case.getValue() = value and + pattern = case.getBranch(_).(CfgNodes::ExprNodes::InClauseCfgNode).getPattern() + | + this.getWriteAccess() = + [pattern, pattern.(CfgNodes::ExprNodes::AsPatternCfgNode).getVariableAccess()] + ) } final override string toString() { result = write.toString() } diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll index 94a6657c0a248..2e2713b3cc213 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll @@ -1093,8 +1093,7 @@ private module TrackSingletonMethodOnInstanceInput implements CallGraphConstruct singletonMethodOnInstance(_, _, nodeFromPreExpr.getExpr()) ) | - nodeFromPreExpr = - LocalFlow::getParameterDefNode(p.getParameter()).getDefinitionExt().getARead() + nodeFromPreExpr = getParameterDef(p.getParameter()).getARead() or nodeFromPreExpr = p.(SelfParameterNodeImpl).getSelfDefinition().getARead() ) diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll index c0bc6ac243d7c..e16e25ee21372 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll @@ -72,134 +72,48 @@ CfgNodes::ExprCfgNode getAPostUpdateNodeForArg(Argument arg) { not exists(getALastEvalNode(result)) } -/** An SSA definition into which another SSA definition may flow. */ -class SsaInputDefinitionExt extends SsaImpl::DefinitionExt { - SsaInputDefinitionExt() { - this instanceof Ssa::PhiNode - or - this instanceof SsaImpl::PhiReadNode - } - - predicate hasInputFromBlock(SsaImpl::DefinitionExt def, BasicBlock bb, int i, BasicBlock input) { - SsaImpl::lastRefBeforeRedefExt(def, bb, i, input, this) - } +/** Gets the SSA definition node corresponding to parameter `p`. */ +pragma[nomagic] +SsaImpl::DefinitionExt getParameterDef(NamedParameter p) { + exists(BasicBlock bb, int i | + bb.getNode(i).getAstNode() = p.getDefiningAccess() and + result.definesAt(_, bb, i, _) + ) } -/** Provides predicates related to local data flow. */ -module LocalFlow { - /** - * Holds if `nodeFrom` is a node for SSA definition `def`, which can reach `next`. - */ - pragma[nomagic] - private predicate localFlowSsaInputFromDef( - SsaDefinitionExtNode nodeFrom, SsaImpl::DefinitionExt def, SsaInputNode nodeTo - ) { - exists(BasicBlock bb, int i, BasicBlock input, SsaInputDefinitionExt next | - next.hasInputFromBlock(def, bb, i, input) and - def = nodeFrom.getDefinitionExt() and - def.definesAt(_, bb, i, _) and - nodeTo = TSsaInputNode(next, input) - ) - } - - /** - * Holds if `nodeFrom` is a last read of SSA definition `def`, which - * can reach `nodeTo`. - */ - pragma[nomagic] - predicate localFlowSsaInputFromRead(SsaImpl::DefinitionExt def, Node nodeFrom, SsaInputNode nodeTo) { - exists( - BasicBlock bb, int i, CfgNodes::ExprCfgNode exprFrom, BasicBlock input, - SsaInputDefinitionExt next - | - next.hasInputFromBlock(def, bb, i, input) and - exprFrom = bb.getNode(i) and - exprFrom.getExpr() instanceof VariableReadAccess and - exprFrom = [nodeFrom.asExpr(), nodeFrom.(PostUpdateNodeImpl).getPreUpdateNode().asExpr()] and - nodeTo = TSsaInputNode(next, input) - ) - } - - /** Gets the SSA definition node corresponding to parameter `p`. */ - pragma[nomagic] - SsaDefinitionExtNode getParameterDefNode(NamedParameter p) { - exists(BasicBlock bb, int i | - bb.getNode(i).getAstNode() = p.getDefiningAccess() and - result.getDefinitionExt().definesAt(_, bb, i, _) - ) - } +/** Provides logic related to SSA. */ +module SsaFlow { + private module Impl = SsaImpl::DataFlowIntegration; - /** - * Holds if `nodeFrom` is a parameter node, and `nodeTo` is a corresponding SSA node. - */ - pragma[nomagic] - predicate localFlowSsaParamInput(ParameterNodeImpl nodeFrom, SsaDefinitionExtNode nodeTo) { - nodeTo = getParameterDefNode(nodeFrom.getParameter()) + private ParameterNodeImpl toParameterNode(SsaImpl::ParameterExt p) { + result = TNormalParameterNode(p.asParameter()) or - nodeTo.getDefinitionExt() = nodeFrom.(SelfParameterNodeImpl).getSelfDefinition() - } - - /** - * Holds if there is a local use-use flow step from `nodeFrom` to `nodeTo` - * involving SSA definition `def`. - */ - predicate localSsaFlowStepUseUse(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) { - SsaImpl::adjacentReadPairExt(def, nodeFrom.asExpr(), nodeTo.asExpr()) - } - - /** - * Holds if SSA definition `def` assigns `value` to the underlying variable. - * - * This is either a direct assignment, `x = value`, or an assignment via - * simple pattern matching - * - * ```rb - * case value - * in Foo => x then ... - * in y => then ... - * end - * ``` - */ - predicate ssaDefAssigns(Ssa::WriteDefinition def, CfgNodes::ExprCfgNode value) { - def.assigns(value) + result = TSelfMethodParameterNode(p.asMethodSelf()) or - exists(CfgNodes::ExprNodes::CaseExprCfgNode case, CfgNodes::AstCfgNode pattern | - case.getValue() = value and - pattern = case.getBranch(_).(CfgNodes::ExprNodes::InClauseCfgNode).getPattern() - | - def.getWriteAccess() = pattern - or - def.getWriteAccess() = pattern.(CfgNodes::ExprNodes::AsPatternCfgNode).getVariableAccess() - ) + result = TSelfToplevelParameterNode(p.asToplevelSelf()) } - /** - * Holds if there is a local flow step from `nodeFrom` to `nodeTo` involving - * SSA definition `def`. - */ - pragma[nomagic] - predicate localSsaFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) { - // Flow from assignment into SSA definition - ssaDefAssigns(def, nodeFrom.asExpr()) and - nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = def - or - // Flow from SSA definition to first read - def = nodeFrom.(SsaDefinitionExtNode).getDefinitionExt() and - SsaImpl::firstReadExt(def, nodeTo.asExpr()) - or - // Flow from post-update read to next read - localSsaFlowStepUseUse(def, nodeFrom.(PostUpdateNodeImpl).getPreUpdateNode(), nodeTo) + Impl::Node asNode(Node n) { + n = TSsaNode(result) or - // Flow into phi (read) SSA definition node from def - localFlowSsaInputFromDef(nodeFrom, def, nodeTo) + result.(Impl::ExprNode).getExpr() = n.asExpr() or - nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = def and - def = nodeFrom.(SsaInputNode).getDefinitionExt() + result.(Impl::ExprPostUpdateNode).getExpr() = n.(PostUpdateNode).getPreUpdateNode().asExpr() or - localFlowSsaParamInput(nodeFrom, nodeTo) and - def = nodeTo.(SsaDefinitionExtNode).getDefinitionExt() + n = toParameterNode(result.(Impl::ParameterNode).getParameter()) + } + + predicate localFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo, boolean isUseStep) { + Impl::localFlowStep(def, asNode(nodeFrom), asNode(nodeTo), isUseStep) + } + + predicate localMustFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) { + Impl::localMustFlowStep(def, asNode(nodeFrom), asNode(nodeTo)) } +} +/** Provides predicates related to local data flow. */ +module LocalFlow { pragma[nomagic] predicate localFlowStepCommon(Node nodeFrom, Node nodeTo) { nodeFrom.asExpr() = nodeTo.asExpr().(CfgNodes::ExprNodes::BlockArgumentCfgNode).getValue() @@ -240,7 +154,7 @@ module LocalFlow { p.(KeywordParameter).getDefaultValue() = nodeFrom.asExpr().getExpr() ) or - nodeTo.(BlockArgumentNode).getParameterNode(true) = nodeFrom + nodeTo.(ImplicitBlockArgumentNode).getParameterNode(true) = nodeFrom } predicate flowSummaryLocalStep( @@ -253,21 +167,13 @@ module LocalFlow { } predicate localMustFlowStep(Node node1, Node node2) { - LocalFlow::localFlowSsaParamInput(node1, node2) - or - exists(SsaImpl::Definition def | - def.(Ssa::WriteDefinition).assigns(node1.asExpr()) and - node2.(SsaDefinitionExtNode).getDefinitionExt() = def - or - def = node1.(SsaDefinitionExtNode).getDefinitionExt() and - node2.asExpr() = SsaImpl::getARead(def) - ) + SsaFlow::localMustFlowStep(_, node1, node2) or node1.asExpr() = node2.asExpr().(CfgNodes::ExprNodes::AssignExprCfgNode).getRhs() or node1.asExpr() = node2.asExpr().(CfgNodes::ExprNodes::BlockArgumentCfgNode).getValue() or - node1 = node2.(BlockArgumentNode).getParameterNode(true) + node1 = node2.(ImplicitBlockArgumentNode).getParameterNode(true) or node1 = unique(FlowSummaryNode n1 | @@ -347,7 +253,7 @@ module VariableCapture { or exists(Ssa::Definition def | def.getARead() = e2 and - LocalFlow::ssaDefAssigns(def.getAnUltimateDefinition(), e1) + def.getAnUltimateDefinition().(Ssa::WriteDefinition).assigns(e1) ) } @@ -538,23 +444,14 @@ private module Cached { newtype TNode = TExprNode(CfgNodes::ExprCfgNode n) or TReturningNode(CfgNodes::ReturningCfgNode n) { exists(n.getReturnedValueNode()) } or - TSsaDefinitionExtNode(SsaImpl::DefinitionExt def) or - TSsaInputNode(SsaInputDefinitionExt def, BasicBlock input) { - def.hasInputFromBlock(_, _, _, input) - } or + TSsaNode(SsaImpl::DataFlowIntegration::SsaNode node) or TCapturedVariableNode(VariableCapture::CapturedVariable v) or - TNormalParameterNode(Parameter p) { - p instanceof SimpleParameter or - p instanceof OptionalParameter or - p instanceof KeywordParameter or - p instanceof HashSplatParameter or - p instanceof SplatParameter - } or + TNormalParameterNode(SsaImpl::NormalParameter p) or TSelfMethodParameterNode(MethodBase m) or TSelfToplevelParameterNode(Toplevel t) or TLambdaSelfReferenceNode(Callable c) { lambdaCreationExpr(_, _, c) } or - TBlockParameterNode(MethodBase m) or - TBlockArgumentNode(CfgNodes::ExprNodes::CallCfgNode yield) { + TImplicitBlockParameterNode(MethodBase m) { not m.getAParameter() instanceof BlockParameter } or + TImplicitBlockArgumentNode(CfgNodes::ExprNodes::CallCfgNode yield) { yield = any(BlockParameterNode b).getAYieldCall() } or TSynthHashSplatParameterNode(DataFlowCallable c) { @@ -600,7 +497,7 @@ private module Cached { class TSelfParameterNode = TSelfMethodParameterNode or TSelfToplevelParameterNode; class TSourceParameterNode = - TNormalParameterNode or TBlockParameterNode or TSelfParameterNode or + TNormalParameterNode or TImplicitBlockParameterNode or TSelfParameterNode or TSynthHashSplatParameterNode or TSynthSplatParameterNode; cached @@ -618,16 +515,13 @@ private module Cached { ( LocalFlow::localFlowStepCommon(nodeFrom, nodeTo) or - exists(SsaImpl::DefinitionExt def | + exists(SsaImpl::DefinitionExt def, boolean isUseStep | + SsaFlow::localFlowStep(def, nodeFrom, nodeTo, isUseStep) and // captured variables are handled by the shared `VariableCapture` library not def instanceof VariableCapture::CapturedSsaDefinitionExt | - LocalFlow::localSsaFlowStep(def, nodeFrom, nodeTo) - or - LocalFlow::localSsaFlowStepUseUse(def, nodeFrom, nodeTo) and - not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _) + isUseStep = false or - LocalFlow::localFlowSsaInputFromRead(def, nodeFrom, nodeTo) and not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _) ) or @@ -643,11 +537,7 @@ private module Cached { predicate localFlowStepImpl(Node nodeFrom, Node nodeTo) { LocalFlow::localFlowStepCommon(nodeFrom, nodeTo) or - LocalFlow::localSsaFlowStep(_, nodeFrom, nodeTo) - or - LocalFlow::localSsaFlowStepUseUse(_, nodeFrom, nodeTo) - or - LocalFlow::localFlowSsaInputFromRead(_, nodeFrom, nodeTo) + SsaFlow::localFlowStep(_, nodeFrom, nodeTo, _) or // Simple flow through library code is included in the exposed local // step relation, even though flow is technically inter-procedural @@ -661,11 +551,7 @@ private module Cached { predicate localFlowStepTypeTracker(Node nodeFrom, Node nodeTo) { LocalFlow::localFlowStepCommon(nodeFrom, nodeTo) or - LocalFlow::localSsaFlowStep(_, nodeFrom, nodeTo) - or - LocalFlow::localSsaFlowStepUseUse(_, nodeFrom, nodeTo) - or - LocalFlow::localFlowSsaInputFromRead(_, nodeFrom, nodeTo) + SsaFlow::localFlowStep(_, nodeFrom, nodeTo, _) or VariableCapture::flowInsensitiveStep(nodeFrom, nodeTo) or @@ -675,7 +561,8 @@ private module Cached { /** Holds if `n` wraps an SSA definition without ingoing flow. */ private predicate entrySsaDefinition(SsaDefinitionExtNode n) { - n.getDefinitionExt() = any(SsaImpl::WriteDefinition def | not LocalFlow::ssaDefAssigns(def, _)) + n.getDefinitionExt() = + any(SsaImpl::WriteDefinition def | not def.(Ssa::WriteDefinition).assigns(_)) } pragma[nomagic] @@ -715,13 +602,15 @@ private module Cached { // Ensure all entry SSA definitions are local sources, except those that correspond // to parameters (which are themselves local sources) entrySsaDefinition(n) and - not LocalFlow::localFlowSsaParamInput(_, n) + not exists(SsaImpl::ParameterExt p | + p.isInitializedBy(n.(SsaDefinitionExtNode).getDefinitionExt()) + ) or isStoreTargetNode(n) or TypeTrackingInput::loadStep(_, n, _) or - n instanceof BlockArgumentNode + n instanceof ImplicitBlockArgumentNode } cached @@ -825,11 +714,7 @@ import Cached /** Holds if `n` should be hidden from path explanations. */ predicate nodeIsHidden(Node n) { - n.(SsaDefinitionExtNode).isHidden() - or - n instanceof SsaInputNode - or - n = LocalFlow::getParameterDefNode(_) + n.(SsaNode).isHidden() or exists(AstNode desug | isDesugarNode(desug) and @@ -861,33 +746,55 @@ predicate nodeIsHidden(Node n) { or n instanceof CaptureNode or - n instanceof BlockArgumentNode + n instanceof ImplicitBlockArgumentNode } -/** An SSA definition, viewed as a node in a data flow graph. */ -class SsaDefinitionExtNode extends NodeImpl, TSsaDefinitionExtNode { +/** An SSA node. */ +abstract class SsaNode extends NodeImpl, TSsaNode { + SsaImpl::DataFlowIntegration::SsaNode node; SsaImpl::DefinitionExt def; - SsaDefinitionExtNode() { this = TSsaDefinitionExtNode(def) } + SsaNode() { + this = TSsaNode(node) and + def = node.getDefinitionExt() + } - /** Gets the underlying SSA definition. */ SsaImpl::DefinitionExt getDefinitionExt() { result = def } + /** Holds if this node should be hidden from path explanations. */ + abstract predicate isHidden(); + + override Location getLocationImpl() { result = node.getLocation() } + + override string toStringImpl() { result = node.toString() } +} + +/** An (extended) SSA definition, viewed as a node in a data flow graph. */ +class SsaDefinitionExtNode extends SsaNode { + override SsaImpl::DataFlowIntegration::SsaDefinitionExtNode node; + /** Gets the underlying variable. */ Variable getVariable() { result = def.getSourceVariable() } - /** Holds if this node should be hidden from path explanations. */ - predicate isHidden() { + override predicate isHidden() { not def instanceof Ssa::WriteDefinition or isDesugarNode(def.(Ssa::WriteDefinition).getWriteAccess().getExpr()) + or + def = getParameterDef(_) } override CfgScope getCfgScope() { result = def.getBasicBlock().getScope() } +} + +class SsaDefinitionNodeImpl extends SsaDefinitionExtNode { + Ssa::Definition ssaDef; + + SsaDefinitionNodeImpl() { ssaDef = def } - override Location getLocationImpl() { result = def.getLocation() } + override Location getLocationImpl() { result = ssaDef.getLocation() } - override string toStringImpl() { result = def.toString() } + override string toStringImpl() { result = ssaDef.toString() } } /** @@ -925,20 +832,12 @@ class SsaDefinitionExtNode extends NodeImpl, TSsaDefinitionExtNode { * * both inputs into the phi read node after the outer condition are guarded. */ -class SsaInputNode extends NodeImpl, TSsaInputNode { - SsaImpl::DefinitionExt def; - BasicBlock input; - - SsaInputNode() { this = TSsaInputNode(def, input) } +class SsaInputNode extends SsaNode { + override SsaImpl::DataFlowIntegration::SsaInputNode node; - /** Gets the underlying SSA definition. */ - SsaImpl::DefinitionExt getDefinitionExt() { result = def } + override predicate isHidden() { any() } - override CfgScope getCfgScope() { result = input.getScope() } - - override Location getLocationImpl() { result = input.getLastNode().getLocation() } - - override string toStringImpl() { result = "[input] " + def } + override CfgScope getCfgScope() { result = node.getDefinitionExt().getBasicBlock().getScope() } } /** An SSA definition for a `self` variable. */ @@ -1026,7 +925,7 @@ private module ParameterNodes { * flow graph. */ class NormalParameterNode extends ParameterNodeImpl, TNormalParameterNode { - private Parameter parameter; + Parameter parameter; NormalParameterNode() { this = TNormalParameterNode(parameter) } @@ -1055,6 +954,9 @@ private module ParameterNodes { // There are no positional parameters after the splat not exists(SimpleParameter p, int m | m > n | p = callable.getParameter(m)) ) + or + parameter = callable.getAParameter().(BlockParameter) and + pos.isBlock() ) } @@ -1162,25 +1064,33 @@ private module ParameterNodes { * The value of a block parameter at function entry, viewed as a node in a data * flow graph. */ - class BlockParameterNode extends ParameterNodeImpl, TBlockParameterNode { + abstract class BlockParameterNode extends ParameterNodeImpl { + abstract MethodBase getMethod(); + + CfgNodes::ExprNodes::CallCfgNode getAYieldCall() { + this.getMethod() = result.getExpr().(YieldCall).getEnclosingMethod() + } + } + + private class ExplicitBlockParameterNode extends BlockParameterNode, NormalParameterNode { + override BlockParameter parameter; + + final override MethodBase getMethod() { result.getAParameter() = parameter } + } + + private class ImplicitBlockParameterNode extends BlockParameterNode, TImplicitBlockParameterNode { private MethodBase method; - BlockParameterNode() { this = TBlockParameterNode(method) } + ImplicitBlockParameterNode() { this = TImplicitBlockParameterNode(method) } - final MethodBase getMethod() { result = method } + final override MethodBase getMethod() { result = method } - override Parameter getParameter() { - result = method.getAParameter() and result instanceof BlockParameter - } + override Parameter getParameter() { none() } override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) { c.asCfgScope() = method and pos.isBlock() } - CfgNodes::ExprNodes::CallCfgNode getAYieldCall() { - this.getMethod() = result.getExpr().(YieldCall).getEnclosingMethod() - } - override CfgScope getCfgScope() { result = method } override Location getLocationImpl() { @@ -1452,10 +1362,10 @@ module ArgumentNodes { } } - class BlockArgumentNode extends NodeImpl, ArgumentNode, TBlockArgumentNode { + class ImplicitBlockArgumentNode extends NodeImpl, ArgumentNode, TImplicitBlockArgumentNode { CfgNodes::ExprNodes::CallCfgNode yield; - BlockArgumentNode() { this = TBlockArgumentNode(yield) } + ImplicitBlockArgumentNode() { this = TImplicitBlockArgumentNode(yield) } CfgNodes::ExprNodes::CallCfgNode getYieldCall() { result = yield } @@ -1893,7 +1803,7 @@ predicate jumpStep(Node pred, Node succ) { or any(AdditionalJumpStep s).step(pred, succ) or - succ.(BlockArgumentNode).getParameterNode(false) = pred + succ.(ImplicitBlockArgumentNode).getParameterNode(false) = pred } private ContentSet getArrayContent(int n) { @@ -2241,7 +2151,7 @@ private predicate lambdaCallExpr( */ predicate lambdaSourceCall(CfgNodes::ExprNodes::CallCfgNode call, LambdaCallKind kind, Node receiver) { kind = TYieldCallKind() and - call = receiver.(BlockArgumentNode).getYieldCall() + call = receiver.(ImplicitBlockArgumentNode).getYieldCall() or kind = TLambdaCallKind() and lambdaCallExpr(call, receiver.asExpr()) diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll index d4e07e0653e10..33ac82448a90a 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll @@ -6,6 +6,7 @@ private import codeql.ruby.typetracking.internal.TypeTrackingImpl private import codeql.ruby.dataflow.SSA private import FlowSummaryImpl as FlowSummaryImpl private import codeql.ruby.ApiGraphs +private import SsaImpl as SsaImpl /** * An element, viewed as a node in a data flow graph. Either an expression @@ -360,16 +361,12 @@ class PostUpdateNode extends Node { } /** An SSA definition, viewed as a node in a data flow graph. */ -class SsaDefinitionNode extends Node instanceof SsaDefinitionExtNode { - Ssa::Definition def; - - SsaDefinitionNode() { this = TSsaDefinitionExtNode(def) } - +class SsaDefinitionNode extends Node instanceof SsaDefinitionNodeImpl { /** Gets the underlying SSA definition. */ - Ssa::Definition getDefinition() { result = def } + Ssa::Definition getDefinition() { result = super.getDefinitionExt() } /** Gets the underlying variable. */ - Variable getVariable() { result = def.getSourceVariable() } + Variable getVariable() { result = this.getDefinition().getSourceVariable() } } cached @@ -870,56 +867,7 @@ private predicate sameSourceVariable(Ssa::Definition def1, Ssa::Definition def2) * in data flow and taint tracking. */ module BarrierGuard { - private import SsaImpl as SsaImpl - - pragma[nomagic] - private predicate guardChecksSsaDef(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def) { - guardChecks(g, def.getARead(), branch) - } - - pragma[nomagic] - private predicate guardControlsSsaRead( - CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def, Node n - ) { - def.getARead() = n.asExpr() and - guardControlsBlock(g, n.asExpr().getBasicBlock(), branch) - } - - pragma[nomagic] - private predicate guardControlsPhiInput( - CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def, BasicBlock input, - SsaInputDefinitionExt phi - ) { - phi.hasInputFromBlock(def, _, _, input) and - ( - guardControlsBlock(g, input, branch) - or - exists(SuccessorTypes::ConditionalSuccessor s | - g = input.getLastNode() and - s.getValue() = branch and - input.getASuccessor(s) = phi.getBasicBlock() - ) - ) - } - - /** Gets a node that is safely guarded by the given guard check. */ - Node getABarrierNode() { - exists(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def | - guardChecksSsaDef(g, branch, def) and - guardControlsSsaRead(g, branch, def, result) - ) - or - exists( - CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def, BasicBlock input, - SsaInputDefinitionExt phi - | - guardChecksSsaDef(g, branch, def) and - guardControlsPhiInput(g, branch, def, input, phi) and - result = TSsaInputNode(phi, input) - ) - or - result.asExpr() = getAMaybeGuardedCapturedDef().getARead() - } + private import codeql.ruby.controlflow.internal.Guards /** * Gets an implicit entry definition for a captured variable that @@ -928,6 +876,7 @@ module BarrierGuard { * This is restricted to calls where the variable is captured inside a * block. */ + pragma[nomagic] private Ssa::CapturedEntryDefinition getAMaybeGuardedCapturedDef() { exists( CfgNodes::ExprCfgNode g, boolean branch, CfgNodes::ExprCfgNode testedNode, @@ -940,15 +889,14 @@ module BarrierGuard { sameSourceVariable(def, result) ) } -} -/** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */ -private predicate guardControlsBlock(CfgNodes::AstCfgNode guard, BasicBlock bb, boolean branch) { - exists(ConditionBlock conditionBlock, SuccessorTypes::ConditionalSuccessor s | - guard = conditionBlock.getLastNode() and - s.getValue() = branch and - conditionBlock.controls(bb, s) - ) + /** Gets a node that is safely guarded by the given guard check. */ + Node getABarrierNode() { + SsaFlow::asNode(result) = + SsaImpl::DataFlowIntegration::BarrierGuard::getABarrierNode() + or + result.asExpr() = getAMaybeGuardedCapturedDef().getARead() + } } /** diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll index 8824d08130847..bc7a9cc0eb1be 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll @@ -6,7 +6,7 @@ private import codeql.ruby.dataflow.SSA private import codeql.ruby.ast.Variable private import Cfg::CfgNodes::ExprNodes -private module SsaInput implements SsaImplCommon::InputSig { +module SsaInput implements SsaImplCommon::InputSig { private import codeql.ruby.controlflow.ControlFlowGraph as Cfg private import codeql.ruby.controlflow.BasicBlocks as BasicBlocks @@ -65,7 +65,7 @@ private module SsaInput implements SsaImplCommon::InputSig { } } -private import SsaImplCommon::Make as Impl +import SsaImplCommon::Make as Impl class Definition = Impl::Definition; @@ -283,15 +283,6 @@ private predicate adjacentDefSkipUncertainReads( SsaInput::variableRead(bb2, i2, _, true) } -/** Same as `adjacentDefReadExt`, but skips uncertain reads. */ -pragma[nomagic] -private predicate adjacentDefSkipUncertainReadsExt( - DefinitionExt def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2 -) { - adjacentDefReachesReadExt(def, bb1, i1, bb2, i2) and - SsaInput::variableRead(bb2, i2, _, true) -} - private predicate adjacentDefReachesUncertainReadExt( DefinitionExt def, SsaInput::BasicBlock bb1, int i1, SsaInput::BasicBlock bb2, int i2 ) { @@ -393,19 +384,6 @@ private module Cached { ) } - /** - * Holds if the value defined at SSA definition `def` can reach a read at `read`, - * without passing through any other non-pseudo read. - */ - cached - predicate firstReadExt(DefinitionExt def, VariableReadAccessCfgNode read) { - exists(Cfg::BasicBlock bb1, int i1, Cfg::BasicBlock bb2, int i2 | - def.definesAt(_, bb1, i1, _) and - adjacentDefSkipUncertainReadsExt(def, bb1, i1, bb2, i2) and - read = bb2.getNode(i2) - ) - } - /** * Holds if the read at `read2` is a read of the same SSA definition `def` * as the read at `read1`, and `read2` can be reached from `read1` without @@ -423,23 +401,6 @@ private module Cached { ) } - /** - * Holds if the read at `read2` is a read of the same SSA definition `def` - * as the read at `read1`, and `read2` can be reached from `read1` without - * passing through another non-pseudo read. - */ - cached - predicate adjacentReadPairExt( - DefinitionExt def, VariableReadAccessCfgNode read1, VariableReadAccessCfgNode read2 - ) { - exists(Cfg::BasicBlock bb1, int i1, Cfg::BasicBlock bb2, int i2 | - read1 = bb1.getNode(i1) and - variableReadActual(bb1, i1, _) and - adjacentDefSkipUncertainReadsExt(def, bb1, i1, bb2, i2) and - read2 = bb2.getNode(i2) - ) - } - /** * Holds if the read of `def` at `read` may be a last read. That is, `read` * can either reach another definition of the underlying source variable or @@ -454,31 +415,41 @@ private module Cached { ) } - /** - * Holds if the reference to `def` at index `i` in basic block `bb` can reach - * another definition `next` of the same underlying source variable, without - * passing through another write or non-pseudo read. - * - * The reference is either a read of `def` or `def` itself. - */ cached - predicate lastRefBeforeRedefExt( - DefinitionExt def, Cfg::BasicBlock bb, int i, Cfg::BasicBlock input, DefinitionExt next - ) { - exists(LocalVariable v | - Impl::lastRefRedefExt(def, v, bb, i, input, next) and - not SsaInput::variableRead(bb, i, v, false) - ) - or - exists(SsaInput::BasicBlock bb0, int i0 | - Impl::lastRefRedefExt(def, _, bb0, i0, input, next) and - adjacentDefReachesUncertainReadExt(def, bb, i, bb0, i0) - ) + Definition uncertainWriteDefinitionInput(UncertainWriteDefinition def) { + Impl::uncertainWriteDefinitionInput(def, result) } cached - Definition uncertainWriteDefinitionInput(UncertainWriteDefinition def) { - Impl::uncertainWriteDefinitionInput(def, result) + module DataFlowIntegration { + import DataFlowIntegrationImpl + + cached + predicate localFlowStep(DefinitionExt def, Node nodeFrom, Node nodeTo, boolean isUseStep) { + DataFlowIntegrationImpl::localFlowStep(def, nodeFrom, nodeTo, isUseStep) + } + + cached + predicate localMustFlowStep(DefinitionExt def, Node nodeFrom, Node nodeTo) { + DataFlowIntegrationImpl::localMustFlowStep(def, nodeFrom, nodeTo) + } + + signature predicate guardChecksSig(Cfg::CfgNodes::AstCfgNode g, Cfg::CfgNode e, boolean branch); + + cached // nothing is actually cached + module BarrierGuard { + private predicate guardChecksAdjTypes( + DataFlowIntegrationInput::Guard g, DataFlowIntegrationInput::Expr e, boolean branch + ) { + guardChecks(g, e, branch) + } + + private Node getABarrierNodeImpl() { + result = DataFlowIntegrationImpl::BarrierGuard::getABarrierNode() + } + + predicate getABarrierNode = getABarrierNodeImpl/0; + } } } @@ -510,3 +481,97 @@ class PhiReadNode extends DefinitionExt, Impl::PhiReadNode { override Location getLocation() { result = Impl::PhiReadNode.super.getLocation() } } + +class NormalParameter extends Parameter { + NormalParameter() { + this instanceof SimpleParameter or + this instanceof OptionalParameter or + this instanceof KeywordParameter or + this instanceof HashSplatParameter or + this instanceof SplatParameter or + this instanceof BlockParameter + } +} + +/** Gets the SSA definition node corresponding to parameter `p`. */ +pragma[nomagic] +DefinitionExt getParameterDef(NamedParameter p) { + exists(Cfg::BasicBlock bb, int i | + bb.getNode(i).getAstNode() = p.getDefiningAccess() and + result.definesAt(_, bb, i, _) + ) +} + +private newtype TParameterExt = + TNormalParameter(NormalParameter p) or + TSelfMethodParameter(MethodBase m) or + TSelfToplevelParameter(Toplevel t) + +/** A normal parameter or an implicit `self` parameter. */ +class ParameterExt extends TParameterExt { + NormalParameter asParameter() { this = TNormalParameter(result) } + + MethodBase asMethodSelf() { this = TSelfMethodParameter(result) } + + Toplevel asToplevelSelf() { this = TSelfToplevelParameter(result) } + + predicate isInitializedBy(WriteDefinition def) { + def = getParameterDef(this.asParameter()) + or + def.(Ssa::SelfDefinition).getSourceVariable().getDeclaringScope() = + [this.asMethodSelf().(Scope), this.asToplevelSelf()] + } + + string toString() { + result = + [ + this.asParameter().toString(), this.asMethodSelf().toString(), + this.asToplevelSelf().toString() + ] + } + + Location getLocation() { + result = + [ + this.asParameter().getLocation(), this.asMethodSelf().getLocation(), + this.asToplevelSelf().getLocation() + ] + } +} + +private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInputSig { + private import codeql.ruby.controlflow.internal.Guards as Guards + + class Parameter = ParameterExt; + + class Expr extends Cfg::CfgNodes::ExprCfgNode { + predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) } + } + + Expr getARead(Definition def) { result = Cached::getARead(def) } + + predicate ssaDefAssigns(WriteDefinition def, Expr value) { + def.(Ssa::WriteDefinition).assigns(value) + } + + predicate ssaDefInitializesParam(WriteDefinition def, Parameter p) { p.isInitializedBy(def) } + + class Guard extends Cfg::CfgNodes::AstCfgNode { + predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) } + } + + /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */ + predicate guardControlsBlock(Guard guard, SsaInput::BasicBlock bb, boolean branch) { + Guards::guardControlsBlock(guard, bb, branch) + } + + /** Gets an immediate conditional successor of basic block `bb`, if any. */ + SsaInput::BasicBlock getAConditionalBasicBlockSuccessor(SsaInput::BasicBlock bb, boolean branch) { + exists(Cfg::SuccessorTypes::ConditionalSuccessor s | + result = bb.getASuccessor(s) and + s.getValue() = branch + ) + } +} + +private module DataFlowIntegrationImpl = Impl::DataFlowIntegration; diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/TaintTrackingPrivate.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/TaintTrackingPrivate.qll index 903c8b562e763..565fea34dbc04 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/TaintTrackingPrivate.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/TaintTrackingPrivate.qll @@ -5,6 +5,7 @@ private import codeql.ruby.CFG private import codeql.ruby.DataFlow private import FlowSummaryImpl as FlowSummaryImpl private import codeql.ruby.dataflow.SSA +private import SsaImpl as SsaImpl /** * Holds if `node` should be a sanitizer in all global taint flow configurations @@ -89,7 +90,7 @@ private module Cached { clause = case.getBranch(_) and def = nodeTo.(SsaDefinitionExtNode).getDefinitionExt() and def.getControlFlowNode() = variablesInPattern(clause.getPattern()) and - not LocalFlow::ssaDefAssigns(def, value) + not def.(Ssa::WriteDefinition).assigns(value) ) or // operation involving `nodeFrom`