Comments (21)
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:
- 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.
- 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
- 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
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 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.
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
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
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.
Related Issues (20)
- Add detection of overflow during integer conversion HOT 1
- Regarding customizing a configuration file to load G101 rules, where keywords with "-" cannot be matched HOT 1
- Documentation on how to use sig files for release binaries HOT 1
- 'go install' doesn't work HOT 2
- Gosec Configuration file to integrate with GitHub ( as a workflow file) HOT 2
- Add an option to set the Go version HOT 4
- criteria for adding new secret patterns versus relying on high entropy detection HOT 1
- Setting the Gosec Confidence level to Medium. HOT 1
- New release for the github action of gosec to include #1153 HOT 1
- possible regression on first g104 sample HOT 1
- Rule G401 covers multiple different CWEs.
- Create a taint analysis engine HOT 2
- Add rules for deprecated hashing algorithms
- Go version issues when running in CI (Github Actions) HOT 4
- can we get a patch release out to support go 1.22 fixes? HOT 2
- Gosec does not detect G204 if user input is from a function parameter
- CheckAnalyzers doesn't seem to respect exclude or nosec (e.g. G602) HOT 2
- GoSec pulling the image before docker hub login HOT 1
- G115 ignores bounds checks HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from gosec.