image/gif: decoding untrusted (very large) images can cause huge memory allocations
gopherbot opened this issue · 15 comments
gopherbot commented
by jeff.allen:
What steps will reproduce the problem? 1. decode attached gif, get bad behavior due to giant malloc followed by giant memset(0). 2. finally get error about UnexpectedEOF because there is not as much pixel data as the bounds say there should be. The problem is that the gif has a frame in it that would need 4.2e9 bytes to hold according to bounding box, but it only has 1 byte. The allocation of the 4.2e9 bytes succeeds, but at considerable pain. Then the UnexpectedEOF is thrown. What is the expected output? Getting the error without allocating a huge amount of memory first. What do you see instead? Long pause and unresponsive computer due to giant memory allocation. Which compiler are you using (5g, 6g, 8g, gccgo)? 6g Which operating system are you using? linux Which version are you using? (run 'go version') tip
Attachments:
- bug525326.gif (11606 bytes)
nigeltao commented
I think that there's a broader issue than just erroring out early if the image header
(width x height) is larger than the actual pixel data. Trying to decode a very large
*legitimate* image can also lead to a long pause and unresponsive computer, even if the
compressed form of that image is very small.
The current image.Decode and {gif,jpeg,png}.Decode functions attempt to return a newly
allocated image. I think that to avoid e.g. denial of service attacks via legitimate but
very large images, it needs to be possible to 1. decode just the WxH (and other
metadata), 2. decide whether to proceed based on that metadata and then 3. decode the
pixels into a buffer.
Note that you can more or less do this already, if you e.g. buffer the first 16K of a
reader, call image.DecodeConfig, and if you're happy with that, rewind the buffer and
call image.Decode.
However, it would be nice if you didn't have to manually rewind a buffer. If it wasn't
for API backwards compatibility constraints, one could imagine image.DecodeConfig
returning metadata as well as some capability of continuing the decoding process from
the io.Reader.
It would also be nice if you could continue to decode into an existing image buffer
instead of allocating a new one. If decoding many still frames of a movie instead of
just a single image, then the ability to decode into an existing buffer probably goes
from nice-to-have to must-have. Without actually trying to implement an MPEG or WEBM
decoder, though, trying to design an API for this is possibly premature.
It would also be nice if the Decode, DecodeConfig, or some similar but new API allowed
for e.g. accessing the EXIF metadata for a JPEG image.
It would also be nice if there was some way to decode and process an image row by row so
that the entire image did not have to be in memory all at once; think of this as
analagous to io.Reader compared to []byte.
All these points suggest to me that there is a larger problem to be solved, and that any
solution would require some deep thinking about API. Given that we are currently in an
API freeze for the upcoming Go 1.1 release, I don't think that this issue will be solved
any time soon. In the meantime, I think that the rewindable buffer technique described
above will let your programs avoid trying to allocate a 1e8 pixel by 1e8 pixel GIF.
Labels changed: added priority-later, removed priority-triage.
Status changed to Thinking.
gopherbot commented
Everything you wrote above is true, and a good idea. This problematic GIF cannot be avoided using your proposed technique. The data returned by DecodeConfig says it's a small GIF. Then once the user calls Decode on it, the second frame tricks us into calling image.NewPaletted() with giant bounds. However, now it occurs to me that capping the frame size of frames larger than the first frame would solve this problem nicely.
gopherbot commented
This issue was fixed by https://golang.org/cl/7602045 Somehow the "Fixes issue #5050." got lost. The current behavior for the reported test image is: package main import ( "os" "image/gif" "fmt" ) func main() { g, err := gif.DecodeAll(os.Stdin) if err != nil { fmt.Println(err) } fmt.Println(g) } $ go run decode.go < bug525326.gif gif: frame bounds larger than image bounds <nil>
gopherbot commented
I thought about this a bit, and it seems like implementing the "decode into an existing
image buffer" feature will need support in pkg image. For example for paletted images,
we'd have to do:
// NewPaletted returns a new Paletted with the given width, height and palette.
func NewPaletted(r Rectangle, p color.Palette) *Paletted {
w, h := r.Dx(), r.Dy()
pix := make([]uint8, 1*w*h)
return NewPalettedFrom(r, p, pix)
}
// NewPalettedFrom returns a new Paletted with the given width, height, and palette.
// It uses the given slice of uint8 as the storage for the pixels of the image.
func NewPalettedFrom(r Rectangle, p color.Palette, pix []uin8) *Paletted {
w, h := r.Dx(), r.Dy()
if len(pix) < 1*w*h {
// not clear what the right answer is here: return nil? panic?
}
return &Paletted{pix, 1 * w, r, p}
}
We'd need a New*From for each type of image, to be consistent.
Then image/gif/reader.go could offer an API to decode into an existing []uint8. Perhaps
DecodeInto(r io.Reader, pix []uint8). DecodeAll would get a twin with prototype
DecodeAllInto(r io.Reader, frames [][]uint) (*GIF, error).
There should also be a DecodeAllConfigs(r io.Reader) ([]image.Config, error) which
returns the sizes of all the frames that will be seen by a later call to DecodeAllInto.
I can work on this, if people (Nigel?) thinks it is the right way to go.
I don't think that the inconvenience of buffering, reading the config and rewinding
should be solved in the std library. I think callers that care about this level of
control should be willing to do that work themselves. What they need to be able to do
that is an entry point to the decoder that lets them limit the memory consumption before
it happens.nigeltao commented
I wouldn't bother with New*From functions. If they have an existing pix buffer, would-be callers can just use an image.Paletted composite literal. As for DecodeInto, I would imagine that its signature was DecodeInto(dst draw.Image, r io.Reader) (image.Rectangle, error) somewhat similar to how io.Reader takes a buffer and returns an int specifying how many bytes were read, but I haven't thought that much about it. As for working on this (and changing the standard library), I don't think it's obvious what the right design is. I'd recommend forking image/gif and experimenting first. As for "What they need to be able to do that is an entry point to the decoder that lets them limit the memory consumption before it happens", I don't see how DecodeInto helps with that, without requiring to allocate a maximal buffer even if you're just decoding a 32x32 image.
rsc commented
robpike commented
robpike commented
rsc commented
rsc commented
rsc commented
yonderblue commented
Is anyone working on the DecodeInto?
vsiv commented
We run into this from time to time:
For gif image - https://media.giphy.com/media/3oKIP94L7OaT9m3DPy/giphy.gif:
Error: gif: frame bounds larger than image bounds
But this issue might resolve it - #16146
gopherbot commented
Change https://go.dev/cl/417477 mentions this issue: image/png: build large images incrementally