1117571 - Fix crash on corrupt Vorbis file (invalid mode index)

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Timothy B. Terriberry (:derf)]

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.

Comment 1

[Timothy B. Terriberry (:derf)]

Attachment: Check mode index in vorbis_packet_blocksize()

Comment 2

[Timothy B. Terriberry (:derf)]

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=48f1dfc76917

Comment 3

[Matthew Gregan [:kinetik]]

It'd be good to add a file that triggers this to dom/media/crashtests.

Comment 4

[Timothy B. Terriberry (:derf)]

Attachment: Check mode index in vorbis_packet_blocksize() v2

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).

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c86509a11bc

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5c86509a11bc

Comment 7

[Timothy B. Terriberry (:derf)]

(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.

Comment 8

[Timothy B. Terriberry (:derf)]

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.

Comment 9

[Matthew Gregan [:kinetik]]

Thanks for investigating so thoroughly!