fix(image): reject decompression-bomb dimensions before raster decode
readAndValidateImage caps the byte size of incoming images but the resize() helper that follows still called image.Decode unconditionally, allocating pixel memory proportional to the *declared* image dimensions. A ~100 KB compressed PNG or GIF that declares 65535x65535 px forces image.Decode to allocate ~17 GB of raster, OOMing the service on a single comment upload (or on the proxy's CacheExternal path when caching a malicious upstream). Hardening: - maxImagePixels = 16 MP constant. Covers any realistic image (~4096x4096) while bounding peak allocation. - resize() now runs image.DecodeConfig first (cheap, no pixel allocation) to read declared width/height before any full decode. - Multiplication of width × height uses int64 to defeat 32-bit overflow (GOARCH=386, 32-bit arm): on those targets, int(cfg.Width)*int(cfg.Height) could wrap below maxImagePixels and bypass the cap. GIF's 16-bit logical screen and JPEG's 16-bit SOF dimensions both reach this if int-multiplied. - Bytes exceeding the cap, or non-image input that fails DecodeConfig, return nil. prepareImage propagates the rejection as a clear error instead of storing the malformed/oversized data verbatim. - The no-resize-needed path returns the validated original bytes verbatim so animated GIFs round-trip without being flattened to a single frame. The DecodeConfig precheck applies even when MaxWidth/MaxHeight are 0 (resize disabled) — the dimension cap is unconditional defense-in-depth. Two adjacent fixes surfaced by the new resize contract: 1. readAndValidateImage previously did `data[:512]` without a bounds check, panicking on any body shorter than 512 bytes. Now bounded with min(). 2. image/webp was listed as an allowed format but no WebP decoder was registered, so DecodeConfig would refuse legitimate WebP uploads. Added `_ "golang.org/x/image/webp"` (already in go.mod via x/image/draw) so the registered decoders match the allowlist. Tests: - TestService_resizeRejectsDecompressionBomb builds a 14-byte GIF87a header declaring 65535x65535 and asserts resize() refuses it both at the unit level and through SaveWithID end-to-end (no store write). - TestService_SaveWithIDShortPayload regression-tests the short-body panic. - TestService_SaveWithIDWebP regression-tests WebP round-trip through prepareImage with the new DecodeConfig requirement. - TestService_resize subtests updated to assert non-image bytes are now refused (previously the helper fell back to returning the raw bytes verbatim, letting malformed content reach the store).
This commit is contained in:
Dmitry Verkhoturov
committed by
Umputun
Umputun
parent
45c17a913f
commit
8224626ed4
@@ -732,7 +732,10 @@ func (s *private) savePictureCtrl(w http.ResponseWriter, r *http.Request) {
|
||||
user := rest.MustGetUserInfo(r)
|
||||
|
||||
r.Body = http.MaxBytesReader(w, r.Body, 32*1024*1024) // hard cap on upload to prevent memory exhaustion
|
||||
if err := r.ParseMultipartForm(5 * 1024 * 1024); err != nil { // 5M max memory, if bigger will make a file
|
||||
// gosec G120: r.Body is already bounded by MaxBytesReader on the line above (32 MB),
|
||||
// so ParseMultipartForm cannot read more than that regardless of the in-memory threshold.
|
||||
// The 5 MB argument is the soft threshold above which the form is spilled to disk.
|
||||
if err := r.ParseMultipartForm(5 * 1024 * 1024); err != nil { //nolint:gosec // bounded by MaxBytesReader above
|
||||
rest.SendErrorJSON(w, r, http.StatusInternalServerError, err, "can't parse multipart form", rest.ErrDecode)
|
||||
return
|
||||
}
|
||||
|
||||
@@ -13,10 +13,8 @@ import (
|
||||
"encoding/base64"
|
||||
"fmt"
|
||||
"image"
|
||||
|
||||
// support gif and jpeg images decoding
|
||||
_ "image/gif"
|
||||
_ "image/jpeg"
|
||||
_ "image/gif" // register gif decoder
|
||||
_ "image/jpeg" // register jpeg decoder
|
||||
"image/png"
|
||||
"io"
|
||||
"net/http"
|
||||
@@ -32,6 +30,7 @@ import (
|
||||
"github.com/hashicorp/go-multierror"
|
||||
"github.com/rs/xid"
|
||||
"golang.org/x/image/draw"
|
||||
_ "golang.org/x/image/webp" // register webp decoder so DecodeConfig accepts what readAndValidateImage allows
|
||||
)
|
||||
|
||||
// Service wraps Store with common functions needed for any store implementation
|
||||
@@ -285,6 +284,13 @@ func (s *Service) extractImageIDs(commentHTML string, includeProxied bool) (ids
|
||||
return ids
|
||||
}
|
||||
|
||||
// maxImagePixels caps the declared pixel count of an image before any raster decode
|
||||
// is allowed. Without this, a tiny compressed "decompression bomb" image declaring
|
||||
// e.g. 65535x65535 px would force image.Decode to allocate gigabytes of pixel memory
|
||||
// and OOM the service on a single comment upload. 16 MP covers any realistic image
|
||||
// (~4096x4096) while keeping peak allocation bounded.
|
||||
const maxImagePixels = 16 * 1024 * 1024
|
||||
|
||||
// prepareImage calls readAndValidateImage and resize on provided image.
|
||||
func (s *Service) prepareImage(r io.Reader) ([]byte, error) {
|
||||
data, err := readAndValidateImage(r, s.MaxSize)
|
||||
@@ -292,32 +298,59 @@ func (s *Service) prepareImage(r io.Reader) ([]byte, error) {
|
||||
return nil, fmt.Errorf("can't load image: %w", err)
|
||||
}
|
||||
|
||||
data = resize(data, s.MaxWidth, s.MaxHeight)
|
||||
return data, nil
|
||||
resized := resize(data, s.MaxWidth, s.MaxHeight)
|
||||
if resized == nil {
|
||||
return nil, fmt.Errorf("image rejected: malformed or exceeds %d-pixel safe limit", maxImagePixels)
|
||||
}
|
||||
return resized, nil
|
||||
}
|
||||
|
||||
// resize an image of supported format (PNG, JPG, GIF) to the size of "limit" px of
|
||||
// the biggest side (width or height) preserving aspect ratio.
|
||||
// Returns original data if resizing is not needed or failed.
|
||||
// If resized the result will be for png format
|
||||
// resize validates an image and, if needed, re-encodes it to fit within the given
|
||||
// pixel limits preserving aspect ratio. Returns nil for malformed input or for
|
||||
// declared dimensions exceeding maxImagePixels so attacker payloads (decompression
|
||||
// bombs) never reach the store. With limit <= 0 or when the image already fits, the
|
||||
// original bytes are returned verbatim so animated GIFs and other multi-frame formats
|
||||
// round-trip without being flattened to one frame.
|
||||
//
|
||||
// Validation uses image.DecodeConfig (cheap — declares dimensions, allocates nothing)
|
||||
// before any full image.Decode, so a 100 KB compressed image declaring 65535x65535 px
|
||||
// is rejected without ever materializing the raster.
|
||||
func resize(data []byte, limitW, limitH int) []byte {
|
||||
if data == nil || limitW <= 0 || limitH <= 0 {
|
||||
return data
|
||||
if len(data) == 0 {
|
||||
return nil
|
||||
}
|
||||
|
||||
src, _, err := image.Decode(bytes.NewBuffer(data))
|
||||
// validate format and dimensions without allocating pixel memory.
|
||||
cfg, _, err := image.DecodeConfig(bytes.NewReader(data))
|
||||
if err != nil {
|
||||
log.Printf("[WARN] can't decode image, %s", err)
|
||||
return data
|
||||
log.Printf("[WARN] can't decode image config, %s", err)
|
||||
return nil
|
||||
}
|
||||
// multiply in int64 — on 32-bit builds (GOARCH=386, 32-bit arm) the int
|
||||
// product of two 16-bit-or-larger dimensions can overflow and wrap below
|
||||
// maxImagePixels, bypassing the cap.
|
||||
if cfg.Width <= 0 || cfg.Height <= 0 || int64(cfg.Width)*int64(cfg.Height) > int64(maxImagePixels) {
|
||||
log.Printf("[WARN] image dimensions %dx%d exceed safe limit", cfg.Width, cfg.Height)
|
||||
return nil
|
||||
}
|
||||
|
||||
// dimensions are bounded — full decode is now safe to allocate. Decode also
|
||||
// validates the raster body: a header that DecodeConfig accepts but with a
|
||||
// corrupt or truncated payload would slip through if we returned early on the
|
||||
// no-resize path without ever touching the pixels. Decode unconditionally,
|
||||
// then either return the original bytes (no resize needed, multi-frame intact)
|
||||
// or the re-encoded result.
|
||||
src, _, err := image.Decode(bytes.NewReader(data))
|
||||
if err != nil {
|
||||
log.Printf("[WARN] can't decode image after dim-check, %s", err)
|
||||
return nil
|
||||
}
|
||||
|
||||
bounds := src.Bounds()
|
||||
w, h := bounds.Dx(), bounds.Dy()
|
||||
if w <= limitW && h <= limitH || w <= 0 || h <= 0 {
|
||||
log.Printf("[DEBUG] resizing image is smaller that the limit or has 0 size")
|
||||
if limitW <= 0 || limitH <= 0 || (cfg.Width <= limitW && cfg.Height <= limitH) {
|
||||
return data
|
||||
}
|
||||
|
||||
w, h := src.Bounds().Dx(), src.Bounds().Dy()
|
||||
newW, newH := getProportionalSizes(w, h, limitW, limitH)
|
||||
m := image.NewRGBA(image.Rect(0, 0, newW, newH))
|
||||
draw.CatmullRom.Scale(m, m.Bounds(), src, src.Bounds(), draw.Src, nil)
|
||||
@@ -325,7 +358,7 @@ func resize(data []byte, limitW, limitH int) []byte {
|
||||
var out bytes.Buffer
|
||||
if err = png.Encode(&out, m); err != nil {
|
||||
log.Printf("[WARN] can't encode resized image to png, %s", err)
|
||||
return data
|
||||
return data // fall back to the validated original
|
||||
}
|
||||
return out.Bytes()
|
||||
}
|
||||
@@ -366,8 +399,14 @@ func readAndValidateImage(r io.Reader, maxSize int) ([]byte, error) {
|
||||
return nil, fmt.Errorf("file is too large (limit=%d)", maxSize)
|
||||
}
|
||||
|
||||
// read header first, needs it to check if data is valid png/gif/jpeg
|
||||
if !isValidImage(data[:512]) {
|
||||
// read header first to check the format. http.DetectContentType inspects up
|
||||
// to the first 512 bytes, but a smaller body is fine — pass the whole slice
|
||||
// rather than panicking on a fixed-size sub-slice.
|
||||
header := data
|
||||
if len(header) > 512 {
|
||||
header = header[:512]
|
||||
}
|
||||
if !isValidImage(header) {
|
||||
return nil, fmt.Errorf("file format not allowed")
|
||||
}
|
||||
|
||||
|
||||
@@ -210,19 +210,17 @@ func TestService_Info(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestService_resize(t *testing.T) {
|
||||
// reader is nil
|
||||
resized := resize(nil, 100, 100)
|
||||
assert.Nil(t, resized)
|
||||
t.Run("empty data returns nil", func(t *testing.T) {
|
||||
assert.Nil(t, resize(nil, 100, 100))
|
||||
assert.Nil(t, resize([]byte{}, 100, 100))
|
||||
})
|
||||
|
||||
// negative limit error
|
||||
resized = resize([]byte("some picture bin data"), -1, -1)
|
||||
require.NotNil(t, resized)
|
||||
assert.Equal(t, resized, []byte("some picture bin data"))
|
||||
|
||||
// decode error
|
||||
resized = resize([]byte("invalid image content"), 100, 100)
|
||||
assert.NotNil(t, resized)
|
||||
assert.Equal(t, resized, []byte("invalid image content"))
|
||||
t.Run("non-image bytes are refused", func(t *testing.T) {
|
||||
// previously resize would fall back to the raw bytes on decode failure, letting
|
||||
// attacker-controlled non-image content reach the store. After hardening, refuse.
|
||||
assert.Nil(t, resize([]byte("some picture bin data"), -1, -1))
|
||||
assert.Nil(t, resize([]byte("invalid image content"), 100, 100))
|
||||
})
|
||||
|
||||
cases := []struct {
|
||||
file string
|
||||
@@ -237,7 +235,7 @@ func TestService_resize(t *testing.T) {
|
||||
require.NoError(t, err, "can't open test file %s", c.file)
|
||||
|
||||
// no need for resize, image dimensions are smaller than resize limit
|
||||
resized = resize(img, 800, 800)
|
||||
resized := resize(img, 800, 800)
|
||||
assert.NotNil(t, resized, "file %s", c.file)
|
||||
assert.Equal(t, resized, img)
|
||||
|
||||
@@ -253,6 +251,76 @@ func TestService_resize(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestService_SaveWithIDShortPayload guards readAndValidateImage from panicking
|
||||
// on a body shorter than 512 bytes — historically it sliced data[:512] without
|
||||
// a bounds check, which would panic before any decode-bomb defense could fire.
|
||||
func TestService_SaveWithIDShortPayload(t *testing.T) {
|
||||
short := []byte("not an image")
|
||||
|
||||
svc := Service{ServiceParams: ServiceParams{ImageAPI: "/blah/", MaxSize: 1500, MaxWidth: 32, MaxHeight: 32}}
|
||||
err := svc.SaveWithID("test_id", bytes.NewReader(short))
|
||||
require.Error(t, err, "short non-image body must return an error, not panic")
|
||||
assert.Contains(t, err.Error(), "file format not allowed")
|
||||
}
|
||||
|
||||
// TestService_SaveWithIDWebP confirms that WebP — listed as an allowed format
|
||||
// in readAndValidateImage — still round-trips through prepareImage now that
|
||||
// resize() runs image.DecodeConfig. Without registering the WebP decoder, a
|
||||
// legitimate WebP upload would fail DecodeConfig and prepareImage would error.
|
||||
func TestService_SaveWithIDWebP(t *testing.T) {
|
||||
webp, err := os.ReadFile("testdata/pixel.webp")
|
||||
require.NoError(t, err)
|
||||
|
||||
// sanity: the fixture must be a well-formed 1x1 WebP that DecodeConfig accepts.
|
||||
cfg, format, err := image.DecodeConfig(bytes.NewReader(webp))
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, "webp", format)
|
||||
require.Equal(t, 1, cfg.Width)
|
||||
require.Equal(t, 1, cfg.Height)
|
||||
|
||||
store := StoreMock{SaveFunc: func(string, []byte) error { return nil }}
|
||||
svc := Service{store: &store, ServiceParams: ServiceParams{MaxSize: 1500}}
|
||||
|
||||
err = svc.SaveWithID("webp_id", bytes.NewReader(webp))
|
||||
require.NoError(t, err, "valid WebP must round-trip through SaveWithID")
|
||||
assert.Equal(t, 1, len(store.SaveCalls()))
|
||||
assert.Equal(t, webp, store.SaveCalls()[0].Img, "no-resize path must return bytes verbatim")
|
||||
}
|
||||
|
||||
// TestService_resizeRejectsDecompressionBomb verifies the dimension-cap defense.
|
||||
// Builds a tiny GIF that declares 65535x65535 (4 gigapixels) in its logical-screen
|
||||
// header — image.DecodeConfig reads the dimensions, the int64 product overflows
|
||||
// any 32-bit int wrap, and resize must refuse before image.Decode allocates ~17 GB
|
||||
// of pixel memory.
|
||||
func TestService_resizeRejectsDecompressionBomb(t *testing.T) {
|
||||
// minimal GIF87a header with 65535x65535 logical screen, no global color table.
|
||||
// Bytes 6-7 are the little-endian width, 8-9 are the little-endian height.
|
||||
bomb := []byte{
|
||||
'G', 'I', 'F', '8', '7', 'a',
|
||||
0xFF, 0xFF,
|
||||
0xFF, 0xFF,
|
||||
0x00,
|
||||
0x00,
|
||||
0x00,
|
||||
0x3B,
|
||||
}
|
||||
cfg, _, err := image.DecodeConfig(bytes.NewReader(bomb))
|
||||
require.NoError(t, err, "bomb header must decode at the config level")
|
||||
assert.Equal(t, 65535, cfg.Width)
|
||||
assert.Equal(t, 65535, cfg.Height)
|
||||
|
||||
assert.Nil(t, resize(bomb, 100, 100), "resize must refuse oversized dimensions before raster decode")
|
||||
assert.Nil(t, resize(bomb, 0, 0), "even with no-resize limits, oversized dims must be refused")
|
||||
|
||||
// integration-level: SaveWithID must reject the same bomb without panicking
|
||||
// or allocating gigabytes of raster memory.
|
||||
store := StoreMock{SaveFunc: func(string, []byte) error { return nil }}
|
||||
svc := Service{store: &store, ServiceParams: ServiceParams{MaxSize: 1500}}
|
||||
err = svc.SaveWithID("bomb_id", bytes.NewReader(bomb))
|
||||
require.Error(t, err, "SaveWithID must reject decompression bomb")
|
||||
assert.Equal(t, 0, len(store.SaveCalls()), "rejected bomb must not be stored")
|
||||
}
|
||||
|
||||
func TestGetProportionalSizes(t *testing.T) {
|
||||
tbl := []struct {
|
||||
inpW, inpH int
|
||||
|
||||
BIN
Binary file not shown.
|
After Width: | Height: | Size: 38 B |
Reference in New Issue
Block a user