Skip to content

fix(mp4): reject undersized boxes instead of underflowing the size - #587

Merged
tobbee merged 1 commit into
masterfrom
fix/container-size-underflow
Sep 13, 2026
Merged

tobbee merged 1 commit into
masterfrom
fix/container-size-underflow

Conversation

@tobbee

@tobbee tobbee commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to #580, which brought the two container-children loops into
agreement and in doing so made the reader path reach a size underflow the SR
path had been hitting on its own.

The underflow

DecodeContainerChildren and DecodeContainerChildrenSR both report a
mismatch with

fmt.Errorf("non-matching children box sizes, parentSize=%d, %s", endPos-startPos, msg)

startPos and endPos are uint64. Callers that read their children from
past the 8-byte header — dref.go, stsd.go and trep.go at startPos+16,
meta.go at a computed offset — never check hdr.Size first, so a box
declaring a size below that offset leaves startPos > endPos and the
subtraction wraps:

before: decode dref pos 0: non-matching children box sizes, parentSize=18446744073709551612,
after:  decode dref pos 0: dref: box size 12 is too small, needs at least 16 bytes

Guarding once before the loop also makes every later endPos-startPos and
pos-startPos safe, since pos only grows from startPos.

This is a bad error message, not a crash: no panic, no allocation driven by the
wrapped value, and the decode already failed either way.

esds

DecodeEsdsSR has the same shape in descSize := uint32(hdr.Size - 12) with no
minimum-size check. The wrapped value is handed to DecodeESDescriptor, which
ignores the parameter entirely today — so nothing misbehaves, and this is a
latent hazard rather than a live bug. Validated so it cannot bite if that
parameter is ever used.

What else was checked

I swept the rest of the box decoders for the same pattern. Everything else is
either already guarded or signed:

Site Verdict
ssix.go:77 hdr.Size-16 guarded by if hdr.Size < 16
uuid.go:254 hdr.Size-16 guarded by if hdr.Size < 16+8
senc.go:151,158 payloadLen-8 guarded by if hdr.payloadLen() < 8
elng.go:61 plLen-4 guarded by the plLen < 7 early return
mdat.go:57 hdr.Size-hdr.Hdrlen DecodeHeader guarantees Size >= Hdrlen
visualsampleentry.go:167 endPos-pos inside for pos < endPos
emsg.go, labl.go, stpp.go, eventmessage.go payloadLen() returns signed int; a negative maxLen makes ReadZeroTerminatedString set "did not find terminating zero" rather than wrap
bits/fixedslicereader.go s.len-N s.len is int; negative compares correctly

Reader/SR asymmetry is not a concern for these: DecodeSsix, DecodeSenc,
DecodeEsds, DecodeElng, DecodeEmsg and DecodeLabl all delegate to their
SR versions via readBoxBody, so a guard in the SR path covers both. The
container-children pair is unusual in having two genuinely separate
implementations, which is why it drifted.

Tests

TestDecodeUndersizedContainerNoSizeUnderflow covers dref, stsd and trep
at sizes 8, 12 and 15, on both decode paths, asserting the error does not
contain a wrapped value. All 18 assertions fail on master.

TestDecodeUndersizedEsdsNoSizeUnderflow asserts the box is rejected as a
size
, not incidentally by the descriptor tag check that runs first without the
guard — without the fix it fails with got tag 0 instead of ESDescriptorTag 3.

Verification

🤖 Generated with Claude Code

@tobbee
tobbee force-pushed the fix/container-size-underflow branch from 25918b1 to 87a7f21 Compare September 13, 2026 19:57
@tobbee

tobbee commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator Author

Updated after review feedback.

The esds check now measures hdr.payloadLen() rather than hdr.Size:

if hdr.payloadLen() < 4 {
    return nil, fmt.Errorf("esds: payload size %d is too small, needs at least 4 bytes", hdr.payloadLen())
}
...
descSize := uint32(hdr.payloadLen() - 4)

hdr.Size-12 assumed an 8-byte header. ISO/IEC 14496-12 permits an extended (large) size on any box, where the header is 16 bytes — so the original form was wrong in principle for both the guard and the descriptor size it computes. Measuring the payload is correct either way, and matches how senc already does it.

This is latent today only because DecodeHeader rejects size == 1 for every type except mdat, so Hdrlen is always 8 here in practice.

Worth noting separately: the same assumption is spread across 53 container call sites that pass startPos+8 / startPos+16 instead of startPos+uint64(hdr.Hdrlen). Those would all need updating if large-size support were ever extended beyond mdat. Out of scope here, but happy to do it as a follow-up.

Also dropped the explanatory comments — the error messages carry the information.

A box whose declared size is smaller than the fixed fields preceding its
children leaves startPos past endPos. dref, stsd and trep read their
children from 16 bytes in and meta from a computed offset, none of them
checking the size first, so endPos-startPos wrapped and the error
reported a parent size of about 1.8e19. Guard in both
DecodeContainerChildren and DecodeContainerChildrenSR, naming the
declared and the minimum size, which also makes every subtraction after
the guard safe.

DecodeEsdsSR had the same shape in hdr.Size-12. The wrapped value went
to DecodeESDescriptor, which ignores the parameter today, so nothing
misbehaved yet. Both the check and the descriptor size now measure the
payload rather than hdr.Size, which hardcoded an 8-byte header and would
be wrong for a box carrying an extended size.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tobbee
tobbee force-pushed the fix/container-size-underflow branch from 87a7f21 to 79de58d Compare September 13, 2026 20:05
@tobbee
tobbee merged commit dbac0e7 into master Sep 13, 2026
9 checks passed
@tobbee
tobbee deleted the fix/container-size-underflow branch September 13, 2026 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant