diff --git a/rules/sdk/int_end_conversion_test.go b/rules/sdk/int_end_conversion_test.go new file mode 100644 index 0000000..55faae2 --- /dev/null +++ b/rules/sdk/int_end_conversion_test.go @@ -0,0 +1,65 @@ +package sdk + +import "testing" + +func TestCanOverflowChecks32Bits(t *testing.T) { + if !is32Bit { + t.Skip("Not running on a 64-bit machine!") + } + + cases := []struct { + endKind string + wantOverflow bool + }{ + {"int8", true}, + {"int16", true}, + {"int32", false}, + {"int64", false}, + {"int", false}, + {"uint8", true}, + {"uint16", true}, + {"uint32", false}, + {"uint64", false}, + {"uint", false}, + } + + for _, tt := range cases { + tt := tt + t.Run(tt.endKind, func(t *testing.T) { + if got := canLenOverflow32(tt.endKind); got != tt.wantOverflow { + t.Fatalf("Mismatch\n\tGot: %t\n\tWant:%t", got, tt.wantOverflow) + } + }) + } +} + +func TestCanOverflowChecks64Bits(t *testing.T) { + if is32Bit { + t.Skip("Not running on a 32-bit machine!") + } + + cases := []struct { + endKind string + wantOverflow bool + }{ + {"int8", true}, + {"int16", true}, + {"int32", true}, + {"int", false}, + {"int64", false}, + {"uint8", true}, + {"uint16", true}, + {"uint32", true}, + {"uint64", false}, + {"uint", false}, + } + + for _, tt := range cases { + tt := tt + t.Run(tt.endKind, func(t *testing.T) { + if got := canLenOverflow64(tt.endKind); got != tt.wantOverflow { + t.Fatalf("Mismatch\n\tGot: %t\n\tWant:%t", got, tt.wantOverflow) + } + }) + } +} diff --git a/rules/sdk/integer.go b/rules/sdk/integer.go index 5e76b20..9870483 100644 --- a/rules/sdk/integer.go +++ b/rules/sdk/integer.go @@ -72,9 +72,23 @@ func (i *integerOverflowCheck) Match(node ast.Node, ctx *gosec.Context) (*gosec. switch arg := arg.(type) { case *ast.CallExpr: // len() returns an int that is always >= 0, so it will fit in a uint, uint64, or int64. - if argFun, ok := arg.Fun.(*ast.Ident); ok && argFun.Name == "len" && (fun.Name == "uint" || fun.Name == "uint64" || fun.Name == "int64") { - return nil, nil + argFun, ok := arg.Fun.(*ast.Ident) + if !ok || argFun.Name != "len" { + break + } + + // Please see the rules for determining if *int*(len(...)) can overflow + // as per: https://github.com/cosmos/gosec/issues/54 + lenCanOverflow := canLenOverflow64 + if is32Bit { + lenCanOverflow = canLenOverflow32 } + + if lenCanOverflow(fun.Name) { + return gosec.NewIssue(ctx, n, i.ID(), i.What, i.Severity, i.Confidence), nil + } + return nil, nil + case *ast.SelectorExpr: // If the argument is being cast to its underlying type, there's no risk. if ctx.Info.TypeOf(arg).Underlying() == ctx.Info.TypeOf(fun) { @@ -104,3 +118,76 @@ func NewIntegerCast(id string, conf gosec.Config) (gosec.Rule, []ast.Node) { }, }, []ast.Node{(*ast.FuncDecl)(nil), (*ast.AssignStmt)(nil), (*ast.CallExpr)(nil)} } + +// Please see the rules at https://github.com/cosmos/gosec/issues/54 +func canLenOverflow64(destKind string) bool { + switch destKind { + case "int8", "uint8", "int16", "uint16": + return true + + case "uint64": + // uint64([0, maxInt64]) + return false + + case "uint32": + // uint32([0, maxInt64]) + return true + + case "uint": + // uint => uint64 => uint64([0, maxInt64]) + return false + + case "int64": + // int64([0, maxInt64]) + return false + + case "int32": + // int32([0, maxInt64]) + return true + + case "int": + // int64([0, maxInt64]) + return false + + default: + return true + } +} + +const s = 1 +const is32Bit = (^uint(s-1))>>32 == 0 // #nosec + +// Please see the rules at https://github.com/cosmos/gosec/issues/54 +func canLenOverflow32(destKind string) bool { + switch destKind { + case "int8", "uint8", "int16", "uint16": + return true + + case "uint64": + // uint64([0, maxInt32]) + return false + + case "uint32": + // uint32([0, maxInt32]) + return false + + case "uint": + // uint => uint32 => uint32([0, maxInt32]) + return false + + case "int64": + // int64([0, maxInt32]) + return false + + case "int32": + // int32([0, maxInt32]) + return false + + case "int": + // int => int32 => int32([0, maxInt32]) + return false + + default: + return true + } +}