diff --git a/internal/imagenorm/imagenorm.go b/internal/imagenorm/imagenorm.go index d8170f2..4461be4 100644 --- a/internal/imagenorm/imagenorm.go +++ b/internal/imagenorm/imagenorm.go @@ -22,11 +22,13 @@ // Go's image decoders register via blank imports, and the failure mode is // backwards from intuition: forget "image/png" and PNG uploads fail with // "unknown format" while the exotic HEIC still works. So the blank imports below -// are load-bearing, and TestNormalize exercises all four formats to keep them so. +// are load-bearing, and TestNormalizeAllFormats exercises all four formats to +// keep them so. package imagenorm import ( "bytes" + "errors" "fmt" "image" "image/jpeg" @@ -55,10 +57,19 @@ const ( // MiB leaves headroom for a large HEIC without inviting a decompression bomb // as an unbounded read. The re-encoded output is far smaller. DefaultMaxBytes = 25 << 20 - // maxDecodePixels bounds the decoded bitmap regardless of input byte size — a - // small file can claim enormous dimensions (a decompression bomb). 100 MP is - // ~400 MB at 4 bytes/px decoded, already generous for any real photo. - maxDecodePixels = 100_000_000 + // maxDecodePixels bounds the DECODED bitmap regardless of input byte size, so + // a small file claiming enormous dimensions (a decompression bomb) is refused + // before its ~4-bytes/px bitmap is allocated. 50 MP ≈ 200 MB peak — above any + // current phone camera (a 48 MP sensor is 48 MP) while capping the + // amplification a hostile header can force. maxDecodePixels and maxDimension + // are internal safety floors, not knobs — unlike MaxDim/MaxBytes there's no + // reason for a caller to raise them. + maxDecodePixels = 50_000_000 + // maxDimension caps EACH side independently. It exists to make the pixel-count + // check overflow-safe: without it, a header claiming ~2^32 on a side could + // wrap int64(w)*int64(h) negative and slip past maxDecodePixels. No real image + // is 50k px on a side. + maxDimension = 50_000 // jpegQuality for the normalized output. 85 is visually clean and keeps the // file small; the model reads text off it, not fine gradients. jpegQuality = 85 @@ -85,56 +96,98 @@ func (o Options) maxBytes() int { } // ErrTooLarge means the input exceeded the byte cap or decoded to an absurd -// pixel count. ErrUnsupported means the bytes weren't a decodable image format. +// pixel count. ErrUnsupported means the bytes weren't a decodable image format — +// or a decoder panicked on them (see the recover in Normalize). var ( - ErrTooLarge = fmt.Errorf("imagenorm: image too large") - ErrUnsupported = fmt.Errorf("imagenorm: unsupported or corrupt image") + ErrTooLarge = errors.New("imagenorm: image too large") + ErrUnsupported = errors.New("imagenorm: unsupported or corrupt image") ) // Normalize reads an image of any supported format (JPEG, PNG, HEIC, WebP), // downscales it to fit opts.MaxDim on its longest edge, and returns it re-encoded -// as JPEG. It also reports the decoded format name (e.g. "heic"), which is handy -// for logging what a phone actually sent. +// as JPEG, plus the decoded format name (e.g. "heic") — handy for logging what a +// phone actually sent. On any error the returned bytes are nil and format is "". // -// It reads at most opts.MaxBytes and refuses a bitmap over maxDecodePixels, so a -// hostile upload can neither exhaust memory by byte count nor by claimed -// dimensions. A read/format error maps to ErrUnsupported; an over-limit input to -// ErrTooLarge. +// Errors, by cause: +// - input over opts.MaxBytes, or a decoded canvas over maxDecodePixels / +// maxDimension → ErrTooLarge, refused before the bitmap is allocated; +// - bytes that aren't a decodable image, or a decoder that panics on them → +// ErrUnsupported; +// - a genuine read or JPEG-encode I/O failure → a wrapped error (not a +// sentinel), since those are the caller's stream/environment, not the image. +// +// It bounds work against a hostile upload three ways: the byte cap, the +// pre-decode pixel/dimension check, and a recover around the third-party decoders +// (a malformed HEIC/WebP shouldn't take the process down). +// +// Two known gaps, both deferred to the upload handler that wires this in (#81): +// - EXIF ORIENTATION is not applied, so a portrait phone photo tagged +// "rotate 90°" comes out sideways. That's best fixed and tested with a real +// oriented photo end-to-end, which the library has no consumer for yet. +// - There is no context: image.Decode is CPU-bound and not cancellable +// mid-decode, so a caller that needs a hard deadline should run Normalize +// under its own timeout. The size guards keep the work finite regardless. func Normalize(r io.Reader, opts Options) (out []byte, format string, err error) { // Cap the read at MaxBytes+1 so we can tell "exactly at the cap" from "over". - limited := io.LimitReader(r, int64(opts.maxBytes())+1) - raw, err := io.ReadAll(limited) + // maxBytes() is always a sane positive (default 25 MiB); guard the +1 anyway. + byteCap := opts.maxBytes() + limit := int64(byteCap) + 1 + if limit < 1 { + limit = int64(DefaultMaxBytes) + 1 + } + raw, err := io.ReadAll(io.LimitReader(r, limit)) if err != nil { return nil, "", fmt.Errorf("imagenorm: read: %w", err) } - if len(raw) > opts.maxBytes() { + if len(raw) > byteCap { return nil, "", ErrTooLarge } // Check dimensions BEFORE a full decode, so a decompression bomb is refused - // before it allocates its bitmap. - cfg, format, err := image.DecodeConfig(bytes.NewReader(raw)) + // before it allocates its bitmap. The per-side maxDimension check runs first + // so the pixel-count multiply below can't overflow. + cfg, _, err := image.DecodeConfig(bytes.NewReader(raw)) if err != nil { return nil, "", ErrUnsupported } - if int64(cfg.Width)*int64(cfg.Height) > maxDecodePixels { - return nil, format, ErrTooLarge + if cfg.Width <= 0 || cfg.Height <= 0 || + cfg.Width > maxDimension || cfg.Height > maxDimension || + int64(cfg.Width)*int64(cfg.Height) > maxDecodePixels { + return nil, "", ErrTooLarge } - img, _, err := image.Decode(bytes.NewReader(raw)) + img, format, err := decodeSafely(raw) if err != nil { - return nil, format, ErrUnsupported + return nil, "", err } img = downscale(img, opts.maxDim()) var buf bytes.Buffer if err := jpeg.Encode(&buf, img, &jpeg.Options{Quality: jpegQuality}); err != nil { - return nil, format, fmt.Errorf("imagenorm: encode jpeg: %w", err) + return nil, "", fmt.Errorf("imagenorm: encode jpeg: %w", err) } return buf.Bytes(), format, nil } +// decodeSafely decodes raw, converting both a decode error and a decoder PANIC +// into ErrUnsupported. The recover matters because the image comes from an +// untrusted upload and the HEIC/WebP decoders are third-party (libheif via WASM, +// x/image/webp): a malformed file that panics one of them must fail this one +// request, not crash the process. +func decodeSafely(raw []byte) (img image.Image, format string, err error) { + defer func() { + if r := recover(); r != nil { + img, format, err = nil, "", ErrUnsupported + } + }() + img, format, err = image.Decode(bytes.NewReader(raw)) + if err != nil { + return nil, "", ErrUnsupported + } + return img, format, nil +} + // downscale returns img shrunk so its longest edge is at most maxDim, preserving // aspect ratio. An image already within bounds is returned unchanged (no // re-sampling, no quality loss beyond the JPEG round-trip). Uses Catmull-Rom for @@ -142,10 +195,7 @@ func Normalize(r io.Reader, opts Options) (out []byte, format string, err error) func downscale(img image.Image, maxDim int) image.Image { b := img.Bounds() w, h := b.Dx(), b.Dy() - longest := w - if h > longest { - longest = h - } + longest := max(w, h) if longest <= maxDim || longest == 0 { return img } diff --git a/internal/imagenorm/imagenorm_test.go b/internal/imagenorm/imagenorm_test.go index 8d8a6dd..4a67309 100644 --- a/internal/imagenorm/imagenorm_test.go +++ b/internal/imagenorm/imagenorm_test.go @@ -2,6 +2,8 @@ package imagenorm import ( "bytes" + "encoding/binary" + "hash/crc32" "image" "image/jpeg" "image/png" @@ -85,8 +87,11 @@ func TestNormalizeAllFormats(t *testing.T) { // TestNormalizeDownscales checks a large image is shrunk to fit MaxDim on its // longest edge with aspect ratio preserved, and a small one is left alone. func TestNormalizeDownscales(t *testing.T) { - // 4000x1000 → longest 4000, MaxDim 2048 → scaled to 2048x512. - big := pngBytes(t, 4000, 1000) + // A 2:1 image twice as wide as DefaultMaxDim → clamped to DefaultMaxDim on the + // long edge with aspect preserved. Derived from the constant, not hard-coded, + // so the test tracks the default rather than silently asserting a magic number. + longEdge := DefaultMaxDim * 2 + big := pngBytes(t, longEdge, longEdge/2) out, _, err := Normalize(bytes.NewReader(big), Options{}) if err != nil { t.Fatalf("Normalize: %v", err) @@ -95,11 +100,11 @@ func TestNormalizeDownscales(t *testing.T) { if err != nil { t.Fatalf("decode out: %v", err) } - if cfg.Width != 2048 { - t.Errorf("width = %d, want 2048 (longest edge clamped)", cfg.Width) + if cfg.Width != DefaultMaxDim { + t.Errorf("width = %d, want %d (longest edge clamped)", cfg.Width, DefaultMaxDim) } - if cfg.Height != 512 { - t.Errorf("height = %d, want 512 (aspect preserved)", cfg.Height) + if cfg.Height != DefaultMaxDim/2 { + t.Errorf("height = %d, want %d (aspect preserved)", cfg.Height, DefaultMaxDim/2) } // A small image within bounds keeps its dimensions. @@ -108,7 +113,10 @@ func TestNormalizeDownscales(t *testing.T) { if err != nil { t.Fatalf("Normalize small: %v", err) } - cfg, _, _ = image.DecodeConfig(bytes.NewReader(out)) + cfg, _, err = image.DecodeConfig(bytes.NewReader(out)) + if err != nil { + t.Fatalf("decode small out: %v", err) + } if cfg.Width != 100 || cfg.Height != 80 { t.Errorf("small image resized to %dx%d, want 100x80", cfg.Width, cfg.Height) } @@ -124,9 +132,8 @@ func TestNormalizeRejectsOversizeInput(t *testing.T) { } } -// TestNormalizeRejectsBombDimensions: a small file claiming a huge canvas is -// refused before the bitmap is allocated. A tiny MaxDim doesn't matter — the -// guard is on the DECODED pixel count, checked from DecodeConfig. +// TestNormalizeRejectsGarbage: unreadable-as-image bytes and a truncated image +// both fail cleanly with ErrUnsupported, not a panic. func TestNormalizeRejectsGarbage(t *testing.T) { _, _, err := Normalize(bytes.NewReader([]byte("not an image at all")), Options{}) if err != ErrUnsupported { @@ -139,3 +146,57 @@ func TestNormalizeRejectsGarbage(t *testing.T) { t.Errorf("truncated image err = %v, want ErrUnsupported", err) } } + +// pngHeader builds a valid PNG signature + IHDR chunk (with a correct CRC, which +// DecodeConfig verifies) for the given dimensions, and nothing else. It's enough +// for image.DecodeConfig to report width/height without a real bitmap — exactly +// what's needed to exercise the pre-decode size guard with a tiny input. +func pngHeader(w, h uint32) []byte { + var buf bytes.Buffer + buf.Write([]byte{0x89, 'P', 'N', 'G', 0x0d, 0x0a, 0x1a, 0x0a}) + ihdr := make([]byte, 13) + binary.BigEndian.PutUint32(ihdr[0:], w) + binary.BigEndian.PutUint32(ihdr[4:], h) + ihdr[8] = 8 // bit depth + ihdr[9] = 6 // colour type: RGBA + // compression/filter/interlace already 0. + binary.Write(&buf, binary.BigEndian, uint32(len(ihdr))) + chunk := append([]byte("IHDR"), ihdr...) + buf.Write(chunk) + binary.Write(&buf, binary.BigEndian, crc32.ChecksumIEEE(chunk)) + return buf.Bytes() +} + +// TestNormalizeRejectsPixelBomb is the guard the review found untested: a small +// input (a bare ~40-byte PNG header) claiming an enormous canvas is refused with +// ErrTooLarge from DecodeConfig alone, before image.Decode allocates anything. +// Covers both the per-side maxDimension trip and the pixel-count trip — and, via +// the near-2^16-per-side case, that the count math doesn't overflow. +func TestNormalizeRejectsPixelBomb(t *testing.T) { + cases := []struct { + name string + w, h uint32 + }{ + {"huge single side", 60000, 10}, // > maxDimension on width + {"huge area within side cap", 40000, 40000}, // sides < cap, area 1.6 GP > maxDecodePixels + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + hdr := pngHeader(tc.w, tc.h) + if len(hdr) > 100 { + t.Fatalf("header unexpectedly large (%d bytes) — not a bomb test", len(hdr)) + } + // Sanity: the header really does decode to those dimensions. + cfg, _, err := image.DecodeConfig(bytes.NewReader(hdr)) + if err != nil { + t.Fatalf("crafted PNG header didn't parse: %v", err) + } + if uint32(cfg.Width) != tc.w || uint32(cfg.Height) != tc.h { + t.Fatalf("header reports %dx%d, want %dx%d", cfg.Width, cfg.Height, tc.w, tc.h) + } + if _, _, err := Normalize(bytes.NewReader(hdr), Options{}); err != ErrTooLarge { + t.Errorf("pixel bomb %dx%d err = %v, want ErrTooLarge", tc.w, tc.h, err) + } + }) + } +} diff --git a/internal/imagenorm/testdata/README.md b/internal/imagenorm/testdata/README.md new file mode 100644 index 0000000..dc036de --- /dev/null +++ b/internal/imagenorm/testdata/README.md @@ -0,0 +1,14 @@ +# imagenorm test fixtures + +Go has no encoder for HEIC or WebP, so these small samples are committed rather +than generated at test time. They are used by `TestNormalizeAllFormats` to prove +every accepted format round-trips to JPEG (and, mainly, to catch a dropped blank +import — see the package doc). + +- `sample.heic` — a 240×160 gradient, created from a Go-generated PNG with macOS + `sips -s format heic`. HEVC still-image profile. +- `sample.webp` — a 16×16 image copied from CPython's stdlib test corpus + (`Lib/test/test_email/data/python.webp`), verified to decode with + `golang.org/x/image/webp` before committing. Used only as a decode fixture. + +Neither contains anything meaningful; they exist purely to be decoded.