Closed ldemailly closed 3 weeks ago
Agree, lots of alerts from yesterday in our CI/CD pipeline
Yes same here, could you add documentation about how to make G115 pass? Otherwise we are going to ignore all alerts :( We've tried to add bound checks and nothing works
Broken in this change I guess https://github.com/securego/gosec/issues/1130
This seems like a seriously under-cooked change which is currently mandating a lot of //nolint flags for us.
Example of another goof -> foo := []int{1,2,3}; bar := uint32(len(foo))
cannot possibly cause data loss yet now fails.
I was also surprised to see #1149 being merged so fast.
While the idea is good, why not after all. But it should have been reviewed by testing the behavior of the rules on large codebase.
Or make this rule optional at first
The rule can be simply excluded from the scanning if is causing too many issues on your code base:
gosec -exclude=G115 ./...
For my toolchain I've opted for adding //nolint:gosec to lines where casting is required and otherwise reviewed casting, so I suppose it's a useful exercise for code tidiness...
Example of another goof -> foo := []int{1,2,3}; bar := uint32(len(foo)) cannot possibly cause data loss yet now fails.
@FairlySadPanda the len
function returns an int
which is variable based on which architecture are you running. So in a 64 bit arch which is the most common these days, the value is actually int64. In the end, you are converting from int64
to uin32
this is a clear overflow.
@ldemailly the false positive from byte
to int64
conversion is not reproducible, see the attached tests in #1186.
Example of another goof -> foo := []int{1,2,3}; bar := uint32(len(foo)) cannot possibly cause data loss yet now fails.
@FairlySadPanda the
len
function returns anint
which is variable based on which architecture are you running. So in a 64 bit arch which is the most common these days, the value is actually int64. In the end, you are converting fromint64
touin32
this is a clear overflow.
I actually had not considered this, and it's a good point, but this doesn't sit completely right still. Casting is ultimately the executive decision of the programmer; having to specifically disable the rule or add a flag to skip specific casts as known-good means the value of the test itself becomes questionable.
Int and the other core builtins being variable in size would be good to communicate as part of the failure string when doing this sort of check, regardless - both for this and 109. Whilst 64-bit is the default these days, the specification itself is written to cope with 32 bit, and it's easy to get led astray by the documentation.
@FairlySadPanda You are free to skip this rule if you don't find it useful for your use case. Nobody is dictating its usage, but it some cases, integer overflow can lead to security issues. Unfortunately go runtime doesn't protect against this.
@ldemailly the false positive from
byte
toint64
conversion is not reproducible, see the attached tests in #1186.
I don't see that test and I definitely see the error. unless it's been fixed since the version golanglint-ci picked in 1.60.2 - on phone but will link ci output as well as repro in a bit
I can reproduce the "uint8 -> int" overflow false positive on my side. See the screenshot. FYI I'm using golangci-lint v1.60.2 (I didn't try gosec itself)
@ccojocar I updated the repro, it was simplified too much: this reproes
main.go:9:12: G115: integer overflow conversion uint8 -> int64 (gosec)
i := int64(str[0])
^
package main
import (
"fmt"
)
func main() {
str := "A\xFF"
i := int64(str[0])
fmt.Printf("%d\n", i)
}
Thanks to the OP for reporting this and the gosec contributors for working on it.
We have this issue with G115: integer overflow conversion uint8 -> int (gosec)
Which should fit according to the spec.
Without being too critical: Should this be enabled at all if we don't have a way to detect if the code block is doing proper bounds checks? Like as suggested here? https://github.com/securego/gosec/issues/1187
Its much more difficult to detect but I think most of the existing detections are a lot more accurate.
FIY bounds checks which are not implicit in the int type are not yet handled, this should be addressed by #1187. Please add any code sample which handles the bound checks explicitly in that issue. Thanks
how im supposed to fix something like this? i mean i parse a string via strconv.Atoi
and need to be sure it fits into an uint64. I think this gosec rule makes alot of sense... i just dont get how i can work through the code that throws this linting error.
@xsteadfastx your options are:
linters-settings:
gosec:
excludes:
# Flags for potentially-unsafe casting of ints, similar problem to globally-disabled G103
- G115
I've gone with a mix of 2 and 3 for the time being.
I suspect there's other folks like me who got caught out here when pulling a latest from golang-ci. Given other bossy rules from gosec are globally-disabled there, I might raise an issue there to see if G115 should get added.
how im supposed to fix something like this? i mean i parse a string via strconv.Atoi and need to be sure it fits into an uint64. I think this gosec rule makes alot of sense... i just dont get how i can work through the code that throws this linting error.
@xsteadfastx the strconv.Atoi
returns an int type which is typically int64 in a 64bit CPU architecture. This is safe to convert to a uint64
. The rule doesn't generate any warning in this case. This use case is coverted by tests.
If you try to scan this code sample, you can see that gosec doesn't return any warning:
package main
import (
"fmt"
"log"
"strconv"
)
func main() {
a, err := strconv.Atoi("1")
if err != nil {
log.Fatalf("converting str to int: %s", err)
}
b := uint64(a)
fmt.Printf("%d\n", b)
}
I've opened a discussion on golangci's repo as this is probably more of a concern for folks doing CI than people wanting to do one-off security sweeps, where detecting possible overflows is of higher value :) https://github.com/golangci/golangci-lint/discussions/4939
For info, G115 was disabled by default in golangci-lint
Thanks for the quick fix for the original issue I reported (and thanks for the thoughts on figuring out code that does bound check to auto not flag)
G115 was disabled
Will be once this merge, which will create yet another set of nolintlint now errors in CI
Summary
uint8 -> int64 has no overflow
Steps to reproduce the behavior
gosec version
81cda2f91fbe
Go version (output of 'go version')
go version go1.22.6 darwin/arm64
Operating system / Environment
n/a
Expected behavior
no complaint
Actual behavior
complains G115: integer overflow conversion uint8 -> int64: