From d2b4d2f45c13a53c539746343a1fd7f05c227ed8 Mon Sep 17 00:00:00 2001 From: czechbol Date: Wed, 28 Aug 2024 16:02:10 +0200 Subject: [PATCH 01/21] add test cases Signed-off-by: czechbol --- testutils/g115_samples.go | 87 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 86 insertions(+), 1 deletion(-) diff --git a/testutils/g115_samples.go b/testutils/g115_samples.go index 9d264d8aef..6aa5e2cae3 100644 --- a/testutils/g115_samples.go +++ b/testutils/g115_samples.go @@ -287,10 +287,34 @@ package main import ( "fmt" "math" + "math/rand" ) func main() { - var a int64 = 13 + a := rand.Int63() + if a < math.MinInt32 { + panic("out of range") + } + if a > math.MaxInt32 { + panic("out of range") + } + b := int32(a) + fmt.Printf("%d\n", b) +} + `, + }, 0, gosec.NewConfig()}, + {[]string{ + ` +package main + +import ( + "fmt" + "math" + "math/rand" +) + +func main() { + a := rand.Int63() if a < math.MinInt32 || a > math.MaxInt32 { panic("out of range") } @@ -390,4 +414,65 @@ func main() { } `, }, 1, gosec.NewConfig()}, + {[]string{ + ` +package main + +import ( + "fmt" + "math" + "math/rand" +) + +func main() { + a := rand.Int63() + if a < 0 { + panic("out of range") + } + if a > math.MaxUint32 { + panic("out of range") + } + b := uint32(a) + fmt.Printf("%d\n", b) +} +`, + }, 0, gosec.NewConfig()}, + {[]string{ + ` +package main + +import ( + "fmt" + "math/rand" +) + +func main() { + a := rand.Int63() + if a < 0 { + panic("out of range") + } + b := uint32(a) + fmt.Printf("%d\n", b) +} +`, + }, 1, gosec.NewConfig()}, + {[]string{ + ` +package main + +import ( + "math" +) + +func foo(x int) uint32 { + if x < 0 { + return 0 + } + if x > math.MaxUint32 { + return math.MaxUint32 + } + return uint32(x) +} +`, + }, 0, gosec.NewConfig()}, } From fb90989d4dd0a24c2ad6d0d065aecea7a222ace9 Mon Sep 17 00:00:00 2001 From: czechbol Date: Wed, 28 Aug 2024 16:02:10 +0200 Subject: [PATCH 02/21] fix bounds check logic Signed-off-by: czechbol --- analyzers/conversion_overflow.go | 177 +++++++++++++++++++++++++++---- 1 file changed, 156 insertions(+), 21 deletions(-) diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go index 1449f945f7..483807e4d9 100644 --- a/analyzers/conversion_overflow.go +++ b/analyzers/conversion_overflow.go @@ -50,10 +50,10 @@ func runConversionOverflow(pass *analysis.Pass) (interface{}, error) { case *ssa.Convert: src := instr.X.Type().Underlying().String() dst := instr.Type().Underlying().String() - if isSafeConversion(instr) { - continue - } if isIntOverflow(src, dst) { + if isSafeConversion(instr) { + continue + } issue := newIssue(pass.Analyzer.Name, fmt.Sprintf("integer overflow conversion %s -> %s", src, dst), pass.Fset, @@ -84,13 +84,13 @@ func isSafeConversion(instr *ssa.Convert) bool { } } - // Check for explicit range checks - if hasExplicitRangeCheck(instr) { + // Check for string to integer conversions with specified bit size + if isStringToIntConversion(instr, dstType) { return true } - // Check for string to integer conversions with specified bit size - if isStringToIntConversion(instr, dstType) { + // Check for explicit range checks + if hasExplicitRangeCheck(instr, dstType) { return true } @@ -114,20 +114,6 @@ func isConstantInRange(constVal *ssa.Const, dstType string) bool { return value >= 0 && value <= (1<= -(1 << (dstInt.size - 1)) + } + // Source is unsigned and destination is signed + return 0 >= -(1 << (dstInt.size - 1)) + } + if srcInt.signed { + // Source is signed and destination is unsigned + return -(1 << (srcInt.size - 1)) >= 0 + } + // Both source and destination are unsigned + return true +} + +func checkSourceMaxBound(srcInt, dstInt integer) bool { + if dstInt.signed { + if srcInt.signed { + // Source and destination are both signed + return (1<<(srcInt.size-1))-1 <= (1<<(dstInt.size-1))-1 + } + // Source is unsigned and destination is signed + var a uint = (1 << srcInt.size) - 1 + var b uint = (1 << (dstInt.size - 1)) - 1 + return a <= b + } + // Destination is unsigned + if srcInt.signed { + // Source is signed and destination is unsigned + var a uint = (1 << (srcInt.size - 1)) - 1 + var b uint = (1 << dstInt.size) - 1 + return a <= b + } + // Both source and destination are unsigned + return (1<= 0 // For unsigned types, the minimum bound is always 0 +} + +func checkMaxBoundValue(value int64, dstInt integer) bool { + if dstInt.signed { + return value == (1<<(dstInt.size-1))-1 + } + return value == (1< Date: Wed, 28 Aug 2024 16:02:10 +0200 Subject: [PATCH 03/21] tweak test cases Signed-off-by: czechbol --- testutils/g115_samples.go | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/testutils/g115_samples.go b/testutils/g115_samples.go index 6aa5e2cae3..028b36f2d0 100644 --- a/testutils/g115_samples.go +++ b/testutils/g115_samples.go @@ -315,7 +315,7 @@ import ( func main() { a := rand.Int63() - if a < math.MinInt32 || a > math.MaxInt32 { + if a < math.MinInt32 && a > math.MaxInt32 { panic("out of range") } b := int32(a) @@ -335,7 +335,27 @@ import ( func main() { a := rand.Int63() - if a < math.MinInt64 || a > math.MaxInt32 { + if a < math.MinInt32 || a > math.MaxInt32 { + panic("out of range") + } + b := int32(a) + fmt.Printf("%d\n", b) +} + `, + }, 1, gosec.NewConfig()}, + {[]string{ + ` +package main + +import ( + "fmt" + "math" + "math/rand" +) + +func main() { + a := rand.Int63() + if a < math.MinInt64 && a > math.MaxInt32 { panic("out of range") } b := int32(a) @@ -354,7 +374,7 @@ import ( func main() { var a int32 = math.MaxInt32 - if a < math.MinInt32 || a > math.MaxInt32 { + if a < math.MinInt32 && a > math.MaxInt32 { panic("out of range") } var b int64 = int64(a) * 2 From ed4e6000f270e3b6dc4462f003ad177afccb5aba Mon Sep 17 00:00:00 2001 From: czechbol Date: Wed, 28 Aug 2024 16:02:10 +0200 Subject: [PATCH 04/21] fix codestyle Signed-off-by: czechbol --- analyzers/conversion_overflow.go | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go index 483807e4d9..daaefffef0 100644 --- a/analyzers/conversion_overflow.go +++ b/analyzers/conversion_overflow.go @@ -155,16 +155,13 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { return false } - minBoundChecked := false - maxBoundChecked := false - srcInt, err := parseIntType(instr.X.Type().String()) if err != nil { return false } - minBoundChecked = checkSourceMinBound(srcInt, dstInt) - maxBoundChecked = checkSourceMaxBound(srcInt, dstInt) + minBoundChecked := checkSourceMinBound(srcInt, dstInt) + maxBoundChecked := checkSourceMaxBound(srcInt, dstInt) // If both bounds are already checked, return true if minBoundChecked && maxBoundChecked { @@ -176,12 +173,10 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { checkPredecessors = func(block *ssa.BasicBlock) bool { for _, pred := range block.Preds { minChecked, maxChecked := checkBlockForRangeCheck(pred, instr, dstInt) - if minChecked { - minBoundChecked = true - } - if maxChecked { - maxBoundChecked = true - } + + minBoundChecked = minBoundChecked || minChecked + maxBoundChecked = maxBoundChecked || maxChecked + if minBoundChecked && maxBoundChecked { return true } From 97552d64b0690d7e68057a9973e1354dacaa8f76 Mon Sep 17 00:00:00 2001 From: czechbol Date: Wed, 28 Aug 2024 16:02:10 +0200 Subject: [PATCH 05/21] improve bounds check logic Signed-off-by: czechbol --- analyzers/conversion_overflow.go | 87 ++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 38 deletions(-) diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go index daaefffef0..5ccdcd8e28 100644 --- a/analyzers/conversion_overflow.go +++ b/analyzers/conversion_overflow.go @@ -197,28 +197,41 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { return false } -func checkBlockForRangeCheck(block *ssa.BasicBlock, instr *ssa.Convert, dstInt integer) (bool, bool) { - minBoundChecked := false - maxBoundChecked := false - +func checkBlockForRangeCheck(block *ssa.BasicBlock, instr *ssa.Convert, dstInt integer) (minBoundChecked, maxBoundChecked bool) { for _, i := range block.Instrs { - if binOp, ok := i.(*ssa.BinOp); ok && isRelevantBinOp(binOp, instr.X) { - constVal := extractConst(binOp) - if constVal == nil { - continue - } - - value, err := strconv.ParseInt(constVal.Value.String(), 10, 64) - if err != nil { - continue - } + switch v := i.(type) { + case *ssa.BinOp: + if isBoundCheck(v, instr.X) { + constVal, isOnLeft := constFromBoundCheck(v) + if constVal == nil { + continue + } - minBoundChecked = minBoundChecked || checkMinBoundValue(value, dstInt) - maxBoundChecked = maxBoundChecked || checkMaxBoundValue(value, dstInt) + value := constVal.Int64() - if minBoundChecked && maxBoundChecked { - break + 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) + } + } } + case *ssa.Phi: + // Handle logical operations + continue + } + + if minBoundChecked && maxBoundChecked { + break } } return minBoundChecked, maxBoundChecked @@ -288,36 +301,34 @@ func isIntOverflow(src string, dst string) bool { return false } -func isRelevantBinOp(binOp *ssa.BinOp, x ssa.Value) bool { +func isBoundCheck(binOp *ssa.BinOp, x ssa.Value) bool { return (binOp.X == x || binOp.Y == x) && (binOp.Op == token.LSS || binOp.Op == token.LEQ || binOp.Op == token.GTR || binOp.Op == token.GEQ) } -func extractConst(binOp *ssa.BinOp) *ssa.Const { - if c, ok := binOp.Y.(*ssa.Const); ok { - return c - } +func constFromBoundCheck(binOp *ssa.BinOp) (constant *ssa.Const, isOnLeft bool) { if c, ok := binOp.X.(*ssa.Const); ok { - return c + return c, true + } + if c, ok := binOp.Y.(*ssa.Const); ok { + return c, false } - return nil + return nil, false } func checkSourceMinBound(srcInt, dstInt integer) bool { - if dstInt.signed { - if srcInt.signed { - // Source and destination are both signed - return -(1 << (srcInt.size - 1)) >= -(1 << (dstInt.size - 1)) - } - // Source is unsigned and destination is signed - return 0 >= -(1 << (dstInt.size - 1)) + if !srcInt.signed { + // For unsigned types, the minimum bound is always 0 and is always safe + return true } - if srcInt.signed { + + if dstInt.signed { + // Source and destination are both signed + return -(1 << (srcInt.size - 1)) >= -(1 << (dstInt.size - 1)) + } else { // Source is signed and destination is unsigned return -(1 << (srcInt.size - 1)) >= 0 } - // Both source and destination are unsigned - return true } func checkSourceMaxBound(srcInt, dstInt integer) bool { @@ -344,14 +355,14 @@ func checkSourceMaxBound(srcInt, dstInt integer) bool { func checkMinBoundValue(value int64, dstInt integer) bool { if dstInt.signed { - return value == -(1 << (dstInt.size - 1)) + return value >= -(1 << (dstInt.size - 1)) } return value >= 0 // For unsigned types, the minimum bound is always 0 } func checkMaxBoundValue(value int64, dstInt integer) bool { if dstInt.signed { - return value == (1<<(dstInt.size-1))-1 + return value <= (1<<(dstInt.size-1))-1 } - return value == (1< Date: Wed, 28 Aug 2024 16:02:10 +0200 Subject: [PATCH 06/21] max recursion depth Signed-off-by: czechbol --- analyzers/conversion_overflow.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go index 5ccdcd8e28..27bccce4f4 100644 --- a/analyzers/conversion_overflow.go +++ b/analyzers/conversion_overflow.go @@ -168,9 +168,13 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { return true } - // Recursive function to check predecessors - var checkPredecessors func(block *ssa.BasicBlock) bool - checkPredecessors = func(block *ssa.BasicBlock) bool { + // 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) @@ -180,7 +184,7 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { if minBoundChecked && maxBoundChecked { return true } - if checkPredecessors(pred) { + if checkPredecessors(pred, depth+1) { return true } } @@ -188,7 +192,7 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { } // Start checking from the initial block - checkPredecessors(block) + checkPredecessors(block, 0) if minBoundChecked && maxBoundChecked { return true From dd2f56b0a59b5f78c53411217b9ebe660108641f Mon Sep 17 00:00:00 2001 From: czechbol Date: Wed, 28 Aug 2024 16:35:01 +0200 Subject: [PATCH 07/21] add test case for len function Signed-off-by: czechbol --- testutils/g115_samples.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/testutils/g115_samples.go b/testutils/g115_samples.go index 028b36f2d0..be6313ddce 100644 --- a/testutils/g115_samples.go +++ b/testutils/g115_samples.go @@ -493,6 +493,23 @@ func foo(x int) uint32 { } return uint32(x) } +`, + }, 0, gosec.NewConfig()}, + {[]string{ + ` +package main + +import ( + "math" +) + +func foo(items []string) uint32 { + x := len(items) + if x > math.MaxUint32 { + return math.MaxUint32 + } + return uint32(x) +} `, }, 0, gosec.NewConfig()}, } From 945c388cfce4c4614ef9d5e2d53b571033bd6e68 Mon Sep 17 00:00:00 2001 From: czechbol Date: Wed, 28 Aug 2024 16:36:00 +0200 Subject: [PATCH 08/21] relax len function bounds checks Co-authored-by: Ben Krieger --- analyzers/conversion_overflow.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go index 27bccce4f4..9f30a51eef 100644 --- a/analyzers/conversion_overflow.go +++ b/analyzers/conversion_overflow.go @@ -229,6 +229,15 @@ func checkBlockForRangeCheck(block *ssa.BasicBlock, instr *ssa.Convert, dstInt i } } } + 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 From 0ad774499fb79748d7055a5fefdd1c44d27b5fe6 Mon Sep 17 00:00:00 2001 From: czechbol Date: Wed, 28 Aug 2024 22:08:40 +0200 Subject: [PATCH 09/21] handle cases when convert instruction is after the if blocks Signed-off-by: czechbol --- analyzers/conversion_overflow.go | 132 +++++++++++++++++-------------- testutils/g115_samples.go | 6 +- 2 files changed, 74 insertions(+), 64 deletions(-) diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go index 9f30a51eef..d8e4763131 100644 --- 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 index be6313ddce..c9d6cdd861 100644 --- 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) From 6fdff933311d1b73e397da67583f87b151835af9 Mon Sep 17 00:00:00 2001 From: czechbol Date: Fri, 30 Aug 2024 22:54:30 +0200 Subject: [PATCH 10/21] improve range check discovery, add tests Signed-off-by: czechbol --- analyzers/conversion_overflow.go | 363 ++++++++++++++------------ analyzers/conversion_overflow_test.go | 139 ++++++++++ testutils/g115_samples.go | 117 +++++++++ 3 files changed, 457 insertions(+), 162 deletions(-) create mode 100644 analyzers/conversion_overflow_test.go diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go index d8e4763131..118d43c23f 100644 --- a/analyzers/conversion_overflow.go +++ b/analyzers/conversion_overflow.go @@ -17,9 +17,11 @@ package analyzers import ( "fmt" "go/token" + "math" "regexp" "strconv" + "golang.org/x/exp/constraints" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/buildssa" "golang.org/x/tools/go/ssa" @@ -27,6 +29,13 @@ import ( "github.com/securego/gosec/v2/issue" ) +type integer struct { + signed bool + size int + min int + max uint +} + func newConversionOverflowAnalyzer(id string, description string) *analysis.Analyzer { return &analysis.Analyzer{ Name: id, @@ -74,6 +83,65 @@ func runConversionOverflow(pass *analysis.Pass) (interface{}, error) { return nil, nil } +func isIntOverflow(src string, dst string) bool { + srcInt, err := parseIntType(src) + if err != nil { + return false + } + + dstInt, err := parseIntType(dst) + if err != nil { + return false + } + + return srcInt.min < dstInt.min || srcInt.max > dstInt.max +} + +func parseIntType(intType string) (integer, error) { + re := regexp.MustCompile(`^(?Pu?int)(?P\d{1,2})?$`) + matches := re.FindStringSubmatch(intType) + if matches == nil { + return integer{}, fmt.Errorf("no integer type match found for %s", intType) + } + + it := matches[re.SubexpIndex("type")] + is := matches[re.SubexpIndex("size")] + + signed := it == "int" + + // use default system int type in case size is not present in the type + intSize := strconv.IntSize + if is != "" { + var err error + intSize, err = strconv.Atoi(is) + if err != nil { + return integer{}, fmt.Errorf("failed to parse the integer type size: %w", err) + } + } + + if intSize != 8 && intSize != 16 && intSize != 32 && intSize != 64 && is != "" { + return integer{}, fmt.Errorf("invalid bit size: %d", intSize) + } + + var min int + var max uint + + if signed { + max = (1 << uint(intSize-1)) - 1 + min = -int(max) - 1 + } else { + max = (1 << uint(intSize)) - 1 + min = 0 + } + + return integer{ + signed: signed, + size: intSize, + min: min, + max: max, + }, nil +} + func isSafeConversion(instr *ssa.Convert) bool { dstType := instr.Type().Underlying().String() @@ -149,6 +217,7 @@ func isStringToIntConversion(instr *ssa.Convert, dstType string) bool { } func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { + fmt.Println("") dstInt, err := parseIntType(dstType) if err != nil { return false @@ -159,39 +228,43 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { return false } - minBoundChecked := checkSourceMinBound(srcInt, dstInt) - maxBoundChecked := checkSourceMaxBound(srcInt, dstInt) + minValue := srcInt.min + maxValue := srcInt.max - // If both bounds are already checked, return true - if minBoundChecked && maxBoundChecked { + if minValue > dstInt.min && maxValue < dstInt.max { return true } 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) + currMinValue, currMaxValue, isRangeCheck, _ := getResultRange(v, instr, visitedIfs) + + if isRangeCheck { + minValue = max(minValue, &currMinValue) + maxValue = min(maxValue, &currMaxValue) + } 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 + if fn, isBuiltin := v.Call.Value.(*ssa.Builtin); isBuiltin { + switch fn.Name() { + case "len", "cap": + minValue = 0 + } } case *ssa.Convert: if v == instr { break } } - minBoundChecked = minBoundChecked || minChecked - maxBoundChecked = maxBoundChecked || maxChecked - if minBoundChecked && maxBoundChecked { + if minValue >= dstInt.min && maxValue <= dstInt.max { return true } } @@ -199,193 +272,159 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { return false } -func checkIfForRangeCheck(ifInstr *ssa.If, instr *ssa.Convert, dstInt integer, visitedIfs map[*ssa.If]bool) (minBoundChecked, maxBoundChecked bool) { +func getResultRange(ifInstr *ssa.If, instr *ssa.Convert, visitedIfs map[*ssa.If]bool) (minValue int, maxValue uint, isRangeChk, convertFound bool) { + minValue = math.MinInt + maxValue = math.MaxUint + if visitedIfs[ifInstr] { - // If the if instruction has already been visited, we can skip it - return false, false + return minValue, maxValue, false, false } visitedIfs[ifInstr] = 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) - } + cond := ifInstr.Cond - // 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) - } - } - } + var thenBounds, elseBounds branchBounds - 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 - } + if binOp, ok := cond.(*ssa.BinOp); ok && isRangeCheck(binOp, instr.X) { + isRangeChk = true - value := constVal.Int64() + // Check the true branch + thenBounds = walkBranchForConvert(ifInstr.Block().Succs[0], instr, visitedIfs) - 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 + x, y := binOp.X, binOp.Y + operandsFlipped := false + if x != instr.X { + y = x + operandsFlipped = true + } + constVal, ok := y.(*ssa.Const) + if !ok { + return minValue, maxValue, false, false + } + + switch binOp.Op { + case token.LEQ, token.LSS: + if thenBounds.convertFound && !operandsFlipped { + maxValue = uint(constVal.Uint64()) + if binOp.Op == token.LEQ { + maxValue-- } - if binOp.Op == token.GTR || binOp.Op == token.GEQ { - newMaxCheck := checkMaxBoundValue(value, dstInt) - maxBoundChecked = maxBoundChecked || newMaxCheck + break + } + minValue = int(constVal.Int64()) + if binOp.Op == token.GTR { + minValue++ + } + case token.GEQ, token.GTR: + if thenBounds.convertFound && !operandsFlipped { + minValue = int(constVal.Int64()) + if binOp.Op == token.GEQ { + minValue++ } + break + } + maxValue = uint(constVal.Uint64()) + if binOp.Op == token.LSS { + maxValue-- } } } - return minBoundChecked, maxBoundChecked -} - -type integer struct { - signed bool - size int -} - -func parseIntType(intType string) (integer, error) { - re := regexp.MustCompile(`(?Pu?int)(?P\d{1,2})?`) - matches := re.FindStringSubmatch(intType) - if matches == nil { - return integer{}, fmt.Errorf("no integer type match found for %s", intType) + if !isRangeChk { + return minValue, maxValue, isRangeChk, convertFound } - it := matches[re.SubexpIndex("type")] - is := matches[re.SubexpIndex("size")] + elseBounds = walkBranchForConvert(ifInstr.Block().Succs[1], instr, visitedIfs) - signed := false - if it == "int" { - signed = true + if thenBounds.convertFound { + return max(minValue, thenBounds.minValue), min(maxValue, thenBounds.maxValue), true, true + } else if elseBounds.convertFound { + return max(minValue, elseBounds.minValue), min(maxValue, elseBounds.maxValue), true, true } - // use default system int type in case size is not present in the type - intSize := strconv.IntSize - if is != "" { - var err error - intSize, err = strconv.Atoi(is) - if err != nil { - return integer{}, fmt.Errorf("failed to parse the integer type size: %w", err) - } - } - - return integer{signed: signed, size: intSize}, nil + return minValue, maxValue, isRangeChk, convertFound } -func isIntOverflow(src string, dst string) bool { - srcInt, err := parseIntType(src) - if err != nil { - return false - } - - dstInt, err := parseIntType(dst) - if err != nil { - return false - } +type branchBounds struct { + minValue *int + maxValue *uint + convertFound bool +} - // converting uint to int of the same size or smaller might lead to overflow - if !srcInt.signed && dstInt.signed && dstInt.size <= srcInt.size { - return true - } - // converting uint to unit of a smaller size might lead to overflow - if !srcInt.signed && !dstInt.signed && dstInt.size < srcInt.size { - return true - } - // converting int to int of a smaller size might lead to overflow - if srcInt.signed && dstInt.signed && dstInt.size < srcInt.size { - return true - } - // converting int to uint of a smaller size might lead to overflow - if srcInt.signed && !dstInt.signed && dstInt.size < srcInt.size && srcInt.size-dstInt.size > 8 { - return true +func walkBranchForConvert(block *ssa.BasicBlock, instr *ssa.Convert, visitedIfs map[*ssa.If]bool) branchBounds { + bounds := branchBounds{} + convertFound := false + for _, blockInstr := range block.Instrs { + switch v := blockInstr.(type) { + case *ssa.If: + currMinValue, currMaxValue, isRangeCheck, cnvrtFound := getResultRange(v, instr, visitedIfs) + convertFound = convertFound || cnvrtFound + + if isRangeCheck { + bounds.minValue = toPtr(min(currMinValue, bounds.minValue)) + bounds.maxValue = toPtr(max(currMaxValue, bounds.maxValue)) + } + case *ssa.Call: + if v != instr.X { + continue + } + if fn, isBuiltin := v.Call.Value.(*ssa.Builtin); isBuiltin { + switch fn.Name() { + case "len", "cap": + bounds.minValue = toPtr(0) + } + } + case *ssa.Convert: + if v == instr { + bounds.convertFound = true + break + } + } } - return false -} + bounds.convertFound = bounds.convertFound || convertFound -func isBoundCheck(binOp *ssa.BinOp, x ssa.Value) bool { - return (binOp.X == x || binOp.Y == x) && - (binOp.Op == token.LSS || binOp.Op == token.LEQ || binOp.Op == token.GTR || binOp.Op == token.GEQ) + return bounds } -func constFromBoundCheck(binOp *ssa.BinOp) (constant *ssa.Const, isOnLeft bool) { - if c, ok := binOp.X.(*ssa.Const); ok { - return c, true - } - if c, ok := binOp.Y.(*ssa.Const); ok { - return c, false +func isRangeCheck(v ssa.Value, x ssa.Value) bool { + switch op := v.(type) { + case *ssa.BinOp: + return (op.X == x || op.Y == x) && + (op.Op == token.LSS || op.Op == token.LEQ || op.Op == token.GTR || op.Op == token.GEQ) + case *ssa.UnOp: + if op.Op == token.NOT { + if binOp, ok := op.X.(*ssa.BinOp); ok { + return (binOp.X == x || binOp.Y == x) && + (binOp.Op == token.EQL || binOp.Op == token.NEQ || + binOp.Op == token.LSS || binOp.Op == token.LEQ || + binOp.Op == token.GTR || binOp.Op == token.GEQ) + } + } } - return nil, false + return false } -func checkSourceMinBound(srcInt, dstInt integer) bool { - if !srcInt.signed { - // For unsigned types, the minimum bound is always 0 and is always safe - return true +func min[T constraints.Integer](a T, b *T) T { + if b == nil { + return a } - - if dstInt.signed { - // Source and destination are both signed - return -(1 << (srcInt.size - 1)) >= -(1 << (dstInt.size - 1)) - } else { - // Source is signed and destination is unsigned - return -(1 << (srcInt.size - 1)) >= 0 + if a < *b { + return a } + return *b } -func checkSourceMaxBound(srcInt, dstInt integer) bool { - if dstInt.signed { - if srcInt.signed { - // Source and destination are both signed - return (1<<(srcInt.size-1))-1 <= (1<<(dstInt.size-1))-1 - } - // Source is unsigned and destination is signed - var a uint = (1 << srcInt.size) - 1 - var b uint = (1 << (dstInt.size - 1)) - 1 - return a <= b +func max[T constraints.Integer](a T, b *T) T { + if b == nil { + return a } - // Destination is unsigned - if srcInt.signed { - // Source is signed and destination is unsigned - var a uint = (1 << (srcInt.size - 1)) - 1 - var b uint = (1 << dstInt.size) - 1 - return a <= b - } - // Both source and destination are unsigned - return (1<= -(1 << (dstInt.size - 1)) + if a > *b { + return a } - return value >= 0 // For unsigned types, the minimum bound is always 0 + return *b } -func checkMaxBoundValue(value int64, dstInt integer) bool { - if dstInt.signed { - return value <= (1<<(dstInt.size-1))-1 - } - return value <= (1<= math.MinInt32 && a <= math.MaxInt32 { + b := int32(a) + fmt.Printf("%d\n", b) + } + panic("out of range") +} + `, + }, 0, gosec.NewConfig()}, + {[]string{ + ` +package main + +import ( + "fmt" + "math" + "math/rand" +) + +func main() { + a := rand.Int63() + if a >= math.MinInt32 && a <= math.MaxInt32 { + b := int32(a) + fmt.Printf("%d\n", b) + } + panic("out of range") +} + `, + }, 0, gosec.NewConfig()}, + {[]string{ + ` +package main + +import ( + "fmt" + "math" + "math/rand" +) + +func main() { + a := rand.Int63() + if !(a >= math.MinInt32) && a > math.MaxInt32 { + b := int32(a) + fmt.Printf("%d\n", b) + } + panic("out of range") +} + `, + }, 0, gosec.NewConfig()}, + {[]string{ + ` +package main + +import ( + "fmt" + "math" + "math/rand" +) + +func main() { + a := rand.Int63() + if !(a >= math.MinInt32) || a > math.MaxInt32 { + panic("out of range") + } + b := int32(a) + fmt.Printf("%d\n", b) +} + `, + }, 0, gosec.NewConfig()}, + {[]string{ + ` +package main + +import ( + "fmt" + "math" + "math/rand" +) + +func main() { + a := rand.Int63() + if math.MinInt32 <= a && math.MaxInt32 >= a { + b := int32(a) + fmt.Printf("%d\n", b) + } + panic("out of range") +} + `, + }, 0, gosec.NewConfig()}, } From 51f95f826bc5101a3a7fe0da9d429519222388a1 Mon Sep 17 00:00:00 2001 From: czechbol Date: Fri, 30 Aug 2024 23:09:22 +0200 Subject: [PATCH 11/21] refactor for readability Signed-off-by: czechbol --- analyzers/conversion_overflow.go | 191 ++++++++++++++++--------------- 1 file changed, 98 insertions(+), 93 deletions(-) diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go index 118d43c23f..389a291de3 100644 --- a/analyzers/conversion_overflow.go +++ b/analyzers/conversion_overflow.go @@ -36,6 +36,19 @@ type integer struct { max uint } +type rangeResult struct { + MinValue int + MaxValue uint + IsRangeCheck bool + ConvertFound bool +} + +type branchBounds struct { + MinValue *int + MaxValue *uint + ConvertFound bool +} + func newConversionOverflowAnalyzer(id string, description string) *analysis.Analyzer { return &analysis.Analyzer{ Name: id, @@ -217,7 +230,6 @@ func isStringToIntConversion(instr *ssa.Convert, dstType string) bool { } func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { - fmt.Println("") dstInt, err := parseIntType(dstType) if err != nil { return false @@ -240,15 +252,13 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { for _, blockInstr := range block.Instrs { switch v := blockInstr.(type) { case *ssa.If: - currMinValue, currMaxValue, isRangeCheck, _ := getResultRange(v, instr, visitedIfs) - - if isRangeCheck { - minValue = max(minValue, &currMinValue) - maxValue = min(maxValue, &currMaxValue) + result := getResultRange(v, instr, visitedIfs) + if result.IsRangeCheck { + minValue = max(minValue, &result.MinValue) + maxValue = min(maxValue, &result.MaxValue) } case *ssa.Call: - // len(slice) results in an int that is guaranteed >= 0, which - // satisfies the lower bound check for int -> uint conversion + // these function return an int of a guaranteed size if v != instr.X { continue } @@ -272,118 +282,122 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { return false } -func getResultRange(ifInstr *ssa.If, instr *ssa.Convert, visitedIfs map[*ssa.If]bool) (minValue int, maxValue uint, isRangeChk, convertFound bool) { - minValue = math.MinInt - maxValue = math.MaxUint - +// getResultRange is a recursive function that walks the branches of the if statement to find the range of the variable +func getResultRange(ifInstr *ssa.If, instr *ssa.Convert, visitedIfs map[*ssa.If]bool) rangeResult { if visitedIfs[ifInstr] { - return minValue, maxValue, false, false + return rangeResult{MinValue: math.MinInt, MaxValue: math.MaxUint} } visitedIfs[ifInstr] = true cond := ifInstr.Cond + binOp, ok := cond.(*ssa.BinOp) + if !ok || !isRangeCheck(binOp, instr.X) { + return rangeResult{MinValue: math.MinInt, MaxValue: math.MaxUint} + } - var thenBounds, elseBounds branchBounds + result := rangeResult{ + MinValue: math.MinInt, + MaxValue: math.MaxUint, + IsRangeCheck: true, + } - if binOp, ok := cond.(*ssa.BinOp); ok && isRangeCheck(binOp, instr.X) { - isRangeChk = true + thenBounds := walkBranchForConvert(ifInstr.Block().Succs[0], instr, visitedIfs) + elseBounds := walkBranchForConvert(ifInstr.Block().Succs[1], instr, visitedIfs) - // Check the true branch - thenBounds = walkBranchForConvert(ifInstr.Block().Succs[0], instr, visitedIfs) + updateResultFromBinOp(&result, binOp, instr, thenBounds.ConvertFound) - x, y := binOp.X, binOp.Y - operandsFlipped := false - if x != instr.X { - y = x - operandsFlipped = true - } - constVal, ok := y.(*ssa.Const) - if !ok { - return minValue, maxValue, false, false - } - - switch binOp.Op { - case token.LEQ, token.LSS: - if thenBounds.convertFound && !operandsFlipped { - maxValue = uint(constVal.Uint64()) - if binOp.Op == token.LEQ { - maxValue-- - } - break - } - minValue = int(constVal.Int64()) - if binOp.Op == token.GTR { - minValue++ - } - case token.GEQ, token.GTR: - if thenBounds.convertFound && !operandsFlipped { - minValue = int(constVal.Int64()) - if binOp.Op == token.GEQ { - minValue++ - } - break - } - maxValue = uint(constVal.Uint64()) - if binOp.Op == token.LSS { - maxValue-- - } - } + if thenBounds.ConvertFound { + result.ConvertFound = true + result.MinValue = max(result.MinValue, thenBounds.MinValue) + result.MaxValue = min(result.MaxValue, thenBounds.MaxValue) + } else if elseBounds.ConvertFound { + result.ConvertFound = true + result.MinValue = max(result.MinValue, elseBounds.MinValue) + result.MaxValue = min(result.MaxValue, elseBounds.MaxValue) } - if !isRangeChk { - return minValue, maxValue, isRangeChk, convertFound + return result +} + +// updateResultFromBinOp updates the rangeResult based on the BinOp instruction and the location of the Convert instruction +func updateResultFromBinOp(result *rangeResult, binOp *ssa.BinOp, instr *ssa.Convert, successPathConvert bool) { + x, y := binOp.X, binOp.Y + operandsFlipped := false + if x != instr.X { + y, operandsFlipped = x, true } - elseBounds = walkBranchForConvert(ifInstr.Block().Succs[1], instr, visitedIfs) + constVal, ok := y.(*ssa.Const) + if !ok { + return + } - if thenBounds.convertFound { - return max(minValue, thenBounds.minValue), min(maxValue, thenBounds.maxValue), true, true - } else if elseBounds.convertFound { - return max(minValue, elseBounds.minValue), min(maxValue, elseBounds.maxValue), true, true + switch binOp.Op { + case token.LEQ, token.LSS: + updateMinMaxForLessOrEqual(result, constVal, binOp.Op, operandsFlipped, successPathConvert) + case token.GEQ, token.GTR: + updateMinMaxForGreaterOrEqual(result, constVal, binOp.Op, operandsFlipped, successPathConvert) } +} - return minValue, maxValue, isRangeChk, convertFound +func updateMinMaxForLessOrEqual(result *rangeResult, constVal *ssa.Const, op token.Token, operandsFlipped bool, successPathConvert bool) { + // If the success path has a conversion and the operands are not flipped, then the constant value is the maximum value + if successPathConvert && !operandsFlipped { + result.MaxValue = uint(constVal.Uint64()) + if op == token.LEQ { + result.MaxValue-- + } + } else { + result.MinValue = int(constVal.Int64()) + if op == token.GTR { + result.MinValue++ + } + } } -type branchBounds struct { - minValue *int - maxValue *uint - convertFound bool +func updateMinMaxForGreaterOrEqual(result *rangeResult, constVal *ssa.Const, op token.Token, operandsFlipped bool, successPathConvert bool) { + // If the success path has a conversion and the operands are not flipped, then the constant value is the minimum value + if successPathConvert && !operandsFlipped { + result.MinValue = int(constVal.Int64()) + if op == token.GEQ { + result.MinValue++ + } + } else { + result.MaxValue = uint(constVal.Uint64()) + if op == token.LSS { + result.MaxValue-- + } + } } +// walkBranchForConvert walks the branch of the if statement to find the range of the variable and where the conversion is func walkBranchForConvert(block *ssa.BasicBlock, instr *ssa.Convert, visitedIfs map[*ssa.If]bool) branchBounds { bounds := branchBounds{} - convertFound := false + for _, blockInstr := range block.Instrs { switch v := blockInstr.(type) { case *ssa.If: - currMinValue, currMaxValue, isRangeCheck, cnvrtFound := getResultRange(v, instr, visitedIfs) - convertFound = convertFound || cnvrtFound + result := getResultRange(v, instr, visitedIfs) + bounds.ConvertFound = bounds.ConvertFound || result.ConvertFound - if isRangeCheck { - bounds.minValue = toPtr(min(currMinValue, bounds.minValue)) - bounds.maxValue = toPtr(max(currMaxValue, bounds.maxValue)) + if result.IsRangeCheck { + bounds.MinValue = toPtr(max(result.MinValue, bounds.MinValue)) + bounds.MaxValue = toPtr(min(result.MaxValue, bounds.MaxValue)) } case *ssa.Call: - if v != instr.X { - continue - } - if fn, isBuiltin := v.Call.Value.(*ssa.Builtin); isBuiltin { - switch fn.Name() { - case "len", "cap": - bounds.minValue = toPtr(0) + if v == instr.X { + if fn, isBuiltin := v.Call.Value.(*ssa.Builtin); isBuiltin && (fn.Name() == "len" || fn.Name() == "cap") { + bounds.MinValue = toPtr(0) } } case *ssa.Convert: if v == instr { - bounds.convertFound = true - break + bounds.ConvertFound = true + return bounds } } } - bounds.convertFound = bounds.convertFound || convertFound - return bounds } @@ -392,15 +406,6 @@ func isRangeCheck(v ssa.Value, x ssa.Value) bool { case *ssa.BinOp: return (op.X == x || op.Y == x) && (op.Op == token.LSS || op.Op == token.LEQ || op.Op == token.GTR || op.Op == token.GEQ) - case *ssa.UnOp: - if op.Op == token.NOT { - if binOp, ok := op.X.(*ssa.BinOp); ok { - return (binOp.X == x || binOp.Y == x) && - (binOp.Op == token.EQL || binOp.Op == token.NEQ || - binOp.Op == token.LSS || binOp.Op == token.LEQ || - binOp.Op == token.GTR || binOp.Op == token.GEQ) - } - } } return false } From 1c4433ccede4ea562952bfde688c18864a9f2c3f Mon Sep 17 00:00:00 2001 From: czechbol Date: Fri, 30 Aug 2024 23:10:56 +0200 Subject: [PATCH 12/21] add cap function test Signed-off-by: czechbol --- testutils/g115_samples.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/testutils/g115_samples.go b/testutils/g115_samples.go index 7ef8591cbf..4c3f0094b5 100644 --- a/testutils/g115_samples.go +++ b/testutils/g115_samples.go @@ -520,6 +520,23 @@ import ( "math" ) +func foo(items []string) uint32 { + x := cap(items) + if x > math.MaxUint32 { + return math.MaxUint32 + } + return uint32(x) +} +`, + }, 0, gosec.NewConfig()}, + {[]string{ + ` +package main + +import ( + "math" +) + func foo(items []string) uint32 { x := len(items) if x < math.MaxUint32 { From 665036b0adbe4e77b6f8a0e6ce5a5dadba9d4c35 Mon Sep 17 00:00:00 2001 From: czechbol Date: Fri, 30 Aug 2024 23:19:49 +0200 Subject: [PATCH 13/21] calculate signed min without throwing overflow warnings Signed-off-by: czechbol --- analyzers/conversion_overflow.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go index 389a291de3..c91913233a 100644 --- a/analyzers/conversion_overflow.go +++ b/analyzers/conversion_overflow.go @@ -141,7 +141,7 @@ func parseIntType(intType string) (integer, error) { if signed { max = (1 << uint(intSize-1)) - 1 - min = -int(max) - 1 + min = -1 << (intSize - 1) } else { max = (1 << uint(intSize)) - 1 min = 0 From 7b4830abe3a12bf8a806791848c99951642af96a Mon Sep 17 00:00:00 2001 From: czechbol Date: Sat, 31 Aug 2024 00:14:46 +0200 Subject: [PATCH 14/21] perform bounds checks int size calculations Signed-off-by: czechbol --- analyzers/conversion_overflow.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go index c91913233a..29caf31db3 100644 --- a/analyzers/conversion_overflow.go +++ b/analyzers/conversion_overflow.go @@ -140,9 +140,22 @@ func parseIntType(intType string) (integer, error) { var max uint if signed { - max = (1 << uint(intSize-1)) - 1 + shiftAmount := intSize - 1 + + // Perform a bounds check + if shiftAmount < 0 { + return integer{}, fmt.Errorf("invalid shift amount: %d", shiftAmount) + } + + max = (1 << uint(shiftAmount)) - 1 min = -1 << (intSize - 1) + } else { + // Perform a bounds check + if intSize < 0 { + return integer{}, fmt.Errorf("invalid bit size: %d", intSize) + } + max = (1 << uint(intSize)) - 1 min = 0 } From 553d10fc1cb4569fa465b2e497405d2bce6a2f44 Mon Sep 17 00:00:00 2001 From: czechbol Date: Sat, 31 Aug 2024 00:56:49 +0200 Subject: [PATCH 15/21] basic equal operator logic Signed-off-by: czechbol --- analyzers/conversion_overflow.go | 88 +++++++++++++++++++++++++------- testutils/g115_samples.go | 38 ++++++++++++++ 2 files changed, 108 insertions(+), 18 deletions(-) diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go index 29caf31db3..7ecb7d8d83 100644 --- a/analyzers/conversion_overflow.go +++ b/analyzers/conversion_overflow.go @@ -20,6 +20,7 @@ import ( "math" "regexp" "strconv" + "strings" "golang.org/x/exp/constraints" "golang.org/x/tools/go/analysis" @@ -37,16 +38,20 @@ type integer struct { } type rangeResult struct { - MinValue int - MaxValue uint - IsRangeCheck bool - ConvertFound bool + MinValue int + MaxValue uint + ExplixitPositiveVals []uint + ExplicitNegativeVals []int + IsRangeCheck bool + ConvertFound bool } -type branchBounds struct { - MinValue *int - MaxValue *uint - ConvertFound bool +type branchResults struct { + MinValue *int + MaxValue *uint + ExplixitPositiveVals []uint + ExplicitNegativeVals []int + ConvertFound bool } func newConversionOverflowAnalyzer(id string, description string) *analysis.Analyzer { @@ -151,11 +156,6 @@ func parseIntType(intType string) (integer, error) { min = -1 << (intSize - 1) } else { - // Perform a bounds check - if intSize < 0 { - return integer{}, fmt.Errorf("invalid bit size: %d", intSize) - } - max = (1 << uint(intSize)) - 1 min = 0 } @@ -255,6 +255,8 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { minValue := srcInt.min maxValue := srcInt.max + explicitPositiveVals := []uint{} + explicitNegativeVals := []int{} if minValue > dstInt.min && maxValue < dstInt.max { return true @@ -269,6 +271,8 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { if result.IsRangeCheck { minValue = max(minValue, &result.MinValue) maxValue = min(maxValue, &result.MaxValue) + explicitPositiveVals = append(explicitPositiveVals, result.ExplixitPositiveVals...) + explicitNegativeVals = append(explicitNegativeVals, result.ExplicitNegativeVals...) } case *ssa.Call: // these function return an int of a guaranteed size @@ -287,7 +291,9 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { } } - if minValue >= dstInt.min && maxValue <= dstInt.max { + if explicitValsInRange(explicitPositiveVals, explicitNegativeVals, dstInt) { + return true + } else if minValue >= dstInt.min && maxValue <= dstInt.max { return true } } @@ -350,6 +356,29 @@ func updateResultFromBinOp(result *rangeResult, binOp *ssa.BinOp, instr *ssa.Con updateMinMaxForLessOrEqual(result, constVal, binOp.Op, operandsFlipped, successPathConvert) case token.GEQ, token.GTR: updateMinMaxForGreaterOrEqual(result, constVal, binOp.Op, operandsFlipped, successPathConvert) + case token.EQL: + if !successPathConvert { + break + } + + // determine if the constant value is positive or negative + if strings.Contains(constVal.String(), "-") { + result.ExplicitNegativeVals = append(result.ExplicitNegativeVals, int(constVal.Int64())) + } else { + result.ExplixitPositiveVals = append(result.ExplixitPositiveVals, uint(constVal.Uint64())) + } + + case token.NEQ: + if successPathConvert { + break + } + + // determine if the constant value is positive or negative + if strings.Contains(constVal.String(), "-") { + result.ExplicitNegativeVals = append(result.ExplicitNegativeVals, int(constVal.Int64())) + } else { + result.ExplixitPositiveVals = append(result.ExplixitPositiveVals, uint(constVal.Uint64())) + } } } @@ -384,8 +413,8 @@ func updateMinMaxForGreaterOrEqual(result *rangeResult, constVal *ssa.Const, op } // walkBranchForConvert walks the branch of the if statement to find the range of the variable and where the conversion is -func walkBranchForConvert(block *ssa.BasicBlock, instr *ssa.Convert, visitedIfs map[*ssa.If]bool) branchBounds { - bounds := branchBounds{} +func walkBranchForConvert(block *ssa.BasicBlock, instr *ssa.Convert, visitedIfs map[*ssa.If]bool) branchResults { + bounds := branchResults{} for _, blockInstr := range block.Instrs { switch v := blockInstr.(type) { @@ -417,12 +446,35 @@ func walkBranchForConvert(block *ssa.BasicBlock, instr *ssa.Convert, visitedIfs func isRangeCheck(v ssa.Value, x ssa.Value) bool { switch op := v.(type) { case *ssa.BinOp: - return (op.X == x || op.Y == x) && - (op.Op == token.LSS || op.Op == token.LEQ || op.Op == token.GTR || op.Op == token.GEQ) + switch op.Op { + case token.LSS, token.LEQ, token.GTR, token.GEQ, + token.EQL, token.NEQ: + return op.X == x || op.Y == x + } } return false } +func explicitValsInRange(explicitPosVals []uint, explicitNegVals []int, dstInt integer) bool { + if len(explicitPosVals) == 0 && len(explicitNegVals) == 0 { + return false + } + + for _, val := range explicitPosVals { + if val > dstInt.max { + return false + } + } + + for _, val := range explicitNegVals { + if val < dstInt.min { + return false + } + } + + return true +} + func min[T constraints.Integer](a T, b *T) T { if b == nil { return a diff --git a/testutils/g115_samples.go b/testutils/g115_samples.go index 4c3f0094b5..e05c4cf7f6 100644 --- a/testutils/g115_samples.go +++ b/testutils/g115_samples.go @@ -643,6 +643,44 @@ func main() { fmt.Printf("%d\n", b) } panic("out of range") +} + `, + }, 0, gosec.NewConfig()}, + {[]string{ + ` +package main + +import ( + "fmt" + "math/rand" +) + +func main() { + a := rand.Int63() + if a == 3 || a == 4 { + b := int32(a) + fmt.Printf("%d\n", b) + } + panic("out of range") +} + `, + }, 0, gosec.NewConfig()}, + {[]string{ + ` +package main + +import ( + "fmt" + "math/rand" +) + +func main() { + a := rand.Int63() + if a != 3 || a != 4 { + panic("out of range") + } + b := int32(a) + fmt.Printf("%d\n", b) } `, }, 0, gosec.NewConfig()}, From b8160ead8bbdb8f6130b82ab894f5fde965f95b1 Mon Sep 17 00:00:00 2001 From: czechbol Date: Mon, 2 Sep 2024 09:39:10 +0200 Subject: [PATCH 16/21] uintptr -> unsafe.Pointer test case Signed-off-by: czechbol --- testutils/g115_samples.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/testutils/g115_samples.go b/testutils/g115_samples.go index e05c4cf7f6..84db8e0244 100644 --- a/testutils/g115_samples.go +++ b/testutils/g115_samples.go @@ -681,6 +681,19 @@ func main() { } b := int32(a) fmt.Printf("%d\n", b) +} + `, + }, 0, gosec.NewConfig()}, + {[]string{ + ` +package main + +import "unsafe" + +func main() { + i := uintptr(123) + p := unsafe.Pointer(i) + _ = p } `, }, 0, gosec.NewConfig()}, From f490e9092f3cd5b1df2f5bb914e6b3cee9a3d9f2 Mon Sep 17 00:00:00 2001 From: czechbol Date: Mon, 2 Sep 2024 10:39:36 +0200 Subject: [PATCH 17/21] fix review comments Signed-off-by: czechbol --- analyzers/conversion_overflow.go | 128 ++++++++++++++++--------------- 1 file changed, 66 insertions(+), 62 deletions(-) diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go index 7ecb7d8d83..4ad144f234 100644 --- a/analyzers/conversion_overflow.go +++ b/analyzers/conversion_overflow.go @@ -38,20 +38,20 @@ type integer struct { } type rangeResult struct { - MinValue int - MaxValue uint - ExplixitPositiveVals []uint - ExplicitNegativeVals []int - IsRangeCheck bool - ConvertFound bool + minValue int + maxValue uint + explixitPositiveVals []uint + explicitNegativeVals []int + isRangeCheck bool + convertFound bool } type branchResults struct { - MinValue *int - MaxValue *uint - ExplixitPositiveVals []uint - ExplicitNegativeVals []int - ConvertFound bool + minValue *int + maxValue *uint + explixitPositiveVals []uint + explicitNegativeVals []int + convertFound bool } func newConversionOverflowAnalyzer(id string, description string) *analysis.Analyzer { @@ -127,7 +127,7 @@ func parseIntType(intType string) (integer, error) { signed := it == "int" - // use default system int type in case size is not present in the type + // use default system int type in case size is not present in the type. intSize := strconv.IntSize if is != "" { var err error @@ -147,7 +147,7 @@ func parseIntType(intType string) (integer, error) { if signed { shiftAmount := intSize - 1 - // Perform a bounds check + // Perform a bounds check. if shiftAmount < 0 { return integer{}, fmt.Errorf("invalid shift amount: %d", shiftAmount) } @@ -171,19 +171,19 @@ func parseIntType(intType string) (integer, error) { func isSafeConversion(instr *ssa.Convert) bool { dstType := instr.Type().Underlying().String() - // Check for constant conversions + // Check for constant conversions. if constVal, ok := instr.X.(*ssa.Const); ok { if isConstantInRange(constVal, dstType) { return true } } - // Check for string to integer conversions with specified bit size + // Check for string to integer conversions with specified bit size. if isStringToIntConversion(instr, dstType) { return true } - // Check for explicit range checks + // Check for explicit range checks. if hasExplicitRangeCheck(instr, dstType) { return true } @@ -209,7 +209,7 @@ func isConstantInRange(constVal *ssa.Const, dstType string) bool { } func isStringToIntConversion(instr *ssa.Convert, dstType string) bool { - // Traverse the SSA instructions to find the original variable + // Traverse the SSA instructions to find the original variable. original := instr.X for { switch v := original.(type) { @@ -268,14 +268,14 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { switch v := blockInstr.(type) { case *ssa.If: result := getResultRange(v, instr, visitedIfs) - if result.IsRangeCheck { - minValue = max(minValue, &result.MinValue) - maxValue = min(maxValue, &result.MaxValue) - explicitPositiveVals = append(explicitPositiveVals, result.ExplixitPositiveVals...) - explicitNegativeVals = append(explicitNegativeVals, result.ExplicitNegativeVals...) + if result.isRangeCheck { + minValue = max(minValue, &result.minValue) + maxValue = min(maxValue, &result.maxValue) + explicitPositiveVals = append(explicitPositiveVals, result.explixitPositiveVals...) + explicitNegativeVals = append(explicitNegativeVals, result.explicitNegativeVals...) } case *ssa.Call: - // these function return an int of a guaranteed size + // These function return an int of a guaranteed size. if v != instr.X { continue } @@ -301,44 +301,48 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { return false } -// getResultRange is a recursive function that walks the branches of the if statement to find the range of the variable +// getResultRange is a recursive function that walks the branches of the if statement to find the range of the variable. func getResultRange(ifInstr *ssa.If, instr *ssa.Convert, visitedIfs map[*ssa.If]bool) rangeResult { if visitedIfs[ifInstr] { - return rangeResult{MinValue: math.MinInt, MaxValue: math.MaxUint} + return rangeResult{minValue: math.MinInt, maxValue: math.MaxUint} } visitedIfs[ifInstr] = true cond := ifInstr.Cond binOp, ok := cond.(*ssa.BinOp) if !ok || !isRangeCheck(binOp, instr.X) { - return rangeResult{MinValue: math.MinInt, MaxValue: math.MaxUint} + return rangeResult{minValue: math.MinInt, maxValue: math.MaxUint} } result := rangeResult{ - MinValue: math.MinInt, - MaxValue: math.MaxUint, - IsRangeCheck: true, + minValue: math.MinInt, + maxValue: math.MaxUint, + isRangeCheck: true, } thenBounds := walkBranchForConvert(ifInstr.Block().Succs[0], instr, visitedIfs) elseBounds := walkBranchForConvert(ifInstr.Block().Succs[1], instr, visitedIfs) - updateResultFromBinOp(&result, binOp, instr, thenBounds.ConvertFound) + updateResultFromBinOp(&result, binOp, instr, thenBounds.convertFound) - if thenBounds.ConvertFound { - result.ConvertFound = true - result.MinValue = max(result.MinValue, thenBounds.MinValue) - result.MaxValue = min(result.MaxValue, thenBounds.MaxValue) - } else if elseBounds.ConvertFound { - result.ConvertFound = true - result.MinValue = max(result.MinValue, elseBounds.MinValue) - result.MaxValue = min(result.MaxValue, elseBounds.MaxValue) + if thenBounds.convertFound { + result.convertFound = true + result.minValue = max(result.minValue, thenBounds.minValue) + result.maxValue = min(result.maxValue, thenBounds.maxValue) + result.explixitPositiveVals = append(result.explixitPositiveVals, thenBounds.explixitPositiveVals...) + result.explicitNegativeVals = append(result.explicitNegativeVals, thenBounds.explicitNegativeVals...) + } else if elseBounds.convertFound { + result.convertFound = true + result.minValue = max(result.minValue, elseBounds.minValue) + result.maxValue = min(result.maxValue, elseBounds.maxValue) + result.explixitPositiveVals = append(result.explixitPositiveVals, elseBounds.explixitPositiveVals...) + result.explicitNegativeVals = append(result.explicitNegativeVals, elseBounds.explicitNegativeVals...) } return result } -// updateResultFromBinOp updates the rangeResult based on the BinOp instruction and the location of the Convert instruction +// updateResultFromBinOp updates the rangeResult based on the BinOp instruction and the location of the Convert instruction. func updateResultFromBinOp(result *rangeResult, binOp *ssa.BinOp, instr *ssa.Convert, successPathConvert bool) { x, y := binOp.X, binOp.Y operandsFlipped := false @@ -361,11 +365,11 @@ func updateResultFromBinOp(result *rangeResult, binOp *ssa.BinOp, instr *ssa.Con break } - // determine if the constant value is positive or negative + // Determine if the constant value is positive or negative. if strings.Contains(constVal.String(), "-") { - result.ExplicitNegativeVals = append(result.ExplicitNegativeVals, int(constVal.Int64())) + result.explicitNegativeVals = append(result.explicitNegativeVals, int(constVal.Int64())) } else { - result.ExplixitPositiveVals = append(result.ExplixitPositiveVals, uint(constVal.Uint64())) + result.explixitPositiveVals = append(result.explixitPositiveVals, uint(constVal.Uint64())) } case token.NEQ: @@ -373,46 +377,46 @@ func updateResultFromBinOp(result *rangeResult, binOp *ssa.BinOp, instr *ssa.Con break } - // determine if the constant value is positive or negative + // Determine if the constant value is positive or negative. if strings.Contains(constVal.String(), "-") { - result.ExplicitNegativeVals = append(result.ExplicitNegativeVals, int(constVal.Int64())) + result.explicitNegativeVals = append(result.explicitNegativeVals, int(constVal.Int64())) } else { - result.ExplixitPositiveVals = append(result.ExplixitPositiveVals, uint(constVal.Uint64())) + result.explixitPositiveVals = append(result.explixitPositiveVals, uint(constVal.Uint64())) } } } func updateMinMaxForLessOrEqual(result *rangeResult, constVal *ssa.Const, op token.Token, operandsFlipped bool, successPathConvert bool) { - // If the success path has a conversion and the operands are not flipped, then the constant value is the maximum value + // If the success path has a conversion and the operands are not flipped, then the constant value is the maximum value. if successPathConvert && !operandsFlipped { - result.MaxValue = uint(constVal.Uint64()) + result.maxValue = uint(constVal.Uint64()) if op == token.LEQ { - result.MaxValue-- + result.maxValue-- } } else { - result.MinValue = int(constVal.Int64()) + result.minValue = int(constVal.Int64()) if op == token.GTR { - result.MinValue++ + result.minValue++ } } } func updateMinMaxForGreaterOrEqual(result *rangeResult, constVal *ssa.Const, op token.Token, operandsFlipped bool, successPathConvert bool) { - // If the success path has a conversion and the operands are not flipped, then the constant value is the minimum value + // If the success path has a conversion and the operands are not flipped, then the constant value is the minimum value. if successPathConvert && !operandsFlipped { - result.MinValue = int(constVal.Int64()) + result.minValue = int(constVal.Int64()) if op == token.GEQ { - result.MinValue++ + result.minValue++ } } else { - result.MaxValue = uint(constVal.Uint64()) + result.maxValue = uint(constVal.Uint64()) if op == token.LSS { - result.MaxValue-- + result.maxValue-- } } } -// walkBranchForConvert walks the branch of the if statement to find the range of the variable and where the conversion is +// walkBranchForConvert walks the branch of the if statement to find the range of the variable and where the conversion is. func walkBranchForConvert(block *ssa.BasicBlock, instr *ssa.Convert, visitedIfs map[*ssa.If]bool) branchResults { bounds := branchResults{} @@ -420,21 +424,21 @@ func walkBranchForConvert(block *ssa.BasicBlock, instr *ssa.Convert, visitedIfs switch v := blockInstr.(type) { case *ssa.If: result := getResultRange(v, instr, visitedIfs) - bounds.ConvertFound = bounds.ConvertFound || result.ConvertFound + bounds.convertFound = bounds.convertFound || result.convertFound - if result.IsRangeCheck { - bounds.MinValue = toPtr(max(result.MinValue, bounds.MinValue)) - bounds.MaxValue = toPtr(min(result.MaxValue, bounds.MaxValue)) + if result.isRangeCheck { + bounds.minValue = toPtr(max(result.minValue, bounds.minValue)) + bounds.maxValue = toPtr(min(result.maxValue, bounds.maxValue)) } case *ssa.Call: if v == instr.X { if fn, isBuiltin := v.Call.Value.(*ssa.Builtin); isBuiltin && (fn.Name() == "len" || fn.Name() == "cap") { - bounds.MinValue = toPtr(0) + bounds.minValue = toPtr(0) } } case *ssa.Convert: if v == instr { - bounds.ConvertFound = true + bounds.convertFound = true return bounds } } From 1c8555a4e4d4c78f4895fe1362bd4d9f567f1445 Mon Sep 17 00:00:00 2001 From: Cosmin Cojocar Date: Mon, 2 Sep 2024 12:49:32 +0000 Subject: [PATCH 18/21] Rebase and fix go module Change-Id: I8da6495eaaf25b1739389aa98492bd7df338085b Signed-off-by: Cosmin Cojocar --- go.mod | 1 + go.sum | 2 ++ 2 files changed, 3 insertions(+) diff --git a/go.mod b/go.mod index a6db75767c..6675ea40fb 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,7 @@ require ( github.com/onsi/gomega v1.34.2 github.com/stretchr/testify v1.9.0 golang.org/x/crypto v0.26.0 + golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 golang.org/x/lint v0.0.0-20210508222113-6edffad5e616 golang.org/x/text v0.17.0 golang.org/x/tools v0.24.0 diff --git a/go.sum b/go.sum index b4740e1615..88328d1dd1 100644 --- a/go.sum +++ b/go.sum @@ -428,6 +428,8 @@ golang.org/x/exp v0.0.0-20200119233911-0405dc783f0a/go.mod h1:2RIsYlXP63K8oxa1u0 golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM= golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU= golang.org/x/exp v0.0.0-20200331195152-e8c3332aa8e5/go.mod h1:4M0jN8W1tt0AVLNr8HDosyJCDCDuyL9N9+3m7wDWgKw= +golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 h1:2dVuKD2vS7b0QIHQbpyTISPd0LeHDbnYEryqj5Q1ug8= +golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56/go.mod h1:M4RDyNAINzryxdtnbRXRL/OHtkFuWGRjvuhBJpk2IlY= golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js= golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0= golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE= From 3c8f85fc99658ca497bc4994fc22e2a3721b1311 Mon Sep 17 00:00:00 2001 From: czechbol Date: Wed, 4 Sep 2024 11:49:55 +0200 Subject: [PATCH 19/21] fix false positive for negated value Signed-off-by: czechbol --- analyzers/conversion_overflow.go | 34 ++++++++++++++++++++++++++------ testutils/g115_samples.go | 19 ++++++++++++++++++ 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go index 4ad144f234..c3de89ed13 100644 --- a/analyzers/conversion_overflow.go +++ b/analyzers/conversion_overflow.go @@ -285,10 +285,6 @@ func hasExplicitRangeCheck(instr *ssa.Convert, dstType string) bool { minValue = 0 } } - case *ssa.Convert: - if v == instr { - break - } } if explicitValsInRange(explicitPositiveVals, explicitNegativeVals, dstInt) { @@ -346,7 +342,9 @@ func getResultRange(ifInstr *ssa.If, instr *ssa.Convert, visitedIfs map[*ssa.If] func updateResultFromBinOp(result *rangeResult, binOp *ssa.BinOp, instr *ssa.Convert, successPathConvert bool) { x, y := binOp.X, binOp.Y operandsFlipped := false - if x != instr.X { + + compareVal, op := getRealValueFromOperation(instr.X) + if x != compareVal { y, operandsFlipped = x, true } @@ -384,6 +382,18 @@ func updateResultFromBinOp(result *rangeResult, binOp *ssa.BinOp, instr *ssa.Con result.explixitPositiveVals = append(result.explixitPositiveVals, uint(constVal.Uint64())) } } + + if op == "neg" { + min := result.minValue + max := result.maxValue + + if min > 0 { + result.maxValue = uint(min) + } + if max < math.MaxInt { + result.minValue = int(max) + } + } } func updateMinMaxForLessOrEqual(result *rangeResult, constVal *ssa.Const, op token.Token, operandsFlipped bool, successPathConvert bool) { @@ -448,17 +458,29 @@ func walkBranchForConvert(block *ssa.BasicBlock, instr *ssa.Convert, visitedIfs } func isRangeCheck(v ssa.Value, x ssa.Value) bool { + compareVal, _ := getRealValueFromOperation(x) + switch op := v.(type) { case *ssa.BinOp: switch op.Op { case token.LSS, token.LEQ, token.GTR, token.GEQ, token.EQL, token.NEQ: - return op.X == x || op.Y == x + return op.X == compareVal || op.Y == compareVal } } return false } +func getRealValueFromOperation(v ssa.Value) (ssa.Value, string) { + switch v := v.(type) { + case *ssa.UnOp: + if v.Op == token.SUB { + return v.X, "neg" + } + } + return v, "" +} + func explicitValsInRange(explicitPosVals []uint, explicitNegVals []int, dstInt integer) bool { if len(explicitPosVals) == 0 && len(explicitNegVals) == 0 { return false diff --git a/testutils/g115_samples.go b/testutils/g115_samples.go index 84db8e0244..62d4993385 100644 --- a/testutils/g115_samples.go +++ b/testutils/g115_samples.go @@ -697,4 +697,23 @@ func main() { } `, }, 0, gosec.NewConfig()}, + {[]string{ + ` + package main + + import ( + "fmt" + "math/rand" + ) + + func main() { + a := rand.Int63() + if a >= 0 { + panic("no positivity allowed") + } + b := uint64(-a) + fmt.Printf("%d\n", b) + } + `, + }, 0, gosec.NewConfig()}, } From 58dc7646aded8f27906d9e245e52053608480448 Mon Sep 17 00:00:00 2001 From: czechbol Date: Wed, 4 Sep 2024 12:58:29 +0200 Subject: [PATCH 20/21] fix range conditions Signed-off-by: czechbol --- analyzers/conversion_overflow.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go index c3de89ed13..83b581f902 100644 --- a/analyzers/conversion_overflow.go +++ b/analyzers/conversion_overflow.go @@ -387,10 +387,10 @@ func updateResultFromBinOp(result *rangeResult, binOp *ssa.BinOp, instr *ssa.Con min := result.minValue max := result.maxValue - if min > 0 { + if min >= 0 { result.maxValue = uint(min) } - if max < math.MaxInt { + if max <= math.MaxInt { result.minValue = int(max) } } From 9f0427a6941f9e9ddb51537875360461c8cf9eb3 Mon Sep 17 00:00:00 2001 From: Cosmin Cojocar Date: Wed, 4 Sep 2024 14:00:19 +0000 Subject: [PATCH 21/21] Ignore the golangci/gosec G115 warning Change-Id: I0db56cb0a5f9ab6e815e2480ec0b66d7061b23d3 Signed-off-by: Cosmin Cojocar --- analyzers/conversion_overflow.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go index 83b581f902..3ef4825af5 100644 --- a/analyzers/conversion_overflow.go +++ b/analyzers/conversion_overflow.go @@ -391,7 +391,7 @@ func updateResultFromBinOp(result *rangeResult, binOp *ssa.BinOp, instr *ssa.Con result.maxValue = uint(min) } if max <= math.MaxInt { - result.minValue = int(max) + result.minValue = int(max) //nolint:gosec } } }