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.

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

Panic in gosec 2.21.3 conversion overflow analyzer #1229

Closed
gmwiz opened this issue Sep 19, 2024 · 4 comments · Fixed by #1232
Closed

Panic in gosec 2.21.3 conversion overflow analyzer #1229

gmwiz opened this issue Sep 19, 2024 · 4 comments · Fixed by #1232

Comments

@gmwiz
Copy link

gmwiz commented Sep 19, 2024

Summary

We started getting panic on some of our routine gosec scans. I'm not certain as to what exactly triggers it, but it happens when scanning a large project.

panic: unexpected constant value: <nil>

goroutine 1 [running]:
golang.org/x/tools/go/ssa.(*Const).Uint64(0xc0098c2300)
        golang.org/x/tools@v0.25.0/go/ssa/const.go:214 +0x105
github.com/securego/gosec/v2/analyzers.updateExplicitValues(0xc00124b250, 0xc0098c2300)
        github.com/securego/gosec/v2@v2.21.3/analyzers/conversion_overflow.go:398 +0xce
github.com/securego/gosec/v2/analyzers.updateResultFromBinOp(0xc00124b250, 0xc008eee1c0, 0xc00124b550?, 0x0)
        github.com/securego/gosec/v2@v2.21.3/analyzers/conversion_overflow.go:378 +0x110
github.com/securego/gosec/v2/analyzers.getResultRange(0xc004f6acd8, 0xc0030c7130, 0xc00124b550)
        github.com/securego/gosec/v2@v2.21.3/analyzers/conversion_overflow.go:322 +0x305
github.com/securego/gosec/v2/analyzers.hasExplicitRangeCheck(0xc0030c7130, {0xc0011a78d8?, 0x6?})
        github.com/securego/gosec/v2@v2.21.3/analyzers/conversion_overflow.go:270 +0x279
github.com/securego/gosec/v2/analyzers.isSafeConversion(0xc0030c7130)
        github.com/securego/gosec/v2@v2.21.3/analyzers/conversion_overflow.go:187 +0xaa
github.com/securego/gosec/v2/analyzers.runConversionOverflow(0xc005cb6460)
        github.com/securego/gosec/v2@v2.21.3/analyzers/conversion_overflow.go:81 +0x27e
github.com/securego/gosec/v2.(*Analyzer).CheckAnalyzers(0xc000e9c480, 0xc000f14000)
        github.com/securego/gosec/v2@v2.21.3/analyzer.go:446 +0x4a2
github.com/securego/gosec/v2.(*Analyzer).Process(0xc000e9c480, {0x0, 0x0, 0x0}, {0xc0002cd6c0, 0xd, 0x3d?})
        github.com/securego/gosec/v2@v2.21.3/analyzer.go:317 +0x488
main.main()
        github.com/securego/gosec/v2@v2.21.3/cmd/gosec/main.go:476 +0xde5

Steps to reproduce the behavior

Scan a directory using:

gosec -concurrency=1 -verbose -nosec=false -confidence=high -severity=high

gosec version

2.21.3

Go version (output of 'go version')

1.22.4

Operating system / Environment

Linux

@ldemailly
Copy link
Contributor

ldemailly commented Sep 19, 2024

I see that too scanning istio/istio for instance

and the code crashing is

func updateExplicitValues(result *rangeResult, constVal *ssa.Const) {
	if strings.Contains(constVal.String(), "-") {
		result.explicitNegativeVals = append(result.explicitNegativeVals, int(constVal.Int64()))
	} else {
		result.explicitPositiveVals = append(result.explicitPositiveVals, uint(constVal.Uint64()))
	}
}

which seems incorrect as it looks at constants that could be a string for instance with a - in it... (unless there is code elsewhere to ensure this is only called for numbers)

I can fix that I think if nobody gets to it (as I have #1231 too)

@ldemailly
Copy link
Contributor

So I have a workaround but I'm finding odd I can't reproduce the issue first (as in running again it disappears)

@ccojocar is there some caching somewhere ? how do I clear it (go clean -cache -testcache -modcache didn't help)

here is the "fix":

--- a/analyzers/conversion_overflow.go
+++ b/analyzers/conversion_overflow.go
@@ -357,7 +357,7 @@ func updateResultFromBinOp(result *rangeResult, binOp *ssa.BinOp, instr *ssa.Con
        }
 
        constVal, ok := y.(*ssa.Const)
-       if !ok {
+       if !ok || constVal == nil {
                return
        }

@ldemailly
Copy link
Contributor

edit: it's constVal.Value which is nil not constVal itself. and I can sort of repro, though in different directories each time, by nuking entirely my ~/go/pkg in between runs

@ldemailly
Copy link
Contributor

I added logging, here is an example:

		log.Fatalf("[gosec] constVal.Value is nil flipped=%t, constVal=%#v, binOp=%#v", operandsFlipped, constVal, binOp)

yields:

2024/09/19 11:53:04 [gosec] constVal.Value is nil flipped=false, constVal=&ssa.Const{typ:(*types.Pointer)(0x14001ec22f0), Value:constant.Value(nil)}, binOp=&ssa.BinOp{register:ssa.register{anInstruction:ssa.anInstruction{block:(*ssa.BasicBlock)(0x14003a89600)}, num:63, typ:(*types.Basic)(0x1019a4320), pos:151020405, referrers:[]ssa.Instruction{(*ssa.If)(0x1400519b0b0)}}, Op:39, X:(*ssa.UnOp)(0x14003600de0), Y:(*ssa.Const)(0x14003a8b920)}

ps: how do I reach the actual logger from that code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants