Closed Bug 504843 Opened 15 years ago Closed 15 years ago

crash (segfault) @ oc_sb_create_plane_mapping when playing corrupted ogg theora file


(Core :: Audio/Video, defect, P2)




Tracking Status
status1.9.2 --- beta1-fixed
blocking1.9.1 --- .6+
status1.9.1 --- .6-fixed


(Reporter: keeler, Assigned: cpearce)



(Keywords: crash, testcase, verified1.9.1, Whiteboard: [sg:critical?])


(3 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv: Gecko/2009060308 Ubuntu/9.04 (jaunty) Firefox/3.0.11
Build Identifier: mozilla-central revision 19e12cf52506

Segmentation fault when playing corrupted ogg theora file (attached).

Reproducible: Always

Steps to Reproduce:
1. Load attached file.
Actual Results:  
firefox crashes

Expected Results:  
some sort of "this file is corrupted" message
I get a different stack in a windows nightly, but also crashing in memset.

0:017> !exploitable -v
Executing Processor Architecture is x86
Debuggee is in User Mode
Debuggee is a live user mode debugging session on the local machine
Event Type: Exception
Exception Faulting Address: 0x4
First Chance Exception Type: STATUS_ACCESS_VIOLATION (0xC0000005)
Exception Sub-Type: Write Access Violation

Exception Hash (Major/Minor): 0x5b2b6b41.0x206f063d

Stack Trace:
Instruction Address: 0x7814830f

Description: User Mode Write AV near NULL
Short Description: WriteAV
Exploitability Classification: PROBABLY_EXPLOITABLE
Recommended Bug Title: Probably Exploitable - User Mode Write AV near NULL starting at MOZCRT19!memset+0x5f (Hash=0x5b2b6b41.0x206f063d)

User mode write access violations that are near NULL are probably exploitable.
Flags: wanted1.9.0.x-
Whiteboard: [sg:critical?]
This crash happens with the player_example program with libtheora and with the version in the thusnelda branch. I've CC'd the libtheora maintainer.
Tim researched the problem and it's due to the large frame width and height in the headers of the Ogg file.

liboggplay decodes the header and later decodes a frame using this width and height - we'll need to have liboggplay check for sane values in here somehow before decoding the frame. I've CC'd the liboggplay maintainer to get his thoughts on this.
although i think theora it self should have a sane value check as basically for it is it theora who crashes, it is would be wise to have a dimension check in oggplay as well.  as i can see it's this video has a  844832x1012384 frame, that is obviously an insane value.

would a 10000x10000 limit be OK? i mean that would require roughly ~380megs/frame in RGBA...

in case of error (aka insane frame dimension) i would simple go by disabling the theora track.

any comments?
There is an insane value check in Thusnelda svn (and there has been for a while). Chris Double told me he was accidentally linking against the system version of libtheora in the crash report above. I was unable to reproduce the crash locally (mostly through luck, even if the decoder fails to initialize, player_example allocates space to hold the picture region, which was still 320x240, but could have been set much larger).

However, libtheora will only reject the file if the size of the frame would overflow the size of a pointer. It doesn't know what kind of hardware you have, and is trying to be a general-purpose library. Perhaps you _want_ to encode 380 MB frames. There are niche applications where 10000x10000 video is perfectly reasonable (think wide-area surveillance). Not everything is web video.

That said, that means there's a non-empty area between, "So large as to overflow your address space," and, "Large enough that your OS will happily map all the pages, and then sit there grinding away while it pages everything else out to make room for them until finally it segfaults when your swap file fills up." This file wouldn't have triggered the latter, but another, slightly more reasonable file would. Chris Double told me (months ago, when I asked) that he had a check for reasonable frame sizes to guard against this, but was worried that liboggplay would attempt to allocate a decoder before he would have a chance to reject the file. I'm almost glad you guys didn't have the Thusnelda sanity check in place, or we might never have noticed this potential DoS.

As for a 10000x10000 limit in the browser, it's at least better than the 2046 limit hard-coded into ffmpeg's swscaler. I have 2160p test sequences that I use with libtheora all the time, and I can't get any player that uses swscaler (including gstreamer) to even display them. liboggplay could also just provide an API to set the limit. This would allow you to use a slightly more aggressive default.
Viktor, is it possible to return from the oggplay_open_with_reader call after reading the headers but before decoding the frame data? That way the application could get a chance to check the values for sanity and prevent further decoding.
(In reply to comment #8)
> Viktor, is it possible to return from the oggplay_open_with_reader call after
> reading the headers but before decoding the frame data? That way the
> application could get a chance to check the values for sanity and prevent
> further decoding.

Yeah, sure thing!

Actually one of the new things coming up - i was mentioning in the email to you -, that oggplay handles/decodes the headers separately in the init process, i.e. oggplay_open_with_reader, and there it does some other sanity checks. By this after calling oggplay_open_with_reader you could check the OggPlayDecode structs' values of the various tracks. Or you rather prefer a separate API for some values?

Although, you could call oggplay_get_video_y_size after init, in this case it wouldn't work, as those dimensions are derived from frame_width and frame_height of theora_info, which is 320x240....
Isn't this just a failure to check the return value of _ogg_calloc (which is #define'd to be plain calloc)?
(in oc_state_frarray_init)
line 430: _state->sbs=_ogg_calloc(nsbs,sizeof(oc_sb));
line 442: oc_sb_create_plane_mapping(_state->sbs+fplane->sboffset,

(in oc_sb_create_plane_mapping)
line 77: oc_sb *sb;
line 80: sb=_sbs;
line 101: memset(sb->map[0],0xFF,sizeof(sb->map));
(some lines omitted for clarity)

The library attempts to allocate a lot of space for _state->sbs (which fails). Then, it tries to use _state->sbs without checking that it isn't null (which it is).
Unless something more subtle is going on, it seems it would make sense to check for null and somehow bail or indicate an error state if necessary.
What's "more subtle" is that you can't rely on malloc to actually fail if the OS runs out of memory. It might for this particular fuzzed file on your particular test system. But every modern fork-based Unix overcommits pages (except Solaris, but they had to rewrite half their tools so exec'ing "ls" from a large process wouldn't run the system out of RAM). A more carefully crafted file could allow the malloc to succeed, only to segfault later when the RAM is used. A slightly less aggressive file could simply make the user sit there paging GBs of RAM in and out (especially on 64-bit systems). No malloc check will prevent that.

There will be malloc checks in the 1.1 release of libtheora, since there are embedded systems whose malloc really fails when they're out of memory. But that doesn't solve the real problem, which was that the sanity checks between Firefox and liboggplay were not functioning correctly.
Flags: blocking1.9.2+
Priority: -- → P2
Bug poke: is this being worked on?
Yes, the thusnelda version of Theora which we're moving too has an overflow check, and the liboggplay maintainer has mentioned in comment 9 that he would implement something that would allow our 'insane value' check to take place before any decoding occurs. When this happens the bug will be fixed.
Depends on: CVE-2009-3378
The fix for bug this depends on bug 512328, i.e. update the liboggplay version in mozilla-central to the latest version of liboggplay in it's official master branch.
Ever confirmed: true
Does this affect the 1.9.1 branch?  If so, it seems like we might need a backportable fix that's narrower than taking the liboggplay update.  (And we might need such a narrower fix if we can't figure out the problems with bug 512328 in time for the 1.9.2 freeze.)
Viktor: Which changeset fixes this in liboggplay?
blocking1.9.1: --- → ?
This still crashes on trunk despite bug 512328 checkin.
No longer depends on: CVE-2009-3378
This is fixed for me when I apply the libtheora update in bug 507554. As per the comments above, we should still handle this separately in liboggplay though.
Depends on: 507554
Adds oggplay_set_max_video_size() to liboggplay, and uses that when initializing decoder. Also sets the maximum video size to 4000x3000 - this is large enough to handle 2160p, which is probably the largest videos we'd want to handle?
Assignee: nobody → chris
Attachment #403148 - Flags: review?(chris.double)
Adding a test should be as easy as adding the test file to gErrorTests in manifest.js.
Flags: in-testsuite?
Comment on attachment 403148 [details] [diff] [review]
Patch v1 - add max video size limits to liboggplay

>+  if ((me = oggplay_new_with_reader(reader)) == NULL)
>+    return NULL;
> ...
>+  if (r != E_OGGPLAY_OK) {
>+    return NULL;
>+  }

Inconsistent bracing style.

>+  r = oggplay_set_max_video_size(me, max_frame_width, max_frame_height);
>+  if (r != E_OGGPLAY_OK) {
>+    return NULL;
>+  }

If oggplay_set_max_video_size fails, how is the reader returned by the call to oggplay_new_with_reader deleted?

Needs test.
 So the oggplay_set_max_video_size  limits width and height individually, to 4000 and 3000 in the current code.  This would reject a 32x4096 video, for example,... which is far fewer pixels (less memory!) than 720p.

It might be better to limit it by the product of width * height (being sure to deal with integer overflow).

Also— at the same point the code should also be checking that the offset values don't push the cropping rectangle out of the coded area. (perhaps thats there already outside of the part visible in the patch).
(In reply to comment #22)
> Also— at the same point the code should also be checking that the offset values
> don't push the cropping rectangle out of the coded area. (perhaps thats there
> already outside of the part visible in the patch).

We already have this code just above the oggplay_set_max_video_size() call:

/* Ensure the offsets do not push the viewable area outside of the decoded frame. */
  ((decoder->video_info.height - decoder->video_info.offset_y)<decoder->video_info.frame_height)
  ((decoder->video_info.width - decoder->video_info.offset_x)<decoder->video_info.frame_width)
  common->initialised |= -1;

Does that handle the issue you're raising here?
Attached patch Patch v2Splinter Review
Limit video frame size to a number of pixels, rather than (width,height) dimensions. I also had to modify oggz_read_sync() so that it returned and propogated when its callbacks return out of memory (as we do when the video frame is too big). Added testcase.
Attachment #403148 - Attachment is obsolete: true
Attachment #403188 - Flags: review?(chris.double)
Attachment #403148 - Flags: review?(chris.double)
[13:17]	<kfish>	cpearce, can you cc me on 504843?
[13:18]	<cpearce>	kfish: sure. :)
[13:19]	<cpearce>	kfish: should be cc'd now. I'd be interested in your comments on my oggz patch.
[13:20]	<kfish>	cpearce, thanks
[13:30]	<kfish>	cpearce, got it ... that oggz_read_sync patch should also check for OGGZ_STOP_OK, which == 1
Attachment #403188 - Flags: review?(chris.double) → review+
blocking1.9.1: ? → needed
Closed: 15 years ago
Resolution: --- → FIXED
blocking1.9.1: needed → .5+
Comment on attachment 403188 [details] [diff] [review]
Patch v2

(In reply to comment #29)
> Pushed to 1.9.1:

Uh, this patch was not approved to land on 1.9.1. Why was it committed without approval?
Attachment #403188 - Flags: approval1.9.1.5?
(In reply to comment #30)
> Uh, this patch was not approved to land on 1.9.1. Why was it committed without
> approval?

Oops, I thought blocking1.9.1=.5+ was enough, I didn't realise the rules are different for 191, sorry!
Attachment #403188 - Flags: approval1.9.1.5? → approval1.9.1.5+
Comment on attachment 403188 [details] [diff] [review]
Patch v2

Approved for, a=dveditz for release-drivers post facto
Verified for with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20091119 Shiretoko/3.5.6pre (.NET CLR 3.5.30729) using existing testcase. Crashes cleanly with
Keywords: verified1.9.1
Group: core-security
You need to log in before you can comment on or make changes to this bug.