diff --git a/go/ql/lib/change-notes/2026-08-17-deprecate-condition-guard-node.md b/go/ql/lib/change-notes/2026-08-17-deprecate-condition-guard-node.md new file mode 100644 index 000000000000..e31683264859 --- /dev/null +++ b/go/ql/lib/change-notes/2026-08-17-deprecate-condition-guard-node.md @@ -0,0 +1,4 @@ +--- +category: deprecated +--- +* `ControlFlow::ConditionGuardNode` is now deprecated and has no instances. Use the API from `semmle.go.controlflow.Guards` instead. \ No newline at end of file diff --git a/go/ql/lib/change-notes/2026-08-17-shared-guards.md b/go/ql/lib/change-notes/2026-08-17-shared-guards.md new file mode 100644 index 000000000000..e51d9758043b --- /dev/null +++ b/go/ql/lib/change-notes/2026-08-17-shared-guards.md @@ -0,0 +1,4 @@ +--- +category: feature +--- +* Added `semmle.go.controlflow.Guards`, including the `Guard` and `GuardValue` classes and the `guardEnsures`, `guardEnsuresEq`, `guardEnsuresNeq`, and `guardEnsuresLeq` predicates. diff --git a/go/ql/lib/semmle/go/controlflow/ControlFlowGraph.qll b/go/ql/lib/semmle/go/controlflow/ControlFlowGraph.qll index 37b81d0f4fe7..80287d7c02b9 100644 --- a/go/ql/lib/semmle/go/controlflow/ControlFlowGraph.qll +++ b/go/ql/lib/semmle/go/controlflow/ControlFlowGraph.qll @@ -258,10 +258,12 @@ module ControlFlow { } /** + * DEPRECATED: Use `Guard` from `semmle.go.controlflow.Guards` instead. + * * A control-flow node recording the fact that a certain expression has a known * Boolean value at this point in the program. */ - class ConditionGuardNode extends IR::Instruction { + deprecated class ConditionGuardNode extends IR::Instruction { Expr cond; boolean outcome; @@ -292,43 +294,69 @@ module ControlFlow { b = false } - /** Holds if this guard ensures that the result of `nd` is `b`. */ - predicate ensures(DataFlow::Node nd, boolean b) { + /** + * DEPRECATED: Use `Guard.controls` from `semmle.go.controlflow.Guards` + * instead. + * + * Holds if this guard ensures that the result of `nd` is `b`. + */ + deprecated predicate ensures(DataFlow::Node nd, boolean b) { this.ensuresAux(any(Expr e | nd = DataFlow::exprNode(e)), b) } - /** Holds if this guard ensures that `lesser <= greater + bias` holds. */ - predicate ensuresLeq(DataFlow::Node lesser, DataFlow::Node greater, int bias) { + /** + * DEPRECATED: Use `guardEnsuresLeq` from `semmle.go.controlflow.Guards` + * instead. + * + * Holds if this guard ensures that `lesser <= greater + bias` holds. + */ + deprecated predicate ensuresLeq(DataFlow::Node lesser, DataFlow::Node greater, int bias) { exists(DataFlow::RelationalComparisonNode rel, boolean b | - this.ensures(rel, b) and + this.ensuresAux(rel.asExpr(), b) and rel.leq(b, lesser, greater, bias) ) or - this.ensuresEq(lesser, greater) and + exists(DataFlow::EqualityTestNode eq, boolean b | + this.ensuresAux(eq.asExpr(), b) and + eq.eq(b, lesser, greater) + ) and bias = 0 } - /** Holds if this guard ensures that `i = j` holds. */ - predicate ensuresEq(DataFlow::Node i, DataFlow::Node j) { + /** + * DEPRECATED: Use `guardEnsuresEq` from `semmle.go.controlflow.Guards` + * instead. + * + * Holds if this guard ensures that `i = j` holds. + */ + deprecated predicate ensuresEq(DataFlow::Node i, DataFlow::Node j) { exists(DataFlow::EqualityTestNode eq, boolean b | - this.ensures(eq, b) and + this.ensuresAux(eq.asExpr(), b) and eq.eq(b, i, j) ) } - /** Holds if this guard ensures that `i != j` holds. */ - predicate ensuresNeq(DataFlow::Node i, DataFlow::Node j) { + /** + * DEPRECATED: Use `guardEnsuresNeq` from `semmle.go.controlflow.Guards` + * instead. + * + * Holds if this guard ensures that `i != j` holds. + */ + deprecated predicate ensuresNeq(DataFlow::Node i, DataFlow::Node j) { exists(DataFlow::EqualityTestNode eq, boolean b | - this.ensures(eq, b.booleanNot()) and + this.ensuresAux(eq.asExpr(), b.booleanNot()) and eq.eq(b, i, j) ) } /** + * DEPRECATED: Use `Guard.controls` from `semmle.go.controlflow.Guards` + * instead. + * * Holds if this guard dominates basic block `bb`, that is, the guard * is known to hold at `bb`. */ - predicate dominates(ReachableBasicBlock bb) { + deprecated predicate dominates(ReachableBasicBlock bb) { this = bb.getANode() or this.dominates(bb.getImmediateDominator()) } diff --git a/go/ql/lib/semmle/go/controlflow/Guards.qll b/go/ql/lib/semmle/go/controlflow/Guards.qll new file mode 100644 index 000000000000..e7c7a241acad --- /dev/null +++ b/go/ql/lib/semmle/go/controlflow/Guards.qll @@ -0,0 +1,403 @@ +/** + * Provides classes and predicates for reasoning about guards and the control + * flow elements controlled by those guards. + * + * This is an instantiation of the shared guards library for Go. + */ +overlay[local?] +module; + +private import go +private import semmle.go.controlflow.ControlFlowGraphImpl +private import semmle.go.dataflow.SSA as GoSsa +private import semmle.go.dataflow.SsaImpl as SsaImpl +private import codeql.controlflow.Guards as SharedGuards +private import codeql.controlflow.SuccessorType + +private module GuardsInput implements + SharedGuards::InputSig +{ + private import go as G + + class NormalExitNode = CfgImpl::ControlFlow::NormalExitNode; + + class AstNode = G::AstNode; + + class Expr extends G::Expr { + /** Gets the associated control flow node. */ + CfgImpl::Cfg::ControlFlowNode getControlFlowNode() { result = IR::evalExprInstruction(this) } + + /** Gets the basic block containing this expression. */ + CfgImpl::Cfg::BasicBlock getBasicBlock() { result = this.getControlFlowNode().getBasicBlock() } + } + + predicate booleanOutcomeBlock(Expr guard, CfgImpl::Cfg::BasicBlock outcomeBlock, boolean branch) { + exists(CfgImpl::Cfg::ControlFlowNode outcomeNode | + branch = true and outcomeNode.isAfterTrue(guard) + or + branch = false and outcomeNode.isAfterFalse(guard) + | + outcomeBlock = outcomeNode.getBasicBlock() + ) + } + + private newtype TConstantValue = TStringValue(string s) { s = any(G::Expr e).getStringValue() } + + class ConstantValue extends TConstantValue { + /** Gets a textual representation of this constant value. */ + string toString() { this = TStringValue(result) } + } + + abstract class ConstantExpr extends Expr { + predicate isNull() { none() } + + boolean asBooleanValue() { none() } + + int asIntegerValue() { none() } + + ConstantValue asConstantValue() { none() } + } + + private class NilConstant extends ConstantExpr { + NilConstant() { exprRefersToNil(this) } + + override predicate isNull() { any() } + } + + private class BooleanConstant extends ConstantExpr { + BooleanConstant() { exists(this.getBoolValue()) } + + override boolean asBooleanValue() { result = this.getBoolValue() } + } + + private class IntegerConstant extends ConstantExpr { + IntegerConstant() { exists(this.getIntValue()) } + + override int asIntegerValue() { result = this.getIntValue() } + } + + private class StringConstant extends ConstantExpr { + StringConstant() { exists(this.getStringValue()) } + + override ConstantValue asConstantValue() { result = TStringValue(this.getStringValue()) } + } + + /** + * An expression that is known not to be `nil`. + */ + class NonNullExpr extends Expr { + NonNullExpr() { + this instanceof G::CompositeLit + or + this instanceof G::FuncLit + or + this instanceof G::AddressExpr + } + } + + /** + * A case expression in an expression `switch` statement. + */ + class Case extends Expr { + G::ExpressionSwitchStmt switch; + + Case() { this = switch.getACase().getAnExpr() } + + Expr getSwitchExpr() { + result = switch.getExpr() + or + not exists(switch.getExpr()) and result = this + } + + predicate isDefaultCase() { none() } + + ConstantExpr asConstantCase() { exists(switch.getExpr()) and result = this } + + predicate matchEdge(CfgImpl::Cfg::BasicBlock bb1, CfgImpl::Cfg::BasicBlock bb2) { + bb1.getLastNode() = this.getControlFlowNode() and + bb1.getASuccessor(any(MatchingSuccessor successor | successor.getValue() = true)) = bb2 + } + + predicate nonMatchEdge(CfgImpl::Cfg::BasicBlock bb1, CfgImpl::Cfg::BasicBlock bb2) { + bb1.getLastNode() = this.getControlFlowNode() and + bb1.getASuccessor(any(MatchingSuccessor successor | successor.getValue() = false)) = bb2 + } + } + + class AndExpr extends Expr instanceof G::LandExpr { + /** Gets an operand of this expression. */ + Expr getAnOperand() { result = super.getAnOperand() } + } + + class OrExpr extends Expr instanceof G::LorExpr { + /** Gets an operand of this expression. */ + Expr getAnOperand() { result = super.getAnOperand() } + } + + class NotExpr extends Expr instanceof G::NotExpr { + /** Gets the operand of this expression. */ + Expr getOperand() { result = super.getOperand() } + } + + private predicate sameNumericTypeFamily(G::NumericType source, G::NumericType target) { + source instanceof G::SignedIntegerType and target instanceof G::SignedIntegerType + or + source instanceof G::UnsignedIntegerType and target instanceof G::UnsignedIntegerType + or + source instanceof G::FloatType and target instanceof G::FloatType + or + source instanceof G::ComplexType and target instanceof G::ComplexType + } + + private predicate isUpcast(G::ConversionExpr conversion) { + conversion.getOperand().getType().getUnderlyingType() = conversion.getType().getUnderlyingType() + or + exists(G::NumericType source, G::NumericType target | + source = conversion.getOperand().getType().getUnderlyingType() and + target = conversion.getType().getUnderlyingType() and + sameNumericTypeFamily(source, target) and + source.getSize() <= target.getSize() + ) + } + + /** + * An expression that has the same value as a specific sub-expression, that + * is, a parenthesized expression or an upcast. + */ + class IdExpr extends Expr { + IdExpr() { this instanceof G::ParenExpr or isUpcast(this) } + + Expr getEqualChildExpr() { + result = this.(G::ParenExpr).getExpr() + or + result = this.(G::ConversionExpr).getOperand() + } + } + + /** + * Holds if `eqtest` is an equality or inequality test between `left` and + * `right`. The `polarity` indicates whether this is an equality test (true) + * or inequality test (false). + */ + pragma[nomagic] + predicate equalityTest(Expr eqtest, Expr left, Expr right, boolean polarity) { + exists(G::EqualityTestExpr eq | eq = eqtest | + left = eq.getLeftOperand() and + right = eq.getRightOperand() and + polarity = eq.getPolarity() + ) + } + + /** + * A conditional expression. Go has no such expression, so this class is + * empty. + */ + class ConditionalExpr extends Expr { + ConditionalExpr() { none() } + + /** Gets the condition of this expression. */ + Expr getCondition() { none() } + + /** Gets the true branch of this expression. */ + Expr getThen() { none() } + + /** Gets the false branch of this expression. */ + Expr getElse() { none() } + } + + class Parameter = G::Parameter; + + private int parameterPosition() { result = any(Parameter p).getIndex() } + + /** A parameter position represented by an integer. */ + class ParameterPosition extends int { + ParameterPosition() { this = parameterPosition() } + } + + /** An argument position represented by an integer. */ + class ArgumentPosition extends int { + ArgumentPosition() { this = parameterPosition() } + } + + /** Holds if arguments at position `apos` match parameters at position `ppos`. */ + pragma[inline] + predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos = apos } + + final private class FinalFunction = G::Function; + + /** + * A function whose calls always dispatch to that same function. + * + * Methods are excluded, since a call to a method may dispatch to a different + * implementation via an interface. + */ + class NonOverridableMethod extends FinalFunction { + NonOverridableMethod() { + not this instanceof G::Method and + exists(super.getFuncDecl()) and + super.getNumResult() = 1 + } + + Parameter getParameter(ParameterPosition ppos) { result = super.getParameter(ppos) } + + /** Gets an expression being returned by this function. */ + Expr getAReturnExpr() { + exists(G::ReturnStmt ret | + ret.getEnclosingFunction() = super.getFuncDecl() and + result = ret.getExpr() + ) + } + } + + private predicate nonOverridableCall(G::CallExpr call, NonOverridableMethod m) { + call = m.getACall().asExpr() + } + + class NonOverridableMethodCall extends Expr instanceof G::CallExpr { + NonOverridableMethodCall() { nonOverridableCall(this, _) } + + NonOverridableMethod getMethod() { nonOverridableCall(this, result) } + + Expr getArgument(ArgumentPosition apos) { result = super.getArgument(apos) } + } +} + +private module GuardsImpl = SharedGuards::Make; + +private module LogicInput implements GuardsImpl::LogicInputSig { + final private class FinalSsaDefinition = GoSsa::SsaDefinition; + + class SsaDefinition extends FinalSsaDefinition { + GuardsInput::Expr getARead() { + result = super.getVariable().getAUse().(IR::EvalInstruction).getExpr() + } + } + + class SsaExplicitWrite extends SsaDefinition instanceof GoSsa::SsaExplicitDefinition { + GuardsInput::Expr getValue() { result = super.getRhs().(IR::EvalInstruction).getExpr() } + } + + class SsaPhiDefinition extends SsaDefinition instanceof GoSsa::SsaPhiNode { + /** Holds if `inp` is an input to the phi node along the edge originating in `bb`. */ + predicate hasInputFromBlock(SsaDefinition inp, BasicBlock bb) { + SsaImpl::phiHasInputFromBlock(this, inp, bb) + } + } + + class SsaParameterInit extends SsaDefinition { + SsaParameterInit() { + this.(GoSsa::SsaExplicitDefinition).getInstruction() instanceof IR::InitParameterInstruction + } + + GuardsInput::Parameter getParameter() { + this.(GoSsa::SsaExplicitDefinition).getInstruction() = IR::initParamInstruction(result) + } + } + + /** + * Holds if `rel` evaluating to `branch` ensures that `lesser` is less than + * `greater`, strictly if `strict` is true. + */ + private predicate comparison( + RelationalComparisonExpr rel, boolean branch, GuardsInput::Expr lesser, + GuardsInput::Expr greater, boolean strict + ) { + branch = true and + lesser = rel.getLesserOperand() and + greater = rel.getGreaterOperand() and + (if rel.isStrict() then strict = true else strict = false) + or + branch = false and + lesser = rel.getGreaterOperand() and + greater = rel.getLesserOperand() and + (if rel.isStrict() then strict = false else strict = true) + } + + /** + * Holds if `guard` evaluating to `val` ensures that: + * `e <= k` when `upper = true` + * `e >= k` when `upper = false` + */ + predicate rangeGuard( + GuardsImpl::PreGuard guard, GuardValue val, GuardsInput::Expr e, int k, boolean upper + ) { + exists( + RelationalComparisonExpr rel, boolean branch, GuardsInput::Expr lesser, + GuardsInput::Expr greater, boolean strict, int strictnessAdjustment + | + guard = rel and + val.asBooleanValue() = branch and + comparison(rel, branch, lesser, greater, strict) and + (if strict = true then strictnessAdjustment = 1 else strictnessAdjustment = 0) + | + // `e < k` or `e <= k` + e = lesser and + upper = true and + k = greater.getIntValue() - strictnessAdjustment + or + // `k < e` or `k <= e` + e = greater and + upper = false and + k = lesser.getIntValue() + strictnessAdjustment + ) + } +} + +/** An abstract value that a `Guard` may evaluate to. */ +class GuardValue = GuardsImpl::GuardValue; + +private module GuardsLogic = GuardsImpl::Logic; + +/** + * A guard. This is an expression whose value determines subsequent control + * flow. + */ +final class Guard extends GuardsLogic::Guard { + /** Gets the innermost function or file to which this guard belongs. */ + ControlFlow::Root getRoot() { result.isRootOf(this) } +} + +/** + * Provides a set of barrier nodes for a guard that validates an expression. + */ +module ValidationWrapper { + import GuardsLogic::ValidationWrapper +} + +/** + * Holds if `bb` can only be reached when the expression `e` evaluates to `b`. + * + * This is the replacement for the old + * `ConditionGuardNode.ensures(e, b) and ConditionGuardNode.dominates(bb)` + * idiom. + */ +pragma[inline] +predicate guardEnsures(Expr e, boolean b, BasicBlock bb) { e.(Guard).controls(bb, b) } + +/** Holds if `guard` evaluating to `branch` ensures that `i = j` holds. */ +predicate guardEnsuresEq(Guard guard, boolean branch, DataFlow::Node i, DataFlow::Node j) { + guard.isEquality(i.asExpr(), j.asExpr(), branch) +} + +/** Holds if `guard` evaluating to `branch` ensures that `i != j` holds. */ +predicate guardEnsuresNeq(Guard guard, boolean branch, DataFlow::Node i, DataFlow::Node j) { + exists(boolean eqval | + guard.isEquality(i.asExpr(), j.asExpr(), eqval) and + branch = eqval.booleanNot() + ) +} + +/** + * Holds if `guard` evaluating to `branch` ensures that `lesser <= greater + bias` + * holds. + */ +predicate guardEnsuresLeq( + Guard guard, boolean branch, DataFlow::Node lesser, DataFlow::Node greater, int bias +) { + exists(DataFlow::RelationalComparisonNode rel | + guard = rel.asExpr() and + rel.leq(branch, lesser, greater, bias) + ) + or + guardEnsuresEq(guard, branch, lesser, greater) and bias = 0 +} diff --git a/go/ql/lib/semmle/go/controlflow/IR.qll b/go/ql/lib/semmle/go/controlflow/IR.qll index a87f1464de12..7e9ace19f4c1 100644 --- a/go/ql/lib/semmle/go/controlflow/IR.qll +++ b/go/ql/lib/semmle/go/controlflow/IR.qll @@ -32,22 +32,6 @@ module IR { n.isAfterValue(cc, any(MatchingSuccessor t | t.isMatch())) } - /** - * Holds if `n` records a boolean outcome, or the matching outcome of an - * expressionless switch case condition. - */ - private predicate isConditionGuardNode(ControlFlow::Node n) { - n.isAfterTrue(_) - or - n.isAfterFalse(_) - or - exists(Expr condition, MatchingSuccessor successor | - condition = - any(ExpressionSwitchStmt switch | not exists(switch.getExpr())).getACase().getAnExpr() and - n.isAfterValue(condition, successor) - ) - } - /** Gets the CFG node representing a basic literal, function literal, or plain identifier reference. */ cached private ControlFlow::Node leafEvaluation(Expr leaf) { @@ -72,17 +56,17 @@ module IR { or this.isAdditional(_, _) or - isConditionGuardNode(this) - or // The successful-match node of a type-switch case that binds an implicit // variable hosts that variable's declaration/assignment (see // `TypeSwitchImplicitVariableInstruction`). typeSwitchCaseMatch(this, _) or // `NotExpr` and `LogicalBinaryExpr` are not in `postOrInOrder`, so they - // have no `isIn` node. Use their combined after-node as the value-producing - // instruction, but not a value-specific after-node, which is already a - // `ConditionGuardInstruction`. + // have no `isIn` node. When such an expression is not in a conditional + // context (so it has a single combined after-node rather than per-branch + // value-after-nodes), use that after-node as the value-producing + // instruction. In conditional contexts the value is already split + // across branches, so no separate value instruction is needed. exists(Expr e | (e instanceof NotExpr or e instanceof LogicalBinaryExpr) and this.isAfter(e) and @@ -182,8 +166,6 @@ module IR { or this instanceof GoInstruction and result = "go" or - this instanceof ConditionGuardInstruction and result = "condition guard" - or this instanceof ReturnInstruction and result = "return" or this instanceof WriteResultInstruction and result = "result write" @@ -207,11 +189,6 @@ module IR { } } - /** A condition guard instruction, representing a known boolean outcome for a condition. */ - private class ConditionGuardInstruction extends Instruction { - ConditionGuardInstruction() { isConditionGuardNode(this) } - } - /** * An IR instruction representing the evaluation of an expression. */ diff --git a/go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll b/go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll index e65b2493dd9b..056d8edd5d9f 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll @@ -2,6 +2,7 @@ overlay[local?] module; private import go +private import semmle.go.controlflow.Guards private import DataFlowUtil private import DataFlowImplCommon private import ContainerFlow @@ -390,19 +391,21 @@ private class ConstantBooleanArgumentNode extends ArgumentNode, ExprNode { } /** - * Returns a guard that will certainly not hold in calling context `call`. + * Holds if `guard` evaluating to `branch` will certainly not happen in calling + * context `call`. * * In particular it does not hold because it checks that `param` has value `b`, but * in context `call` it is known to have value `!b`. Note this is `noinline`d in order * to avoid a bad join order in `isUnreachableInCall`. */ pragma[noinline] -private ControlFlow::ConditionGuardNode getAFalsifiedGuard(DataFlowCall call) { +private predicate falsifiedGuard(DataFlowCall call, Guard guard, boolean branch) { exists(SsaParameterNode param, ConstantBooleanArgumentNode arg | // get constant bool argument and parameter for this call viableParamArg(call, pragma[only_bind_into](param), pragma[only_bind_into](arg)) and // which is used in a guard controlling `n` with the opposite value of `arg` - result.ensures(param.getAUse(), arg.getBooleanValue().booleanNot()) + guard = param.getAUse().asExpr() and + branch = arg.getBooleanValue().booleanNot() ) } @@ -416,7 +419,10 @@ class NodeRegion instanceof BasicBlock { * Holds if the nodes in `nr` are unreachable when the call context is `call`. */ predicate isUnreachableInCall(NodeRegion nr, DataFlowCall call) { - getAFalsifiedGuard(call).dominates(nr) + exists(Guard guard, boolean branch | + falsifiedGuard(call, guard, branch) and + guard.controls(nr, branch) + ) } /** diff --git a/go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll b/go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll index 98e7d90667e0..3845726917e9 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -5,6 +5,7 @@ overlay[local?] module; private import go +private import semmle.go.controlflow.Guards private import semmle.go.dataflow.FunctionInputsAndOutputs private import semmle.go.dataflow.ExternalFlow private import DataFlowPrivate @@ -388,11 +389,11 @@ module BarrierGuard { module ParameterizedBarrierGuard::guardChecksSig/4 guardChecks> { /** Gets a node that is safely guarded by the given guard check. */ Node getABarrierNode(P param) { - exists(ControlFlow::ConditionGuardNode guard, SsaWithFields var | + exists(Guard guard, boolean branch, SsaWithFields var | result = pragma[only_bind_out](var).getAUse() | - guards(_, guard, _, var, param) and - pragma[only_bind_out](guard).dominates(result.getBasicBlock()) + guards(_, guard, branch, _, var, param) and + pragma[only_bind_out](guard).controls(result.getBasicBlock(), branch) ) } @@ -400,39 +401,35 @@ module ParameterizedBarrierGuard::guardChecksSig/4 guar * Gets a node that is safely guarded by the given guard check. */ Node getABarrierNodeForGuard(Node guardCheck, P param) { - exists(ControlFlow::ConditionGuardNode guard, SsaWithFields var | result = var.getAUse() | - guards(guardCheck, guard, _, var, param) and - guard.dominates(result.getBasicBlock()) + exists(Guard guard, boolean branch, SsaWithFields var | result = var.getAUse() | + guards(guardCheck, guard, branch, _, var, param) and + guard.controls(result.getBasicBlock(), branch) ) } /** - * Holds if `guard` marks a point in the control-flow graph where `g` - * is known to validate `nd`, which is represented by `ap`. + * Holds if `guard` evaluating to `branch` marks a point in the control-flow + * graph where `g` is known to validate `nd`, which is represented by `ap`. * * This predicate exists to enforce a good join order in `getAGuardedNode`. */ pragma[noinline] - private predicate guards( - Node g, ControlFlow::ConditionGuardNode guard, Node nd, SsaWithFields ap, P param - ) { - guards(g, guard, nd, param) and nd = ap.getAUse() + private predicate guards(Node g, Guard guard, boolean branch, Node nd, SsaWithFields ap, P param) { + guards(g, guard, branch, nd, param) and nd = ap.getAUse() } /** - * Holds if `guard` marks a point in the control-flow graph where `g` - * is known to validate `nd`. + * Holds if `guard` evaluating to `branch` marks a point in the control-flow + * graph where `g` is known to validate `nd`. */ - private predicate guards(Node g, ControlFlow::ConditionGuardNode guard, Node nd, P param) { - exists(boolean branch | - guardChecks(g, nd.asExpr(), branch, param) and - guard.ensures(g, branch) - ) + private predicate guards(Node g, Guard guard, boolean branch, Node nd, P param) { + guardChecks(g, nd.asExpr(), branch, param) and + guard = g.asExpr() or - exists(DataFlow::Property p, Node resNode, Node check, boolean outcome | + exists(DataFlow::Property p, Node resNode, Node check | guardingCall(g, _, _, _, p, _, nd, resNode, param) and - p.checkOn(check, outcome, resNode) and - guard.ensures(pragma[only_bind_into](check), outcome) + p.checkOn(check, branch, resNode) and + guard = pragma[only_bind_into](check).asExpr() ) } @@ -487,9 +484,9 @@ module ParameterizedBarrierGuard::guardChecksSig/4 guar localFlow(inp.getExitNode(fd), pragma[only_bind_out](arg)) and ( // Case: a function like "if someBarrierGuard(arg) { return true } else { return false }" - exists(ControlFlow::ConditionGuardNode guard | - guards(g, pragma[only_bind_out](guard), arg, param) and - guard.dominates(pragma[only_bind_out](ret).getBasicBlock()) + exists(Guard guard, boolean branch | + guards(g, pragma[only_bind_out](guard), branch, arg, param) and + guard.controls(pragma[only_bind_out](ret).getBasicBlock(), branch) | onlyPossibleReturnSatisfyingProperty(fd, outp, ret, p) ) diff --git a/go/ql/lib/semmle/go/security/InsecureFeatureFlag.qll b/go/ql/lib/semmle/go/security/InsecureFeatureFlag.qll index 293f78507cc3..ba45e7b520c0 100644 --- a/go/ql/lib/semmle/go/security/InsecureFeatureFlag.qll +++ b/go/ql/lib/semmle/go/security/InsecureFeatureFlag.qll @@ -3,6 +3,7 @@ */ import go +private import semmle.go.controlflow.Guards /** * Provides classes and predicates relating to flags that may indicate security expectations. @@ -114,9 +115,23 @@ module InsecureFeatureFlag { } /** - * Gets a control-flow node that represents a (likely) security feature-flag check + * Holds if `block` is controlled by a flag of kind `flagKind`. + * + * For a switch case expression, only the matching branch is controlled by that flag. Other + * branches, including the default case, are reached when the flag does not match. */ - ControlFlow::ConditionGuardNode getASecurityFeatureFlagCheck() { - result.ensures(any(SecurityFeatureFlag f).getAFlag().getANode(), _) + predicate flagControls(FlagKind flagKind, BasicBlock block) { + exists(GVN flag, Guard guard, boolean branch | + flag = flagKind.getAFlag() and + guard = flag.getANode().asExpr() and + guard.controls(block, branch) and + ( + branch = true + or + not exists(Expr caseExpr | + caseExpr = flag.getANode().asExpr() and caseExpr.getParent() instanceof CaseClause + ) + ) + ) } } diff --git a/go/ql/src/InconsistentCode/ConstantLengthComparison.ql b/go/ql/src/InconsistentCode/ConstantLengthComparison.ql index d0bcec7a89cb..6b722d887ae6 100644 --- a/go/ql/src/InconsistentCode/ConstantLengthComparison.ql +++ b/go/ql/src/InconsistentCode/ConstantLengthComparison.ql @@ -13,10 +13,11 @@ */ import go +private import semmle.go.controlflow.Guards from - ForStmt fs, Variable i, DataFlow::ElementReadNode idx, GVN a, - ControlFlow::ConditionGuardNode cond, DataFlow::CallNode lenA + ForStmt fs, Variable i, DataFlow::ElementReadNode idx, GVN a, Guard cond, boolean branch, + DataFlow::CallNode lenA where // `i` is incremented in `fs` fs.getPost().(IncStmt).getOperand() = i.getAReference() and @@ -27,11 +28,11 @@ where lenA.getArgument(0) = a.getANode() and // and is checked against a constant exists(DataFlow::Node const | exists(const.getIntValue()) | - cond.ensuresNeq(lenA, const) or - cond.ensuresLeq(const, lenA, _) + guardEnsuresNeq(cond, branch, lenA, const) or + guardEnsuresLeq(cond, branch, const, lenA, _) ) and - cond.dominates(idx.getBasicBlock()) and + cond.controls(idx.getBasicBlock(), branch) and // and that check happens inside the loop body - cond.getCondition().getParent+() = fs -select cond.getCondition(), "This checks the length against a constant, but it $@.", idx, + cond.(Expr).getParent+() = fs +select cond, "This checks the length against a constant, but it $@.", idx, "is indexed using a variable" diff --git a/go/ql/src/InconsistentCode/LengthComparisonOffByOne.ql b/go/ql/src/InconsistentCode/LengthComparisonOffByOne.ql index 176e34bc9bbb..8b8fdd0cfac9 100644 --- a/go/ql/src/InconsistentCode/LengthComparisonOffByOne.ql +++ b/go/ql/src/InconsistentCode/LengthComparisonOffByOne.ql @@ -13,6 +13,7 @@ */ import go +private import semmle.go.controlflow.Guards newtype TIndex = VariableIndex(DataFlow::SsaNode v) { v.getAUse() = any(DataFlow::ElementReadNode e).getIndex() } or @@ -41,21 +42,23 @@ DataFlow::CallNode arrayLen(DataFlow::SsaNode array) { } /** - * Gets a condition that checks that `index` is less than or equal to `array.length`. + * Holds if `guard` evaluating to `branch` checks that `index` is less than or + * equal to `array.length`. */ -ControlFlow::ConditionGuardNode getLengthLEGuard(Index index, DataFlow::SsaNode array) { - result.ensuresLeq(getAUse(index), arrayLen(array), 0) +predicate lengthLeGuard(Guard guard, boolean branch, Index index, DataFlow::SsaNode array) { + guardEnsuresLeq(guard, branch, getAUse(index), arrayLen(array), 0) or exists(int i, int bias | index = ConstantIndex(i) | - result.ensuresLeq(getAUse(ConstantIndex(i + bias)), arrayLen(array), bias) + guardEnsuresLeq(guard, branch, getAUse(ConstantIndex(i + bias)), arrayLen(array), bias) ) } /** - * Gets a condition that checks that `index` is not equal to `array.length`. + * Holds if `guard` evaluating to `branch` checks that `index` is not equal to + * `array.length`. */ -ControlFlow::ConditionGuardNode getLengthNEGuard(Index index, DataFlow::SsaNode array) { - result.ensuresNeq(getAUse(index), arrayLen(array)) +predicate lengthNeGuard(Guard guard, boolean branch, Index index, DataFlow::SsaNode array) { + guardEnsuresNeq(guard, branch, getAUse(index), arrayLen(array)) } /** @@ -78,23 +81,24 @@ predicate isRegexpMethodCall(DataFlow::MethodCallNode c) { } from - ControlFlow::ConditionGuardNode cond, DataFlow::SsaNode array, Index index, - DataFlow::ElementReadNode ea, BasicBlock bb + Guard cond, boolean branch, DataFlow::SsaNode array, Index index, DataFlow::ElementReadNode ea, + BasicBlock bb where // there is a comparison `index <= len(array)` - cond = getLengthLEGuard(index, array) and + lengthLeGuard(cond, branch, index, array) and // there is a read from `array[index]` elementRead(ea, array, index, bb) and // and the read is guarded by the comparison - cond.dominates(bb) and + cond.controls(bb, branch) and // but the read is not guarded by another check that `index != len(array)` - not getLengthNEGuard(index, array).dominates(bb) and + not exists(Guard ne, boolean neBranch | + lengthNeGuard(ne, neBranch, index, array) and ne.controls(bb, neBranch) + ) and // and it is not additionally guarded by a stronger index check - not exists(Index index2, int i, int i2 | + not exists(Index index2, int i, int i2, Guard g2, boolean b2 | index = ConstantIndex(i) and index2 = ConstantIndex(i2) and i < i2 | - getLengthLEGuard(index2, array).dominates(bb) + lengthLeGuard(g2, b2, index2, array) and g2.controls(bb, b2) ) and not isRegexpMethodCall(array.getInit()) -select cond.getCondition(), - "Off-by-one index comparison against length may lead to out-of-bounds $@.", ea, "read" +select cond, "Off-by-one index comparison against length may lead to out-of-bounds $@.", ea, "read" diff --git a/go/ql/src/Security/CWE-020/IncompleteHostnameRegexp.ql b/go/ql/src/Security/CWE-020/IncompleteHostnameRegexp.ql index a6321b7d7cb3..76b694e3bfe9 100644 --- a/go/ql/src/Security/CWE-020/IncompleteHostnameRegexp.ql +++ b/go/ql/src/Security/CWE-020/IncompleteHostnameRegexp.ql @@ -13,6 +13,7 @@ */ import go +private import semmle.go.controlflow.Guards /** * Holds if `pattern` is a regular expression pattern for URLs with a host matched by `hostPart`, @@ -70,12 +71,12 @@ predicate regexpGuardsHandler(RegexpPattern regexp, Http::RequestHandler handler /** Holds if `regexp` guards an HTTP error write. */ predicate regexpGuardsError(RegexpPattern regexp) { - exists(ControlFlow::ConditionGuardNode cond, RegexpMatchFunction match, DataFlow::CallNode call | + exists(Guard cond, RegexpMatchFunction match, DataFlow::CallNode call | call.getTarget() = match and match.getRegexp(call) = regexp | - cond.ensures(match.getResult().getNode(call).getASuccessor*(), true) and - cond.dominates(any(ReachableBasicBlock b | writesHttpError(b))) + cond = match.getResult().getNode(call).getASuccessor*().asExpr() and + cond.controls(any(ReachableBasicBlock b | writesHttpError(b)), true) ) } diff --git a/go/ql/src/Security/CWE-209/StackTraceExposure.ql b/go/ql/src/Security/CWE-209/StackTraceExposure.ql index 45d58f442c32..d4aa8ff21e79 100644 --- a/go/ql/src/Security/CWE-209/StackTraceExposure.ql +++ b/go/ql/src/Security/CWE-209/StackTraceExposure.ql @@ -15,6 +15,7 @@ import go import semmle.go.security.InsecureFeatureFlag::InsecureFeatureFlag +private import semmle.go.controlflow.Guards /** * A flag indicating the program is in debug or development mode, or that stack @@ -56,10 +57,8 @@ module StackTraceExposureConfig implements DataFlow::ConfigSig { // Sanitize everything controlled by an is-debug-mode check. // Imprecision: I don't try to guess which arm of a branch is intended // to mean debug mode, and which is production mode. - exists(ControlFlow::ConditionGuardNode cgn | - cgn.ensures(any(DebugModeFlag f).getAFlag().getANode(), _) - | - cgn.dominates(node.getBasicBlock()) + exists(Guard g | g = any(DebugModeFlag f).getAFlag().getANode().asExpr() | + g.controls(node.getBasicBlock(), _) ) } diff --git a/go/ql/src/Security/CWE-295/DisabledCertificateCheck.ql b/go/ql/src/Security/CWE-295/DisabledCertificateCheck.ql index bc05c8cf4aa8..80b19196cc1b 100644 --- a/go/ql/src/Security/CWE-295/DisabledCertificateCheck.ql +++ b/go/ql/src/Security/CWE-295/DisabledCertificateCheck.ql @@ -24,6 +24,7 @@ import go import semmle.go.security.InsecureFeatureFlag::InsecureFeatureFlag +private import semmle.go.controlflow.Guards /** * Holds if `part` becomes a part of `whole`, either by (local) data flow or by being incorporated @@ -49,13 +50,6 @@ class InsecureCertificateFlag extends FlagKind { } } -/** - * Gets a control-flow node that represents a (likely) flag controlling an insecure certificate setup. - */ -ControlFlow::ConditionGuardNode getAnInsecureCertificateCheck() { - result.ensures(any(InsecureCertificateFlag f).getAFlag().getANode(), _) -} - /** * Returns flag kinds relevant to this query: a generic security feature flag, or one * specifically controlling insecure certificate configuration. @@ -80,7 +74,7 @@ where f.hasQualifiedName("crypto/tls", "Config", "InsecureSkipVerify") and rhs.getBoolValue() = true and // exclude writes guarded by a feature flag - not [getASecurityFeatureFlagCheck(), getAnInsecureCertificateCheck()].dominatesNode(w) and + not flagControls(securityOrTlsVersionFlag(), w.getBasicBlock()) and // exclude results in functions whose name documents the insecurity not exists(FuncDef fn | fn = w.getRoot() | isSecurityOrCertificateConfigFlag(fn.getEnclosingFunction*().getName()) diff --git a/go/ql/src/Security/CWE-327/InsecureTLS.ql b/go/ql/src/Security/CWE-327/InsecureTLS.ql index b5d8a81f3d82..559af3060c1d 100644 --- a/go/ql/src/Security/CWE-327/InsecureTLS.ql +++ b/go/ql/src/Security/CWE-327/InsecureTLS.ql @@ -13,6 +13,7 @@ import go import semmle.go.security.InsecureFeatureFlag::InsecureFeatureFlag +private import semmle.go.controlflow.Guards /** * Holds if it is insecure to assign TLS version `val` named `name` to `tls.Config` field `fieldName`. @@ -246,13 +247,6 @@ class LegacyTlsVersionFlag extends FlagKind { override string getAFlagName() { result.regexpMatch("(?i).*(old|intermediate|legacy).*") } } -/** - * Gets a control-flow node that represents a (likely) flag controlling TLS version selection. - */ -ControlFlow::ConditionGuardNode getALegacyTlsVersionCheck() { - result.ensures(any(LegacyTlsVersionFlag f).getAFlag().getANode(), _) -} - /** * Returns flag kinds relevant to this query: a generic security feature flag, or one * specifically controlling TLS version selection. @@ -275,8 +269,7 @@ where isInsecureTlsCipherFlow(source.asPathNode2(), sink.asPathNode2(), message) ) and // Exclude sources or sinks guarded by a feature or legacy flag - not [getASecurityFeatureFlagCheck(), getALegacyTlsVersionCheck()] - .dominatesNode([source, sink].getNode().asInstruction()) and + not flagControls(securityOrTlsVersionFlag(), [source, sink].getNode().getBasicBlock()) and // Exclude sources or sinks that occur lexically within a block related to a feature or legacy flag not astNodeIsFlag([source, sink].getNode().asExpr().getParent*(), securityOrTlsVersionFlag()) and // Exclude results in functions whose name documents insecurity diff --git a/go/ql/src/experimental/CWE-807/SensitiveConditionBypass.ql b/go/ql/src/experimental/CWE-807/SensitiveConditionBypass.ql index 554e271492e4..7770b05f1e22 100644 --- a/go/ql/src/experimental/CWE-807/SensitiveConditionBypass.ql +++ b/go/ql/src/experimental/CWE-807/SensitiveConditionBypass.ql @@ -14,20 +14,18 @@ import go import SensitiveConditionBypass +private import semmle.go.controlflow.Guards from - ControlFlow::ConditionGuardNode guard, DataFlow::Node sensitiveSink, - SensitiveExpr::Classification classification, DataFlow::Node source, DataFlow::Node operand, - ComparisonExpr comp + DataFlow::Node sensitiveSink, SensitiveExpr::Classification classification, DataFlow::Node source, + DataFlow::Node operand, ComparisonExpr comp where // there should be a flow between source and the operand sink Flow::flow(source, operand) and // both the operand should belong to the same comparison expression operand.asExpr() = comp.getAnOperand() and - // get the ConditionGuardNode corresponding to the comparison expr. - guard.getCondition() = comp and // the sink `sensitiveSink` should be sensitive, isSensitive(sensitiveSink, classification) and - // the guard should control the sink - guard.dominates(sensitiveSink.getBasicBlock()) + // the comparison should control the sink + comp.(Guard).controls(sensitiveSink.getBasicBlock(), _) select comp, "This sensitive comparision check can potentially be bypassed." diff --git a/go/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/go/ql/src/experimental/CWE-942/CorsMisconfiguration.ql index d0ef8514d5f9..0a571d6a2f35 100644 --- a/go/ql/src/experimental/CWE-942/CorsMisconfiguration.ql +++ b/go/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -14,6 +14,7 @@ import go import semmle.go.security.InsecureFeatureFlag::InsecureFeatureFlag +private import semmle.go.controlflow.Guards /** * A flag indicating a check for satisfied permissions or test configuration. @@ -59,10 +60,8 @@ module UntrustedToAllowOriginHeaderConfig implements DataFlow::ConfigSig { } predicate isBarrier(DataFlow::Node node) { - exists(ControlFlow::ConditionGuardNode cgn | - cgn.ensures(any(AllowedFlag f).getAFlag().getANode(), _) - | - cgn.dominates(node.getBasicBlock()) + exists(Guard g | g = any(AllowedFlag f).getAFlag().getANode().asExpr() | + g.controls(node.getBasicBlock(), _) ) } @@ -171,9 +170,9 @@ class MapRead extends DataFlow::ElementReadNode { module FromUntrustedConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { source instanceof ActiveThreatModelSource } - predicate isSink(DataFlow::Node sink) { isSinkCgn(sink, _) } + predicate isSink(DataFlow::Node sink) { isSinkGuard(sink, _) } - additional predicate isSinkCgn(DataFlow::Node sink, ControlFlow::ConditionGuardNode cgn) { + additional predicate isSinkGuard(DataFlow::Node sink, Guard guard) { exists(IfStmt ifs | exists(Expr operand | operand = ifs.getCondition().getAChildExpr*() and @@ -202,7 +201,7 @@ module FromUntrustedConfig implements DataFlow::ConfigSig { ) ) | - cgn.getCondition() = ifs.getCondition() + guard = ifs.getCondition() ) } } @@ -217,10 +216,10 @@ module FromUntrustedFlow = TaintTracking::Global; * Holds if the provided `allowOriginHW` is also destination of a `ActiveThreatModelSource`. */ predicate flowsToGuardedByCheckOnUntrusted(DataFlow::ExprNode allowOriginHW) { - exists(DataFlow::Node sink, ControlFlow::ConditionGuardNode cgn | - FromUntrustedFlow::flowTo(sink) and FromUntrustedConfig::isSinkCgn(sink, cgn) + exists(DataFlow::Node sink, Guard guard | + FromUntrustedFlow::flowTo(sink) and FromUntrustedConfig::isSinkGuard(sink, guard) | - cgn.dominates(allowOriginHW.getBasicBlock()) + guard.controls(allowOriginHW.getBasicBlock(), _) ) } @@ -233,9 +232,7 @@ where allowOriginIsNull(allowOriginHW, message) ) and not flowsToGuardedByCheckOnUntrusted(allowOriginHW) and - not exists(ControlFlow::ConditionGuardNode cgn | - cgn.ensures(any(AllowedFlag f).getAFlag().getANode(), _) - | - cgn.dominates(allowOriginHW.getBasicBlock()) + not exists(Guard g | g = any(AllowedFlag f).getAFlag().getANode().asExpr() | + g.controls(allowOriginHW.getBasicBlock(), _) ) select allowOriginHW, message diff --git a/go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll b/go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll index 2d7e249fbc03..615ea485ac42 100644 --- a/go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll +++ b/go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll @@ -1,4 +1,5 @@ import go +private import semmle.go.controlflow.Guards Expr getAUse(SsaDefinition def) { result = def.getVariable().getAUse().(IR::EvalInstruction).getExpr() @@ -40,12 +41,12 @@ float getAnUpperBound(Expr expr) { if //if a condition expression exists before and one of the operand happens to be the identifier, we use this condition expression to narrow down the range. exists( - ControlFlow::ConditionGuardNode n, DataFlow::Node lesser, DataFlow::Node greater, + Guard n, boolean branch, DataFlow::Node lesser, DataFlow::Node greater, ReachableBasicBlock bb | - n.ensuresLeq(lesser, greater, _) and + guardEnsuresLeq(n, branch, lesser, greater, _) and IR::evalExprInstruction(lesser.asExpr()) = v.getAUse() and - n.dominates(bb) and + n.controls(bb, branch) and bb.getANode() = IR::evalExprInstruction(identifier) and not exists(Expr e | e = v.getAUse().(IR::EvalInstruction).getExpr() and @@ -54,12 +55,12 @@ float getAnUpperBound(Expr expr) { ) then exists( - ControlFlow::ConditionGuardNode n, ReachableBasicBlock bb, DataFlow::Node lesser, + Guard n, boolean branch, ReachableBasicBlock bb, DataFlow::Node lesser, DataFlow::Node greater, int bias | - n.dominates(bb) and + n.controls(bb, branch) and bb.getANode() = IR::evalExprInstruction(identifier) and - n.ensuresLeq(lesser, greater, bias) and + guardEnsuresLeq(n, branch, lesser, greater, bias) and v.getAUse() = IR::evalExprInstruction(lesser.asExpr()) and not exists(Expr e | e = v.getAUse().(IR::EvalInstruction).getExpr() and @@ -191,12 +192,12 @@ float getALowerBound(Expr expr) { //if exists a condition expression before this identifier if exists( - ControlFlow::ConditionGuardNode n, DataFlow::Node greater, DataFlow::Node lesser, + Guard n, boolean branch, DataFlow::Node greater, DataFlow::Node lesser, ReachableBasicBlock bb | - n.ensuresLeq(lesser, greater, _) and + guardEnsuresLeq(n, branch, lesser, greater, _) and IR::evalExprInstruction(greater.asExpr()) = v.getAUse() and - n.dominates(bb) and + n.controls(bb, branch) and bb.getANode() = IR::evalExprInstruction(identifier) and not exists(Expr e | e = v.getAUse().(IR::EvalInstruction).getExpr() and @@ -205,12 +206,12 @@ float getALowerBound(Expr expr) { ) then exists( - ControlFlow::ConditionGuardNode n, ReachableBasicBlock bb, DataFlow::Node lesser, + Guard n, boolean branch, ReachableBasicBlock bb, DataFlow::Node lesser, DataFlow::Node greater, int bias, float lbs | - n.dominates(bb) and + n.controls(bb, branch) and bb.getANode() = IR::evalExprInstruction(identifier) and - n.ensuresLeq(lesser, greater, bias) and + guardEnsuresLeq(n, branch, lesser, greater, bias) and v.getAUse() = IR::evalExprInstruction(greater.asExpr()) and not exists(Expr e | e = v.getAUse().(IR::EvalInstruction).getExpr() and diff --git a/go/ql/test/library-tests/semmle/go/controlflow/Guards/Guards.expected b/go/ql/test/library-tests/semmle/go/controlflow/Guards/Guards.expected new file mode 100644 index 000000000000..34282f857253 --- /dev/null +++ b/go/ql/test/library-tests/semmle/go/controlflow/Guards/Guards.expected @@ -0,0 +1,78 @@ +controlsResult +| guards.go:7:7:7:15 | ...<... | negative | true | +| guards.go:7:7:7:15 | ...<... | positive | false | +| guards.go:7:7:7:15 | ...<... | zero | false | +| guards.go:9:7:9:16 | ...==... | positive | false | +| guards.go:9:7:9:16 | ...==... | zero | true | +| guards.go:18:7:18:7 | 0 | tagged one or two | false | +| guards.go:18:7:18:7 | 0 | tagged other | false | +| guards.go:18:7:18:7 | 0 | tagged zero | true | +| guards.go:20:7:20:7 | 1 | tagged other | false | +| guards.go:20:10:20:10 | 2 | tagged other | false | +| guards.go:28:5:28:5 | a | compound true | true | +| guards.go:28:5:28:17 | ...&&... | compound false | false | +| guards.go:28:5:28:17 | ...&&... | compound true | true | +| guards.go:28:11:28:16 | ...\|\|... | compound true | true | +| guards.go:36:5:36:20 | ...==... | upcast | true | +| guards.go:39:5:39:19 | ...==... | downcast | true | +| guards.go:42:5:42:22 | ...==... | upcast narrow | true | +| guards.go:48:5:48:15 | ...<=... | above ten | false | +| guards.go:48:5:48:15 | ...<=... | at most ten | true | +| guards.go:53:5:53:15 | ...<=... | at least ten | true | +| guards.go:53:5:53:15 | ...<=... | below ten | false | +valueControlsResult +| guards.go:7:7:7:11 | value | negative | Upper bound -1 | +| guards.go:7:7:7:11 | value | positive | Lower bound 0 | +| guards.go:7:7:7:11 | value | zero | Lower bound 0 | +| guards.go:7:7:7:15 | ...<... | negative | true | +| guards.go:7:7:7:15 | ...<... | positive | false | +| guards.go:7:7:7:15 | ...<... | zero | false | +| guards.go:9:7:9:11 | value | positive | not 0 | +| guards.go:9:7:9:11 | value | zero | 0 | +| guards.go:9:7:9:16 | ...==... | positive | false | +| guards.go:9:7:9:16 | ...==... | zero | true | +| guards.go:17:9:17:13 | value | tagged one or two | not 0 | +| guards.go:17:9:17:13 | value | tagged other | not 0 | +| guards.go:17:9:17:13 | value | tagged other | not 1 | +| guards.go:17:9:17:13 | value | tagged other | not 2 | +| guards.go:17:9:17:13 | value | tagged zero | 0 | +| guards.go:18:7:18:7 | 0 | tagged one or two | false | +| guards.go:18:7:18:7 | 0 | tagged other | false | +| guards.go:18:7:18:7 | 0 | tagged zero | true | +| guards.go:20:7:20:7 | 1 | tagged other | false | +| guards.go:20:10:20:10 | 2 | tagged other | false | +| guards.go:28:5:28:5 | a | compound true | true | +| guards.go:28:5:28:17 | ...&&... | compound false | false | +| guards.go:28:5:28:17 | ...&&... | compound true | true | +| guards.go:28:11:28:16 | ...\|\|... | compound true | true | +| guards.go:36:5:36:15 | type conversion | upcast | 0 | +| guards.go:36:5:36:20 | ...==... | upcast | true | +| guards.go:36:11:36:14 | wide | upcast | 0 | +| guards.go:39:5:39:14 | type conversion | downcast | 0 | +| guards.go:39:5:39:19 | ...==... | downcast | true | +| guards.go:42:5:42:17 | type conversion | upcast narrow | 0 | +| guards.go:42:5:42:22 | ...==... | upcast narrow | true | +| guards.go:42:11:42:16 | narrow | upcast narrow | 0 | +| guards.go:48:5:48:9 | value | above ten | Lower bound 11 | +| guards.go:48:5:48:9 | value | at most ten | Upper bound 10 | +| guards.go:48:5:48:15 | ...<=... | above ten | false | +| guards.go:48:5:48:15 | ...<=... | at most ten | true | +| guards.go:53:5:53:15 | ...<=... | at least ten | true | +| guards.go:53:5:53:15 | ...<=... | below ten | false | +| guards.go:53:11:53:15 | value | at least ten | Lower bound 10 | +| guards.go:53:11:53:15 | value | below ten | Upper bound 9 | +ensuresEqResult +| guards.go:9:7:9:16 | ...==... | 0 = value | true | +| guards.go:9:7:9:16 | ...==... | value = 0 | true | +| guards.go:18:7:18:7 | 0 | 0 = value | true | +| guards.go:18:7:18:7 | 0 | value = 0 | true | +| guards.go:20:7:20:7 | 1 | 1 = value | true | +| guards.go:20:7:20:7 | 1 | value = 1 | true | +| guards.go:20:10:20:10 | 2 | 2 = value | true | +| guards.go:20:10:20:10 | 2 | value = 2 | true | +| guards.go:36:5:36:20 | ...==... | 0 = type conversion | true | +| guards.go:36:5:36:20 | ...==... | type conversion = 0 | true | +| guards.go:39:5:39:19 | ...==... | 0 = type conversion | true | +| guards.go:39:5:39:19 | ...==... | type conversion = 0 | true | +| guards.go:42:5:42:22 | ...==... | 0 = type conversion | true | +| guards.go:42:5:42:22 | ...==... | type conversion = 0 | true | diff --git a/go/ql/test/library-tests/semmle/go/controlflow/Guards/Guards.ql b/go/ql/test/library-tests/semmle/go/controlflow/Guards/Guards.ql new file mode 100644 index 000000000000..2772228af34a --- /dev/null +++ b/go/ql/test/library-tests/semmle/go/controlflow/Guards/Guards.ql @@ -0,0 +1,31 @@ +import go +import semmle.go.controlflow.Guards + +predicate sinkCall(DataFlow::CallNode call, string label) { + call.getTarget().getName() = "sink" and + label = call.getArgument(0).getExactValue() +} + +query predicate controlsResult(Guard guard, string label, string outcome) { + exists(DataFlow::CallNode call, boolean branch | + sinkCall(call, label) and + guard.controls(call.getBasicBlock(), branch) and + outcome = branch.toString() + ) +} + +query predicate valueControlsResult(Guard guard, string label, string outcome) { + exists(DataFlow::CallNode call, GuardValue value | + sinkCall(call, label) and + guard.valueControls(call.getBasicBlock(), value) and + outcome = value.toString() + ) +} + +query predicate ensuresEqResult(Guard guard, string label, string outcome) { + exists(boolean branch, DataFlow::Node left, DataFlow::Node right | + guardEnsuresEq(guard, branch, left, right) and + label = left.toString() + " = " + right.toString() and + outcome = branch.toString() + ) +} diff --git a/go/ql/test/library-tests/semmle/go/controlflow/Guards/guards.go b/go/ql/test/library-tests/semmle/go/controlflow/Guards/guards.go new file mode 100644 index 000000000000..24cb5589612d --- /dev/null +++ b/go/ql/test/library-tests/semmle/go/controlflow/Guards/guards.go @@ -0,0 +1,58 @@ +package guards + +func sink(string) {} + +func taglessSwitch(value int) { + switch { + case value < 0: + sink("negative") + case value == 0: + sink("zero") + default: + sink("positive") + } +} + +func taggedSwitch(value int) { + switch value { + case 0: + sink("tagged zero") + case 1, 2: + sink("tagged one or two") + default: + sink("tagged other") + } +} + +func compoundCondition(a, b, c bool) { + if a && (b || c) { + sink("compound true") + } else { + sink("compound false") + } +} + +func conversions(wide int16, narrow int8) { + if int32(wide) == 0 { + sink("upcast") + } + if int8(wide) == 0 { + sink("downcast") + } + if int16(narrow) == 0 { + sink("upcast narrow") + } +} + +func nonStrictComparisons(value int) { + if value <= 10 { + sink("at most ten") + } else { + sink("above ten") + } + if 10 <= value { + sink("at least ten") + } else { + sink("below ten") + } +} diff --git a/go/ql/test/library-tests/semmle/go/dataflow/GuardingFunctions/test.go b/go/ql/test/library-tests/semmle/go/dataflow/GuardingFunctions/test.go index a7a595509f9c..8f7fa74bc0a7 100644 --- a/go/ql/test/library-tests/semmle/go/dataflow/GuardingFunctions/test.go +++ b/go/ql/test/library-tests/semmle/go/dataflow/GuardingFunctions/test.go @@ -842,7 +842,7 @@ func test() { s := source() isValid := !guardBool(s) if isValid { - sink(s) // $ SPURIOUS: hasValueFlow="s" + sink(s) } else { sink(s) // $ hasValueFlow="s" } diff --git a/shared/controlflow/codeql/controlflow/Guards.qll b/shared/controlflow/codeql/controlflow/Guards.qll index e12535b4b328..c167c6de4d20 100644 --- a/shared/controlflow/codeql/controlflow/Guards.qll +++ b/shared/controlflow/codeql/controlflow/Guards.qll @@ -78,6 +78,14 @@ signature module InputSig