Skip to content

Fix negative bodysize leading to heap overflow in ogg_stream_pagein#102

Open
MH1983-web wants to merge 1 commit into
xiph:mainfrom
MH1983-web:security-hardening-pagein-bodysize
Open

Fix negative bodysize leading to heap overflow in ogg_stream_pagein#102
MH1983-web wants to merge 1 commit into
xiph:mainfrom
MH1983-web:security-hardening-pagein-bodysize

Conversation

@MH1983-web

Copy link
Copy Markdown

What this changes

Adds a single defensive check in ogg_stream_pagein() (src/framing.c) to
reject a page whose computed bodysize is negative before it reaches memcpy().

if(bodysize<0) return -1;

Why

bodysize starts as og->body_len (a long). In the continued-packet branch it
is decremented by each skipped leading segment value taken from the page's lacing
table:

if(continued){
  ...
  for(;segptr<segments;segptr++){
    int val=header[27+segptr];
    body+=val;
    bodysize-=val;
    if(val<255){ segptr++; break; }
  }
}

if(bodysize){
  if(_os_body_expand(os,bodysize)) return -1;
  memcpy(os->body_data+os->body_fill,body,bodysize);
  os->body_fill+=bodysize;
}

If a page's body_len is smaller than the sum of those skipped segment values,
bodysize underflows below zero. It is then passed to memcpy(), whose third
parameter is size_t, so a negative long is reinterpreted as a very large
unsigned length and overflows os->body_data. _os_body_expand() does not catch
this: it also takes a long needed and its overflow guard only checks against
LONG_MAX, not for negative values.

Reachability / severity

This is low severity and, as far as I can tell, not reachable through libogg's
normal data flow. Pages produced by ogg_sync_pageseek() / ogg_sync_pageout()
derive body_len from the same segment table this code re-reads, so the invariant
holds and the subtraction cannot go negative. libvorbisfile only obtains pages
through that path.

It is reachable only when a caller hands ogg_stream_pagein() a page whose
body_len is inconsistent with its own segment table — e.g. a struct built or
mutated outside the sync layer. The change is therefore proposed as defensive
hardening: the function currently trusts a precondition it does not enforce, and
this makes the contract explicit. return -1 is the function's existing error
convention (already used for serial mismatch, version mismatch, and allocation
failure), so callers need no changes.

Testing

  • Built with -fsanitize=address,undefined and exercised ogg_stream_pagein()
    via a libFuzzer harness that constructs pages directly.
  • Without the change, a minimal input (a continued-flag page with 20 lacing
    segments of 255 and a smaller body_len) triggers:
    AddressSanitizer: negative-size-param: (size=-5100) at the memcpy in
    ogg_stream_pagein.
  • With the change, that input returns -1 cleanly; valid pages still decode; a
    follow-up fuzzing run (1.3M+ executions) showed no crashes and no regression.

I'm happy to share the libFuzzer harness and the 48-byte reproducer on request.

Patch is against commit 06a5e0262cdc28aa4ae6797627a783b5010440f0.

A page whose segment table is inconsistent with body_len can drive
bodysize negative. Passing a negative long to memcpy's size_t parameter
would request an enormous copy and overflow the body buffer.

Add a defensive check: if (bodysize < 0) return -1.
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