diff --git a/linearized.go b/linearized.go index 8353dde..97f30c5 100644 --- a/linearized.go +++ b/linearized.go @@ -526,22 +526,36 @@ func checkHintStream(b []byte, lin *Linearization) error { "integer position for every hint table it names", ErrLinearizationStale, sv) } - // ⛔ AND A POSITION CANNOT BE NEGATIVE. F.3.6: the positions "shall be - // counted in bytes relative to the beginning of the stream data". Review + // ⛔⛔ AND THE SHARED OBJECT TABLE CANNOT START BEFORE BYTE 36, which is + // arithmetic rather than a guess and needs no decoding at all. + // + // F.3.6: the page offset hint table "shall be the first table in the + // stream and shall start at offset 0". Table F.3 gives that table's + // header as exactly thirteen items of 32, 32, 16, 32, 16, 32, 16, 32, 16, + // 16, 16, 16 and 16 bits = 288 bits = 36 bytes, before a single per-page + // entry. So whatever follows it begins at 36 at the very earliest, and /S + // names where the shared object table begins. + // + // ⛔ 36 AND NOT MORE, BECAUSE THE MARGIN WAS MEASURED. Ghostscript emits + // /S 38 — two bytes above the floor — against qpdf's 43 for the same + // document. A bound picked by eye from qpdf alone would have refused + // every Ghostscript file. This also subsumes the negative case: review // measured `/S -1` passing, with qpdf answering "overflow reading bit - // stream" — a symptom of seeking outside the data rather than a statement + // stream", a symptom of seeking outside the data rather than a statement // about /S. // - // ⚠ THE UPPER BOUND IS NOT CHECKED, and that is an open limit rather than - // an oversight: /S is an offset into the DECODED stream data, so bounding - // it means decoding the hint stream — which is the first step of reading - // the tables themselves (F.4) and belongs with that work. qpdf reports - // "/S (shared object) offset is out of bounds" for `/S 99` here and this - // does not. Recorded on #40. - if pos < 0 { - return fmt.Errorf("%w: the primary hint stream's /S is %d, and a position counted in bytes "+ - "from the start of the stream data cannot be negative", - ErrLinearizationStale, int64(pos)) + // ⚠ THE UPPER BOUND IS STILL NOT CHECKED, and that is an open limit with + // a reason: /S indexes the DECODED data, so bounding it above means + // decoding an attacker's bytes in the cheap path — capped at 1 GiB by + // filter.go, which refuses a bomb but is three orders of magnitude more + // than any hint stream needs. That wants a size bound derived from /N and + // the object count, which wants F.4's layouts. #40 carries the ordering. + const pageOffsetHeaderBytes = 36 + if pos < pageOffsetHeaderBytes { + return fmt.Errorf("%w: the primary hint stream's /S is %d, and the shared object table "+ + "cannot begin before byte %d — the page offset table starts at 0 and its header alone "+ + "is %d bytes (Table F.3, thirteen items of 288 bits)", + ErrLinearizationStale, int64(pos), pageOffsetHeaderBytes, pageOffsetHeaderBytes) } // ⛔ AND THE LENGTH IS COMPARED WITH THE OBJECT THAT IS ACTUALLY THERE, // which review found it was not: the pair was bounds-checked against the diff --git a/linearized_test.go b/linearized_test.go index 096b2ed..f148d55 100644 --- a/linearized_test.go +++ b/linearized_test.go @@ -893,23 +893,49 @@ func TestAPrimaryHintStreamWhoseSharedTablePositionIsNotANumberIsRefused(t *test } } -// ⛔ A POSITION CANNOT BE NEGATIVE, which review measured passing: `/S -1` -// was accepted here while qpdf answered "overflow reading bit stream" — a -// symptom of seeking outside the data rather than a statement about /S. +// ⛔⛔ THE SHARED OBJECT TABLE CANNOT START BEFORE BYTE 36, and that is +// arithmetic rather than a guess. F.3.6 puts the page offset hint table first +// and at offset 0; Table F.3 gives its header as thirteen items totalling 288 +// bits = 36 bytes before a single per-page entry. So /S, which names where the +// shared object table begins, is at least 36. // -// `/S 43` -> `/S -1` is the same width, so again only the value changes. -func TestANegativeSharedTablePositionIsRefused(t *testing.T) { - b := editedFixturePadded(t, [2]string{"/S 43", "/S -1"}) - if _, err := Linearized(b); err != nil { - t.Fatalf("the cheap check refused it, so this is not about /S: %v", err) - } - _, err := CheckLinearized(b) - if !errors.Is(err, ErrLinearizationStale) { - t.Fatalf("error is %v, want ErrLinearizationStale", err) - } - if !strings.Contains(err.Error(), "cannot be negative") { - t.Errorf("it was refused, but not for the negative position: %v", err) +// ⛔ AND 36 IS THE FLOOR BECAUSE THE MARGIN WAS MEASURED, not eyeballed from +// qpdf. The three committed fixtures emit: +// +// qpdf, table form /S 43 (decoded 71 bytes) +// qpdf, object streams /S 43 (decoded 71 bytes) +// Ghostscript /S 38 (decoded 67 bytes) +// +// Ghostscript is two bytes above the floor. A bound chosen by looking at qpdf +// alone — 40, say — would have refused every file that tool writes, which is +// the same trap the /H exclusion walked into before a second producer existed. +// +// Both edits are same-width against `/S 43`, so only the value changes. +func TestASharedTablePositionInsideThePageOffsetHeaderIsRefused(t *testing.T) { + for _, to := range []string{"/S -1", "/S 35"} { + t.Run(to, func(t *testing.T) { + b := editedFixturePadded(t, [2]string{"/S 43", to}) + if _, err := Linearized(b); err != nil { + t.Fatalf("the cheap check refused it, so this is not about /S: %v", err) + } + _, err := CheckLinearized(b) + if !errors.Is(err, ErrLinearizationStale) { + t.Fatalf("error is %v, want ErrLinearizationStale", err) + } + if !strings.Contains(err.Error(), "cannot begin before byte 36") { + t.Errorf("it was refused, but not by the page offset header's own size: %v", err) + } + }) } + + // ⛔ AND 36 ITSELF IS ACCEPTED, or the bound would be off by one and no + // test above could tell: both refusals assert a message, not a boundary. + t.Run("36 is the floor and not past it", func(t *testing.T) { + b := editedFixturePadded(t, [2]string{"/S 43", "/S 36"}) + if _, err := CheckLinearized(b); err != nil && strings.Contains(err.Error(), "cannot begin before") { + t.Errorf("36 is the first legal position and was refused: %v", err) + } + }) } // ⛔⛔ AND EVERY OTHER WRONG KIND, WHICH THE FILE-LEVEL TEST CANNOT REACH.