G115 is reporting false positives (a summary)

[czechbol]

trafficstars

Summary

G115 is still reporting false positives even after #1189 and #1194.

Known false positives

I'm writing these down without much investigation whether it is possible to implement fixes for them.

• any kind of arithmetic done on the checked variable after the check:

func foo(a int) uint {
    if a != 3 || a != 4 {
        panic("not supported")
    }
    // the value being passed here is different than the one checked 
    // so it is tricky to determine if the check is valid
    //
    // this will be difficult to implement without introducing false negatives
    return uint(a-1)  // false positive 
}

• explicit value checks are lacking:

func foo(a int) uint {
    if a == 3 || a != 4 {
        // see that sneaky NEQ there?
        return uint(a)  // false negative 
    }
    panic("not supported")
}

• binary truncation (reported by @stephenc):

func foo(a int) uint16 {
    return uint16(a &0xffff) // false positive 
}

• builtin min and max functions (reported by @ben-krieger):

func foo() uint16 {
    a, b := 1234, 2345
    result := min(a,b)

    return uint(result) // false positive 
}

• loop indices (reported by @PlasmaPower):

func foo(myArr []string) {
    // these seem to have a similar implementation difficulty to the arithmetic example
    for i, _ := range myArr {
        _ = uint64(i) // false positive 
    }

    for i := 0; i < 10; i++ {
        _ = uint64(i) // false positive 
    }

    for i := randIntMightBeNegativeEven(); i >= 0; i-- {
        _ = uint64(i) // false positive 
    }
}

Notes:

I recommend opening multiple small PRs that fix one issue at a time.

[ccoVeille]

At least these use cases are less frequent than the previous ones.

[deerbone]

I'd say that people will hit the first one pretty frequently, and it also seems to me it's the most difficult to tackle without bringing in any false negatives.

[Victoremepunto]

I think I may be hitting this: https://github.com/RedHatInsights/frontend-operator/pull/188/files#diff-e8b328ffb5ced885894c80b942846601597e05020f831c2b1442d8fb8a654c8dR120

[czechbol]

@Victoremepunto you're getting that warning because golangci-lint has not released a new version with the changes from #1194 yet. The code you have there looks sound so once we have a new golangci-lint release, it should work.

@ccoVeille do you know and can share at least approximately when we can expect it?

[ccoVeille]

I'm just a random gopher. I'm pretty active, but I'm not part of golangci-lint release.

@ldez might be a better candidate to reply that question

[ldez]

do you know and can share at least approximately when we can expect it?

I have no precise release date for now but soon, mainly because of G115 false positives.

[PlasmaPower]

I'm not sure if I should open a separate issue for this, but another false positive is casting the index produced by iterating over a range to an unsigned int. We do this a lot in our codebase. E.g.:

myArr := []string{"hello", "world"}

for i := range len(myArr) {
    _ = uint64(i) // shouldn't error but does with G115
}

for i, _ := range myArr {
    _ = uint64(i) // shouldn't error but does with G115
}

Ideally it'd be able to look at bounds in for loops as well, but I get that's more complicated:

for i := 0; i < 10; i++ {
    _ = uint64(i) // shouldn't error but does with G115
}

for i := randIntMightBeNegativeEven(); i >= 0; i-- {
    _ = uint64(i) // shouldn't error but does with G115
}

[ccoVeille]

Quick follow up, golangci-lint 1.61.0 was shipped with gosec 2.21.2 :muscle:

So everything that was made in gosec to improve G115 detection are now released :tada:

[czechbol]

I'm not sure if I should open a separate issue for this, but another false positive is casting the index produced by iterating over a range to an unsigned int. We do this a lot in our codebase. E.g.:

@PlasmaPower You are correct, this seems like another issue. I'll get back to it a bit later and update my summary.

[Victoremepunto]

@Victoremepunto you're getting that warning because golangci-lint has not released a new version with the changes from #1194 yet. The code you have there looks sound so once we have a new golangci-lint release, it should work.

@ccoVeille do you know and can share at least approximately when we can expect it?

Hi! unfortunately, using golangci-lint 1.60.1 the problem is still there - might worth trying to get a simpler reproducer and add it to this issue?

check this: https://github.com/RedHatInsights/frontend-operator/actions/runs/10835444044/job/30066989278?pr=188#step:5:26

[ldez]

Hi! unfortunately, using golangci-lint 1.60.1 the problem is still there

Maybe it's because the fix is inside golangci-lint v1.61.0 and not v1.60.1 :wink:

[Victoremepunto]

Hi! unfortunately, using golangci-lint 1.60.1 the problem is still there

Maybe it's because the fix is inside golangci-lint v1.61.0 and not v1.60.1 :wink:

lol, I'm sorry the version I used was v1.61.0, I just mistyped it. Here's an excerpt of the GHA logs:

run golangci-lint
  Running [/home/runner/golangci-lint-1.61.0-linux-amd64/golangci-lint run] in [/home/runner/work/frontend-operator/frontend-operator] ...
  Error: controllers/status.go:120:44: G115: integer overflow conversion int -> int32 (gosec)

[ben-krieger]

Hi! unfortunately, using golangci-lint 1.60.1 the problem is still there

Maybe it's because the fix is inside golangci-lint v1.61.0 and not v1.60.1 😉

lol, I'm sorry the version I used was v1.61.0, I just mistyped it. Here's an excerpt of the GHA logs:

run golangci-lint
  Running [/home/runner/golangci-lint-1.61.0-linux-amd64/golangci-lint run] in [/home/runner/work/frontend-operator/frontend-operator] ...
  Error: controllers/status.go:120:44: G115: integer overflow conversion int -> int32 (gosec)

In order to keep this thread a useful source of truth, let me fill in some details.

@Victoremepunto's issue was that accessing a struct field is considered a different value every time in SSA. This is because without deeper analysis (than SSA), you can't rule out a concurrent routine modifying that value between accesses.

Therefore, the issue could have been resolved without any changes to gosec by simply changing

if results.Managed < math.MinInt32 || results.Managed > math.MaxInt32 {
	return crd.FrontendDeployments{}, "", errors.NewClowderError("value out of range for int32")
}
deploymentStats.ManagedDeployments = int32(results.Managed)

to

managed := results.Managed
if managed < math.MinInt32 || managed > math.MaxInt32 {
	return crd.FrontendDeployments{}, "", errors.NewClowderError("value out of range for int32")
}
deploymentStats.ManagedDeployments = int32(managed)

However, since struct field access is a common use case and modifying the value at the same time as reading it is a race condition (which is usually considered a bug), #1221 treats field accesses as not being modified between time-of-check and time-of-use (TOCTOU).

And yes, TOCTOU is its own type of potential security vulnerability. Maybe a future linter can better look for this.

[czechbol]

Thanks @ben-krieger for the explanation. I purposefully decided to ignore TOCTOU in that PR.

The proposal of a separate lint rule for it is a very good idea since a different lint message would explain the issue better.

[ldemailly]

Contributing a real world example of false positive with min:

p1.R = uint8(min(255, uint16(p1.R)+uint16(p2.R)))

[ccojocar]

On this, I think we need to start the Obfuscated Go Integer Overflow Contest. 😄

[MichalSalon]

Another one I stumbled upon after updating golangci-lint to 1.61.0

func foo(hash string) (uint32, error) {
	decodedHash, err := base64.RawStdEncoding.DecodeString(hash)
	if err != nil {
		return 0, err
	}

	if hashLen := len(decodedHash); hashLen > math.MaxUint32 {
		return 0, fmt.Errorf("invalid decoded hash length %d, exceeds max value for uint32", hashLen)
	}
	return uint32(len(decodedHash)), nil
}

It works if I change it to:

func foo(hash string) (uint32, error) {
	decodedHash, err := base64.RawStdEncoding.DecodeString(hash)
	if err != nil {
		return 0, err
	}

	hashLen := len(decodedHash)
	if hashLen > math.MaxUint32 {
		return 0, fmt.Errorf("invalid decoded hash length %d, exceeds max value for uint32", hashLen)
	}
	return uint32(hashLen), nil
}

But since the slice was created inside of the function and is not changed in any way, I would still say that this is a false positive.

[ctarsjp]

Another false positive (similar to binary truncation with '&', but using bit shifting):

var (
	u32 uint32
	u8  uint8
)

u8 = uint8(u32 >> 24)

[hairyhenderson]

I ran into a false G115 positive just now, using strconv.ParseUint:

func main() {
	a := "13"
	b, _ := strconv.ParseUint(a, 10, 8)
	c := int(b)
	fmt.Printf("%d\n", c)
}

This fails with G115: integer overflow conversion uint64 -> int

There's actually a unit test similar to this issue, except it casts to a uint8 rather than an int.

[hairyhenderson]

I've issued https://github.com/securego/gosec/pull/1254 to fix the ParseUint issue I described above.

[theory]

Another false positive with min:

func main() {
	if len(os.Args) > 1 {
		num, err := strconv.ParseInt(os.Args[1], 10, 64)
		if err != nil { panic(err) }
		var x uint32
		if num >= 0 {
			x = uint32(min(num, math.MaxUint32))
		}
		fmt.Printf("%v\n", x)
	}
}

Can only avoid the issue by changing the if statement to

		if num >= 0 && num <= math.MaxUint32 {
			x = uint32(num)
		}

[ad-zsolt-imre]

Golang version: 1.23.5 Gosec version: v2.22.1

The below also reports "integer overflow conversion".

uint64(time.Now().UnixMilli())

Given max uint64 (18446744073709551615) is greater than max int64 (9223372036854775807), how can a conversion from int64 to uint64 result in an "overflow"? Wouldn't the opposite, uint64 -> int64 result in an overflow if the value of uint64 greater than max int64?

[kaovilai]

pkg/repository/udmrepo/kopialib/lib_repo.go:527:17: G115: integer overflow conversion int -> int32 (gosec)
                Length:  int32(min(mani.Length, math.MaxInt32)),

another false postive.

[theory]

Here's another one:

const multiple = 4
need := uint8(multiple - (len(digest) % multiple))

[eest]

I seem to be getting problems with nested structs, here is a contrived example:

package main

import (
	"fmt"
	"math"
)

type nestedStruct struct {
	i *innerStruct
}

type innerStruct struct {
	u32 *uint32
}

func main() {
	n := nestedStruct{
		i: &innerStruct{
			u32: new(uint32),
		},
	}

	i := innerStruct{
		u32: new(uint32),
	}

	var input uint32
	_, err := fmt.Scanf("%d", &input)
	if err != nil {
		panic(err)
	}

	n.i.u32 = &input
	i.u32 = &input

	if *n.i.u32 > math.MaxInt32 {
		panic("out of range")
	} else {
		i32 := int32(*n.i.u32)
		fmt.Println(i32)
	}

	if *i.u32 > math.MaxInt32 {
		panic("out of range")
	} else {
		i32 := int32(*i.u32)
		fmt.Println(i32)
	}
}

This gives me:

$ gosec ./...
[...]
[.../main.go:39] - G115 (CWE-190): integer overflow conversion uint32 -> int32 (Confidence: MEDIUM, Severity: HIGH)
    38: 	} else {
  > 39: 		i32 := int32(*n.i.u32)
    40: 		fmt.Println(i32)

So it seems happy with the the bounds check on *i.u32, but not with *n.i.u32

[dolzenko]

package main

func main() {
	var x int32
	switch {
	case x > 0:
		switch {
		// if just the default case is left G115 doesn't trigger
		// default:
		// 	f(uint64(x))
		case true:
			f(uint64(x)) // triggers G115
		}			
	}
}

func f(_ uint64) {}