handle cases when convert instruction is after the if blocks

Signed-off-by: czechbol <[email protected]>
securego · Aug 28, 2024 · 72db69f · 72db69f
1 parent 4f3c567
commit 72db69f
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 64 deletions.
diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go
@@ -149,7 +149,6 @@ func isStringToIntConversion(instr *ssa.Convert, dstType string) bool {
 }
 
 func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool {
-	block := instr.Block()
 	dstInt, err := parseIntType(dstType)
 	if err != nil {
 		return false
@@ -168,85 +167,96 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool {
 		return true
 	}
 
-	// Recursive depth-first search of predecessor blocks of the SSA function to find bounds checks on the value being converted
-	var checkPredecessors func(block *ssa.BasicBlock, depth int) bool
-	checkPredecessors = func(block *ssa.BasicBlock, depth int) bool {
-		if depth > maxDepth {
-			return false
-		}
-
-		for _, pred := range block.Preds {
-			minChecked, maxChecked := checkBlockForRangeCheck(pred, instr, dstInt)
-
+	visitedIfs := make(map[*ssa.If]bool)
+	for _, block := range instr.Parent().Blocks {
+		var minChecked, maxChecked bool
+		for _, blockInstr := range block.Instrs {
+			switch v := blockInstr.(type) {
+			case *ssa.If:
+				minChecked, maxChecked = checkIfForRangeCheck(v, instr, dstInt, visitedIfs)
+			case *ssa.Call:
+				// len(slice) results in an int that is guaranteed >= 0, which
+				// satisfies the lower bound check for int -> uint conversion
+				if v != instr.X {
+					continue
+				}
+				if fn, isBuiltin := v.Call.Value.(*ssa.Builtin); isBuiltin && fn.Name() == "len" && !dstInt.signed {
+					minChecked = true
+				}
+			case *ssa.Convert:
+				if v == instr {
+					break
+				}
+			}
 			minBoundChecked = minBoundChecked || minChecked
 			maxBoundChecked = maxBoundChecked || maxChecked
 
 			if minBoundChecked && maxBoundChecked {
 				return true
 			}
-			if checkPredecessors(pred, depth+1) {
-				return true
-			}
 		}
-		return false
 	}
+	return false
+}
 
-	// Start checking from the initial block
-	checkPredecessors(block, 0)
+func checkIfForRangeCheck(ifInstr *ssa.If, instr *ssa.Convert, dstInt integer, visitedIfs map[*ssa.If]bool) (minBoundChecked, maxBoundChecked bool) {
+	if visitedIfs[ifInstr] {
+		// If the if instruction has already been visited, we can skip it
+		return false, false
+	}
+	visitedIfs[ifInstr] = true
 
-	if minBoundChecked && maxBoundChecked {
-		return true
+	// check Instrs for other bound checks
+	condBlock := ifInstr.Block()
+	if succIf, ok := condBlock.Succs[1].Instrs[1].(*ssa.If); ok {
+		// this is an OR condition and should be sufficient if it contains the other bound check
+		minBoundChecked, maxBoundChecked = checkIfForRangeCheck(succIf, instr, dstInt, visitedIfs)
 	}
 
-	return false
-}
+	// check the instructions of the if block for other bound checks
+	for _, succ := range condBlock.Succs {
+		for _, blockInstr := range succ.Instrs {
+			if succIf, ok := blockInstr.(*ssa.If); ok {
+				// this is an AND condition and is insufficient to check the bounds but we need to visit it
+				// because walking the parent block will visit it again
+				_, _ = checkIfForRangeCheck(succIf, instr, dstInt, visitedIfs)
+			}
+		}
+	}
 
-func checkBlockForRangeCheck(block *ssa.BasicBlock, instr *ssa.Convert, dstInt integer) (minBoundChecked, maxBoundChecked bool) {
-	for _, i := range block.Instrs {
-		switch v := i.(type) {
-		case *ssa.BinOp:
-			if isBoundCheck(v, instr.X) {
-				constVal, isOnLeft := constFromBoundCheck(v)
-				if constVal == nil {
-					continue
-				}
+	cond := ifInstr.Cond
+	// Check if the condition is a bound check
+	if binOp, ok := cond.(*ssa.BinOp); ok {
+		if isBoundCheck(binOp, instr.X) {
+			constVal, isOnLeft := constFromBoundCheck(binOp)
+			if constVal == nil {
+				return false, false
+			}
 
-				value := constVal.Int64()
+			value := constVal.Int64()
 
-				if isOnLeft {
-					if v.Op == token.LSS || v.Op == token.LEQ {
-						maxBoundChecked = maxBoundChecked || checkMaxBoundValue(value, dstInt)
-					}
-					if v.Op == token.GTR || v.Op == token.GEQ {
-						minBoundChecked = minBoundChecked || checkMinBoundValue(value, dstInt)
-					}
-				} else {
-					if v.Op == token.LSS || v.Op == token.LEQ {
-						minBoundChecked = minBoundChecked || checkMinBoundValue(value, dstInt)
-					}
-					if v.Op == token.GTR || v.Op == token.GEQ {
-						maxBoundChecked = maxBoundChecked || checkMaxBoundValue(value, dstInt)
-					}
+			if isOnLeft {
+				if binOp.Op == token.LSS || binOp.Op == token.LEQ {
+					newMaxCheck := checkMaxBoundValue(value, dstInt)
+					maxBoundChecked = maxBoundChecked || newMaxCheck
+				}
+				if binOp.Op == token.GTR || binOp.Op == token.GEQ {
+					newMinCheck := checkMinBoundValue(value, dstInt)
+					minBoundChecked = minBoundChecked || newMinCheck
+				}
+			} else {
+				if binOp.Op == token.LSS || binOp.Op == token.LEQ {
+					newMinCheck := checkMinBoundValue(value, dstInt)
+					minBoundChecked = minBoundChecked || newMinCheck
+				}
+				if binOp.Op == token.GTR || binOp.Op == token.GEQ {
+					newMaxCheck := checkMaxBoundValue(value, dstInt)
+					maxBoundChecked = maxBoundChecked || newMaxCheck
 				}
 			}
-		case *ssa.Call:
-			// len(slice) results in an int that is guaranteed >= 0, which
-			// satisfies the lower bound check for int -> uint conversion
-			if v != instr.X {
-				continue
-			}
-			if fn, isBuiltin := v.Call.Value.(*ssa.Builtin); isBuiltin && fn.Name() == "len" && !dstInt.signed {
-				minBoundChecked = true
-			}
-		case *ssa.Phi:
-			// Handle logical operations
-			continue
-		}
-
-		if minBoundChecked && maxBoundChecked {
-			break
 		}
 	}
+
 	return minBoundChecked, maxBoundChecked
 }
 

diff --git a/testutils/g115_samples.go b/testutils/g115_samples.go
@@ -322,7 +322,7 @@ func main() {
         fmt.Printf("%d\n", b)
 }
 	`,
-	}, 0, gosec.NewConfig()},
+	}, 1, gosec.NewConfig()},
 	{[]string{
 		`
 package main
@@ -342,7 +342,7 @@ func main() {
         fmt.Printf("%d\n", b)
 }
 	`,
-	}, 1, gosec.NewConfig()},
+	}, 0, gosec.NewConfig()},
 	{[]string{
 		`
 package main
@@ -355,7 +355,7 @@ import (
 
 func main() {
         a := rand.Int63()
-        if a < math.MinInt64 && a > math.MaxInt32 {
+        if a < math.MinInt64 || a > math.MaxInt32 {
             panic("out of range")
         }
         b := int32(a)