G115: integer overflow conversion uint8 -> int64 about gosec HOT 21 CLOSED

Agree, lots of alerts from yesterday in our CI/CD pipeline

from gosec.

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

from gosec.

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 :) golangci/golangci-lint#4939

from gosec.

Broken in this change I guess #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.

from gosec.

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)

from gosec.

@xsteadfastx your options are:

1. Rearchitect to avoid using specific integer types and orient around int only. This may well be Go best practice; Effective Go pretty much only uses int, outside of one instance where it refers to uint64 and then casts to int64 (which itself would fail G115) and there's other instances where "standardize around int" has been advocated, although I'm unsure how common it is outside people who debate best practice.
2. Disable the rule, either when running gosec specifically or by adding a flag to your golangci config:

linters-settings:
  gosec:
    excludes:
      # Flags for potentially-unsafe casting of ints, similar problem to globally-disabled G103
      - G115

3. If you runner supports //nolint, you can manually review every flag and mark up each one you're assuming safe in your context with //nolint:gosec

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.

from gosec.

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

from gosec.

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.

from gosec.

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

from gosec.

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)
}

from gosec.

For info, G115 was disabled by default in golangci-lint

• golangci/golangci-lint#4941

from gosec.

The rule can be simply excluded from the scanning if is causing too many issues on your code base:

gosec -exclude=G115 ./...

from gosec.

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...

from gosec.

@ldemailly the false positive from byte to int64 conversion is not reproducible, see the attached tests in #1186.

from gosec.

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.

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.

from gosec.

@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.

from gosec.

@ldemailly the false positive from byte to int64 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

from gosec.

@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)
}

from gosec.

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? #1187

Its much more difficult to detect but I think most of the existing detections are a lot more accurate.

from gosec.

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.

from gosec.

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

from gosec.