From ab2c2ef6ae034923810a815e5f4a6ba870e600a2 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Fri, 31 Oct 2025 13:52:56 +0100 Subject: [PATCH 1/5] C#: Update isUnreachableInCall in dataflow to use Guards library. --- .../semmle/code/csharp/controlflow/Guards.qll | 11 ++++-- .../dataflow/internal/DataFlowPrivate.qll | 36 ++++++------------- 2 files changed, 20 insertions(+), 27 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll index 4914450dfc9a..040ea47f7859 100644 --- a/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll +++ b/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll @@ -49,8 +49,15 @@ private module GuardsInput implements override predicate isNull() { any() } } - private class BooleanConstant extends ConstantExpr instanceof BoolLiteral { - override boolean asBooleanValue() { result = super.getBoolValue() } + private predicate boolConst(Expr e, boolean b) { + e.getType() instanceof BoolType and + e.getValue() = b.toString() + } + + private class BooleanConstant extends ConstantExpr { + BooleanConstant() { boolConst(this, _) } + + override boolean asBooleanValue() { boolConst(this, result) } } private predicate intConst(Expr e, int i) { 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 f4a76b2f5770..4f7f0141da2a 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll @@ -2583,10 +2583,10 @@ class NodeRegion instanceof ControlFlow::BasicBlock { * Holds if the nodes in `nr` are unreachable when the call context is `call`. */ predicate isUnreachableInCall(NodeRegion nr, DataFlowCall call) { - exists(ExplicitParameterNode paramNode, Guard guard, ControlFlow::BooleanSuccessor bs | - viableConstantBooleanParamArg(paramNode, bs.getValue().booleanNot(), call) and + exists(ExplicitParameterNode paramNode, Guard guard, GuardValue val | + viableConstantParamArg(paramNode, val.getDualValue(), call) and paramNode.getSsaDefinition().getARead() = guard and - guard.controlsBlock(nr, bs, _) + guard.valueControls(nr, val) ) } @@ -2904,33 +2904,19 @@ class CastNode extends Node { class DataFlowExpr = Expr; -/** Holds if `e` is an expression that always has the same Boolean value `val`. */ -private predicate constantBooleanExpr(Expr e, boolean val) { - e.getType() instanceof BoolType and - e.getValue() = val.toString() - or - exists(Ssa::ExplicitDefinition def, Expr src | - e = def.getARead() and - src = def.getADefinition().getSource() and - constantBooleanExpr(src, val) - ) -} +/** An argument that always has the same value. */ +private class ConstantArgumentNode extends ExprNode { + ConstantArgumentNode() { Guards::InternalUtil::exprHasValue(this.(ArgumentNode).asExpr(), _) } -/** An argument that always has the same Boolean value. */ -private class ConstantBooleanArgumentNode extends ExprNode { - ConstantBooleanArgumentNode() { constantBooleanExpr(this.(ArgumentNode).asExpr(), _) } - - /** Gets the Boolean value of this expression. */ - boolean getBooleanValue() { constantBooleanExpr(this.getExpr(), result) } + /** Gets the value of this expression. */ + GuardValue getValue() { Guards::InternalUtil::exprHasValue(this.getExpr(), result) } } pragma[noinline] -private predicate viableConstantBooleanParamArg( - ParameterNode paramNode, boolean b, DataFlowCall call -) { - exists(ConstantBooleanArgumentNode arg | +private predicate viableConstantParamArg(ParameterNode paramNode, GuardValue val, DataFlowCall call) { + exists(ConstantArgumentNode arg | viableParamArg(call, paramNode, arg) and - b = arg.getBooleanValue() + val = arg.getValue() ) } From f6dfcf1ca4252eb354c7f96e5f1f17eb5d99c8db Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Wed, 29 Oct 2025 16:02:55 +0100 Subject: [PATCH 2/5] C#: Delete splitting-aware controls implementation. --- csharp/ql/lib/semmle/code/csharp/Caching.qll | 2 - .../csharp/controlflow/ControlFlowElement.qll | 132 ------------------ 2 files changed, 134 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/Caching.qll b/csharp/ql/lib/semmle/code/csharp/Caching.qll index 50a789f79897..bbe310fe69e5 100644 --- a/csharp/ql/lib/semmle/code/csharp/Caching.qll +++ b/csharp/ql/lib/semmle/code/csharp/Caching.qll @@ -33,8 +33,6 @@ module Stages { cached private predicate forceCachingInSameStageRev() { - any(ControlFlowElement cfe).controlsBlock(_, _, _) - or exists(GuardedExpr ge) or forceCachingInSameStageRev() diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowElement.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowElement.qll index 784dab415f3e..ee2ae8c22a7c 100644 --- a/csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowElement.qll +++ b/csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowElement.qll @@ -87,148 +87,16 @@ class ControlFlowElement extends ExprOrStmtParent, @control_flow_element { result.getAControlFlowNode() } - pragma[noinline] - private predicate immediatelyControlsBlockSplit0( - ConditionBlock cb, BasicBlock succ, ConditionalSuccessor s - ) { - // Only calculate dominance by explicit recursion for split nodes; - // all other nodes can use regular CFG dominance - this instanceof Impl::SplitAstNode and - cb.getLastNode() = this.getAControlFlowNode() and - succ = cb.getASuccessor(s) - } - - pragma[noinline] - private predicate immediatelyControlsBlockSplit1( - ConditionBlock cb, BasicBlock succ, ConditionalSuccessor s, BasicBlock pred, SuccessorType t - ) { - this.immediatelyControlsBlockSplit0(cb, succ, s) and - pred = succ.getAPredecessorByType(t) and - pred != cb - } - - pragma[noinline] - private predicate immediatelyControlsBlockSplit2( - ConditionBlock cb, BasicBlock succ, ConditionalSuccessor s, BasicBlock pred, SuccessorType t - ) { - this.immediatelyControlsBlockSplit1(cb, succ, s, pred, t) and - ( - succ.dominates(pred) - or - // `pred` might be another split of this element - pred.getLastNode().getAstNode() = this and - t = s - ) - } - - /** - * Holds if basic block `succ` is immediately controlled by this control flow - * element with conditional value `s`. That is, `succ` can only be reached from - * the callable entry point by going via the `s` edge out of *some* basic block - * `pred` ending with this element, and `pred` is an immediate predecessor - * of `succ`. - * - * Moreover, this control flow element corresponds to multiple control flow nodes, - * which is why - * - * ```ql - * exists(ConditionBlock cb | - * cb.getLastNode() = this.getAControlFlowNode() | - * cb.immediatelyControls(succ, s) - * ) - * ``` - * - * does not work. - * - * `cb` records all of the possible condition blocks for this control flow element - * that a path from the callable entry point to `succ` may go through. - */ - pragma[nomagic] - private predicate immediatelyControlsBlockSplit( - BasicBlock succ, ConditionalSuccessor s, ConditionBlock cb - ) { - this.immediatelyControlsBlockSplit0(cb, succ, s) and - forall(BasicBlock pred, SuccessorType t | - this.immediatelyControlsBlockSplit1(cb, succ, s, pred, t) - | - this.immediatelyControlsBlockSplit2(cb, succ, s, pred, t) - ) - } - - pragma[noinline] - private predicate controlsJoinBlockPredecessor( - JoinBlock controlled, ConditionalSuccessor s, int i, ConditionBlock cb - ) { - this.controlsBlockSplit(controlled.getJoinBlockPredecessor(i), s, cb) - } - - private predicate controlsJoinBlockSplit(JoinBlock controlled, ConditionalSuccessor s, int i) { - i = -1 and - this.controlsJoinBlockPredecessor(controlled, s, _, _) - or - this.controlsJoinBlockSplit(controlled, s, i - 1) and - ( - this.controlsJoinBlockPredecessor(controlled, s, i, _) - or - controlled.dominates(controlled.getJoinBlockPredecessor(i)) - ) - } - - cached - private predicate controlsBlockSplit( - BasicBlock controlled, ConditionalSuccessor s, ConditionBlock cb - ) { - Stages::GuardsStage::forceCachingInSameStage() and - this.immediatelyControlsBlockSplit(controlled, s, cb) - or - // Equivalent with - // - // ```ql - // exists(JoinBlockPredecessor pred | pred = controlled.getAPredecessor() | - // this.controlsBlockSplit(pred, s) - // ) and - // forall(JoinBlockPredecessor pred | pred = controlled.getAPredecessor() | - // this.controlsBlockSplit(pred, s) - // or - // controlled.dominates(pred) - // ) - // ``` - // - // but uses no universal recursion for better performance. - exists(int last | - last = max(int i | exists(controlled.(JoinBlock).getJoinBlockPredecessor(i))) - | - this.controlsJoinBlockSplit(controlled, s, last) - ) and - this.controlsJoinBlockPredecessor(controlled, s, _, cb) - or - not controlled instanceof JoinBlock and - this.controlsBlockSplit(controlled.getAPredecessor(), s, cb) - } - /** * Holds if basic block `controlled` is controlled by this control flow element * with conditional value `s`. That is, `controlled` can only be reached from * the callable entry point by going via the `s` edge out of *some* basic block * ending with this element. * - * This predicate is different from - * - * ```ql - * exists(ConditionBlock cb | - * cb.getLastNode() = this.getAControlFlowNode() | - * cb.controls(controlled, s) - * ) - * ``` - * - * as control flow splitting is taken into account. - * * `cb` records all of the possible condition blocks for this control flow element * that a path from the callable entry point to `controlled` may go through. */ predicate controlsBlock(BasicBlock controlled, ConditionalSuccessor s, ConditionBlock cb) { - this.controlsBlockSplit(controlled, s, cb) - or cb.getLastNode() = this.getAControlFlowNode() and cb.edgeDominates(controlled, s) } From 4de564eb4e35d300865e788f229456dc8875acad Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Fri, 31 Oct 2025 14:01:12 +0100 Subject: [PATCH 3/5] C#: Replace reference to controlsBlock and simplify. --- .../csharp/security/dataflow/ConditionalBypassQuery.qll | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/ConditionalBypassQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/ConditionalBypassQuery.qll index f2b46e4ebac1..53b44f873a64 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/ConditionalBypassQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/ConditionalBypassQuery.qll @@ -72,17 +72,10 @@ class ReverseDnsSource extends Source { } } -pragma[noinline] -private predicate conditionControlsCall0( - SensitiveExecutionMethodCall call, Expr e, ControlFlow::BooleanSuccessor s -) { - forex(BasicBlock bb | bb = call.getAControlFlowNode().getBasicBlock() | e.controlsBlock(bb, s, _)) -} - private predicate conditionControlsCall( SensitiveExecutionMethodCall call, SensitiveExecutionMethod def, Expr e, boolean cond ) { - exists(ControlFlow::BooleanSuccessor s | cond = s.getValue() | conditionControlsCall0(call, e, s)) and + e.(Guard).directlyControls(call.getBasicBlock(), cond) and def = call.getTarget().getUnboundDeclaration() } From eb93e8ed41c0eecd166cc228bd5e2248bbacfdf9 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Fri, 31 Oct 2025 14:22:11 +0100 Subject: [PATCH 4/5] C#: Deprecate controlsBlock. --- .../semmle/code/csharp/controlflow/ControlFlowElement.qll | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowElement.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowElement.qll index ee2ae8c22a7c..0d0ed6819698 100644 --- a/csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowElement.qll +++ b/csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowElement.qll @@ -88,6 +88,8 @@ class ControlFlowElement extends ExprOrStmtParent, @control_flow_element { } /** + * DEPRECATED: Use `Guard` class instead. + * * Holds if basic block `controlled` is controlled by this control flow element * with conditional value `s`. That is, `controlled` can only be reached from * the callable entry point by going via the `s` edge out of *some* basic block @@ -96,7 +98,9 @@ class ControlFlowElement extends ExprOrStmtParent, @control_flow_element { * `cb` records all of the possible condition blocks for this control flow element * that a path from the callable entry point to `controlled` may go through. */ - predicate controlsBlock(BasicBlock controlled, ConditionalSuccessor s, ConditionBlock cb) { + deprecated predicate controlsBlock( + BasicBlock controlled, ConditionalSuccessor s, ConditionBlock cb + ) { cb.getLastNode() = this.getAControlFlowNode() and cb.edgeDominates(controlled, s) } From 7ab25b593d990b8fed63e29e1eca6473eb29dba4 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Tue, 4 Nov 2025 15:43:49 +0100 Subject: [PATCH 5/5] C#: Change note. --- .../ql/lib/change-notes/2025-10-04-deprecate-controlsblock.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 csharp/ql/lib/change-notes/2025-10-04-deprecate-controlsblock.md diff --git a/csharp/ql/lib/change-notes/2025-10-04-deprecate-controlsblock.md b/csharp/ql/lib/change-notes/2025-10-04-deprecate-controlsblock.md new file mode 100644 index 000000000000..a3c69932917d --- /dev/null +++ b/csharp/ql/lib/change-notes/2025-10-04-deprecate-controlsblock.md @@ -0,0 +1,4 @@ +--- +category: deprecated +--- +* `ControlFlowElement.controlsBlock` has been deprecated in favor of the Guards library.