Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rules/sdk: a conversion of a smaller uint type to a larger uint type MUST NEVER report an overflow/false positive #56

Closed
odeke-em opened this issue Oct 13, 2022 · 1 comment · Fixed by #58
Closed

rules/sdk: a conversion of a smaller uint type to a larger uint type MUST NEVER report an overflow/false positive #56

odeke-em opened this issue Oct 13, 2022 · 1 comment · Fixed by #58
Assignees
@odeke-em
Labels
bug Something isn't working

Comments

@odeke-em
Copy link
Collaborator

This code flags overflows

package inttests

func it() {
        _  = uint64(uint32(0))
        _  = uint(uint32(0))
        _  = uint(uint16(0))
        _  = uint(uint8(0))
}

sadly per

$ gosec ./...
[/inttests/it.go:7] - G701 (CWE-): Potential integer overflow by integer type conversion (Confidence: MEDIUM, Severity: HIGH)
    6:         _  = uint(uint16(0))
  > 7:         _  = uint(uint8(0))
    8: }

[/inttests/it.go:6] - G701 (CWE-): Potential integer overflow by integer type conversion (Confidence: MEDIUM, Severity: HIGH)
    5:         _  = uint(uint32(0))
  > 6:         _  = uint(uint16(0))
    7:         _  = uint(uint8(0))

[/inttests/it.go:5] - G701 (CWE-): Potential integer overflow by integer type conversion (Confidence: MEDIUM, Severity: HIGH)
    4:         _  = uint64(uint32(0))
  > 5:         _  = uint(uint32(0))
    6:         _  = uint(uint16(0))

[/inttests/it.go:4] - G701 (CWE-): Potential integer overflow by integer type conversion (Confidence: MEDIUM, Severity: HIGH)
    3: func it() {
  > 4:         _  = uint64(uint32(0))
    5:         _  = uint(uint32(0))

Summary:
   Files: 1
   Lines: 8
   Nosec: 0
  Issues: 4

but it really shouldn't report that as an overflow because for every uint* values that's smaller in range , the bits can fit in e..g

  • max(uint8) < max(uint16) < max(uint32) < max(uint64)
@odeke-em odeke-em added the bug Something isn't working label Oct 13, 2022
@odeke-em odeke-em self-assigned this Oct 13, 2022
@odeke-em
Copy link
Collaborator Author

The same thing happens for int to int conversions

odeke-em added a commit that referenced this issue Oct 14, 2022
@odeke-em
rules/sdk: intelligently flag overflowing uint*->uint* + int*->int* c…
e5034d7 
…onversions

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
@odeke-em odeke-em mentioned this issue Oct 14, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@odeke-em odeke-em

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

rules/sdk: intelligently flag overflowing uint*->uint* + int*->int* conversions cosmos/gosec
1 participant
@odeke-em