From 557f7d6db6c7d1b88cf4699a518d59b1090f001a Mon Sep 17 00:00:00 2001 From: tannevaled Date: Wed, 4 Nov 2026 16:51:16 +0100 Subject: [PATCH] feat: fuzz the parsers, and fix the two divergences it found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pdfkit had NO fuzz targets, and it has a parser: jpegInfo walks caller-supplied bytes by hand -- markers, segment lengths, offsets, every one of them a number out of the file. FuzzJPEGInfo drives it against image/jpeg as a differential oracle; FuzzDrawImageSamples drives the premultiply arithmetic in samples(). ⛔ THE ORACLE IS NOT THE SPECIFICATION, it is what every consumer of our output will have used to read the same bytes. So the property is NOT "jpegInfo accepts what image/jpeg accepts" -- it accepts more, deliberately, because DrawJPEG embeds without decoding -- but that the two never AGREE TO DISAGREE: a file both read must have the same dimensions in both, or we write a /Width the reader's own decoder contradicts and the picture comes out sheared with nothing to blame. Two divergences, both within the first second, both files jpegInfo refused and every other decoder accepts: 1. FILL BYTES. "Any marker may optionally be preceded by any number of fill bytes, which are bytes assigned code X'FF'" (ISO/IEC 10918-1 / CCITT T.81 §B.1.1.2). The walk took the second X'FF' for the marker byte and then read the real marker as a segment length: "corrupt JPEG (bad segment length)" on a file image/jpeg reads as 4x4. Found on the FIRST SEED. 2. EXTRANEOUS DATA between segments. libjpeg's next_marker raises JWRN_EXTRANEOUS_DATA and skips to the next X'FF'; image/jpeg copies that and says why (reader.go:542-556). We said "corrupt JPEG (expected marker)" on a file image/jpeg reads as 12336x12336. Also handled, from the same reading: X'FF00' is byte stuffing and not a marker (§B.1.1.2 -- a marker's second byte is "not equal to 0 or X'FF'"), and TEM (X'FF01') stands alone and carries no segment (Table B.1), neither of which the walk knew. Two bounds that were implicit in the old loop condition are now explicit errors: a marker with nothing behind it to hold its length, and a frame header too short to hold a frame. The SOF selection is unchanged and was already right: C0-CF less C4 (DHT), C8 (JPG) and CC (DAC), exactly Table B.1. CI gains the two gates this repository never had: a minute of fuzzing per target (which is not a formality -- it is four times what it took to find both of the above), and govulncheck in a lane of its own. Corpus committed under testdata/fuzz. Coverage stays at exactly 100%. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 30 ++++ README.md | 23 ++- fuzz_test.go | 180 ++++++++++++++++++++ image.go | 38 ++++- testdata/fuzz/FuzzJPEGInfo/1e609ca636cd5bfc | 2 + 5 files changed, 269 insertions(+), 4 deletions(-) create mode 100644 fuzz_test.go create mode 100644 testdata/fuzz/FuzzJPEGInfo/1e609ca636cd5bfc diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d2b616c..c9381cb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -63,6 +63,17 @@ jobs: fi echo "all statements covered (100.0%)" + # The seeds run above as ordinary subtests; this is the half that + # explores. A minute each found both of jpegInfo's divergences from + # image/jpeg within the first second, so a minute is not a formality. + # A new crasher is written under testdata/fuzz and fails this step. + - name: Fuzz the parsers + run: | + for target in FuzzJPEGInfo FuzzDrawImageSamples; do + echo "== $target ==" + go test -run XXX -fuzz "$target" -fuzztime 60s . + done + - name: Cross-compile (6 64-bit arches + wasm + darwin + windows) run: | for arch in amd64 arm64 riscv64 loong64 ppc64le s390x; do @@ -72,3 +83,22 @@ jobs: GOOS=js GOARCH=wasm go build ./... GOOS=darwin GOARCH=arm64 go build ./... GOOS=windows GOARCH=amd64 go build ./... + + vuln: + name: govulncheck + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + + - uses: actions/setup-go@v7 + with: + go-version: "1.27.3" + check-latest: true + cache: true + + # This lane never had one. pdfkit parses fonts and image headers out of + # files, so a known vulnerability in anything it links is a defect in it. + - name: govulncheck + run: | + go install golang.org/x/vuln/cmd/govulncheck@latest + govulncheck ./... diff --git a/README.md b/README.md index d4a39fd..405f49c 100644 --- a/README.md +++ b/README.md @@ -39,7 +39,11 @@ Go-idiomatic rather than a gem port. - **Images** — JPEG embedded directly (DCTDecode); PNG and any `image.Image` rasterised as XObjects (FlateDecode) with an `/SMask` for alpha, the colour samples written straight (not premultiplied), as a soft mask without - `/Matte` requires. + `/Matte` requires. The JPEG marker walk reads frame geometry the way libjpeg + and `image/jpeg` do — skipping fill bytes, `X'FF00'` stuffing and extraneous + data between segments — because `DrawJPEG` embeds the bytes untouched, and a + writer that refuses what every reader of its own output accepts is the one + in the wrong. **Pixel-identical bitmaps are shared**: each image is content-addressed by its *uncompressed* samples, so a repeat is embedded — and compressed — once per document and every placement, on any page, references the one XObject. @@ -178,6 +182,23 @@ smaller than the whole `CFF` table and re-parsed so each kept glyph still render intact. Tests are deterministic and network-free: they use a synthesised TrueType font, a synthesised CFF2 font and a bundled OFL OpenType/CFF font. +**Fuzzing.** `jpegInfo` is the one parser here that reads caller-supplied bytes +by hand — markers, segment lengths and offsets, every one a number out of the +file — so `FuzzJPEGInfo` drives it against `image/jpeg` as a differential +oracle, and `FuzzDrawImageSamples` drives the sample splitter's premultiply +arithmetic. The seed corpus is committed under `testdata/fuzz/`; CI runs the +seeds on every pull request and fuzzes each target for a minute. + +⛔ The oracle is not "the specification" but *what every consumer of our output +will have used to read the same bytes*. The property is therefore **not** that +`jpegInfo` accepts what `image/jpeg` accepts — it accepts more, deliberately, +since `DrawJPEG` embeds without decoding — but that the two never **agree to +disagree**: a file both read must have the same dimensions in both, or the PDF +carries a `/Width` the reader's own decoder contradicts. That found two real +divergences in the first second of fuzzing, both of them files +`jpegInfo` refused and every other decoder accepts. See +*Scope and limitations*. + ## Scope and limitations - Both outline flavours are **subsetted**: TrueType `glyf` fonts via diff --git a/fuzz_test.go b/fuzz_test.go new file mode 100644 index 0000000..c3b6a2c --- /dev/null +++ b/fuzz_test.go @@ -0,0 +1,180 @@ +// Copyright (c) 2026 the go-pdfkit/pdfkit authors. All rights reserved. +// Use of this source code is governed by a BSD-3-Clause license that can be +// found in the LICENSE file at the root of this repository. + +package pdfkit + +import ( + "bytes" + "image" + "image/jpeg" + "testing" +) + +// jpegInfo is the only parser in this package that reads bytes a caller +// supplied, and it reads them by hand: markers, segment lengths and offsets, +// every one of them a number out of the file. image/jpeg is the oracle it is +// measured against — not because it is the specification, but because it is +// what every consumer of the PDF we write will have used to look at the same +// bytes. +// +// ⛔ The property is NOT "jpegInfo accepts what image/jpeg accepts". jpegInfo +// stops at the first start-of-frame and never looks at the entropy-coded data, +// so it accepts files image/jpeg rejects, and that is by design: pdfkit +// embeds the bytes without decoding them. What must never happen is the two +// AGREEING TO DISAGREE — a file both read, with different dimensions — because +// then pdfkit writes a /Width into the PDF that the consumer's own decoder +// contradicts, and the picture comes out sheared with nothing to blame. +func FuzzJPEGInfo(f *testing.F) { + f.Add(jpegBytes()) + f.Add(grayJPEGBytes(f)) + f.Add(fillBytesJPEG(f)) + f.Add([]byte{0xFF, 0xD8}) + f.Add([]byte{0xFF, 0xD8, 0xFF, 0xC0, 0x00, 0x0B, 0x08, 0x00, 0x10, 0x00, 0x10, 0x01}) + f.Add([]byte("not a jpeg at all")) + // TEM, which stands alone (Table B.1), then a frame header. + f.Add([]byte{0xFF, 0xD8, 0xFF, 0x01, 0xFF, 0xC0, 0x00, 0x0B, 0x08, 0x00, 0x10, 0x00, 0x10, 0x01}) + // X'FF00': byte stuffing, not a marker. + f.Add([]byte{0xFF, 0xD8, 0xFF, 0x00, 0x00, 0x0B}) + // A marker with nothing behind it to hold its length. + f.Add([]byte{0xFF, 0xD8, 0xFF, 0xC0, 0x00}) + // A frame header that declares more than it carries. + f.Add([]byte{0xFF, 0xD8, 0xFF, 0xC0, 0x00, 0x07, 0x08, 0x00, 0x10, 0x00}) + // A frame header too short to hold a frame: present, well-formed, empty. + f.Add([]byte{0xFF, 0xD8, 0xFF, 0xC0, 0x00, 0x04, 0xAA, 0xBB}) + // Extraneous bytes between segments, which libjpeg warns about and skips. + f.Add([]byte{0xFF, 0xD8, 0x30, 0xFF, 0xC0, 0x00, 0x0B, 0x08, 0x00, 0x10, 0x00, 0x10, 0x01}) + + f.Fuzz(func(t *testing.T, data []byte) { + w, h, comps, err := jpegInfo(data) + + cfg, cfgErr := jpeg.DecodeConfig(bytes.NewReader(data)) + if cfgErr != nil { + return // image/jpeg could not read it; there is nothing to compare against + } + if err != nil { + t.Fatalf("image/jpeg read this as %dx%d and jpegInfo refused it (%v): a JPEG a "+ + "consumer can decode is one DrawJPEG should be able to embed", + cfg.Width, cfg.Height, err) + } + if w != cfg.Width || h != cfg.Height { + t.Fatalf("jpegInfo says %dx%d, image/jpeg says %dx%d: DrawJPEG would write a "+ + "/Width the reader's own decoder contradicts", + w, h, cfg.Width, cfg.Height) + } + if comps < 1 || comps > 4 { + t.Fatalf("jpegInfo reports %d components for a file image/jpeg decoded", comps) + } + }) +} + +// grayJPEGBytes is a one-component frame: the other colour space DrawJPEG +// names, and a different SOF component count for the parser to walk past. +func grayJPEGBytes(f *testing.F) []byte { + f.Helper() + var buf bytes.Buffer + g := image.NewGray(image.Rect(0, 0, 8, 8)) + for i := range g.Pix { + g.Pix[i] = uint8(i) + } + if err := jpeg.Encode(&buf, g, nil); err != nil { + f.Fatal(err) + } + return buf.Bytes() +} + +// fillBytesJPEG is a valid JPEG with a fill byte inserted before the marker +// that follows SOI. ⛔ A marker "may optionally be preceded by any number of +// fill bytes" (ITU-T T.81 §B.1.1.2), so this is a legal file every decoder +// accepts — and the shape a hand-written marker walk is most likely to lose +// its place in. It is a seed rather than a test so the fuzzer can build on it. +func fillBytesJPEG(f *testing.F) []byte { + f.Helper() + src := jpegBytes() + out := make([]byte, 0, len(src)+2) + out = append(out, src[0], src[1]) // SOI + out = append(out, 0xFF, 0xFF) // two fill bytes + out = append(out, src[2:]...) + return out +} + +// FuzzDrawImageSamples drives the sample splitter over images whose pixels +// come from the fuzzer. It allocates nothing a caller did not already hold -- +// the geometry is this test's, so the bytes are the only thing being varied -- +// and it exists because unpremultiply does integer arithmetic on numbers the +// image chose. +// +// ⛔ The property is one no bug in the arithmetic can satisfy by accident: a +// fully opaque pixel must come back EXACTLY as it went in. Half-transparent +// pixels are checked by round-trip bound instead, because dividing by alpha +// and multiplying back is lossy and an exact assertion there would be a test +// of rounding rather than of correctness. +func FuzzDrawImageSamples(f *testing.F) { + f.Add([]byte{0xFF, 0x00, 0x00, 0xFF}) + f.Add([]byte{0x80, 0x40, 0x20, 0x80}) + f.Add([]byte{0x00, 0x00, 0x00, 0x00}) + + f.Fuzz(func(t *testing.T, pix []byte) { + const w, h = 4, 4 + img := image.NewRGBA(image.Rect(0, 0, w, h)) + for i := range img.Pix { + if i < len(pix) { + img.Pix[i] = pix[i] + } + } + // Premultiplied storage: a channel above its alpha is a state no + // encoder produces and image/color does not define. Clamp rather than + // skip, so every input still exercises the walk. + for i := 0; i+3 < len(img.Pix); i += 4 { + a := img.Pix[i+3] + for c := range 3 { + if img.Pix[i+c] > a { + img.Pix[i+c] = a + } + } + } + + rgb, alpha, hasAlpha := samples(img) + if len(rgb) != w*h*3 || len(alpha) != w*h { + t.Fatalf("samples returned %d colour and %d alpha bytes for %d pixels", + len(rgb), len(alpha), w*h) + } + opaque := true + for i := 0; i+3 < len(img.Pix); i += 4 { + px := i / 4 + if a := img.Pix[i+3]; a != 0xFF { + opaque = false + continue + } + for c := range 3 { + if rgb[px*3+c] != img.Pix[i+c] { + t.Fatalf("pixel %d channel %d went in as %d and came out as %d, and it "+ + "was fully opaque: nothing should have been divided", + px, c, img.Pix[i+c], rgb[px*3+c]) + } + } + if alpha[px] != 0xFF { + t.Fatalf("pixel %d is opaque and its mask sample is %d", px, alpha[px]) + } + } + if opaque && hasAlpha { + t.Fatal("every pixel is opaque and samples asked for a soft mask anyway: " + + "the document pays for an /SMask that masks nothing") + } + // The straight samples must be a colour, not an overflow. Only + // reachable through unpremultiply, which is where the arithmetic is. + for px := range w * h { + a := img.Pix[px*4+3] + if a == 0 || a == 0xFF { + continue + } + for c := range 3 { + if got := rgb[px*3+c]; int(got) < int(img.Pix[px*4+c]) { + t.Fatalf("pixel %d channel %d: %d premultiplied became %d straight at "+ + "alpha %d, which is darker than it went in", + px, c, img.Pix[px*4+c], got, a) + } + } + } + }) +} diff --git a/image.go b/image.go index 108afde..4e48fd1 100644 --- a/image.go +++ b/image.go @@ -211,16 +211,48 @@ func jpegInfo(data []byte) (w, h, comps int, err error) { if len(data) < 2 || data[0] != 0xFF || data[1] != 0xD8 { return 0, 0, 0, fmt.Errorf("pdfkit: not a JPEG (bad SOI)") } + // ⛔ The three skips below are not leniency for its own sake. DrawJPEG + // embeds these bytes UNTOUCHED, so whoever opens the PDF decodes the same + // file with a libjpeg-shaped decoder — and a writer that refuses what every + // reader of its own output accepts is the one in the wrong. The rules are + // taken from Go's image/jpeg (reader.go:542-581), which states where each + // comes from; both divergences below were found by FuzzJPEGInfo, within a + // second of each other, against image/jpeg as the oracle. i := 2 - for i+4 <= len(data) { + for i+1 < len(data) { + // Extraneous non-marker bytes between segments. libjpeg's next_marker + // raises JWRN_EXTRANEOUS_DATA and skips to the next X'FF'; image/jpeg + // does the same and so do we. (Found: a file image/jpeg read as + // 12336x12336 was refused here as "expected marker".) if data[i] != 0xFF { - return 0, 0, 0, fmt.Errorf("pdfkit: corrupt JPEG (expected marker)") + i++ + continue } marker := data[i+1] - if marker == 0xD8 || marker == 0xD9 || (marker >= 0xD0 && marker <= 0xD7) { + // X'FF00' is byte stuffing inside entropy-coded data, not a marker — + // a marker's second byte is "not equal to 0 or X'FF'" (T.81 §B.1.1.2). + if marker == 0x00 { i += 2 continue } + // "Any marker may optionally be preceded by any number of fill bytes, + // which are bytes assigned code X'FF'" (§B.1.1.2). (Found: a legal + // file image/jpeg read as 4x4 was refused here as a bad segment + // length, because the walk took the second X'FF' for the marker and + // then read the real marker as a length.) + if marker == 0xFF { + i++ + continue + } + // The markers that stand alone and carry no segment: Table B.1's + // asterisked rows — SOI, EOI, the eight restarts, and TEM. + if marker == 0xD8 || marker == 0xD9 || marker == 0x01 || (marker >= 0xD0 && marker <= 0xD7) { + i += 2 + continue + } + if i+4 > len(data) { + return 0, 0, 0, fmt.Errorf("pdfkit: corrupt JPEG (truncated segment)") + } seg := int(data[i+2])<<8 | int(data[i+3]) if seg < 2 || i+2+seg > len(data) { return 0, 0, 0, fmt.Errorf("pdfkit: corrupt JPEG (bad segment length)") diff --git a/testdata/fuzz/FuzzJPEGInfo/1e609ca636cd5bfc b/testdata/fuzz/FuzzJPEGInfo/1e609ca636cd5bfc new file mode 100644 index 0000000..27007c7 --- /dev/null +++ b/testdata/fuzz/FuzzJPEGInfo/1e609ca636cd5bfc @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("\xff\xd80\xff\xc0\x00\x11\b0000\x030A\x001A\x012A\x03\xff\xda00")