504613 - crash (segfault) @ oggplay_data_handle_theora_frame (memcpy) when playing corrupted ogg theora file

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

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 c121e64e9940

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

Backtrace from a program that uses the ogg libraries similarly to firefox (i.e. oggplayer):
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xb6e69b90 (LWP 31797)]
0xb7b45896 in memcpy () from /lib/tls/i686/cmov/libc.so.6
(gdb) backtrace
#0  0xb7b45896 in memcpy () from /lib/tls/i686/cmov/libc.so.6
#1  0x0930bfd8 in ?? ()
#2  0x08057449 in oggplay_callback_theora (oggz=0x929d200, op=0x930c128, 
    serialno=855307237, user_data=0x930bfd8) at oggplay_callback.c:201
#3  0x0805fdfd in oggz_read_deliver_packet (elem=0x930c128) at oggz_read.c:303
#4  0x080649b8 in oggz_dlist_deliter (dlist=0x929d440, 
    func=0x805fd00 <oggz_read_deliver_packet>) at oggz_dlist.c:183
#5  0x0806045f in oggz_read_sync (oggz=0x929d200) at oggz_read.c:462
#6  0x08060783 in oggz_read (oggz=0x929d200, n=8192) at oggz_read.c:606
#7  0x080555e6 in oggplay_step_decoding (me=0x929d040) at oggplay.c:691
#8  0x0804d04b in decode_thread (p=0xbfac0548) at oggplayer.cpp:313
#9  0xb7e2983b in ?? () from /usr/lib/libSDL-1.2.so.0
#10 0xb7e7771d in ?? () from /usr/lib/libSDL-1.2.so.0
#11 0xb7eb74ff in start_thread () from /lib/tls/i686/cmov/libpthread.so.0
#12 0xb7bb049e in clone () from /lib/tls/i686/cmov/libc.so.6

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: test file that causes crash

if the previous test case doesn't immediately crash firefox, try this one

Comment 3

[Daniel Veditz [:dveditz]]

This is a Read AV, exploitability would depend on what we're going to do with the data. If its just jumbled video that's probably OK, if we're reading it into a structure that might later be used as pointers or buffer sizes that isn't (assuming the attacker can control the junk you're copying from).

eax=11fa1ba5 ebx=11fa16a5 ecx=00000140 edx=00000000 esi=11fa16a5 edi=1e200030
eip=7814838a esp=056ffd10 ebp=056ffd18 iopl=0         nv up ei pl nz ac po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010212
MOZCRT19!memcpy+0x5a:
7814838a f3a5            rep movs dword ptr es:[edi],dword ptr [esi] es:0023:1e200030=00000000 ds:0023:11fa16a5=????????
0:020> !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: 0x11fa16a5
First Chance Exception Type: STATUS_ACCESS_VIOLATION (0xC0000005)
Exception Sub-Type: Read Access Violation

Faulting Instruction:7814838a rep movs dword ptr es:[edi],dword ptr [esi]

Exception Hash (Major/Minor): 0x6216665a.0x5b20632d

Stack Trace:
MOZCRT19!memcpy+0x5a
xul!JSD_DebuggerOnForUser+0x3331
xul!JSD_DebuggerOn+0x4900
xul!NS_LogAddRef_P+0x4f24
xul!gfxFontTestStore::gfxFontTestStore+0x349f
xul!gfxFontTestStore::gfxFontTestStore+0x3fc0
xul!gfxWindowsFontGroup::operator=+0xa57
xul!gfxContext::GetFlags+0xac6c
Instruction Address: 0x7814838a

Description: Read Access Violation on Block Data Move
Short Description: ReadAVonBlockMove
Exploitability Classification: PROBABLY_EXPLOITABLE
Recommended Bug Title: Probably Exploitable - Read Access Violation on Block Data Move starting at MOZCRT19!memcpy+0x5a (Hash=0x6216665a.0x5b20632d)

This is a read access violation in a block data move, and is therefore classified as probably exploitable.

Comment 4

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

Bug poke: is this being worked on?

Comment 6

[Matthew Gregan [:kinetik]]

So, this is a variant of the bug fixed in bug 500311.  The first time we call theora_decode_packetin, th_decode_packetin returns OC_DUPFRAME.  theora_decode_packetin ignores the result and returns 0.  oggplay_callback_theora then calls theora_decode_YUVout, which returns an all-zeros (not sure if this is guaranteed, or if we're just reading uninitialized memory here) yuv_buffer.  oggplay_data_handle_theora_frame then crashes trying to memcpy with a null source pointer.

Comment 7

[Matthew Gregan [:kinetik]]

In bug 500311, we crashed because the decoding resumed in a random place in the middle of the stream.

Here, the stream is reset to 0 due to the call to oggplay_get_duration in LoadOggHeaders.  Then, DecodeFrame calls oggplay_step_decoding, which resumes decoding from the first packet.  The packet is identified as a header in oggplay_callback_theora and passed to theora_decode_header which returns OC_BADPACKET because the decoder has already been initialized.

oggplay_callback_theora returns OGGZ_STOP_ERR, which eventually causes oggz_read to return OGGZ_ERR_STOP_ERR.  oggplay_step_decoding ignores this (as it only checks for r == 0 and r == OGGZ_ERR_HOLE_IN_DATA), and calls oggz_read again.

As the first three packets are passed to the decoder again, the first two are rejected by theora_decode_header and oggplay_callback_theora returns an error as described above.  The third is not identified as a header by theora_packet_isheader (as it is a 0 byte packet, not the same third packet as the first time through), and is passed to theora_decode_packetin (and th_decode_packetin which returns OC_DUPFRAME, which is then swallowed by the caller and 0 is returned).

Stream is theora only, packets are:
-00:00:00.008: serialno 0855307237, granulepos 0|0, packetno 0 *** bos: 42 bytes
-00:00:00.008: serialno 0855307237, calc. gpos 0|0, packetno 1: 47 bytes
-00:00:00.008: serialno 0855307237, granulepos 0|0, packetno 2: 2.552 kB
-00:00:00.008: serialno 0855307237, calc. gpos 0|0, packetno 4: 0 bytes

Note that packetno 3 is missing.

So:
- should oggplay_step_decoding ignore that error?
- where does the 0 byte third packet come from?  are we processing packets 0, 1, and then 4 the second time?

Comment 8

[Matthew Gregan [:kinetik]]

cpearce mentioned that this might be fixed by the initialization changes in bug 512328 (update liboggplay), but it still crashes the same way.  Curiously, the issue where packetno==2 is not presented to oggplay_callback_theora has gone away.

Thinking about it, looking at the stream dump, and ignoring the error handling/packet reprocessing stuff... the first non-header packet that's going to be processed is packetno==4, which is zero bytes, and thus will be treated by Theora as a duplicate frame.  Since there's no previous frame data to fall back on, we're going to get either an all-zeroes or uninitialized buffer.  The old pre 1.0 Theora API also doesn't report the packet is a duplicate frame:

  ret=th_decode_packetin(api->decode,_op,&gp);
  if(ret<0)return OC_BADPACKET;
  _td->granulepos=gp;
  return 0;

th_decode_packetin returns TH_DUPFRAME, but instead of the old API wrapper returning OC_DUPFRAME, it returns 0.  So it might be difficult to detect this case correctly.

Comment 9

[Matthew Gregan [:kinetik]]

th_decode_packetin reports a duplicate frame when _op->bytes == 0.

oggplay could perform the same check after calling theora_decode_packetin (it's necessary to call this so the internal decoder state is kept up to date).  It also needs a flag in the OggPlayTheoraDecode instance to track when the first valid frame is seen, to deal with the case where there is a sequence of 0 byte packets before a valid frame is seen.

I'm not sure if it makes sense to do this and try to play invalid files, or to reject them outright when we detect this case.  If we try to play them, the attached file doesn't crash, but also doesn't fire an ended event (probably because there are no valid frames).  Also, do we need to be careful to only call the decoder once we see the first valid *intra* frame?  If so, we may have a similar bug when a stream begins with an inter frame...

Comment 10

[Matthew Gregan [:kinetik]]

Attachment: theora 1.0 patch v0

Discussed this with Timothy Terriberry and Gregory Maxwell in #theora and determined that the Theora decoder should be treating these zero byte packets like other inter frames that are decoded before an intra frame has been seen.  In this case, the decoder initializes the frame buffer with solid gray.

Timothy has committed a fix to Theora SVN as revision r16557.  Attached patch is a trivial backport of that to our in-tree Theora 1.0 source.

Comment 11

[Matthew Gregan [:kinetik]]

Attachment: patch v0

Same patch, but with README_MOZILLA and update.sh updated, and with a crashtest included.

Comment 12

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/mozilla-central/rev/52f06b726ca5

Comment 13

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

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

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 400690 [details] [diff] [review]
patch v0

Approved for 1.9.1.5, a=dveditz for release-drivers

Should we upgrade Theora as we have done with other media libraries in 1.9.1.4, or is this one patch all we need?

Comment 15

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

Pushed to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/982600e5e62a

Comment 16

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

(In reply to comment #14)
> (From update of attachment 400690 [details] [diff] [review])
> Approved for 1.9.1.5, a=dveditz for release-drivers
> 
> Should we upgrade Theora as we have done with other media libraries in 1.9.1.4,
> or is this one patch all we need?

I think we don't need libtheora 1.1. It's a large patch, and we're a bit unsure about taking it because of that. We also don't think we need it in order to prevent these "crash with large frame size" bugs, as thanks to bug 504843's patch, we abort decoder initialization if we encounter excessively large theora frame sizes before initializing the frame decoder. Provided the decode of the header packets is safe (and it seems to be, we don't crash in the test files), we don't need the libtheora 1.1 update.

dveditz: does that satisfy you? We can mark bug 515882 as fixed-1.9.1 as well, since the frame size integers can't be big enough to overflow thanks to bug 504843's patch.

Comment 17

[Daniel Veditz [:dveditz]]

That makes sense to me.

Comment 18

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

Verified fix using nightly 1.9.1 build for 11/9 using attached testcases.