chore: Enable G110 rule for gosec #13044
Conversation
powersj
added
the
ready for final review
There was a problem hiding this comment.
I don't think those changes are actual fixes. In all cases you copy an unlimited amount of data. You are just silencing the (valid) warnings by replacing the triggering io.Copy() with an infinite loop of copying chunks not triggering the dumb linter by using io.CopyN().
I think the only fix in all those spots is to either determine the _actual _ upper limit of the source's content size or by applying a user-set upper limit, either through code or via config options.
internal/content_coding.go
Outdated
| _, err = io.Copy(d.buf, r) | ||
| if err != nil && !errors.Is(err, io.EOF) { | ||
| return nil, err | ||
|
|
||
| for { | ||
| _, err = io.CopyN(d.buf, r, 1024*1024) | ||
| if err != nil { | ||
| if errors.Is(err, io.EOF) { | ||
| break | ||
| } | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
How is this different to the code before? You can still exhaust memory by a so called ZIP bomb. The only valid defense is to provide a upper decompression limit, i.e. specify n value passed to io.CopyN.
There was a problem hiding this comment.
For tests it is easy, we can hardcode n value.
But ContentDecoders from internal/content_coding.go are used in:
- AMQP Consumer Input Plugin
- Socket Listener Input Plugin
For these two new configuration field should be added (with reasonable default value) to hold upper size limit.
@srebhan Will that be ok?
There was a problem hiding this comment.
Yes I think that's OK. We should set this to unlimited somehow... @powersj any opinion?
There was a problem hiding this comment.
Sounds go to add, but how do we keep the unlimited? Maybe a setting of 0 == unlimited and if so we use io.Copy?
There was a problem hiding this comment.
Please, review current implementation.
Understood, but what risk do we run by putting in a new default and breaking users?
There was a problem hiding this comment.
@powersj For both plugins default was set to 500MB.
It will not change anything for users operating on payload of smaller size.
Users with bigger payload will need to set larger value in config. We may call it "breaking users" or "fixing security gaps" ;)
There was a problem hiding this comment.
ok we chatted and agree with the new default, we will want to call this out in change log for the next release
There was a problem hiding this comment.
@zak-pawel I like the approach. I do have some more suggestions though... Sorry!
@srebhan All addressed. |
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
There was a problem hiding this comment.
Awesome work @zak-pawel! Thank you for addressing this issue!
Co-authored-by: Pawel Zak <Pawel Zak> (cherry picked from commit ba16eeb)
resolves #12897