Go: migrate remaining guard consumers

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

Author: owen-mc

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"

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,28 +42,29 @@ 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)
   )
 }
 
-predicate isDominatingLengthLEGuard(
-  ControlFlow::ConditionGuardNode guard, Index index, DataFlow::SsaNode array, BasicBlock bb
+predicate isControllingLengthLeGuard(
+  Guard guard, boolean branch, Index index, DataFlow::SsaNode array, BasicBlock bb
 ) {
-  guard = getLengthLEGuard(index, array) and
-  guard.dominates(bb)
+  lengthLeGuard(guard, branch, index, array) and
+  guard.controls(bb, branch)
 }
 
 /**
- * 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))
 }
 
 /**
@@ -85,26 +87,27 @@ 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 read from `array[index]`
   elementRead(ea, array, index, bb) and
   // and it is guarded by a comparison `index <= len(array)`
-  isDominatingLengthLEGuard(cond, index, array, bb) and
+  isControllingLengthLeGuard(cond, branch, index, array, bb) and
   // and report the innermost guard that establishes the comparison
-  not exists(ControlFlow::ConditionGuardNode innerCond |
-    isDominatingLengthLEGuard(innerCond, index, array, bb) and
-    innerCond.getCondition().getParent+() = cond.getCondition()
+  not exists(Guard innerCond |
+    isControllingLengthLeGuard(innerCond, _, index, array, bb) and
+    innerCond.(Expr).getParent+() = cond.(Expr)
   ) 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 |
     index = ConstantIndex(i) and index2 = ConstantIndex(i2) and i < i2
   |
-    isDominatingLengthLEGuard(_, index2, array, bb)
+    isControllingLengthLeGuard(_, _, index2, array, bb)
   ) 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"

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

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,11 +60,7 @@ 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())
-    )
+    flagControls(any(AllowedFlag f), node.getBasicBlock())
   }
 
   predicate isSink(DataFlow::Node sink) { isSinkHW(sink, _) }
@@ -171,9 +168,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 +199,7 @@ module FromUntrustedConfig implements DataFlow::ConfigSig {
         )
       )
     |
-      cgn.getCondition() = ifs.getCondition()
+      guard = ifs.getCondition()
     )
   }
 }
@@ -217,10 +214,10 @@ module FromUntrustedFlow = TaintTracking::Global<FromUntrustedConfig>;
  * 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 +230,5 @@ 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 flagControls(any(AllowedFlag f), allowOriginHW.getBasicBlock())
 select allowOriginHW, message

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

go/ql/test/experimental/CWE-942/CorsMisconfiguration.expected

@@ -7,7 +7,6 @@
 | CorsMisconfiguration.go:53:4:53:44 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` |
 | CorsMisconfiguration.go:60:4:60:56 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` |
 | CorsMisconfiguration.go:67:5:67:57 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` |
-| CorsMisconfiguration.go:222:5:222:57 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` |
 | CorsMisconfiguration.go:224:5:224:57 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` |
 | RsCors.go:11:21:11:59 | slice literal | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` |
 | RsCors.go:31:23:31:61 | slice literal | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` |

go/ql/test/experimental/CWE-942/CorsMisconfiguration.go

@@ -219,7 +219,7 @@ func main() {
 			origin := req.Header.Get("origin")
 			switch req.Method {
 			case "allowed":
-				w.Header().Set("Access-Control-Allow-Origin", origin) // $ SPURIOUS: Alert
+				w.Header().Set("Access-Control-Allow-Origin", origin)
 			default:
 				w.Header().Set("Access-Control-Allow-Origin", origin) // $ Alert
 			}