498815 - Crash in oggplay_data_handle_theora_frame

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Lucas Adamski [:ladamski]]

Likely exploitable issue in Theora at memcpy below.  Destination address (q) appears to be under attacker control via decode->video_info.offset_y == 4287758336

  p = data->y;
  q = buffer->y + (decode->video_info.offset_x&~1)+buffer->y_stride*(decode->video_info.offset_y&~1);
  for (i = 0; i < decode->y_height; i++) {
    memcpy(p, q, decode->y_width);
    p += decode->y_width;
    q += buffer->y_stride;
  }

#0	0x120bbbb7 in oggplay_data_handle_theora_frame at oggplay_data.c:365
#1	0x120ba48b in oggplay_callback_theora at oggplay_callback.c:201
#2	0x120c805e in oggz_read_sync at oggz_read.c:483
#3	0x120c8469 in oggz_read at oggz_read.c:606
#4	0x120b9ba5 in oggplay_step_decoding at oggplay.c:691
#5	0x120a6ffc in nsOggDecodeStateMachine::DecodeFrame at nsOggDecoder.cpp:732
#6	0x120ac693 in nsOggDecodeStateMachine::Run at nsOggDecoder.cpp:1428
#7	0x005759e4 in nsThread::ProcessNextEvent at nsThread.cpp:510
#8	0x004fe968 in NS_ProcessNextEvent_P at nsThreadUtils.cpp:227
#9	0x00575bf3 in nsThread::ThreadFunc at nsThread.cpp:254
#10	0x00728465 in _pt_root at ptthread.c:228
#11	0x96291155 in _pthread_start
#12	0x96291012 in thread_start

This is a result of fuzzing after I disabled some page checksum checks in libogg.  Note that the checksums don't affect exploitability, they just made fuzzing difficult since the fuzzer isn't smart enough to fix checksums after munging data.  I will attach repro but you will need to build with CRC checks disabled (will attach patches to do so in a few).

Comment 1

[Lucas Adamski [:ladamski]]

Attachment: crash repro... if you have CRC checks disabled

Comment 2

[cajbir (:cajbir)]

I can reproduce this in the standalone liboggplay based 'oggplayer' program with the latest liboggplay library once the checksum in libogg is disabled. Viktor, can you take a look?

Comment 3

[Lucas Adamski [:ladamski]]

Attachment: simple patch to disable CRC check for repro purposes (1 of 2)

Comment 4

[Lucas Adamski [:ladamski]]

Attachment: patch to disable CRC checks (2 of 2)

Comment 5

[cajbir (:cajbir)]

Attachment: Patch from Tim

Patch from Tim, libtheora maintainer. This fixes the crash when tested on oggplayer. Testing on Firefox now.

Comment 6

[cajbir (:cajbir)]

Attachment: Fix

Patch from Tim applied to our tree with updates to README_MOZILLA, etc. This stops the crash with this file when the checksum patches are applied.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

No review?

As per shaver, this is blocking 3.5 final. How do we land it on trunk/1.9.1 without zero-daying ourselves? Perhaps mask with the patch from bug 493936?

Comment 8

[cajbir (:cajbir)]

Patch written by [e-mail]. Reviewed by me. I added the README_MOZILLA changes.

Comment 9

[Lucas Adamski [:ladamski]]

Attachment: another repro

Comment 10

[Lucas Adamski [:ladamski]]

Attachment: one final repro

Comment 11

[cajbir (:cajbir)]

Files in comment 9 and comment 10 are also fixed by patch in comment 6.

Comment 12

[cajbir (:cajbir)]

Further testing shows a 100% cpu peg sometimes - I'm looking into it.

Comment 13

[cajbir (:cajbir)]

Attachment: Fix for 100% cpu

The previous patch fixes the libtheora side of things. Unfortunately liboggplay drops the return code from libtheora. This patch fixes liboggplay to propogate the return code down.

Both this and attachment 383629 [details] [diff] [review] are needed to fix this bug.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Do we need review on the patch in comment #13?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh, is the patch in comment #13 the same as in https://bugzilla.mozilla.org/show_bug.cgi?id=498824#c14, which Viktor approved?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 383629 [details] [diff] [review]
Fix

This patch is from Tim and is as reviewed as it can be

Comment 17

[Viktor Gal]

(In reply to comment #15)
> Oh, is the patch in comment #13 the same as in
> https://bugzilla.mozilla.org/show_bug.cgi?id=498824#c14, which Viktor approved?

no it's not. attachment 383705 [details] [diff] [review] and attachment 383698 [details] [diff] [review] are both required.

Comment 18

[Viktor Gal]

Comment on attachment 383698 [details] [diff] [review]
Fix for 100% cpu

It's a definite go just as attachment  383705 [details] [diff] [review].

Comment 19

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

Attachment: Video 1 with CRC correct - crashes browser

Comment 20

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

Attachment: Video 2 with CRC correct - crashes browser

Comment 21

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

Attachment: Video 3 with CRC correct - crashes browser

Comment 22

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

Attachment: Program to correct CRC checksums in fuzzed Ogg files

This C++ program reads Ogg files and corrects Ogg page CRC checksums so that they correspond to the data in the page. This means you can generate "bad" ogg files which have correct CRC checksums - so you can run the "CRC is valid" code paths. Note this program won't correct bad page layouts, e.g. if the header is invalid, it won't fix such files. 

To be clear, the 3 videos I just uploaded were the 3 test files provided by Lucas with CRC checksums corrected. So when you open them in (unpatched) FF you will crash.

Comment 23

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

To be even more clear, with the libtheora fix and liboggplay fix posted here applied, we don't crash when loading the three "Video X with CRC correct" files, but without those two patches applied we do crash. No need to apply the "disable CRC checks patch" to reproduce the crashes.

Comment 24

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

Attachment: Program to correct CRC checksums in fuzzed Ogg files - v2

Similar to previous program to fix CRC checksum in Ogg files, but this one handles files with missing capture patterns - when it misses a capture pattern where it was expecting to get one, it will search forward for the next ogg page capture pattern, similar to how (I assume) the ogg libraries must work. This program can now convert the Ogg files in bug 498824, bug 498853 and bug 498855 into files that crash FF without the need of the "disable CRC check patch".

Comment 25

[Matthew Gregan [:kinetik]]

For anyone playing along at home, the liboggplay part of this patch won't apply cleanly to trunk because bug 496684 landed and causes a couple of rejects.

Comment 26

[cajbir (:cajbir)]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/c5e23f5c7e7b
http://hg.mozilla.org/mozilla-central/rev/e83515363df8

Comment 27

[cajbir (:cajbir)]

Pushed to mozilla-1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/596410de934a
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e1286eba84f6

Comment 28

[Aaron Train [:aaronmt]]

Verified FIXED on latest 1.9.1 branch

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090623 Shiretoko/3.5pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090623 Shiretoko/3.5pre

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1pre) Gecko/20090623 Shiretoko/3.5pre