From 0e357f74d582b3da29813cdda9a9dff855d08a13 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Thu, 4 Jul 2024 17:31:09 +0200 Subject: [PATCH] perf --- .../dataflow/internal/DataFlowPrivate.qll | 79 +---- .../dataflow/internal/DataFlowPublic.qll | 5 +- .../code/csharp/dataflow/internal/SsaImpl.qll | 65 ++++ .../dataflow/internal/DataFlowPrivate.qll | 141 +-------- .../ruby/dataflow/internal/DataFlowPublic.qll | 7 +- .../codeql/ruby/dataflow/internal/SsaImpl.qll | 113 +++++++ .../internal/TaintTrackingPrivate.qll | 3 +- shared/ssa/codeql/ssa/Ssa.qll | 293 +++++++++--------- 8 files changed, 355 insertions(+), 351 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll index 230d65cf959bd..a638c16c326e0 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll @@ -256,8 +256,9 @@ module VariableCapture { private predicate closureFlowStep(ControlFlow::Nodes::ExprNode e1, ControlFlow::Nodes::ExprNode e2) { e1 = LocalFlow::getALastEvalNode(e2) or - exists(Ssa::Definition def | - SsaFlow::Input::ssaDefAssigns(def.getAnUltimateDefinition(), e1) and + exists(Ssa::Definition def, AssignableDefinition adef | + LocalFlow::defAssigns(adef, _, e1) and + def.getAnUltimateDefinition().(Ssa::ExplicitDefinition).getADefinition() = adef and exists(def.getAReadAtNode(e2)) ) } @@ -483,42 +484,7 @@ module VariableCapture { /** Provides logic related to SSA. */ module SsaFlow { - module Input implements SsaImpl::Impl::DataFlowIntegrationInputSig { - private import csharp as Cs - - class Expr extends ControlFlow::Node { - predicate hasCfgNode(ControlFlow::BasicBlock bb, int i) { this = bb.getNode(i) } - } - - predicate ssaDefAssigns(SsaImpl::WriteDefinition def, Expr value) { - exists(AssignableDefinition adef, ControlFlow::Node cfnDef | - any(LocalFlow::LocalExprStepConfiguration conf).hasDefPath(_, value, adef, cfnDef) and - def.(Ssa::ExplicitDefinition).getADefinition() = adef and - def.(Ssa::ExplicitDefinition).getControlFlowNode() = cfnDef - ) - } - - class Parameter = Cs::Parameter; - - predicate ssaDefInitializesParam(SsaImpl::WriteDefinition def, Parameter p) { - def.(Ssa::ImplicitParameterDefinition).getParameter() = p - } - - /** - * Allows for flow into uncertain defintions that are not call definitions, - * as we, conservatively, consider such definitions to be certain. - */ - predicate allowFlowIntoUncertainDef(SsaImpl::UncertainWriteDefinition def) { - def instanceof Ssa::ExplicitDefinition - or - def = - any(Ssa::ImplicitQualifierDefinition qdef | - allowFlowIntoUncertainDef(qdef.getQualifierDefinition()) - ) - } - } - - module Impl = SsaImpl::Impl::DataFlowIntegration; + module Impl = SsaImpl::Integration; Impl::Node asNode(Node n) { n = TSsaNode(result) @@ -532,47 +498,12 @@ module SsaFlow { } predicate localFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) { - Impl::localFlowStep(def, asNode(nodeFrom), asNode(nodeTo)) and - // exclude flow directly from RHS to SSA definition, as we instead want to - // go from RHS to matching assingnable definition, and from there to SSA definition - not Input::ssaDefAssigns(def, nodeFrom.(ExprNode).getControlFlowNode()) + Impl::localFlowStep(def, asNode(nodeFrom), asNode(nodeTo)) } predicate localMustFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) { Impl::localMustFlowStep(def, asNode(nodeFrom), asNode(nodeTo)) } - - module BarrierGuardsInput implements Impl::BarrierGuardsInputSig { - private import semmle.code.csharp.controlflow.BasicBlocks - private import semmle.code.csharp.controlflow.Guards as Guards - - class Guard extends Guards::Guard { - predicate hasCfgNode(ControlFlow::BasicBlock bb, int i) { - this.getAControlFlowNode() = bb.getNode(i) - } - } - - /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */ - predicate guardControlsBlock(Guard guard, ControlFlow::BasicBlock bb, boolean branch) { - exists(ConditionBlock conditionBlock, ControlFlow::SuccessorTypes::ConditionalSuccessor s | - guard.getAControlFlowNode() = conditionBlock.getLastNode() and - s.getValue() = branch and - conditionBlock.controls(bb, s) - ) - } - - /** Gets an immediate conditional successor of basic block `bb`, if any. */ - ControlFlow::BasicBlock getAConditionalBasicBlockSuccessor( - ControlFlow::BasicBlock bb, boolean branch - ) { - exists(ControlFlow::SuccessorTypes::ConditionalSuccessor s | - result = bb.getASuccessorByType(s) and - s.getValue() = branch - ) - } - } - - module BarrierGuardsImpl = Impl::BarrierGuards; } /** Provides predicates related to local data flow. */ diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll index c1f67407f39be..be45923f50daf 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll @@ -3,6 +3,7 @@ private import DataFlowDispatch private import DataFlowPrivate private import semmle.code.csharp.controlflow.Guards private import semmle.code.csharp.Unification +private import SsaImpl as SsaImpl /** * An element, viewed as a node in a data flow graph. Either an expression @@ -172,7 +173,7 @@ signature predicate guardChecksSig(Guard g, Expr e, AbstractValue v); */ module BarrierGuard { private predicate guardChecksAdj( - SsaFlow::BarrierGuardsInput::Guard g, SsaFlow::Input::Expr e, boolean branch + SsaImpl::IntegrationInput::Guard g, SsaImpl::IntegrationInput::Expr e, boolean branch ) { exists(AbstractValues::BooleanValue v | guardChecks(g, e.getAstNode(), v) and @@ -184,7 +185,7 @@ module BarrierGuard { pragma[nomagic] Node getABarrierNode() { SsaFlow::asNode(result) = - SsaFlow::BarrierGuardsImpl::BarrierGuard::getABarrierNode() + SsaImpl::Integration::BarrierGuard::getABarrierNode() or exists(Guard g, Expr e, AbstractValue v | guardChecks(g, e, v) and diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll index 8c86f33c5df52..b7f163928c2c5 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll @@ -977,6 +977,9 @@ private module Cached { outRefExitRead(bb, i, v) ) } + + cached + predicate forceCachingInSameStage() { Integration::localFlowStep(_, _, _) } } import Cached @@ -1034,3 +1037,65 @@ class PhiReadNode extends DefinitionExt, Impl::PhiReadNode { result = this.getSourceVariable().getEnclosingCallable() } } + +module IntegrationInput implements Impl::DataFlowIntegrationInputSig { + private import csharp as Cs + private import semmle.code.csharp.controlflow.BasicBlocks + private import semmle.code.csharp.controlflow.Guards as Guards + + class Expr extends ControlFlow::Node { + predicate hasCfgNode(ControlFlow::BasicBlock bb, int i) { this = bb.getNode(i) } + } + + predicate ssaDefAssigns(WriteDefinition def, Expr value) { + // exclude flow directly from RHS to SSA definition, as we instead want to + // go from RHS to matching assingnable definition, and from there to SSA definition + none() + } + + class Parameter = Cs::Parameter; + + predicate ssaDefInitializesParam(WriteDefinition def, Parameter p) { + def.(Ssa::ImplicitParameterDefinition).getParameter() = p + } + + /** + * Allows for flow into uncertain defintions that are not call definitions, + * as we, conservatively, consider such definitions to be certain. + */ + predicate allowFlowIntoUncertainDef(UncertainWriteDefinition def) { + def instanceof Ssa::ExplicitDefinition + or + def = + any(Ssa::ImplicitQualifierDefinition qdef | + allowFlowIntoUncertainDef(qdef.getQualifierDefinition()) + ) + } + + class Guard extends Guards::Guard { + predicate hasCfgNode(ControlFlow::BasicBlock bb, int i) { + this.getAControlFlowNode() = bb.getNode(i) + } + } + + /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */ + predicate guardControlsBlock(Guard guard, ControlFlow::BasicBlock bb, boolean branch) { + exists(ConditionBlock conditionBlock, ControlFlow::SuccessorTypes::ConditionalSuccessor s | + guard.getAControlFlowNode() = conditionBlock.getLastNode() and + s.getValue() = branch and + conditionBlock.controls(bb, s) + ) + } + + /** Gets an immediate conditional successor of basic block `bb`, if any. */ + ControlFlow::BasicBlock getAConditionalBasicBlockSuccessor( + ControlFlow::BasicBlock bb, boolean branch + ) { + exists(ControlFlow::SuccessorTypes::ConditionalSuccessor s | + result = bb.getASuccessorByType(s) and + s.getValue() = branch + ) + } +} + +module Integration = Impl::DataFlowIntegration; diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll index 62e8ba4e90a53..f53af20487666 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll @@ -81,97 +81,9 @@ SsaImpl::DefinitionExt getParameterDef(NamedParameter p) { ) } -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 - } -} - /** Provides logic related to SSA. */ module SsaFlow { - module Input implements SsaImpl::Impl::DataFlowIntegrationInputSig { - class Expr extends CfgNodes::ExprCfgNode { - predicate hasCfgNode(SsaImpl::SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) } - } - - /** - * 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(SsaImpl::WriteDefinition def, Expr value) { - def.(Ssa::WriteDefinition).assigns(value) - or - exists(CfgNodes::ExprNodes::CaseExprCfgNode case, CfgNodes::AstCfgNode pattern | - case.getValue() = value and - pattern = case.getBranch(_).(CfgNodes::ExprNodes::InClauseCfgNode).getPattern() - | - def.(Ssa::WriteDefinition).getWriteAccess() = - [pattern, pattern.(CfgNodes::ExprNodes::AsPatternCfgNode).getVariableAccess()] - ) - } - - private newtype TParameter = - TNormalParameter(NormalParameter p) or - TSelfMethodParameter(MethodBase m) or - TSelfToplevelParameter(Toplevel t) - - // We could also use `class Parameter = ParameterNodeImpl`, which would be simpler, but - // it makes the `TNode` construction unecessarily recursive. - class Parameter extends TParameter { - NormalParameter asParameter() { this = TNormalParameter(result) } - - MethodBase asMethodSelf() { this = TSelfMethodParameter(result) } - - Toplevel asToplevelSelf() { this = TSelfToplevelParameter(result) } - - ParameterNodeImpl toParameterNode() { - result = TNormalParameterNode(this.asParameter()) - or - result = TSelfMethodParameterNode(this.asMethodSelf()) - or - result = TSelfToplevelParameterNode(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() - ] - } - } - - predicate ssaDefInitializesParam(SsaImpl::WriteDefinition def, Parameter p) { - def = getParameterDef(p.asParameter()) - or - def.(Ssa::SelfDefinition).getSourceVariable().getDeclaringScope() = - [p.asMethodSelf().(Scope), p.asToplevelSelf()] - } - } - - module Impl = SsaImpl::Impl::DataFlowIntegration; + module Impl = SsaImpl::Integration; Impl::Node asNode(Node n) { n = TSsaNode(result) @@ -180,7 +92,15 @@ module SsaFlow { or result.(Impl::ExprPostUpdateNode).getExpr() = n.(PostUpdateNode).getPreUpdateNode().asExpr() or - result.(Impl::ParameterNode).getParameter().toParameterNode() = n + exists(SsaImpl::IntegrationInput::Parameter p | + p = result.(Impl::ParameterNode).getParameter() + | + n = TNormalParameterNode(p.asParameter()) + or + n = TSelfMethodParameterNode(p.asMethodSelf()) + or + n = TSelfToplevelParameterNode(p.asToplevelSelf()) + ) } predicate localFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) { @@ -190,33 +110,6 @@ module SsaFlow { predicate localMustFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) { Impl::localMustFlowStep(def, asNode(nodeFrom), asNode(nodeTo)) } - - module BarrierGuardsInput implements Impl::BarrierGuardsInputSig { - class Guard extends CfgNodes::AstCfgNode { - predicate hasCfgNode(SsaImpl::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, SsaImpl::SsaInput::BasicBlock bb, boolean branch) { - exists(ConditionBlock conditionBlock, SuccessorTypes::ConditionalSuccessor s | - guard = conditionBlock.getLastNode() and - s.getValue() = branch and - conditionBlock.controls(bb, s) - ) - } - - /** Gets an immediate conditional successor of basic block `bb`, if any. */ - SsaImpl::SsaInput::BasicBlock getAConditionalBasicBlockSuccessor( - SsaImpl::SsaInput::BasicBlock bb, boolean branch - ) { - exists(SuccessorTypes::ConditionalSuccessor s | - result = bb.getASuccessor(s) and - s.getValue() = branch - ) - } - } - - module BarrierGuardsImpl = Impl::BarrierGuards; } /** Provides predicates related to local data flow. */ @@ -360,7 +253,7 @@ module VariableCapture { or exists(Ssa::Definition def | def.getARead() = e2 and - SsaFlow::Input::ssaDefAssigns(def.getAnUltimateDefinition(), e1) + SsaImpl::IntegrationInput::ssaDefAssigns(def.getAnUltimateDefinition(), e1) ) } @@ -553,7 +446,7 @@ private module Cached { TReturningNode(CfgNodes::ReturningCfgNode n) { exists(n.getReturnedValueNode()) } or TSsaNode(SsaFlow::Impl::SsaNode node) or TCapturedVariableNode(VariableCapture::CapturedVariable v) or - TNormalParameterNode(NormalParameter p) or + TNormalParameterNode(SsaImpl::NormalParameter p) or TSelfMethodParameterNode(MethodBase m) or TSelfToplevelParameterNode(Toplevel t) or TLambdaSelfReferenceNode(Callable c) { lambdaCreationExpr(_, _, c) } or @@ -666,7 +559,7 @@ 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 SsaFlow::Input::ssaDefAssigns(def, _)) + any(SsaImpl::WriteDefinition def | not SsaImpl::IntegrationInput::ssaDefAssigns(def, _)) } pragma[nomagic] @@ -706,7 +599,8 @@ 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 SsaFlow::Input::ssaDefInitializesParam(n.(SsaDefinitionExtNode).getDefinitionExt(), _) + not SsaImpl::IntegrationInput::ssaDefInitializesParam(n.(SsaDefinitionExtNode) + .getDefinitionExt(), _) or isStoreTargetNode(n) or @@ -936,13 +830,10 @@ class SsaDefinitionNodeImpl extends SsaDefinitionExtNode { */ class SsaInputNode extends SsaNode { override SsaFlow::Impl::SsaInputNode node; - BasicBlock input; - - SsaInputNode() { node.isInputInto(def, input) } override predicate isHidden() { any() } - override CfgScope getCfgScope() { result = input.getScope() } + override CfgScope getCfgScope() { result = node.getDefinitionExt().getBasicBlock().getScope() } } /** An SSA definition for a `self` variable. */ diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll index ceef160f92650..89d077a0417f0 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 @@ -881,14 +882,14 @@ module BarrierGuard { | def.getARead() = testedNode and guardChecks(g, testedNode, branch) and - SsaFlow::BarrierGuardsInput::guardControlsBlock(g, call.getBasicBlock(), branch) and + SsaImpl::IntegrationInput::guardControlsBlock(g, call.getBasicBlock(), branch) and result.getBasicBlock().getScope() = call.getExpr().(MethodCall).getBlock() and sameSourceVariable(def, result) ) } private predicate guardChecksAdj( - SsaFlow::BarrierGuardsInput::Guard g, SsaFlow::Input::Expr e, boolean branch + SsaImpl::IntegrationInput::Guard g, SsaImpl::IntegrationInput::Expr e, boolean branch ) { guardChecks(g, e, branch) } @@ -896,7 +897,7 @@ module BarrierGuard { /** Gets a node that is safely guarded by the given guard check. */ Node getABarrierNode() { SsaFlow::asNode(result) = - SsaFlow::BarrierGuardsImpl::BarrierGuard::getABarrierNode() + SsaImpl::Integration::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 477110dbe4c6f..496bc93f8af90 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll @@ -419,6 +419,9 @@ private module Cached { Definition uncertainWriteDefinitionInput(UncertainWriteDefinition def) { Impl::uncertainWriteDefinitionInput(def, result) } + + cached + predicate forceCachingInSameStage() { Integration::localFlowStep(_, _, _) } } import Cached @@ -449,3 +452,113 @@ 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, _) + ) +} + +module IntegrationInput implements Impl::DataFlowIntegrationInputSig { + class Expr extends Cfg::CfgNodes::ExprCfgNode { + predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) } + } + + /** + * 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(WriteDefinition def, Expr value) { + def.(Ssa::WriteDefinition).assigns(value) + or + exists(Cfg::CfgNodes::ExprNodes::CaseExprCfgNode case, Cfg::CfgNodes::AstCfgNode pattern | + case.getValue() = value and + pattern = case.getBranch(_).(Cfg::CfgNodes::ExprNodes::InClauseCfgNode).getPattern() + | + def.(Ssa::WriteDefinition).getWriteAccess() = + [pattern, pattern.(Cfg::CfgNodes::ExprNodes::AsPatternCfgNode).getVariableAccess()] + ) + } + + private newtype TParameter = + TNormalParameter(NormalParameter p) or + TSelfMethodParameter(MethodBase m) or + TSelfToplevelParameter(Toplevel t) + + class Parameter extends TParameter { + NormalParameter asParameter() { this = TNormalParameter(result) } + + MethodBase asMethodSelf() { this = TSelfMethodParameter(result) } + + Toplevel asToplevelSelf() { this = TSelfToplevelParameter(result) } + + string toString() { + result = + [ + this.asParameter().toString(), this.asMethodSelf().toString(), + this.asToplevelSelf().toString() + ] + } + + Location getLocation() { + result = + [ + this.asParameter().getLocation(), this.asMethodSelf().getLocation(), + this.asToplevelSelf().getLocation() + ] + } + } + + predicate ssaDefInitializesParam(WriteDefinition def, Parameter p) { + def = getParameterDef(p.asParameter()) + or + def.(Ssa::SelfDefinition).getSourceVariable().getDeclaringScope() = + [p.asMethodSelf().(Scope), p.asToplevelSelf()] + } + + 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) { + exists(Cfg::ConditionBlock conditionBlock, Cfg::SuccessorTypes::ConditionalSuccessor s | + guard = conditionBlock.getLastNode() and + s.getValue() = branch and + conditionBlock.controls(bb, s) + ) + } + + /** 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 + ) + } +} + +module Integration = 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 6a17f8b74a9f4..f7dc0323a2a27 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 SsaFlow::Input::ssaDefAssigns(def, value) + not SsaImpl::IntegrationInput::ssaDefAssigns(def, value) ) or // operation involving `nodeFrom` diff --git a/shared/ssa/codeql/ssa/Ssa.qll b/shared/ssa/codeql/ssa/Ssa.qll index 92c2dfc237a7a..88676ea75135c 100644 --- a/shared/ssa/codeql/ssa/Ssa.qll +++ b/shared/ssa/codeql/ssa/Ssa.qll @@ -1198,6 +1198,21 @@ module Make Input> { * previous definitions or reads. */ default predicate allowFlowIntoUncertainDef(UncertainWriteDefinition def) { none() } + + /** A (potential) guard. */ + class Guard { + /** Gets a textual representation of this guard. */ + string toString(); + + /** Holds if the `i`th node of basic block `bb` evaluates this guard. */ + predicate hasCfgNode(BasicBlock bb, int i); + } + + /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */ + predicate guardControlsBlock(Guard guard, BasicBlock bb, boolean branch); + + /** Gets an immediate conditional successor of basic block `bb`, if any. */ + BasicBlock getAConditionalBasicBlockSuccessor(BasicBlock bb, boolean branch); } /** @@ -1294,21 +1309,6 @@ module Make Input> { } } - private newtype TNode = - TParamNode(DfInput::Parameter p) or - TExprNode(DfInput::Expr e, Boolean isPost) { - exists(BasicBlock bb, int i | - variableRead(bb, i, _, true) and - e.hasCfgNode(bb, i) - ) - or - isPost = false - } or - TSsaDefinitionNode(DefinitionExt def) or - TSsaInputNode(SsaInputDefinitionExt def, BasicBlock input) { - def.hasInputFromBlock(_, _, _, _, input) - } - /** * A data flow node that we need to reference in the value step relation. * @@ -1522,148 +1522,149 @@ module Make Input> { ) } - /** Holds if there is a local flow step from `nodeFrom` to `nodeTo`. */ - predicate localFlowStep(DefinitionExt def, Node nodeFrom, Node nodeTo) { - ( - // Flow from assignment into SSA definition - DfInput::ssaDefAssigns(def, nodeFrom.(ExprNode).getExpr()) + cached + private module Cached { + cached + newtype TNode = + TParamNode(DfInput::Parameter p) or + TExprNode(DfInput::Expr e, Boolean isPost) { + exists(BasicBlock bb, int i | + variableRead(bb, i, _, true) and + e.hasCfgNode(bb, i) + ) + or + isPost = false + } or + TSsaDefinitionNode(DefinitionExt def) or + TSsaInputNode(SsaInputDefinitionExt def, BasicBlock input) { + def.hasInputFromBlock(_, _, _, _, input) + } + + /** Holds if there is a local flow step from `nodeFrom` to `nodeTo`. */ + cached + predicate localFlowStep(DefinitionExt def, Node nodeFrom, Node nodeTo) { + ( + // Flow from assignment into SSA definition + DfInput::ssaDefAssigns(def, nodeFrom.(ExprNode).getExpr()) + or + // Flow from parameter into entry definition + DfInput::ssaDefInitializesParam(def, nodeFrom.(ParameterNode).getParameter()) + ) and + nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = def or - // Flow from parameter into entry definition - DfInput::ssaDefInitializesParam(def, nodeFrom.(ParameterNode).getParameter()) - ) and - nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = def - or - // Flow from SSA definition to first read - def = nodeFrom.(SsaDefinitionExtNode).getDefinitionExt() and - firstReadExt(def, nodeTo) - or - // Flow from (post-update) read to next read - adjacentReadPairExt(def, [nodeFrom, nodeFrom.(ExprPostUpdateNode).getPreUpdateNode()], nodeTo) and - nodeFrom != nodeTo - or - // Flow into phi (read) SSA definition node from def - inputFromDef(def, nodeFrom, nodeTo) - or - // Flow into phi (read) SSA definition node from (post-update) read - inputFromRead(def, [nodeFrom, nodeFrom.(ExprPostUpdateNode).getPreUpdateNode()], nodeTo) - or - // Flow from input node to def - nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = def and - def = nodeFrom.(SsaInputNode).getDefinitionExt() - } + // Flow from SSA definition to first read + def = nodeFrom.(SsaDefinitionExtNode).getDefinitionExt() and + firstReadExt(def, nodeTo) + or + // Flow from (post-update) read to next read + adjacentReadPairExt(def, [nodeFrom, nodeFrom.(ExprPostUpdateNode).getPreUpdateNode()], + nodeTo) and + nodeFrom != nodeTo + or + // Flow into phi (read) SSA definition node from def + inputFromDef(def, nodeFrom, nodeTo) + or + // Flow into phi (read) SSA definition node from (post-update) read + inputFromRead(def, [nodeFrom, nodeFrom.(ExprPostUpdateNode).getPreUpdateNode()], nodeTo) + or + // Flow from input node to def + nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = def and + def = nodeFrom.(SsaInputNode).getDefinitionExt() + } - pragma[nomagic] - private DfInput::Expr getARead(Definition def) { - exists(SourceVariable v, BasicBlock bb, int i | - ssaDefReachesRead(v, def, bb, i) and - variableRead(bb, i, v, true) and - result.hasCfgNode(bb, i) - ) - } + cached + DfInput::Expr getARead(Definition def) { + exists(SourceVariable v, BasicBlock bb, int i | + ssaDefReachesRead(v, def, bb, i) and + variableRead(bb, i, v, true) and + result.hasCfgNode(bb, i) + ) + } - /** Holds if the value of `nodeTo` is given by `nodeFrom`. */ - predicate localMustFlowStep(DefinitionExt def, Node nodeFrom, Node nodeTo) { - ( - // Flow from assignment into SSA definition - DfInput::ssaDefAssigns(def, nodeFrom.(ExprNode).getExpr()) + /** Holds if the value of `nodeTo` is given by `nodeFrom`. */ + cached + predicate localMustFlowStep(DefinitionExt def, Node nodeFrom, Node nodeTo) { + ( + // Flow from assignment into SSA definition + DfInput::ssaDefAssigns(def, nodeFrom.(ExprNode).getExpr()) + or + // Flow from parameter into entry definition + DfInput::ssaDefInitializesParam(def, nodeFrom.(ParameterNode).getParameter()) + ) and + nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = def or - // Flow from parameter into entry definition - DfInput::ssaDefInitializesParam(def, nodeFrom.(ParameterNode).getParameter()) - ) and - nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = def - or - // Flow from SSA definition to read - nodeFrom.(SsaDefinitionExtNode).getDefinitionExt() = def and - nodeTo.(ExprNode).getExpr() = getARead(def) - } + // Flow from SSA definition to read + nodeFrom.(SsaDefinitionExtNode).getDefinitionExt() = def and + nodeTo.(ExprNode).getExpr() = getARead(def) + } - /** Provides the input to `BarrierGuards`. */ - signature module BarrierGuardsInputSig { - /** A (potential) guard. */ - class Guard { - /** Gets a textual representation of this guard. */ - string toString(); + cached + predicate guardControlsSsaRead(DfInput::Guard g, boolean branch, Definition def, ExprNode n) { + exists(BasicBlock bb, DfInput::Expr e | + e = n.getExpr() and + getARead(def) = e and + DfInput::guardControlsBlock(g, bb, branch) and + e.hasCfgNode(bb, _) + ) + } - /** Holds if the `i`th node of basic block `bb` evaluates this guard. */ - predicate hasCfgNode(BasicBlock bb, int i); + cached + predicate guardControlsPhiInput( + DfInput::Guard g, boolean branch, Definition def, BasicBlock input, + SsaInputDefinitionExt phi + ) { + phi.hasInputFromBlock(def, _, _, _, input) and + ( + DfInput::guardControlsBlock(g, input, branch) + or + exists(int last | + last = input.length() - 1 and + g.hasCfgNode(input, last) and + DfInput::getAConditionalBasicBlockSuccessor(input, branch) = phi.getBasicBlock() + ) + ) } + } - /** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */ - predicate guardControlsBlock(Guard guard, BasicBlock bb, boolean branch); - - /** Gets an immediate conditional successor of basic block `bb`, if any. */ - BasicBlock getAConditionalBasicBlockSuccessor(BasicBlock bb, boolean branch); - } - - /** Provides an implementatino of the `BarrierGuard` module. */ - module BarrierGuards { - /** - * Holds if the guard `g` validates the expression `e` upon evaluating to `branch`. - * - * The expression `e` is expected to be a syntactic part of the guard `g`. - * For example, the guard `g` might be a call `isSafe(x)` and the expression `e` - * the argument `x`. - */ - signature predicate guardChecksSig(BgInput::Guard g, DfInput::Expr e, boolean branch); - - /** - * Provides a set of barrier nodes for a guard that validates an expression. - * - * This is expected to be used in `isBarrier`/`isSanitizer` definitions - * in data flow and taint tracking. - */ - module BarrierGuard { - pragma[nomagic] - private predicate guardChecksSsaDef(BgInput::Guard g, boolean branch, Definition def) { - guardChecks(g, getARead(def), branch) - } + import Cached - pragma[nomagic] - private predicate guardControlsSsaRead( - BgInput::Guard g, boolean branch, Definition def, ExprNode n - ) { - exists(BasicBlock bb, DfInput::Expr e | - e = n.getExpr() and - getARead(def) = e and - BgInput::guardControlsBlock(g, bb, branch) and - e.hasCfgNode(bb, _) - ) - } + /** + * Holds if the guard `g` validates the expression `e` upon evaluating to `branch`. + * + * The expression `e` is expected to be a syntactic part of the guard `g`. + * For example, the guard `g` might be a call `isSafe(x)` and the expression `e` + * the argument `x`. + */ + signature predicate guardChecksSig(DfInput::Guard g, DfInput::Expr e, boolean branch); - pragma[nomagic] - private predicate guardControlsPhiInput( - BgInput::Guard g, boolean branch, Definition def, BasicBlock input, - SsaInputDefinitionExt phi - ) { - phi.hasInputFromBlock(def, _, _, _, input) and - ( - BgInput::guardControlsBlock(g, input, branch) - or - exists(int last | - last = input.length() - 1 and - g.hasCfgNode(input, last) and - BgInput::getAConditionalBasicBlockSuccessor(input, branch) = phi.getBasicBlock() - ) - ) - } + /** + * Provides a set of barrier nodes for a guard that validates an expression. + * + * This is expected to be used in `isBarrier`/`isSanitizer` definitions + * in data flow and taint tracking. + */ + module BarrierGuard { + pragma[nomagic] + private predicate guardChecksSsaDef(DfInput::Guard g, boolean branch, Definition def) { + guardChecks(g, getARead(def), branch) + } - /** Gets a node that is safely guarded by the given guard check. */ - pragma[nomagic] - Node getABarrierNode() { - exists(BgInput::Guard g, boolean branch, Definition def | - guardChecksSsaDef(g, branch, def) and - guardControlsSsaRead(g, branch, def, result) - ) - or - exists( - BgInput::Guard g, boolean branch, Definition def, BasicBlock input, - SsaInputDefinitionExt phi - | - guardChecksSsaDef(g, branch, def) and - guardControlsPhiInput(g, branch, def, input, phi) and - result.(SsaInputNode).isInputInto(phi, input) - ) - } + /** Gets a node that is safely guarded by the given guard check. */ + pragma[nomagic] + Node getABarrierNode() { + exists(DfInput::Guard g, boolean branch, Definition def | + guardChecksSsaDef(g, branch, def) and + guardControlsSsaRead(g, branch, def, result) + ) + or + exists( + DfInput::Guard g, boolean branch, Definition def, BasicBlock input, + SsaInputDefinitionExt phi + | + guardChecksSsaDef(g, branch, def) and + guardControlsPhiInput(g, branch, def, input, phi) and + result.(SsaInputNode).isInputInto(phi, input) + ) } } }