From 1f3bdd93493b70e06e508b51ea7ad757e8f2f21e Mon Sep 17 00:00:00 2001 From: czechbol Date: Mon, 16 Sep 2024 10:30:54 +0200 Subject: [PATCH] G115 Struct Attribute Checks (#1221) * allow struct attributes checks * fix explicit check results --- analyzers/conversion_overflow.go | 86 +++++++++++++++++++---------- testutils/g115_samples.go | 93 ++++++++++++++++++++++++++++++++ 2 files changed, 151 insertions(+), 28 deletions(-) diff --git a/analyzers/conversion_overflow.go b/analyzers/conversion_overflow.go index 3ef4825..d8efd50 100644 --- a/analyzers/conversion_overflow.go +++ b/analyzers/conversion_overflow.go @@ -40,7 +40,7 @@ type integer struct { type rangeResult struct { minValue int maxValue uint - explixitPositiveVals []uint + explicitPositiveVals []uint explicitNegativeVals []int isRangeCheck bool convertFound bool @@ -271,7 +271,7 @@ 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...) + explicitPositiveVals = append(explicitPositiveVals, result.explicitPositiveVals...) explicitNegativeVals = append(explicitNegativeVals, result.explicitNegativeVals...) } case *ssa.Call: @@ -325,16 +325,17 @@ func getResultRange(ifInstr *ssa.If, instr *ssa.Convert, visitedIfs map[*ssa.If] 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...) } + result.explicitPositiveVals = append(result.explicitPositiveVals, thenBounds.explixitPositiveVals...) + result.explicitNegativeVals = append(result.explicitNegativeVals, thenBounds.explicitNegativeVals...) + result.explicitPositiveVals = append(result.explicitPositiveVals, elseBounds.explixitPositiveVals...) + result.explicitNegativeVals = append(result.explicitNegativeVals, elseBounds.explicitNegativeVals...) + return result } @@ -344,8 +345,15 @@ func updateResultFromBinOp(result *rangeResult, binOp *ssa.BinOp, instr *ssa.Con operandsFlipped := false compareVal, op := getRealValueFromOperation(instr.X) - if x != compareVal { - y, operandsFlipped = x, true + + // Handle FieldAddr + if fieldAddr, ok := compareVal.(*ssa.FieldAddr); ok { + compareVal = fieldAddr + } + + if !isSameOrRelated(x, compareVal) { + y = x + operandsFlipped = true } constVal, ok := y.(*ssa.Const) @@ -362,25 +370,12 @@ func updateResultFromBinOp(result *rangeResult, binOp *ssa.BinOp, instr *ssa.Con 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())) - } - + updateExplicitValues(result, constVal) 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())) - } + updateExplicitValues(result, constVal) } if op == "neg" { @@ -391,11 +386,19 @@ func updateResultFromBinOp(result *rangeResult, binOp *ssa.BinOp, instr *ssa.Con result.maxValue = uint(min) } if max <= math.MaxInt { - result.minValue = int(max) //nolint:gosec + result.minValue = int(max) } } } +func updateExplicitValues(result *rangeResult, constVal *ssa.Const) { + if strings.Contains(constVal.String(), "-") { + result.explicitNegativeVals = append(result.explicitNegativeVals, int(constVal.Int64())) + } else { + result.explicitPositiveVals = append(result.explicitPositiveVals, 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 successPathConvert && !operandsFlipped { @@ -439,6 +442,8 @@ func walkBranchForConvert(block *ssa.BasicBlock, instr *ssa.Convert, visitedIfs if result.isRangeCheck { bounds.minValue = toPtr(max(result.minValue, bounds.minValue)) bounds.maxValue = toPtr(min(result.maxValue, bounds.maxValue)) + bounds.explixitPositiveVals = append(bounds.explixitPositiveVals, result.explicitPositiveVals...) + bounds.explicitNegativeVals = append(bounds.explicitNegativeVals, result.explicitNegativeVals...) } case *ssa.Call: if v == instr.X { @@ -463,9 +468,10 @@ func isRangeCheck(v ssa.Value, x ssa.Value) bool { 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 == compareVal || op.Y == compareVal + case token.LSS, token.LEQ, token.GTR, token.GEQ, token.EQL, token.NEQ: + leftMatch := isSameOrRelated(op.X, compareVal) + rightMatch := isSameOrRelated(op.Y, compareVal) + return leftMatch || rightMatch } } return false @@ -475,12 +481,36 @@ func getRealValueFromOperation(v ssa.Value) (ssa.Value, string) { switch v := v.(type) { case *ssa.UnOp: if v.Op == token.SUB { - return v.X, "neg" + val, _ := getRealValueFromOperation(v.X) + return val, "neg" } + return getRealValueFromOperation(v.X) + case *ssa.FieldAddr: + return v, "field" + case *ssa.Alloc: + return v, "alloc" } return v, "" } +func isSameOrRelated(a, b ssa.Value) bool { + aVal, _ := getRealValueFromOperation(a) + bVal, _ := getRealValueFromOperation(b) + + if aVal == bVal { + return true + } + + // Check if both are FieldAddr operations referring to the same field of the same struct + if aField, aOk := aVal.(*ssa.FieldAddr); aOk { + if bField, bOk := bVal.(*ssa.FieldAddr); bOk { + return aField.X == bField.X && aField.Field == bField.Field + } + } + + return false +} + 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 62d4993..9c889dc 100644 --- a/testutils/g115_samples.go +++ b/testutils/g115_samples.go @@ -716,4 +716,97 @@ func main() { } `, }, 0, gosec.NewConfig()}, + {[]string{ + ` + package main + + import ( + "fmt" + "math" + ) + + type CustomStruct struct { + Value int + } + + func main() { + results := CustomStruct{Value: 0} + if results.Value < math.MinInt32 || results.Value > math.MaxInt32 { + panic("value out of range for int32") + } + convertedValue := int32(results.Value) + + fmt.Println(convertedValue) + } + `, + }, 0, gosec.NewConfig()}, + {[]string{ + ` + package main + + import ( + "fmt" + "math" + ) + + type CustomStruct struct { + Value int + } + + func main() { + results := CustomStruct{Value: 0} + if results.Value >= math.MinInt32 && results.Value <= math.MaxInt32 { + convertedValue := int32(results.Value) + fmt.Println(convertedValue) + } + panic("value out of range for int32") + } + `, + }, 0, gosec.NewConfig()}, + {[]string{ + ` + package main + + import ( + "fmt" + "math" + ) + + type CustomStruct struct { + Value int + } + + func main() { + results := CustomStruct{Value: 0} + if results.Value < math.MinInt32 || results.Value > math.MaxInt32 { + panic("value out of range for int32") + } + // checked value is decremented by 1 before conversion which is unsafe + convertedValue := int32(results.Value-1) + + fmt.Println(convertedValue) + } + `, + }, 1, 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") + } + // checked value is incremented by 1 before conversion which is unsafe + b := int32(a+1) + fmt.Printf("%d\n", b) + } + `, + }, 1, gosec.NewConfig()}, }