Closed Bug 1117571 Opened 5 years ago Closed 5 years ago

Fix crash on corrupt Vorbis file (invalid mode index)

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: derf, Assigned: derf)

References

()

Details

Attachments

(1 file, 1 obsolete file)

vorbis_packet_blocksize() crashes with a NULL pointer dereference, if the "mode" index read from the packet is too large. Analysis confirms that thanks to static array sizes and calloc, this is a clean NULL deref (not a potential OOB read). We should check this value immediately after reading it and before accessing the mode parameters.

Upstream bug: https://trac.xiph.org/ticket/2140
Debian bug: https://bugs.debian.org/774516

A file that triggers the crash is in the URL field.
Assignee: nobody → tterribe
Status: NEW → ASSIGNED
Attachment #8543682 - Flags: review?(kinetik)
Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=48f1dfc76917
OS: Linux → All
Hardware: x86_64 → All
Attachment #8543682 - Flags: review?(kinetik) → review+
It'd be good to add a file that triggers this to dom/media/crashtests.
Updated patch with the bug number in the commit message, carrying forward r=kinetik

I'll see about adding something to the crashtests in a separate patch when I'm not in an airport/train station (almost out of battery).
Attachment #8543682 - Attachment is obsolete: true
Attachment #8543712 - Flags: review+
Whiteboard: checkin-needed
Flags: in-testsuite?
Whiteboard: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5c86509a11bc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
(In reply to Timothy B. Terriberry (:derf) from comment #4)
> I'll see about adding something to the crashtests in a separate patch when
> I'm not in an airport/train station (almost out of battery).

Unfortunately, we can't use the file from the Debian bug report, because it works by feeding libvorbis two comment headers instead of a comment header and a setup header. libvorbisfile is dumb enough to count both towards the requisite number of headers at the start of the file and proceed with empty codebooks and mode tables. Firefox is smart enough to insist on getting something actually tagged as a proper setup header.

That's not enough to avoid the crash reported here in all cases, but it does mean we need to generate a better test file, which is a bit of work.
So, further testing reveals that another bug prevents this crash from ever occurring in Firefox. vorbis_packet_blocksize() has an inline implementation of ilog2() that actually computes floor(log2(ci->modes)) rather than floor(log2(ci->modes-1))+1 (the latter being the number of bits required to store integers less than ci->modes).

That's a spec violation that breaks any stream where ci->modes is not a power of two. We read too few bits for the mode index in that case, which in this case nicely prevents us from ever reading an index that's too large. No existing encoder has ever produced such a stream (that I know of). The only exception is when ci->modes is 0 (the cause of the crash in the Debian bug report), but that can't happen if we actually pass headerin a setup packet: either it will initialize ci->modes to a positive value or fail. Since Firefox ensures we pass a setup packet in the Ogg code path, and never calls vorbis_packet_blocksize() in the WebM code path, it is not vulnerable to the crash.

So I think at this point the best thing to do is sort this out upstream, and take fixes for the spec violation the next time we import a new version of libvorbis.
Flags: in-testsuite? → in-testsuite-
Keywords: crash
Thanks for investigating so thoroughly!
You need to log in before you can comment on or make changes to this bug.