Skip to content

ogg_stream_pagein: validate page segment table against body length - #108

Open
XananasX7 wants to merge 8 commits into
xiph:masterfrom
XananasX7:pagein-validate-segment-table
Open

ogg_stream_pagein: validate page segment table against body length#108
XananasX7 wants to merge 8 commits into
xiph:masterfrom
XananasX7:pagein-validate-segment-table

Conversation

@XananasX7

Copy link
Copy Markdown

Problem

ogg_stream_pagein() is a public entry point and may be handed pages that were parsed or constructed by the caller (custom demuxers, remuxers, seek/index layers, or sync builds with DISABLE_CRC). Pages produced by ogg_sync_pageout() are length- and checksum-verified, but callers using their own page parsing can supply an ogg_page whose segment table claims more body data than body_len contains.

In that case bodysize goes negative while the segment table is walked: the continued-packet skip loop (or a final short body) then reaches memcpy with a negative length (huge size_t), or leaves body_fill negative for subsequent packet assembly — a heap-corruption / out-of-bounds condition reachable from malformed media. ASan reports negative-size-param at framing.c:862.

Minimal reproducer against current master:

ogg_stream_init(&os, 0x1234);
unsigned char hdr[28], body[16];
memcpy(hdr, "OggS", 4);
hdr[5] = 0x01;            /* continued packet */
hdr[14] = 0x34; hdr[15] = 0x12; /* serial */
hdr[26] = 1;              /* segments */
hdr[27] = 250;            /* lacing value > body_len */
ogg_page og = { hdr, sizeof(hdr), body, 10 };
ogg_stream_pagein(&os, &og);   /* -> negative-size-param memcpy */

Fix

Reject any page whose lacing values overshoot its body (or whose header_len can't contain the segment table) before trusting the table, returning -1 as with other invalid pages. The check is O(segments) with an early exit, and validates pages produced by ogg_sync_pageout() (which satisfy it by construction).

Testing

  • Malformed page now returns -1 (previously: ASan negative-size-param in memcpy).
  • A full encode/decode round trip — including packets spanning multiple 255-lacing segments and BOS/EOS flags — decodes byte-identically to before the change.

tmatth and others added 8 commits June 16, 2025 14:14
Found with codespell.
Checksums of the source packages, produced by the gitlab ci runner,
as published to https://downloads.xiph.org/releases/ogg/SHA256SUMS.
Remove the obsolete Travis and Jenkins continuous integration status
badges from the README and add ones for Gitlab and Github.
The opus website no longer references libogg, and we publish
directly to ftp these days without going through svn.
Add some newer files generated by the autotools build to the git
ignore list so they don't clutter up status output or end up
accidentally included in a commit to the repository.
Support for earlier versions is deprecated.

Addresses a deprecation warning building with recent cmake versions.

Signed-off-by: Ralph Giles <giles@thaumas.net>
The original commit from 2007 (440e37e) accumulated all sizes
 in ints (despite ogg_iovec_t.iov_len being a size_t and all
 relevant fields of ogg_stream_state being longs).
I expanded this to longs in 2013 when I added overflow checks
 (commit 85dbd8d), but apparently did not read far enough down
 in the function to catch this cast back to int the second time the
 sizes are accumulated.

Patch from: Chengyu Song <csong@ucr.edu>

Fixes #2308.
ogg_stream_pagein() is a public entry point and may be handed pages
that were parsed or constructed by the caller (custom demuxers,
remuxers, seek/index layers, or sync builds with DISABLE_CRC).  Pages
produced by ogg_sync_pageout() are length- and checksum-verified, but
callers using their own page parsing can supply an ogg_page whose
segment table claims more body data than body_len contains.

In that case bodysize went negative when the segment table was walked:
the continued-packet skip loop (or a final short body) then reached a
memcpy with a negative length (huge size_t), or left body_fill negative
for subsequent packet assembly -- a heap-corruption / out-of-bounds
condition reachable from malformed media.

Reject any page whose lacing values overshoot its body (or whose
header_len can't contain the segment table), before trusting the table,
returning -1 as with other invalid pages.

Tested: malformed pages now return -1 (previously ASan reported
'negative-size-param' in memcpy); a full encode/decode round trip,
including packets spanning multiple 255-lacing segments and BOS/EOS
flags, decodes identically to before.
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.

4 participants