Created attachment 385938 [details]
#0 res0_unpack at vorbis_res0.c:225
#1 _vorbis_unpack_books at vorbis_info.c:313
#2 vorbis_synthesis_headerin at vorbis_info.c:425
#3 fs_vorbis_decode at fishsound_vorbis.c:131
#4 fish_sound_decode at fishsound_decode.c:117
#5 oggplay_callback_audio at oggplay_callback.c:392
#6 oggz_read_sync at oggz_read.c:483
#7 oggz_read at oggz_read.c:606
#8 oggplay_initialise at oggplay.c:122
225 if(ci->book_param[info->booklist[j]]->maptype==0)goto errout;
(gdb) p j
$6 = 0
(gdb) p info->booklist[j]
$7 = -1
(gdb) p ci->book_param[info->booklist[j]]
$8 = (static_codebook *) 0x0
Crashes trunk with the Vorbis update from bug 500254 applied. Also crashes decoder_example from Vorbis SVN trunk.
Fixed in Subversion r16218 upstream, but it sounds like Monty plans to conduct a pattern review for this, so we should probably wait before picking up a new version.
Yes, pattern review plus a new release soon. There's also a small-file open bug that Mozilla doesn't care about but is important for gaming developers, so there will be a 1.2.3 shortly.
Not going to block 18.104.22.168 for this, but we should take this when we can.
the pattern review was completed and 1.2.3 released several days ago.
[12:06] <CIA-28> xiphmont * r16326 vorbis/lib/backends.h: Eliminate possibility of booklist overflow in res0/1/2 unpacking.
[12:07] <derf> cpearce: You guys may be interested in that r16326 commit.
This must be the result of Monty's pattern review which Matthew mentioned in comment #1.
Unfortunately, this was en entirely different bug due to a different pattern error.
That was caught by me trying to answer my own questions about how the code worked for https://bugzilla.mozilla.org/show_bug.cgi?id=506094. The issue, is, however, entirely unrelated to the issue found by that file (or this file), and should be tracked as a separate bug. A fuzz tester would've had to get almost 200 bits in a row exactly right to trigger it, but a sentient attacker would've had no trouble producing a heap overflow, writing up to 128 words past the end of a block with the low 8 bits set to attacker-chosen values (and the high bits cleared).
Created attachment 391795 [details] [diff] [review]
Update libvorbis to r16335.
Created attachment 392431 [details] [diff] [review]
Patch - add test
Adds test that ensures that the testcase doesn't load, and throws an error. Also includes the testcase from bug 504644 and bug 506094 as well, as this bug fixes those bug's testcase as too. This is rebased on the testcase patches for bug 500311, bug 498855 and bug 499519.
Pushed patch with testcase to m-c:
Code freeze for 22.214.171.124 is Tuesday Aug 11, could this be gotten in by then? If so please request approval on the patch.
I'm guessing the answer is "no". We'll have to get this for 126.96.36.199...
Chris Pearce is currently away. The patch is an update to libvorbis - 500Kb or so. In a previous point release it was suggested to cherry pick the actual change to fix the bug since people were nervous about approving a 500Kb patch. Is that still the case?
I think we're "okay" with taking an update to libvorbis if it's well-baked/tested and lands right when the 1.9.1 tree opens. Ideally we'd cherry pick all fixes, but I'd hate to miss some fixes because we didn't take a full update. Likewise, there might be ogg changes we *want* but aren't security issues in such an update.
The tree has been open for 188.8.131.52, in fact code freeze is in a week (Sept 22). Whatever we're going to do we need to do ASAP.
Yes, we're nervous about a 600K patch, but can you safely cherry pick out the fixes for this bug, the two that were duped to it, and some of the new ones that have come in recently? (e.g. bug 515889)
The patch is large due to white space and formatting changes in the Vorbis library. It has been trunk for about 6 weeks now without problems. I think it is safer to land this patch, which is known to work, on 184.108.40.206 than to cherry-pick individual commits and hope that we don't break Vorbis somehow.
Comment on attachment 391795 [details] [diff] [review]
Approved for 220.127.116.11, a=dveditz for release-drivers
So, the bad news is that this patch depends on the earlier libvorbis update from bug 500254. We cherrypicked the necessary bits for the 1.9.1 branch, which is why this patch won't apply to 1.9.1.
If we want this on 1.9.1, then I think the best course of action is to back out the cherrypicked fixes on 1.9.1 and apply the complete patches from bug 500254 and here.
That's fine -- the approval granted here was on the assumption that we'd end up matching a particular upstream version of the library.
Verified for 18.104.22.168 using testcase with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:22.214.171.124pre) Gecko/20090924 Shiretoko/3.5.4pre. Crashes 126.96.36.199 easily.