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

image/gif: decoding untrusted (very large) images can cause huge memory allocations #5050

Open
gopherbot opened this issue Mar 14, 2013 · 14 comments

Comments

@gopherbot
Copy link

commented Mar 14, 2013

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:

  1. bug525326.gif (11606 bytes)
@nigeltao

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2013

Comment 1:

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

This comment has been minimized.

Copy link
Author

commented Mar 15, 2013

Comment 2 by jeff.allen:

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

This comment has been minimized.

Copy link
Author

commented Jun 25, 2013

Comment 3 by jeff.allen:

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

This comment has been minimized.

Copy link
Author

commented Jul 10, 2013

Comment 4 by jeff.allen:

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

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2013

Comment 5:

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

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2013

Comment 7:

Labels changed: added go1.2maybe.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2013

Comment 8:

Not clear what the right answer is. Deferring to 1.3.

Labels changed: added go1.3maybe, removed go1.2maybe.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2013

Comment 9:

Labels changed: removed go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2013

Comment 10:

Labels changed: added go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 11:

Labels changed: added release-none, removed go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 12:

Labels changed: added repo-main.

@gaillard

This comment has been minimized.

Copy link

commented Nov 29, 2016

Is anyone working on the DecodeInto?

@vsiv

This comment has been minimized.

Copy link

commented Mar 31, 2017

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 2, 2017

@vsiv, yeah, I think you're commenting on the wrong issue here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.