Skip to content

Commit 9592313

Browse files
committed
rules/sdk: intelligently flag overflowing uint*->uint* + int*->int* conversions
Retrieve the underlying types to perform smarter conversions. More importantly, this change intelligently flags overflowing conversions for homogenous signedness aka: * int* -> int* * uint* -> uint* whereby for each same signedness, just check widths where: + 64-bit machines: uint64 == uint > uint32 > uint16 > uint8 int64 == int > int32 > int16 > int8 + 32-bit machines: uint64 > uint == uint32 > uint16 > uint8 int64 > int == int32 > int16 > int8 and this change only flags the offending non-fitting conversions. For an exhibit, this code ```go package inttests type in = int type uin = uint func it() { _ = uint64(uint32(0)) _ = uint(uint32(0)) _ = uint(uint16(0)) _ = uint(uint8(0)) _ = uint(uint64(0)) _ = uint32(uint64(0)) _ = uint16(uint64(0)) _ = uint8(uint64(0)) _ = uint8(uint(0)) _ = uint8(uint32(0)) _ = uint8(uint64(0)) _ = int(uint(0)) _ = in(uint(0)) _ = uin(uint(0)) _ = uin(uint32(0)) } ``` * Previously: + Could not catch the aliased int with overflowing potential from uint + Falsely flagged all the rest as overflowing so 12 issues * Currently: + Catches the aliased int with overflowing potential from uint + Only flags the actually overflowing conversions so only 8 issues Fixes #56 Fixes #57
1 parent 7457825 commit 9592313

File tree

1 file changed

+93
-12
lines changed

1 file changed

+93
-12
lines changed

rules/sdk/integer.go

Lines changed: 93 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,15 @@ func (i *integerOverflowCheck) ID() string {
3232
return i.MetaData.ID
3333
}
3434

35+
func hasAnyPrefix(src string, prefixes ...string) bool {
36+
for _, prefix := range prefixes {
37+
if strings.HasPrefix(src, prefix) {
38+
return true
39+
}
40+
}
41+
return false
42+
}
43+
3544
// To catch integer type conversion, check if we ever
3645
// call functions `uintX(y)` or `intX(y)` for any X and y,
3746
// where y is not an int literal.
@@ -51,7 +60,15 @@ func (i *integerOverflowCheck) Match(node ast.Node, ctx *gosec.Context) (*gosec.
5160
return nil, nil
5261
}
5362

54-
intCast := strings.HasPrefix(fun.Name, "int") || strings.HasPrefix(fun.Name, "uint")
63+
if len(n.Args) == 0 {
64+
return nil, nil
65+
}
66+
67+
arg := n.Args[0]
68+
argType := ctx.Info.TypeOf(arg).Underlying()
69+
destType := ctx.Info.TypeOf(fun).Underlying()
70+
71+
intCast := hasAnyPrefix(destType.String(), "int", "uint")
5572
if !intCast {
5673
return nil, nil
5774
}
@@ -60,7 +77,6 @@ func (i *integerOverflowCheck) Match(node ast.Node, ctx *gosec.Context) (*gosec.
6077
// n.Args[0] is of type ast.Expr. It's the arg to the type conversion.
6178
// If the expression string is a constant integer, then ignore.
6279
// TODO: check that the constant will actually fit and wont overflow?
63-
arg := n.Args[0]
6480
exprString := types.ExprString(arg)
6581
intLiteral, err := strconv.Atoi(exprString)
6682
if err == nil {
@@ -88,19 +104,26 @@ func (i *integerOverflowCheck) Match(node ast.Node, ctx *gosec.Context) (*gosec.
88104
return gosec.NewIssue(ctx, n, i.ID(), i.What, i.Severity, i.Confidence), nil
89105
}
90106
return nil, nil
107+
}
91108

92-
case *ast.SelectorExpr:
93-
// If the argument is being cast to its underlying type, there's no risk.
94-
if ctx.Info.TypeOf(arg).Underlying() == ctx.Info.TypeOf(fun) {
95-
return nil, nil
96-
}
109+
// If the argument is being cast to its underlying type, there's no risk.
110+
if argType == destType {
111+
return nil, nil
112+
}
113+
114+
// Check if both are uint* values.
115+
argIsUint := hasAnyPrefix(argType.String(), "uint")
116+
if argIsUint && !canBothUintsOverflow(argType.String(), destType.String()) {
117+
return nil, nil
97118
}
98119

99-
// TODO: run the go type checker to determine the
100-
// type of arg so we can check if the type
101-
// conversion is reducing the bit-size and could overflow.
102-
// If not, this will be a false positive for now ...
103-
// See https://golang.org/pkg/go/types/#Config.Check
120+
// Check if both are int* values.
121+
argIsInt := hasAnyPrefix(argType.String(), "int")
122+
if argIsInt && !canBothIntToIntOverflow(argType.String(), destType.String()) {
123+
return nil, nil
124+
}
125+
126+
// ALl other cases should be flagged.
104127
return gosec.NewIssue(ctx, n, i.ID(), i.What, i.Severity, i.Confidence), nil
105128
}
106129

@@ -191,3 +214,61 @@ func canLenOverflow32(destKind string) bool {
191214
return true
192215
}
193216
}
217+
218+
func canBothUintsOverflow(srcKind, destKind string) bool {
219+
bothUints := hasAnyPrefix(srcKind, "uint") && hasAnyPrefix(destKind, "uint")
220+
if !bothUints {
221+
return true
222+
}
223+
224+
if destKind == "uint" {
225+
// Only in 32-bit is uint equal to uint32 hence can it overflow if src is uint64.
226+
return srcKind == "uint64" && is32Bit
227+
}
228+
if destKind == "uint64" {
229+
// Casting any uint type to uint64 cannot overflow.
230+
return false
231+
}
232+
if destKind == "uint32" {
233+
// Only uint64 or uint (when in 64-bits) can overflow when being cast to uint32.
234+
return srcKind == "uint64" || (srcKind == "uint" && !is32Bit)
235+
}
236+
if destKind == "uint16" {
237+
// Everything except "uint8" and "uint16" can overflow when cast to uint16.
238+
return srcKind == "uint64" || srcKind == "uint32" || srcKind == "uint"
239+
}
240+
if destKind == "uint8" {
241+
// Everything that isn't "uint8" will overflow when cast to uint8.
242+
return srcKind != "uint8"
243+
}
244+
return true
245+
}
246+
247+
func canBothIntToIntOverflow(srcKind, destKind string) bool {
248+
bothInts := hasAnyPrefix(srcKind, "int") && hasAnyPrefix(destKind, "int")
249+
if !bothInts {
250+
return true
251+
}
252+
253+
if destKind == "int" {
254+
// Only in 32-bit is int equal to int32 hence can it overflow if src is int64.
255+
return srcKind == "int64" && is32Bit
256+
}
257+
if destKind == "int64" {
258+
// Casting any int type to int64 cannot overflow.
259+
return false
260+
}
261+
if destKind == "int32" {
262+
// Only int64 or int (when in 64-bits) can overflow when being cast to int32.
263+
return srcKind == "int64" || (srcKind == "int" && !is32Bit)
264+
}
265+
if destKind == "int16" {
266+
// Everything except "int8" and "int16" can overflow when cast to int16.
267+
return srcKind == "int64" || srcKind == "int32" || srcKind == "int"
268+
}
269+
if destKind == "int8" {
270+
// Everything that isn't "int8" will overflow when cast to int8.
271+
return srcKind != "int8"
272+
}
273+
return true
274+
}

0 commit comments

Comments
 (0)