Merge pull request #16234 from owen-mc/go/incorrect-integer-conversion-type-switch-fp

Go: Fix FPs in `go/incorrect-integer-conversion` query

Author: owen-mc

go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll

@@ -258,13 +258,14 @@ private class MaxValueState extends TMaxValueState {
  * A node that blocks some flow states and transforms some others as they flow
  * through it.
  */
-abstract class BarrierFlowStateTransformer extends DataFlow::Node {
+abstract class FlowStateTransformer extends DataFlow::Node {
   /**
-   * Holds if this should be a barrier for `flowstate`.
+   * Holds if this should be a barrier for a flow state with bit size `bitSize`
+   * and architecture bit size `architectureBitSize`.
    *
    * This includes flow states which are transformed into other flow states.
    */
-  abstract predicate barrierFor(MaxValueState flowstate);
+  abstract predicate barrierFor(int bitSize, int architectureBitSize);
 
   /**
    * Gets the flow state that `flowstate` is transformed into.
@@ -275,7 +276,20 @@ abstract class BarrierFlowStateTransformer extends DataFlow::Node {
    * transform(transform(x)) = transform(x)
    * ```
    */
-  abstract MaxValueState transform(MaxValueState flowstate);
+  MaxValueState transform(MaxValueState state) {
+    this.barrierFor(state.getBitSize(), state.getSinkBitSize()) and
+    result.getBitSize() =
+      max(int bitsize |
+        bitsize = validBitSize() and
+        bitsize < state.getBitSize() and
+        not this.barrierFor(bitsize, state.getSinkBitSize())
+      ) and
+    (
+      result.getArchitectureBitSize() = state.getArchitectureBitSize()
+      or
+      state.architectureBitSizeUnknown() and result.architectureBitSizeUnknown()
+    )
+  }
 }
 
 private predicate upperBoundCheckGuard(DataFlow::Node g, Expr e, boolean branch) {
@@ -372,31 +386,69 @@ class UpperBoundCheckGuard extends DataFlow::RelationalComparisonNode {
  * for all flow states and would not transform any flow states, thus
  * effectively blocking them.
  */
-class UpperBoundCheck extends BarrierFlowStateTransformer {
+class UpperBoundCheck extends FlowStateTransformer {
   UpperBoundCheckGuard g;
 
   UpperBoundCheck() {
     this = DataFlow::BarrierGuard<upperBoundCheckGuard/3>::getABarrierNodeForGuard(g)
   }
 
-  override predicate barrierFor(MaxValueState flowstate) {
-    g.isBoundFor(flowstate.getBitSize(), flowstate.getSinkBitSize())
+  override predicate barrierFor(int bitSize, int architectureBitSize) {
+    g.isBoundFor(bitSize, architectureBitSize)
   }
+}
 
-  override MaxValueState transform(MaxValueState state) {
-    this.barrierFor(state) and
-    result.getBitSize() =
-      max(int bitsize |
-        bitsize = validBitSize() and
-        bitsize < state.getBitSize() and
-        not g.isBoundFor(bitsize, state.getSinkBitSize())
-      ) and
-    (
-      result.getArchitectureBitSize() = state.getArchitectureBitSize()
-      or
-      state.architectureBitSizeUnknown() and result.architectureBitSizeUnknown()
+private predicate integerTypeBound(IntegerType it, int bitSize, int architectureBitSize) {
+  bitSize = validBitSize() and
+  architectureBitSize = [32, 64] and
+  exists(int offset | if it instanceof SignedIntegerType then offset = 1 else offset = 0 |
+    if it instanceof IntType or it instanceof UintType
+    then bitSize >= architectureBitSize - offset
+    else bitSize >= it.getSize() - offset
+  )
+}
+
+/**
+ * An expression which a type assertion guarantees will have a particular
+ * integer type.
+ *
+ * If this is a checked type expression then this value will only be used if
+ * the type assertion succeeded. If it is not checked then there will be a
+ * run-time panic if the type assertion fails, so we can assume it succeeded.
+ */
+class TypeAssertionCheck extends DataFlow::ExprNode, FlowStateTransformer {
+  IntegerType it;
+
+  TypeAssertionCheck() {
+    exists(TypeAssertExpr tae |
+      this = DataFlow::exprNode(tae.getExpr()) and
+      it = tae.getTypeExpr().getType()
+    )
+  }
+
+  override predicate barrierFor(int bitSize, int architectureBitSize) {
+    integerTypeBound(it, bitSize, architectureBitSize)
+  }
+}
+
+/**
+ * The implicit definition of a variable with integer type for a case clause of
+ * a type switch statement which declares a variable in its guard, which has
+ * effectively had a checked type assertion.
+ */
+class TypeSwitchVarFlowStateTransformer extends DataFlow::SsaNode, FlowStateTransformer {
+  IntegerType it;
+
+  TypeSwitchVarFlowStateTransformer() {
+    exists(IR::TypeSwitchImplicitVariableInstruction insn, LocalVariable lv | insn.writes(lv, _) |
+      this.getSourceVariable() = lv and
+      it = lv.getType()
     )
   }
+
+  override predicate barrierFor(int bitSize, int architectureBitSize) {
+    integerTypeBound(it, bitSize, architectureBitSize)
+  }
 }
 
 /**
@@ -497,7 +549,10 @@ private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConf
 
   predicate isBarrier(DataFlow::Node node, FlowState state) {
     // Safely guarded by a barrier guard.
-    exists(BarrierFlowStateTransformer bfst | node = bfst and bfst.barrierFor(state))
+    exists(FlowStateTransformer fst |
+      node = fst and
+      fst.barrierFor(state.getBitSize(), state.getSinkBitSize())
+    )
     or
     // When there is a flow from a source to a sink, do not allow the flow to
     // continue to a further sink.
@@ -507,8 +562,8 @@ private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConf
   predicate isAdditionalFlowStep(
     DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2
   ) {
-    // Create additional flow steps for `BarrierFlowStateTransformer`s
-    state2 = node2.(BarrierFlowStateTransformer).transform(state1) and
+    // Create additional flow steps for `FlowStateTransformer`s
+    state2 = node2.(FlowStateTransformer).transform(state1) and
     DataFlow::simpleLocalFlowStep(node1, node2, _)
   }
 }

go/ql/src/change-notes/2024-04-17-incorrect-integer-conversion-barriers.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Added some more barriers to flow for `go/incorrect-integer-conversion` to reduce false positives, especially around type switches.

go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go

@@ -482,19 +482,61 @@ func parsePositiveInt2(value string) (int, error) {
 	return int(i64), nil
 }
 
-func typeAssertion(s string) {
-	n, err := strconv.ParseInt(s, 10, 0)
-	if err == nil {
-		var itf interface{} = n
-		i32 := itf.(int32)
-		println(i32)
-	}
-
-}
-
 func dealWithArchSizeCorrectly(s string) uint {
 	if i, err := strconv.ParseUint(s, 10, 64); err == nil && i < math.MaxUint {
 		return uint(i)
 	}
 	return 0
 }
+
+func typeSwitch1(s string) {
+	i64, _ := strconv.ParseInt(s, 10, 64)
+	var input any = i64
+	switch v := input.(type) {
+	case int16, string:
+		if _, ok := input.(string); ok {
+			return
+		}
+		_ = int16(v.(int16))
+		_ = int8(v.(int16)) // $ hasValueFlow="type assertion"
+	case int32:
+		_ = int32(v)
+		_ = int8(v) // $ hasValueFlow="v"
+	case int64:
+		_ = int8(v) // $ hasValueFlow="v"
+	default:
+		_ = int8(v.(int64)) // $ hasValueFlow="type assertion"
+	}
+}
+
+func typeSwitch2(s string) {
+	i64, _ := strconv.ParseInt(s, 10, 64)
+	var input any = i64
+	switch input.(type) {
+	case int16, string:
+		if _, ok := input.(string); ok {
+			return
+		}
+		_ = int16(input.(int16))
+		_ = int8(input.(int16)) // $ hasValueFlow="type assertion"
+	case int32:
+		_ = int32(input.(int32))
+		_ = int8(input.(int32)) // $ hasValueFlow="type assertion"
+	case int64:
+		_ = int8(input.(int64)) // $ hasValueFlow="type assertion"
+	default:
+		_ = int8(input.(int64)) // $ hasValueFlow="type assertion"
+	}
+}
+
+func checkedTypeAssertion(s string) {
+	i64, _ := strconv.ParseInt(s, 10, 64)
+	var input any = i64
+	if v, ok := input.(int16); ok {
+		// Need to account for the fact that within this case clause, v is an int16
+		_ = int16(v)
+		_ = int8(v) // $ hasValueFlow="v"
+	} else if v, ok := input.(int32); ok {
+		_ = int16(v) // $ hasValueFlow="v"
+	}
+}