From 8224626ed46d10e98d8fee29c9e9e1083a82b12c Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Thu, 21 May 2026 00:24:15 +0100 Subject: [PATCH] fix(image): reject decompression-bomb dimensions before raster decode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- backend/app/rest/api/rest_private.go | 5 +- backend/app/store/image/image.go | 83 ++++++++++++----- backend/app/store/image/image_test.go | 94 +++++++++++++++++--- backend/app/store/image/testdata/pixel.webp | Bin 0 -> 38 bytes 4 files changed, 146 insertions(+), 36 deletions(-) create mode 100644 backend/app/store/image/testdata/pixel.webp diff --git a/backend/app/rest/api/rest_private.go b/backend/app/rest/api/rest_private.go index 5ccc7642..2e264952 100644 --- a/backend/app/rest/api/rest_private.go +++ b/backend/app/rest/api/rest_private.go @@ -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 } diff --git a/backend/app/store/image/image.go b/backend/app/store/image/image.go index fa7017dd..542cedb4 100644 --- a/backend/app/store/image/image.go +++ b/backend/app/store/image/image.go @@ -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") } diff --git a/backend/app/store/image/image_test.go b/backend/app/store/image/image_test.go index 6b0171f6..fe26218e 100644 --- a/backend/app/store/image/image_test.go +++ b/backend/app/store/image/image_test.go @@ -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 diff --git a/backend/app/store/image/testdata/pixel.webp b/backend/app/store/image/testdata/pixel.webp new file mode 100644 index 0000000000000000000000000000000000000000..9652d2750366b504aa7473b3069624cd7816d6a9 GIT binary patch literal 38 rcmWIYbaRtqU|