From 4f6528a9fbfe564b3fdec40d61e854998c59243d Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Fri, 31 Oct 2025 11:08:40 +0100 Subject: [PATCH 1/2] C#: Deprecate AbstractValue. --- .../2025-10-31-deprecate-abstractvalue.md | 4 + .../semmle/code/csharp/controlflow/Guards.qll | 84 ++++++++++--------- .../semmle/code/csharp/dataflow/Nullness.qll | 5 +- .../dataflow/internal/DataFlowPublic.qll | 4 +- .../code/csharp/dataflow/internal/SsaImpl.qll | 6 +- .../internal/rangeanalysis/RangeUtils.qll | 6 +- .../security/dataflow/TaintedPathQuery.qll | 5 +- .../security/dataflow/UrlRedirectQuery.qll | 20 ++--- .../csharp/security/dataflow/ZipSlipQuery.qll | 4 +- .../experimental/CWE-918/RequestForgery.qll | 8 +- .../controlflow/guards/AbstractValue.ql | 2 +- .../controlflow/guards/Collections.ql | 2 +- .../guards/GuardedControlFlowNode.ql | 2 +- .../controlflow/guards/GuardedExpr.ql | 2 +- .../dataflow/barrier-guards/barrier-flow.ql | 6 +- .../dataflow/callablereturnsarg/Common.qll | 4 +- 16 files changed, 81 insertions(+), 83 deletions(-) create mode 100644 csharp/ql/lib/change-notes/2025-10-31-deprecate-abstractvalue.md diff --git a/csharp/ql/lib/change-notes/2025-10-31-deprecate-abstractvalue.md b/csharp/ql/lib/change-notes/2025-10-31-deprecate-abstractvalue.md new file mode 100644 index 000000000000..65bb9032b12e --- /dev/null +++ b/csharp/ql/lib/change-notes/2025-10-31-deprecate-abstractvalue.md @@ -0,0 +1,4 @@ +--- +category: deprecated +--- +* The class `AbstractValue` in the `Guards` library has been deprecated and replaced with the class `GuardValue`. diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll index 4914450dfc9a..ed0238d1f093 100644 --- a/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll +++ b/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll @@ -227,7 +227,7 @@ private module LogicInput implements GuardsImpl::LogicInputSig { e instanceof DereferenceableExpr and ct.getAnArgument() = e and ct.getAnArgument() = arg and - arg = any(NullValue nv | nv.isNonNull()).getAnExpr() and + nonNullValueImplied(arg) and ck = ct.getComparisonKind() and e != arg and isNull = false and @@ -314,7 +314,7 @@ class Guard extends Guards::Guard { * In case `cfn` or `sub` access an SSA variable in their left-most qualifier, then * so must the other (accessing the same SSA variable). */ - predicate controlsNode(ControlFlow::Nodes::ElementNode cfn, AccessOrCallExpr sub, AbstractValue v) { + predicate controlsNode(ControlFlow::Nodes::ElementNode cfn, AccessOrCallExpr sub, GuardValue v) { isGuardedByNode(cfn, this, sub, v) } @@ -324,26 +324,31 @@ class Guard extends Guards::Guard { * Note: This predicate is inlined. */ pragma[inline] - predicate controlsNode(ControlFlow::Nodes::ElementNode cfn, AbstractValue v) { + predicate controlsNode(ControlFlow::Nodes::ElementNode cfn, GuardValue v) { guardControls(this, cfn.getBasicBlock(), v) } /** * Holds if basic block `bb` is guarded by this expression having value `v`. */ - predicate controlsBasicBlock(BasicBlock bb, AbstractValue v) { guardControls(this, bb, v) } + predicate controlsBasicBlock(BasicBlock bb, GuardValue v) { guardControls(this, bb, v) } /** * Gets a valid value for this guard. For example, if this guard is a test, then * it can have Boolean values `true` and `false`. */ - deprecated AbstractValue getAValue() { isGuard(this, result) } + deprecated GuardValue getAValue() { isGuard(this, result) } } -class AbstractValue = GuardValue; +/** DEPRECATED: Use `GuardValue` instead. */ +deprecated class AbstractValue = GuardValue; -/** Provides different types of `AbstractValues`s. */ -module AbstractValues { +/** + * DEPRECATED: Use `GuardValue` member predicates instead. + * + * Provides different types of `AbstractValues`s. + */ +deprecated module AbstractValues { class BooleanValue extends AbstractValue { BooleanValue() { exists(this.asBooleanValue()) } @@ -369,8 +374,7 @@ module AbstractValues { } } -private import AbstractValues - +// private import AbstractValues /** Gets the value resulting from matching `null` against `pat`. */ private boolean patternMatchesNull(PatternExpr pat) { pat instanceof NullLiteral and result = true @@ -431,22 +435,22 @@ class DereferenceableExpr extends Expr { /** * Gets an expression that tests via nullness whether this expression is `null`. * - * If the returned expression evaluates to `null` (`v.isNull()`) or evaluates to - * non-`null` (`not v.isNull()`), then this expression is guaranteed to be `null` + * If the returned expression evaluates to `null` (`v.isNullValue()`) or evaluates to + * non-`null` (`not v.isNullValue()`), then this expression is guaranteed to be `null` * if `isNull` is true, and non-`null` if `isNull` is false. * * For example, if `x` evaluates to `null` in `x ?? y` then `y` is evaluated, and * `x` is guaranteed to be `null`. */ - private Expr getANullnessNullCheck(NullValue v, boolean isNull) { + private Expr getANullnessNullCheck(GuardValue v, boolean isNull) { exists(NullnessCompletion c | c.isValidFor(this) | result = this and if c.isNull() then ( - v.isNull() and + v.isNullValue() and isNull = true ) else ( - v.isNonNull() and + v.isNonNullValue() and isNull = false ) ) @@ -513,8 +517,8 @@ class EnumerableCollectionExpr extends Expr { ) } - private Expr getABooleanEmptinessCheck(BooleanValue v, boolean isEmpty) { - exists(boolean branch | branch = v.getValue() | + private Expr getABooleanEmptinessCheck(GuardValue v, boolean isEmpty) { + exists(boolean branch | branch = v.asBooleanValue() | result = any(ComparisonTest ct | exists(boolean lowerBound | @@ -578,7 +582,7 @@ class EnumerableCollectionExpr extends Expr { * For example, if the expression `x.Length != 0` evaluates to `true` then the * expression `x` is guaranteed to be non-empty. */ - Expr getAnEmptinessCheck(AbstractValue v, boolean isEmpty) { + Expr getAnEmptinessCheck(GuardValue v, boolean isEmpty) { result = this.getABooleanEmptinessCheck(v, isEmpty) } } @@ -692,14 +696,14 @@ class GuardedExpr extends AccessOrCallExpr { * left-most qualifier, then so must the other (accessing the same SSA * variable). */ - Guard getAGuard(Expr sub, AbstractValue v) { isGuardedByExpr(this, result, sub, v) } + Guard getAGuard(Expr sub, GuardValue v) { isGuardedByExpr(this, result, sub, v) } /** * Holds if this expression must have abstract value `v`. That is, this * expression is guarded by a structurally equal expression having abstract * value `v`. */ - predicate mustHaveValue(AbstractValue v) { + predicate mustHaveValue(GuardValue v) { exists(Guard g | g = this.getAGuard(g, v)) or ssaMustHaveValue(this, v) } @@ -713,7 +717,7 @@ class GuardedExpr extends AccessOrCallExpr { * variable). */ predicate isGuardedBy(Expr cond, Expr sub, boolean b) { - cond = this.getAGuard(sub, any(BooleanValue v | v.getValue() = b)) + cond = this.getAGuard(sub, any(GuardValue v | v.asBooleanValue() = b)) } } @@ -738,7 +742,7 @@ class GuardedExpr extends AccessOrCallExpr { class GuardedControlFlowNode extends ControlFlow::Nodes::ElementNode { private Guard g; private AccessOrCallExpr sub0; - private AbstractValue v0; + private GuardValue v0; GuardedControlFlowNode() { g.controlsNode(this, sub0, v0) } @@ -753,7 +757,7 @@ class GuardedControlFlowNode extends ControlFlow::Nodes::ElementNode { * left-most qualifier, then so must the other (accessing the same SSA * variable). */ - Guard getAGuard(Expr sub, AbstractValue v) { + Guard getAGuard(Expr sub, GuardValue v) { result = g and sub = sub0 and v = v0 @@ -764,7 +768,7 @@ class GuardedControlFlowNode extends ControlFlow::Nodes::ElementNode { * control flow node is guarded by a structurally equal expression having * abstract value `v`. */ - predicate mustHaveValue(AbstractValue v) { g = this.getAGuard(g, v) } + predicate mustHaveValue(GuardValue v) { g = this.getAGuard(g, v) } } /** @@ -788,7 +792,7 @@ class GuardedControlFlowNode extends ControlFlow::Nodes::ElementNode { class GuardedDataFlowNode extends DataFlow::ExprNode { private Guard g; private AccessOrCallExpr sub0; - private AbstractValue v0; + private GuardValue v0; GuardedDataFlowNode() { exists(ControlFlow::Nodes::ElementNode cfn | exists(this.getExprAtNode(cfn)) | @@ -807,7 +811,7 @@ class GuardedDataFlowNode extends DataFlow::ExprNode { * left-most qualifier, then so must the other (accessing the same SSA * variable). */ - Guard getAGuard(Expr sub, AbstractValue v) { + Guard getAGuard(Expr sub, GuardValue v) { result = g and sub = sub0 and v = v0 @@ -818,17 +822,17 @@ class GuardedDataFlowNode extends DataFlow::ExprNode { * data flow node is guarded by a structurally equal expression having * abstract value `v`. */ - predicate mustHaveValue(AbstractValue v) { g = this.getAGuard(g, v) } + predicate mustHaveValue(GuardValue v) { g = this.getAGuard(g, v) } } /** An expression guarded by a `null` check. */ class NullGuardedExpr extends GuardedExpr { - NullGuardedExpr() { this.mustHaveValue(any(NullValue v | v.isNonNull())) } + NullGuardedExpr() { this.mustHaveValue(any(GuardValue v | v.isNonNullValue())) } } /** A data flow node guarded by a `null` check. */ class NullGuardedDataFlowNode extends GuardedDataFlowNode { - NullGuardedDataFlowNode() { this.mustHaveValue(any(NullValue v | v.isNonNull())) } + NullGuardedDataFlowNode() { this.mustHaveValue(any(GuardValue v | v.isNonNullValue())) } } /** INTERNAL: Do not use. */ @@ -931,7 +935,7 @@ module Internal { bao.getAnOperand() = o and // The other operand must be provably non-null in order // for `only if` to hold - o = any(NullValue nv | nv.isNonNull()).getAnExpr() and + nonNullValueImplied(o) and e != o ) } @@ -973,7 +977,7 @@ module Internal { nonEmptyValue(def.getDefinition().getSource()) } - deprecated predicate isGuard(Expr e, AbstractValue val) { + deprecated predicate isGuard(Expr e, GuardValue val) { ( e.getType() instanceof BoolType and not e instanceof BoolLiteral and @@ -1207,7 +1211,7 @@ module Internal { * Holds if basic block `bb` only is reached when guard `g` has abstract value `v`. */ cached - predicate guardControls(Guard g, BasicBlock bb, AbstractValue v) { + predicate guardControls(Guard g, BasicBlock bb, GuardValue v) { g.(Guards::Guard).valueControls(bb, v) } @@ -1220,7 +1224,7 @@ module Internal { pragma[nomagic] private predicate nodeIsGuardedBySameSubExpr0( ControlFlow::Node guardedCfn, BasicBlock guardedBB, AccessOrCallExpr guarded, Guard g, - AccessOrCallExpr sub, AbstractValue v + AccessOrCallExpr sub, GuardValue v ) { Stages::GuardsStage::forceCachingInSameStage() and guardedCfn = guarded.getAControlFlowNode() and @@ -1233,7 +1237,7 @@ module Internal { pragma[nomagic] private predicate nodeIsGuardedBySameSubExpr( ControlFlow::Node guardedCfn, BasicBlock guardedBB, AccessOrCallExpr guarded, Guard g, - AccessOrCallExpr sub, AbstractValue v + AccessOrCallExpr sub, GuardValue v ) { nodeIsGuardedBySameSubExpr0(guardedCfn, guardedBB, guarded, g, sub, v) and guardControlsSub(g, guardedBB, sub) @@ -1242,7 +1246,7 @@ module Internal { pragma[nomagic] private predicate nodeIsGuardedBySameSubExprSsaDef0( ControlFlow::Node cfn, BasicBlock guardedBB, AccessOrCallExpr guarded, Guard g, - ControlFlow::Node subCfn, BasicBlock subCfnBB, AccessOrCallExpr sub, AbstractValue v, + ControlFlow::Node subCfn, BasicBlock subCfnBB, AccessOrCallExpr sub, GuardValue v, Ssa::Definition def ) { nodeIsGuardedBySameSubExpr(cfn, guardedBB, guarded, g, sub, v) and @@ -1253,7 +1257,7 @@ module Internal { pragma[nomagic] private predicate nodeIsGuardedBySameSubExprSsaDef( ControlFlow::Node guardedCfn, AccessOrCallExpr guarded, Guard g, ControlFlow::Node subCfn, - AccessOrCallExpr sub, AbstractValue v, Ssa::Definition def + AccessOrCallExpr sub, GuardValue v, Ssa::Definition def ) { exists(BasicBlock guardedBB, BasicBlock subCfnBB | nodeIsGuardedBySameSubExprSsaDef0(guardedCfn, guardedBB, guarded, g, subCfn, subCfnBB, sub, @@ -1264,7 +1268,7 @@ module Internal { pragma[noinline] private predicate isGuardedByExpr0( - AccessOrCallExpr guarded, Guard g, AccessOrCallExpr sub, AbstractValue v + AccessOrCallExpr guarded, Guard g, AccessOrCallExpr sub, GuardValue v ) { forex(ControlFlow::Node cfn | cfn = guarded.getAControlFlowNode() | nodeIsGuardedBySameSubExpr(cfn, _, guarded, g, sub, v) @@ -1272,9 +1276,7 @@ module Internal { } cached - predicate isGuardedByExpr( - AccessOrCallExpr guarded, Guard g, AccessOrCallExpr sub, AbstractValue v - ) { + predicate isGuardedByExpr(AccessOrCallExpr guarded, Guard g, AccessOrCallExpr sub, GuardValue v) { isGuardedByExpr0(guarded, g, sub, v) and forall(ControlFlow::Node subCfn, Ssa::Definition def | nodeIsGuardedBySameSubExprSsaDef(_, guarded, g, subCfn, sub, v, def) @@ -1285,7 +1287,7 @@ module Internal { cached predicate isGuardedByNode( - ControlFlow::Nodes::ElementNode guarded, Guard g, AccessOrCallExpr sub, AbstractValue v + ControlFlow::Nodes::ElementNode guarded, Guard g, AccessOrCallExpr sub, GuardValue v ) { nodeIsGuardedBySameSubExpr(guarded, _, _, g, sub, v) and forall(ControlFlow::Node subCfn, Ssa::Definition def | diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/Nullness.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/Nullness.qll index 55c0324e7c5e..bf9d8d6c0ae3 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/Nullness.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/Nullness.qll @@ -21,7 +21,6 @@ import csharp private import ControlFlow private import internal.CallableReturns private import semmle.code.csharp.controlflow.Guards as G -private import semmle.code.csharp.controlflow.Guards::AbstractValues private import semmle.code.csharp.dataflow.internal.SsaImpl as SsaImpl private import semmle.code.csharp.frameworks.System private import semmle.code.csharp.frameworks.Test @@ -368,9 +367,9 @@ class Dereference extends G::DereferenceableExpr { ( forex(Ssa::Definition def0 | this = def0.getARead() | this.isAlwaysNull0(def0)) or - exists(NullValue nv | + exists(G::GuardValue nv | this.(G::GuardedExpr).mustHaveValue(nv) and - nv.isNull() + nv.isNullValue() ) ) and not this instanceof G::NullGuardedExpr 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 b21d5e2c3efb..4023d6c4597c 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll @@ -173,7 +173,7 @@ abstract class NonLocalJumpNode extends Node { * For example, the guard `g` might be a call `isSafe(x)` and the expression `e` * the argument `x`. */ -signature predicate guardChecksSig(Guard g, Expr e, AbstractValue v); +signature predicate guardChecksSig(Guard g, Expr e, GuardValue v); /** * Provides a set of barrier nodes for a guard that validates an expression. @@ -190,7 +190,7 @@ module BarrierGuard { SsaFlow::asNode(result) = SsaImpl::DataFlowIntegration::BarrierGuard::getABarrierNode() or - exists(Guard g, Expr e, AbstractValue v | + exists(Guard g, Expr e, GuardValue v | guardChecks(g, e, v) and g.controlsNode(result.getControlFlowNode(), e, v) ) 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 1b703d70dbdc..70fda2b12964 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll @@ -963,7 +963,7 @@ private module Cached { DataFlowIntegrationImpl::localMustFlowStep(v, nodeFrom, nodeTo) } - signature predicate guardChecksSig(Guards::Guard g, Expr e, Guards::AbstractValue v); + signature predicate guardChecksSig(Guards::Guard g, Expr e, Guards::GuardValue v); cached // nothing is actually cached module BarrierGuard { @@ -971,9 +971,9 @@ private module Cached { DataFlowIntegrationInput::Guard g, DataFlowIntegrationInput::Expr e, DataFlowIntegrationInput::GuardValue branch ) { - exists(Guards::AbstractValues::BooleanValue v | + exists(Guards::GuardValue v | guardChecks(g, e.getAstNode(), v) and - branch = v.getValue() + branch = v.asBooleanValue() ) } diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/RangeUtils.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/RangeUtils.qll index 069d4e6e83ac..71d177a48bb1 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/RangeUtils.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/RangeUtils.qll @@ -10,8 +10,6 @@ private module Impl { private import semmle.code.csharp.controlflow.Guards as G private import ControlFlowReachability - private class BooleanValue = G::AbstractValues::BooleanValue; - private class ExprNode = ControlFlow::Nodes::ExprNode; private class ExprChildReachability extends ControlFlowReachabilityConfiguration { @@ -93,7 +91,7 @@ private module Impl { /** * Holds if basic block `bb` is guarded by this guard having value `v`. */ - predicate controlsBasicBlock(ControlFlow::BasicBlock bb, G::AbstractValue v) { + predicate controlsBasicBlock(ControlFlow::BasicBlock bb, G::GuardValue v) { super.controlsBasicBlock(bb, v) } @@ -130,7 +128,7 @@ private module Impl { * Holds if `guard` controls the position `controlled` with the value `testIsTrue`. */ predicate guardControlsSsaRead(Guard guard, SsaReadPosition controlled, boolean testIsTrue) { - exists(BooleanValue b | b.getValue() = testIsTrue | + exists(G::GuardValue b | b.asBooleanValue() = testIsTrue | guard.controlsBasicBlock(controlled.(SsaReadPositionBlock).getBlock(), b) ) } diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll index 2f20eb6e3421..668e3ddcb201 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/TaintedPathQuery.qll @@ -116,7 +116,7 @@ private class WeakGuard extends Guard { ) or // Checking against `null` has no bearing on path traversal. - this.controlsNode(_, _, any(AbstractValues::NullValue nv)) + this.controlsNode(_, _, any(GuardValue nv | nv.isNullness(_))) or this.(LogicalOperation).getAnOperand() instanceof WeakGuard } @@ -130,8 +130,9 @@ private class WeakGuard extends Guard { class PathCheck extends Sanitizer { PathCheck() { // This expression is structurally replicated in a dominating guard which is not a "weak" check - exists(Guard g, AbstractValues::BooleanValue v | + exists(Guard g, GuardValue v | g = this.(GuardedDataFlowNode).getAGuard(_, v) and + exists(v.asBooleanValue()) and not g instanceof WeakGuard ) } diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll index b095305742dd..15ba99aedf0d 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/UrlRedirectQuery.qll @@ -111,7 +111,7 @@ class HttpServerTransferSink extends Sink { } } -private predicate isLocalUrlSanitizerMethodCall(MethodCall guard, Expr e, AbstractValue v) { +private predicate isLocalUrlSanitizerMethodCall(MethodCall guard, Expr e, GuardValue v) { exists(Method m | m = guard.getTarget() | m.hasName("IsLocalUrl") and e = guard.getArgument(0) @@ -119,10 +119,10 @@ private predicate isLocalUrlSanitizerMethodCall(MethodCall guard, Expr e, Abstra m.hasName("IsUrlLocalToHost") and e = guard.getArgument(1) ) and - v.(AbstractValues::BooleanValue).getValue() = true + v.asBooleanValue() = true } -private predicate isLocalUrlSanitizer(Guard g, Expr e, AbstractValue v) { +private predicate isLocalUrlSanitizer(Guard g, Expr e, GuardValue v) { isLocalUrlSanitizerMethodCall(g, e, v) } @@ -137,14 +137,14 @@ class LocalUrlSanitizer extends Sanitizer { /** * An argument to a call to `List.Contains()` that is a sanitizer for URL redirects. */ -private predicate isContainsUrlSanitizer(Guard guard, Expr e, AbstractValue v) { +private predicate isContainsUrlSanitizer(Guard guard, Expr e, GuardValue v) { guard = any(MethodCall method | exists(Method m | m = method.getTarget() | m.hasName("Contains") and e = method.getArgument(0) ) and - v.(AbstractValues::BooleanValue).getValue() = true + v.asBooleanValue() = true ) } @@ -163,12 +163,12 @@ class ContainsUrlSanitizer extends Sanitizer { /** * A check that the URL is relative, and therefore safe for URL redirects. */ -private predicate isRelativeUrlSanitizer(Guard guard, Expr e, AbstractValue v) { +private predicate isRelativeUrlSanitizer(Guard guard, Expr e, GuardValue v) { guard = any(PropertyAccess access | access.getProperty().hasFullyQualifiedName("System", "Uri", "IsAbsoluteUri") and e = access.getQualifier() and - v.(AbstractValues::BooleanValue).getValue() = false + v.asBooleanValue() = false ) } @@ -185,16 +185,14 @@ class RelativeUrlSanitizer extends Sanitizer { * A comparison on the `Host` property of a url, that is a sanitizer for URL redirects. * E.g. `url.Host == "example.org"` */ -private predicate isHostComparisonSanitizer(Guard guard, Expr e, AbstractValue v) { +private predicate isHostComparisonSanitizer(Guard guard, Expr e, GuardValue v) { guard = any(EqualityOperation comparison | exists(PropertyAccess access | access = comparison.getAnOperand() | access.getProperty().hasFullyQualifiedName("System", "Uri", "Host") and e = access.getQualifier() ) and - if comparison instanceof EQExpr - then v.(AbstractValues::BooleanValue).getValue() = true - else v.(AbstractValues::BooleanValue).getValue() = false + if comparison instanceof EQExpr then v.asBooleanValue() = true else v.asBooleanValue() = false ) } diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll index 1639563e9640..4a2b27591433 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll @@ -130,7 +130,7 @@ class SubstringSanitizer extends Sanitizer { } } -private predicate stringCheckGuard(Guard g, Expr e, AbstractValue v) { +private predicate stringCheckGuard(Guard g, Expr e, GuardValue v) { g.(MethodCall).getTarget().hasFullyQualifiedName("System", "String", "StartsWith") and g.(MethodCall).getQualifier() = e and // A StartsWith check against Path.Combine is not sufficient, because the ".." elements have @@ -139,7 +139,7 @@ private predicate stringCheckGuard(Guard g, Expr e, AbstractValue v) { combineCall.getTarget().hasFullyQualifiedName("System.IO", "Path", "Combine") and DataFlow::localExprFlow(combineCall, e) ) and - v.(AbstractValues::BooleanValue).getValue() = true + v.asBooleanValue() = true } /** diff --git a/csharp/ql/src/experimental/CWE-918/RequestForgery.qll b/csharp/ql/src/experimental/CWE-918/RequestForgery.qll index 9ab1351f4142..84ea534a50fa 100644 --- a/csharp/ql/src/experimental/CWE-918/RequestForgery.qll +++ b/csharp/ql/src/experimental/CWE-918/RequestForgery.qll @@ -133,14 +133,14 @@ module RequestForgery { * to be a guard for Server Side Request Forgery(SSRF) Vulnerabilities. * This guard considers all checks as valid. */ - private predicate baseUriGuard(Guard g, Expr e, AbstractValue v) { + private predicate baseUriGuard(Guard g, Expr e, GuardValue v) { g.(MethodCall).getTarget().hasFullyQualifiedName("System", "Uri", "IsBaseOf") and // we consider any checks against the tainted value to sainitize the taint. // This implies any check such as shown below block the taint flow. // Uri url = new Uri("whitelist.com") // if (url.isBaseOf(`taint1)) (e = g.(MethodCall).getArgument(0) or e = g.(MethodCall).getQualifier()) and - v.(AbstractValues::BooleanValue).getValue() = true + v.asBooleanValue() = true } private class BaseUriBarrier extends Barrier { @@ -152,14 +152,14 @@ module RequestForgery { * to be a guard for Server Side Request Forgery(SSRF) Vulnerabilities. * This guard considers all checks as valid. */ - private predicate stringStartsWithGuard(Guard g, Expr e, AbstractValue v) { + private predicate stringStartsWithGuard(Guard g, Expr e, GuardValue v) { g.(MethodCall).getTarget().hasFullyQualifiedName("System", "String", "StartsWith") and // Any check such as the ones shown below // "https://myurl.com/".startsWith(`taint`) // `taint`.startsWith("https://myurl.com/") // are assumed to sainitize the taint (e = g.(MethodCall).getQualifier() or g.(MethodCall).getArgument(0) = e) and - v.(AbstractValues::BooleanValue).getValue() = true + v.asBooleanValue() = true } private class StringStartsWithBarrier extends Barrier { diff --git a/csharp/ql/test/library-tests/controlflow/guards/AbstractValue.ql b/csharp/ql/test/library-tests/controlflow/guards/AbstractValue.ql index a28da604ff62..8d86e312cb45 100644 --- a/csharp/ql/test/library-tests/controlflow/guards/AbstractValue.ql +++ b/csharp/ql/test/library-tests/controlflow/guards/AbstractValue.ql @@ -1,6 +1,6 @@ import csharp private import semmle.code.csharp.controlflow.Guards -query predicate abstractValue(AbstractValue value, Expr e) { +query predicate abstractValue(GuardValue value, Expr e) { Guards::InternalUtil::exprHasValue(e, value) and e.fromSource() } diff --git a/csharp/ql/test/library-tests/controlflow/guards/Collections.ql b/csharp/ql/test/library-tests/controlflow/guards/Collections.ql index 1d5d3ad5a6e0..578a08de3079 100644 --- a/csharp/ql/test/library-tests/controlflow/guards/Collections.ql +++ b/csharp/ql/test/library-tests/controlflow/guards/Collections.ql @@ -2,7 +2,7 @@ import csharp private import semmle.code.csharp.controlflow.Guards query predicate emptinessCheck( - Expr check, EnumerableCollectionExpr collection, AbstractValue v, boolean isEmpty + Expr check, EnumerableCollectionExpr collection, GuardValue v, boolean isEmpty ) { check = collection.getAnEmptinessCheck(v, isEmpty) } diff --git a/csharp/ql/test/library-tests/controlflow/guards/GuardedControlFlowNode.ql b/csharp/ql/test/library-tests/controlflow/guards/GuardedControlFlowNode.ql index 82d843a2a411..183a097c27e4 100644 --- a/csharp/ql/test/library-tests/controlflow/guards/GuardedControlFlowNode.ql +++ b/csharp/ql/test/library-tests/controlflow/guards/GuardedControlFlowNode.ql @@ -1,5 +1,5 @@ import csharp import semmle.code.csharp.controlflow.Guards -from GuardedControlFlowNode gcfn, Expr sub, AbstractValue v +from GuardedControlFlowNode gcfn, Expr sub, GuardValue v select gcfn, gcfn.getAGuard(sub, v), sub, v diff --git a/csharp/ql/test/library-tests/controlflow/guards/GuardedExpr.ql b/csharp/ql/test/library-tests/controlflow/guards/GuardedExpr.ql index 4cdb4606dac3..c825873677a7 100644 --- a/csharp/ql/test/library-tests/controlflow/guards/GuardedExpr.ql +++ b/csharp/ql/test/library-tests/controlflow/guards/GuardedExpr.ql @@ -1,5 +1,5 @@ import csharp import semmle.code.csharp.controlflow.Guards -from GuardedExpr ge, Expr sub, AbstractValue v +from GuardedExpr ge, Expr sub, GuardValue v select ge, ge.getAGuard(sub, v), sub, v diff --git a/csharp/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.ql b/csharp/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.ql index 5d63ff124caa..08579f1528f2 100644 --- a/csharp/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.ql +++ b/csharp/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.ql @@ -5,10 +5,8 @@ import csharp import semmle.code.csharp.controlflow.Guards -private predicate stringConstCompare(Guard guard, Expr testedNode, AbstractValue value) { - guard - .isEquality(any(StringLiteral lit), testedNode, - value.(AbstractValues::BooleanValue).getValue()) +private predicate stringConstCompare(Guard guard, Expr testedNode, GuardValue value) { + guard.isEquality(any(StringLiteral lit), testedNode, value.asBooleanValue()) } class StringConstCompareBarrier extends DataFlow::Node { diff --git a/csharp/ql/test/library-tests/dataflow/callablereturnsarg/Common.qll b/csharp/ql/test/library-tests/dataflow/callablereturnsarg/Common.qll index ae9b56cd0385..3a3a55e42ccc 100644 --- a/csharp/ql/test/library-tests/dataflow/callablereturnsarg/Common.qll +++ b/csharp/ql/test/library-tests/dataflow/callablereturnsarg/Common.qll @@ -18,9 +18,7 @@ module Config implements DataFlow::ConfigSig { } predicate isBarrier(DataFlow::Node node) { - exists(AbstractValues::NullValue nv | node.(GuardedDataFlowNode).mustHaveValue(nv) | - nv.isNull() - ) + exists(GuardValue nv | node.(GuardedDataFlowNode).mustHaveValue(nv) | nv.isNullValue()) } } From fa20075a4dd93204e125bfe8f27ff0a6c830a28a Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Fri, 31 Oct 2025 14:41:32 +0100 Subject: [PATCH 2/2] C#: Review fix and simplification. --- .../semmle/code/csharp/controlflow/Guards.qll | 27 +------------------ 1 file changed, 1 insertion(+), 26 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll index ed0238d1f093..32519ba2348a 100644 --- a/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll +++ b/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll @@ -374,7 +374,6 @@ deprecated module AbstractValues { } } -// private import AbstractValues /** Gets the value resulting from matching `null` against `pat`. */ private boolean patternMatchesNull(PatternExpr pat) { pat instanceof NullLiteral and result = true @@ -432,35 +431,11 @@ class DereferenceableExpr extends Expr { /** Holds if this expression has a nullable type `T?`. */ predicate hasNullableType() { isNullableType = true } - /** - * Gets an expression that tests via nullness whether this expression is `null`. - * - * If the returned expression evaluates to `null` (`v.isNullValue()`) or evaluates to - * non-`null` (`not v.isNullValue()`), then this expression is guaranteed to be `null` - * if `isNull` is true, and non-`null` if `isNull` is false. - * - * For example, if `x` evaluates to `null` in `x ?? y` then `y` is evaluated, and - * `x` is guaranteed to be `null`. - */ - private Expr getANullnessNullCheck(GuardValue v, boolean isNull) { - exists(NullnessCompletion c | c.isValidFor(this) | - result = this and - if c.isNull() - then ( - v.isNullValue() and - isNull = true - ) else ( - v.isNonNullValue() and - isNull = false - ) - ) - } - /** Holds if `guard` suggests that this expression may be `null`. */ predicate guardSuggestsMaybeNull(Guards::Guard guard) { not nonNullValueImplied(this) and ( - guard = this.getANullnessNullCheck(_, true) + exists(NullnessCompletion c | c.isValidFor(this) and c.isNull() and guard = this) or LogicInput::additionalNullCheck(guard, _, this, true) or