Closed Bug 498815 Opened 13 years ago Closed 13 years ago

Crash in oggplay_data_handle_theora_frame

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
blocker

Tracking

()

VERIFIED FIXED

People

(Reporter: ladamski, Assigned: cajbir)

Details

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

Attachments

(11 files, 2 obsolete files)

20.00 KB, video/ogg
Details
456 bytes, application/octet-stream
Details
471 bytes, application/octet-stream
Details
Fix
4.31 KB, patch
roc
: review+
Details | Diff | Splinter Review
20.00 KB, video/ogg
Details
20.00 KB, video/ogg
Details
2.86 KB, patch
wiking
: review+
Details | Diff | Splinter Review
20.00 KB, video/ogg
Details
20.00 KB, video/ogg
Details
20.00 KB, video/ogg
Details
11.33 KB, text/x-c++src
Details
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).
Flags: blocking1.9.1?
Whiteboard: [sg:critical]
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?
Assignee: nobody → chris.double
Summary: Crash → Crash in oggplay_data_handle_theora_frame
Attached patch Patch from Tim (obsolete) — Splinter Review
Patch from Tim, libtheora maintainer. This fixes the crash when tested on oggplayer. Testing on Firefox now.
Attached patch FixSplinter Review
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.
Attachment #383628 - Attachment is obsolete: true
Keywords: crash, testcase
Flags: wanted1.9.1.x+
Flags: wanted1.9.0.x-
Flags: wanted1.8.1.x-
Whiteboard: [sg:critical] → [sg:critical][needs landing]
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?
Flags: blocking1.9.1? → blocking1.9.1+
Patch written by tterribe@vt.edu. Reviewed by me. I added the README_MOZILLA changes.
Attached video another repro
Attached video one final repro
Files in comment 9 and comment 10 are also fixed by patch in comment 6.
Further testing shows a 100% cpu peg sometimes - I'm looking into it.
Whiteboard: [sg:critical][needs landing] → [sg:critical]
Attached patch Fix for 100% cpuSplinter Review
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.
Status: NEW → ASSIGNED
Comment on attachment 383629 [details] [diff] [review]
Fix

This patch is from Tim and is as reviewed as it can be
Attachment #383629 - Flags: review+
Attachment #383698 - Flags: review?(wiking)
(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.
Attachment #383698 - Flags: review?(wiking) → review+
Comment on attachment 383698 [details] [diff] [review]
Fix for 100% cpu

It's a definite go just as attachment  383705 [details] [diff] [review].
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.
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.
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".
Attachment #383860 - Attachment is obsolete: true
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.
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/c5e23f5c7e7b
http://hg.mozilla.org/mozilla-central/rev/e83515363df8
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical] → [sg:critical][baking for 1.9.1]
Whiteboard: [sg:critical][baking for 1.9.1] → [sg:critical]
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
Status: RESOLVED → VERIFIED
Group: core-security
You need to log in before you can comment on or make changes to this bug.