Closed
Bug 504843
Opened 15 years ago
Closed 15 years ago
crash (segfault) @ oc_sb_create_plane_mapping when playing corrupted ogg theora file
Categories
(Core :: Audio/Video, defect, P2)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
blocking1.9.1 | --- | .6+ |
status1.9.1 | --- | .6-fixed |
People
(Reporter: keeler, Assigned: cpearce)
References
Details
(Keywords: crash, testcase, verified1.9.1, Whiteboard: [sg:critical?])
Attachments
(3 files, 1 obsolete file)
64.00 KB,
audio/ogg
|
Details | |
3.17 KB,
text/plain
|
Details | |
108.93 KB,
patch
|
cajbir
:
review+
dveditz
:
approval1.9.1.6+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11) 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
![]() |
Reporter | |
Comment 1•15 years ago
|
||
![]() |
Reporter | |
Comment 2•15 years ago
|
||
![]() |
Reporter | |
Updated•15 years ago
|
Comment 3•15 years ago
|
||
I get a different stack in a windows nightly, but also crashing in memset. 0:017> !exploitable -v HostMachine\HostUser 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: MOZCRT19!memset+0x5f xul!JSD_ScriptDestroyed+0x910f xul!JSD_ScriptCreated+0x370c xul!JSD_DebuggerOnForUser+0x2c67 xul!JSD_DebuggerOn+0x3e09 xul!JSD_DebuggerOn+0x4829 xul!NS_LogAddRef_P+0x4f24 xul!gfxFontTestStore::gfxFontTestStore+0x349f xul!gfxFontTestStore::gfxFontTestStore+0x4100 xul!gfxWindowsFontGroup::operator=+0xa29 xul!gfxContext::GetFlags+0xac6c 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.
Comment 4•15 years ago
|
||
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.
Comment 5•15 years ago
|
||
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.
Comment 6•15 years ago
|
||
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?
Comment 7•15 years ago
|
||
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.
Comment 8•15 years ago
|
||
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.
Comment 9•15 years ago
|
||
(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....
![]() |
Reporter | |
Comment 10•15 years ago
|
||
Isn't this just a failure to check the return value of _ogg_calloc (which is #define'd to be plain calloc)? Consider: libtheora/lib/dec/state.c: (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.
Comment 11•15 years ago
|
||
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
![]() |
Reporter | |
Comment 12•15 years ago
|
||
Bug poke: is this being worked on?
Comment 13•15 years ago
|
||
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.
Updated•15 years ago
|
Depends on: CVE-2009-3378
Comment 14•15 years ago
|
||
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.
Updated•15 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 15•15 years ago
|
||
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.)
Assignee | ||
Comment 16•15 years ago
|
||
Viktor: Which changeset fixes this in liboggplay?
Updated•15 years ago
|
blocking1.9.1: --- → ?
Assignee | ||
Comment 17•15 years ago
|
||
This still crashes on trunk despite bug 512328 checkin.
No longer depends on: CVE-2009-3378
Assignee | ||
Comment 18•15 years ago
|
||
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
Assignee | ||
Comment 19•15 years ago
|
||
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 | ||
Comment 20•15 years ago
|
||
Adding a test should be as easy as adding the test file to gErrorTests in manifest.js.
Flags: in-testsuite?
Comment 21•15 years ago
|
||
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.
Comment 22•15 years ago
|
||
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).
Assignee | ||
Comment 23•15 years ago
|
||
(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. */ if ( ((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; return OGGZ_CONTINUE; } Does that handle the issue you're raising here?
Assignee | ||
Comment 24•15 years ago
|
||
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)
Assignee | ||
Comment 25•15 years ago
|
||
[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
Updated•15 years ago
|
Attachment #403188 -
Flags: review?(chris.double) → review+
Updated•15 years ago
|
blocking1.9.1: ? → needed
Assignee | ||
Comment 26•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/0527053700f1
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 27•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fe325885038b
status1.9.2:
--- → beta1-fixed
Updated•15 years ago
|
blocking1.9.1: needed → .5+
Assignee | ||
Comment 29•15 years ago
|
||
Pushed to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4c94f5535bee
Comment 30•15 years ago
|
||
Comment on attachment 403188 [details] [diff] [review] Patch v2 (In reply to comment #29) > Pushed to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4c94f5535bee 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?
Assignee | ||
Comment 31•15 years ago
|
||
(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!
Updated•15 years ago
|
Attachment #403188 -
Flags: approval1.9.1.5? → approval1.9.1.5+
Comment 32•15 years ago
|
||
Comment on attachment 403188 [details] [diff] [review] Patch v2 Approved for 1.9.1.5, a=dveditz for release-drivers post facto
Comment 33•15 years ago
|
||
Verified for 1.9.1.6 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091119 Shiretoko/3.5.6pre (.NET CLR 3.5.30729) using existing testcase. Crashes cleanly with 1.9.1.5.
Keywords: verified1.9.1
Updated•15 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•