Go: migrate simple guard consumers

Move simple data-flow and security consumers to shared guards, including value-aware barriers and the corrected feature-flag semantics.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: owen-mc

go/ql/lib/semmle/go/dataflow/ExternalFlow.qll

@@ -104,6 +104,7 @@ overlay[local?]
 module;
 
 private import go
+private import semmle.go.controlflow.Guards
 private import internal.ExternalFlowExtensions::Extensions as Extensions
 private import FlowSummary as FlowSummary
 private import internal.DataFlowPrivate
@@ -459,24 +460,23 @@ private module Cached {
   private newtype TKindModelPair =
     TMkPair(string kind, string model) { isBarrierGuardNode(_, _, kind, model) }
 
-  private boolean convertAcceptingValue(Public::AcceptingValue av) {
-    av.isTrue() and result = true
+  private GuardValue convertAcceptingValue(Public::AcceptingValue av) {
+    av.isTrue() and result.asBooleanValue() = true
     or
-    av.isFalse() and result = false
-    // Remaining cases are not supported yet, they depend on the shared Guards library.
-    // or
-    // av.isNoException() and result.getDualValue().isThrowsException()
-    // or
-    // av.isZero() and result.asIntValue() = 0
-    // or
-    // av.isNotZero() and result.getDualValue().asIntValue() = 0
-    // or
-    // av.isNull() and result.isNullValue()
-    // or
-    // av.isNotNull() and result.isNonNullValue()
+    av.isFalse() and result.asBooleanValue() = false
+    or
+    av.isNoException() and result.getDualValue().isThrowsException()
+    or
+    av.isZero() and result.asIntValue() = 0
+    or
+    av.isNotZero() and result.getDualValue().asIntValue() = 0
+    or
+    av.isNull() and result.isNullValue()
+    or
+    av.isNotNull() and result.isNonNullValue()
   }
 
-  private predicate barrierGuardChecks(DataFlow::Node g, Expr e, boolean gv, TKindModelPair kmp) {
+  private predicate barrierGuardChecks(Guard g, Expr e, GuardValue gv, TKindModelPair kmp) {
     exists(
       SourceSinkInterpretationInput::InterpretNode n, Public::AcceptingValue acceptingValue,
       string kind, string model
@@ -486,7 +486,7 @@ private module Cached {
       kmp = TMkPair(kind, model) and
       gv = convertAcceptingValue(acceptingValue)
     |
-      g.asExpr().(CallExpr).getAnArgument() = e // TODO: qualifier?
+      g.(CallExpr).getAnArgument() = e // TODO: qualifier?
     )
   }
 
@@ -500,7 +500,7 @@ private module Cached {
       isBarrierNode(n, kind, model) and n.asNode() = node
     )
     or
-    DataFlow::ParameterizedBarrierGuard<TKindModelPair, barrierGuardChecks/4>::getABarrierNode(TMkPair(kind,
+    DataFlow::ParameterizedBarrierGuardValue<TKindModelPair, barrierGuardChecks/4>::getABarrierNode(TMkPair(kind,
         model)) = node
   }
 }

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)
+  )
 }
 
 /**

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,30 @@ 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 |
+      flag = flagKind.getAFlag() and
+      guard = flag.getANode().asExpr() and
+      (
+        exists(CaseClause cc, ExpressionSwitchStmt switch |
+          guard.getParent() = cc and
+          cc = switch.getACase() and
+          exists(switch.getExpr()) and
+          caseExpressionMatchControls(guard, block)
+        )
+        or
+        not exists(CaseClause cc, ExpressionSwitchStmt switch |
+          guard.getParent() = cc and
+          cc = switch.getACase() and
+          exists(switch.getExpr())
+        ) and
+        guard.controls(block, _)
+      )
+    )
   }
 }

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)
   )
 }
 

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,11 +57,7 @@ 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())
-    )
+    flagControls(any(DebugModeFlag f), node.getBasicBlock())
   }
 
   predicate observeDiffInformedIncrementalMode() { any() }

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())

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

go/ql/test/library-tests/semmle/go/controlflow/Guards/Guards.expected

@@ -266,3 +266,8 @@ defaultCaseResult
 | guards.go:164:2:165:32 | case clause | expressionless default | true |
 modelBarrierResult
 | guards.go:351:27:351:33 | pointer | model bool | model-bool |
+| guards.go:354:27:354:33 | pointer | model zero | model-zero |
+| guards.go:357:31:357:37 | pointer | model not zero | model-not-zero |
+| guards.go:360:27:360:33 | pointer | model null | model-null |
+| guards.go:363:31:363:37 | pointer | model not null | model-not-null |
+| guards.go:366:34:366:40 | pointer | model no exception | model-no-exception |

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"
 		}

go/ql/test/query-tests/Security/CWE-209/StackTraceExposure.expected

@@ -1,16 +1,13 @@
 #select
 | test.go:18:10:18:12 | buf | test.go:15:28:15:30 | buf [postupdate] | test.go:18:10:18:12 | buf | HTTP response depends on $@ and may be exposed to an external user. | test.go:15:28:15:30 | buf [postupdate] | stack trace information |
-| test.go:44:11:44:13 | buf | test.go:15:28:15:30 | buf [postupdate] | test.go:44:11:44:13 | buf | HTTP response depends on $@ and may be exposed to an external user. | test.go:15:28:15:30 | buf [postupdate] | stack trace information |
 | test.go:46:11:46:13 | buf | test.go:15:28:15:30 | buf [postupdate] | test.go:46:11:46:13 | buf | HTTP response depends on $@ and may be exposed to an external user. | test.go:15:28:15:30 | buf [postupdate] | stack trace information |
 edges
 | test.go:15:28:15:30 | buf [postupdate] | test.go:18:10:18:12 | buf | provenance |  |
 | test.go:15:28:15:30 | buf [postupdate] | test.go:21:29:21:31 | buf | provenance |  |
-| test.go:21:29:21:31 | buf | test.go:44:11:44:13 | buf | provenance |  |
 | test.go:21:29:21:31 | buf | test.go:46:11:46:13 | buf | provenance |  |
 nodes
 | test.go:15:28:15:30 | buf [postupdate] | semmle.label | buf [postupdate] |
 | test.go:18:10:18:12 | buf | semmle.label | buf |
 | test.go:21:29:21:31 | buf | semmle.label | buf |
-| test.go:44:11:44:13 | buf | semmle.label | buf |
 | test.go:46:11:46:13 | buf | semmle.label | buf |
 subpaths