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

Status: RESOLVED FIXED

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

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: test file that causes crash

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: stack trace of relevant thread

Comment 3

[Daniel Veditz [:dveditz]]

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

[cajbir (:cajbir)]

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

[cajbir (:cajbir)]

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

[Viktor Gal]

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

[Timothy B. Terriberry (:derf)]

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

[cajbir (:cajbir)]

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

[Viktor Gal]

(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....

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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

[Timothy B. Terriberry (:derf)]

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.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Bug poke: is this being worked on?

Comment 13

[cajbir (:cajbir)]

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.

Comment 14

[Viktor Gal]

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.

Comment 15

[Mike Shaver (:shaver emeritus)]

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.)

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Viktor: Which changeset fixes this in liboggplay?

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

This still crashes on trunk despite bug 512328 checkin.

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1 - add max video size limits to liboggplay

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?

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Adding a test should be as easy as adding the test file to gErrorTests in manifest.js.

Comment 21

[cajbir (:cajbir)]

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

[Gregory Maxwell]

 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).

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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?

Comment 24

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2

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.

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

[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

Comment 26

[Chris Pearce [:cpearce (Not reading bugmail)]]

http://hg.mozilla.org/mozilla-central/rev/0527053700f1

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fe325885038b

Comment 29

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pushed to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4c94f5535bee

Comment 30

[Reed Loden [:reed]]

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?

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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!

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 403188 [details] [diff] [review]
Patch v2

Approved for 1.9.1.5, a=dveditz for release-drivers post facto

Comment 33

[Al Billings [:abillings - ex-MoCo]]

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.