Closed
Bug 1117571
Opened 6 years ago
Closed 6 years ago
Fix crash on corrupt Vorbis file (invalid mode index)
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: derf, Assigned: derf)
References
()
Details
Attachments
(1 file, 1 obsolete file)
4.48 KB,
patch
|
derf
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=48f1dfc76917
OS: Linux → All
Hardware: x86_64 → All
Updated•6 years ago
|
Attachment #8543682 -
Flags: review?(kinetik) → review+
Comment 3•6 years ago
|
||
It'd be good to add a file that triggers this to dom/media/crashtests.
Assignee | ||
Comment 4•6 years ago
|
||
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+
Assignee | ||
Updated•6 years ago
|
Whiteboard: checkin-needed
Updated•6 years ago
|
Flags: in-testsuite?
Updated•6 years ago
|
Whiteboard: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5c86509a11bc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Assignee | ||
Comment 7•6 years ago
|
||
(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.
Assignee | ||
Comment 8•6 years ago
|
||
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
Comment 9•6 years ago
|
||
Thanks for investigating so thoroughly!
You need to log in
before you can comment on or make changes to this bug.
Description
•