From 3409cf840d1138cac8aa6d1f899eaa9a64c2a32a Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 4 Oct 2026 19:47:11 +0200 Subject: [PATCH] security: refuse raster images declaring more than 25 megapixels A valid 62KB PNG declaring 4000x4000 allocated 128MB on decode, and page images decode in parallel. decodeRaster now checks the declared size with image.DecodeConfig before decoding. README documents that the library does not guard SSRF; embedders need a dial guard (browserproxy has one). Co-Authored-By: Claude Sonnet 5 --- FIDELITY.md | 17 +++++++++++++ README.md | 11 +++++++++ images.go | 27 +++++++++++++++++++-- images_bomb_test.go | 58 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 2 deletions(-) create mode 100644 images_bomb_test.go diff --git a/FIDELITY.md b/FIDELITY.md index c8ccd2a..23062d0 100644 --- a/FIDELITY.md +++ b/FIDELITY.md @@ -47,6 +47,23 @@ A survey of how other renderers accelerate loading recommended honouring Retry-A - **Test**: `TestFetchModuleSourceHonoursRetryAfter` serves 429 with `Retry-After: 1` for the first second. With the fix it succeeds after about 1.0s. Stash-verified: the old code drops the module at 0.46s. - **Corpus effect**: none measured. The saved Tailwind, Wikipedia and Hacker News renders are pixel-identical before and after, because no module on those pages was rate-limited. The change corrects a case the corpus does not exercise. +## 2026-10-04 (round 161) — security audit: a 62KB image file could allocate 128MB; raster decode now refuses declared canvases above 25 megapixels; SSRF and toolchain findings recorded (engine#261, patch) + +Scope: outbound fetches the engine makes for a page (images, sheets, fonts, modules, fetch/XHR, form posts), the decode path for untrusted bytes, the link hit-map, and the Go toolchain (govulncheck). + +**Found and fixed** +- **Decompression bomb (image decode)**. `codec.Decode` has no size guard, and the PNG decoder allocates the canvas from a valid compressed stream. Measured: a 62KB PNG of zeros declaring 4000×4000 allocates 128MB in 40ms. Fixed: `decodeRaster` reads the declared dimensions with `image.DecodeConfig` and refuses anything above 25 megapixels (about 100MB of RGBA, still admitting a 6000×4000 photograph). Both raster decode sites use it. Test: `TestDecodeRasterRefusesOversizedDeclaredImage`, which fails with the guard disabled. +- **A correction to my own first measurement**: a header alone (IHDR with no pixel data) allocates nothing on the old path. It fails at the missing data. The allocation risk needs a valid compressed stream, as above. The test is named for what it proves (the declared size is refused), not for an allocation it does not reproduce. + +**Checked and found adequate** +- Response bodies are capped on every fetch path (`LimitReader`: 32MB pages, 4MB sheets, 8MB fonts, 16MB images, 48MB module source, 32MB JS responses). +- Scheme restrictions: images, fonts and sheets accept only http(s) and data:. Modules require an http prefix. The link hit-map rejects `javascript:`, `mailto:`, `tel:` and `data:` hrefs. +- JS `fetch`/XHR go through the same client as the engine's own fetches. + +**Recorded, not changed (decisions for the owner)** +- **SSRF is not guarded by the library.** A page can name any URL in an image, sheet, font, module, or script `fetch`, and the engine will fetch it, including private and link-local addresses. `browserproxy` guards this at dial time (`guard.go`, a `net.Dialer.Control` hook). A program that embeds the engine directly must add an equivalent dial guard through `Engine.Client`. The README now says so. +- **Go toolchain**: govulncheck reports six standard-library findings fixed in go1.26.5 and go1.26.6 (`net/http`, `crypto/tls`, `net/url`, `encoding/xml`, `encoding/asn1`). The module pins go1.26.4 in `go.mod` and in CI. Moving the pin is a CI and supply-chain change, so it is left to the owner. + ## 2026-10-04 (round 158) — the settle loop is a small lever: about 0.26s of CPU on tailwindcss.com, all of it full re-cascades, bounded by the network wait (docs only, no behaviour change) Third candidate after the cascade and the image path: the settle-then-render loop. Profiling one Tailwind render shows `settle` at about 12% of CPU samples, and of that the cascade (`CascadeMediaContainers`) is almost all of it, about 0.26s. JavaScript execution is about 0.07s. A render performs five full cascades. Each is a whole-document pass. diff --git a/README.md b/README.md index bf241b7..0d4c796 100644 --- a/README.md +++ b/README.md @@ -196,6 +196,17 @@ while go.dev, tailwindcss.com and the large pkg.go.dev page are slower. Re-run t harness with `cd bench && go run ./cmd/compare -urls urls.txt` (needs a Chrome/Chromium binary). +## Security + +The engine fetches every resource a page names (images, stylesheets, fonts, +scripts and modules, `fetch`/XHR targets, form posts). It does **not** filter +destinations itself, so rendering an untrusted page from a machine with access to +private networks can reach internal addresses. If you embed the engine directly, +add a dial-time guard through `Engine.Client` that refuses loopback, private and +link-local addresses. `browserproxy` ships one (`guard.go`). Decoding is +bounded: raster images declaring more than 25 megapixels are refused, and every +fetch is size-capped. + ## Test ``` diff --git a/images.go b/images.go index ed0c62a..ebe29a8 100644 --- a/images.go +++ b/images.go @@ -4,8 +4,10 @@ package engine import ( + "bytes" "context" "encoding/base64" + "errors" "image" "io" "net/http" @@ -48,6 +50,27 @@ func resizeRaster(src *raster.Image, w, h int, mode resample.Mode) *raster.Image return out } +// maxImagePixels bounds how many pixels one raster image may decode to. A +// file's header can declare a far larger canvas than its compressed body, and +// the decoder allocates that canvas before reading a pixel, so an unchecked +// declared size lets a small file exhaust memory: a 62KB PNG of zeros declaring +// 4000x4000 allocates 128MB in 40ms. Twenty-five megapixels (about 100MB of +// RGBA) still admits a 6000x4000 photograph. +const maxImagePixels = 25_000_000 + +var errImageTooLarge = errors.New("image declares more pixels than the engine decodes") + +// decodeRaster decodes a raster image after checking its declared dimensions. +// Formats the standard image package cannot read a header for fall through to +// the codec unchecked, as before. +func decodeRaster(data []byte) (*raster.Image, error) { + if cfg, _, err := image.DecodeConfig(bytes.NewReader(data)); err == nil && + int64(cfg.Width)*int64(cfg.Height) > maxImagePixels { + return nil, errImageTooLarge + } + return codec.Decode(data) +} + // imgWorkers is the concurrency bound for image fetch+decode: enough to hide // per-image network latency (the dominant cost) without unbounded fan-out. func imgWorkers(n int) int { @@ -295,7 +318,7 @@ func (e *Engine) loadOneImage(ctx context.Context, doc *Document, sm css.StyleMa } return &LoadedImage{Size: [2]float64{float64(w), float64(h)}, Bitmap: b, Data: data, Format: "svg", SourceW: w, SourceH: h} } - src0, err := codec.Decode(data) + src0, err := decodeRaster(data) if err != nil { return nil } @@ -457,7 +480,7 @@ func (e *Engine) loadOneBackground(ctx context.Context, doc *Document, raw strin } return b } - img, err := codec.Decode(data) + img, err := decodeRaster(data) if err != nil { return nil } diff --git a/images_bomb_test.go b/images_bomb_test.go new file mode 100644 index 0000000..6dfa4f9 --- /dev/null +++ b/images_bomb_test.go @@ -0,0 +1,58 @@ +// Copyright (c) the go-webengine/engine authors. +// SPDX-License-Identifier: BSD-3-Clause + +package engine + +import ( + "bytes" + "encoding/binary" + "hash/crc32" + "image" + "image/color" + "image/png" + "testing" +) + +// pngHeaderOnly is a PNG whose IHDR declares w×h but whose body is just the +// signature and that header: a few dozen bytes that claim a huge canvas. +func pngHeaderOnly(w, h uint32) []byte { + var b bytes.Buffer + b.Write([]byte("\x89PNG\r\n\x1a\n")) + ihdr := make([]byte, 13) + binary.BigEndian.PutUint32(ihdr[0:4], w) + binary.BigEndian.PutUint32(ihdr[4:8], h) + ihdr[8], ihdr[9] = 8, 6 // 8-bit RGBA + chunk := append([]byte("IHDR"), ihdr...) + _ = binary.Write(&b, binary.BigEndian, uint32(len(ihdr))) + b.Write(chunk) + _ = binary.Write(&b, binary.BigEndian, crc32.ChecksumIEEE(chunk)) + return b.Bytes() +} + +// A header declaring far more pixels than maxImagePixels is refused before any +// decoding. (A header alone does not allocate on the old path, which fails at +// the missing pixel data; the allocation risk needs a valid compressed stream, +// measured in FIDELITY.md round 161.) +func TestDecodeRasterRefusesOversizedDeclaredImage(t *testing.T) { + _, err := decodeRaster(pngHeaderOnly(50000, 50000)) + if err != errImageTooLarge { + t.Fatalf("decodeRaster(50000x50000 header) err = %v, want errImageTooLarge", err) + } +} + +// Ordinary images still decode. +func TestDecodeRasterDecodesOrdinaryImages(t *testing.T) { + img := image.NewNRGBA(image.Rect(0, 0, 8, 4)) + img.Set(1, 1, color.NRGBA{R: 255, A: 255}) + var buf bytes.Buffer + if err := png.Encode(&buf, img); err != nil { + t.Fatal(err) + } + got, err := decodeRaster(buf.Bytes()) + if err != nil { + t.Fatalf("decodeRaster(8x4 png) error: %v", err) + } + if b := got.Bounds(); b.Dx() != 8 || b.Dy() != 4 { + t.Errorf("decoded bounds = %v, want 8x4", b) + } +}