Created attachment 391362 [details] test file that causes crash User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:184.108.40.206) Gecko/2009070811 Ubuntu/9.04 (jaunty) Firefox/3.0.12 Build Identifier: mozilla-central revision bf4d4c37361c Segmentation fault when playing corrupted ogg vorbis 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
Hmm, this file also crashes a stand alone ogg program I'm writing, it looks like a bug in libvorbis, not libfishsound. It's crashing in vorbis_synthesis_init() after the last header packet is read, Monty, can you take a look?
Interestingly it doesn't crash the vorbisfile_decode example in libvorbis.
I checked out http://svn.xiph.org/trunk/vorbis and used that, and I still crash, so this isn't fixed (on Windows at least) by the latest trunk. The decoder_example program crashed for me with vorbis trunk, I'm using libogg 1.1.4 stable, on Windows.
decoder_example segfaults for me. vorbisfile_example doesn't.
Bug poke: is this being worked on?
Fixed by bug 512328.
Fixed on trunk and 1.9.2 by bug 512328 checkin.
This was *not* fixed on 1.9.2 or 1.9.1 by the ogg libraries update. It is fixed on trunk though.
This is not fixed in trunk nightlies either! For some reason this is fixed in my local opt build from 2009-09-27, but not in a local debug or any other nightly build. This appearing fixed in my local build is probably why I thought it was fixed by the ogg libraries update in the first place. Reopening. :(
Created attachment 403951 [details] [diff] [review] Patch v1 We worked with the vorbis developers, and they commited r16957 upstream to fix this. Here's that patch cherry picked. It's a simple 1 line fix which adds input validation to the vorbis header decoding. We don't crash loading the test file on trunk with this patch.
Need to add the test file as a test?
(In reply to comment #13) > Need to add the test file as a test? I'm never sure whether to includes test cases in security patches for fear of exposing some exploit to the bad guys. Under what conditions should we not include test cases? When the bug is exploitable? How can I tell? This test file causes stuff on the stack to be overwritten, but I'm unsure if it's exploitable...
okay, let's make it in-testsuite? and check in the test later.
Rather than take this as a cherrypicked patch, I think we should just update to libvorbis SVN. There's another fix we want for trunk (and should take on 1.9.2 and 1.9.1) in bug 518941, which also cherrypicks the patch from SVN... but these two changes are the only significant changes in SVN since we took the last update. So I think it'd keep things simpler to sync with SVN again rather than carrying more patches. Opinions?
Created attachment 404188 [details] [diff] [review] patch v2 Update libvorbis to SVN r16597. Fixes this bug and bug 518941. We want this on all three branches.
OK, go for it on the 1.9.1 branch.(In reply to comment #14) > (In reply to comment #13) > I'm never sure whether to includes test cases in security patches for fear of > exposing some exploit to the bad guys. Under what conditions should we not > include test cases? When the bug is exploitable? We've 0day'd ourselves by checking in security bug regression tests on the trunk in the past (a security bug that turned WFM w/out investigation what fixed it, but with the release branches still vulnerable). Check in the test when the bug is fixed on all supported branches that are vulnerable. Preferably request approval on the old branches when you've made the fix, you don't need to wait for the branch drivers to notice and flag the bug as "wanted". If you don't know if the old branches are vulnerable then assume they are until you've tested. > How can I tell? This test file causes stuff on the stack to be > overwritten, but I'm unsure if it's exploitable... Assume it is exploitable. It's not worth the effort to prove that in a given specific instance there are enough mitigations to make it safe.
Comment on attachment 404188 [details] [diff] [review] patch v2 We'll have to pick up this upgrade in 220.127.116.11
Comment on attachment 404188 [details] [diff] [review] patch v2 not particularly happy rushing this in at the last minute, but it's safer than not fixing it given the upstream's "exploit me here" comment :-( Approved for 18.104.22.168, a=dveditz for release-drivers
Verified fixed on trunk and 1.9.2 with builds on all platforms like: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091006 Minefield/3.7a1pre ID:20091006031035 Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b1pre) Gecko/20091006 Namoroka/3.6b1pre ID:20091006034008
Verified fixed on 1.9.1 with builds on Windows and OS X like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:22.214.171.124) Gecko/20091006 Firefox/3.5.4 ID:20091006224018