Bug 1080995 (CVE-2015-0797)

heap-buffer-overflow (read of size 0xffffffff) when playing a m4v video (GStreamer)

VERIFIED FIXED in Firefox 38

Status

()

defect
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: aki.helin, Assigned: rillian)

Tracking

(Depends on 1 bug, 5 keywords)

unspecified
mozilla40
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox34 wontfix, firefox35+ wontfix, firefox36+ wontfix, firefox37+ wontfix, firefox38+ fixed, firefox39+ fixed, firefox40 verified, firefox-esr3138+ verified, firefox-esr38 verified, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-master unaffected)

Details

(Whiteboard: [adv-main38+][adv-esr31.7+])

Attachments

(3 attachments, 5 obsolete attachments)

Posted video ff-bigbof.m4v
ASan reports the buffer overflow when the attached video is played. Current normal build crashes at null. The size suggests that it could be static and guaranteed to just crash, but worth checking how this ends up happening. Filing as a security bug to be on the safe side.

==13165==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61d000b4da6f at pc 0x43b8be bp 0x7f73e11dd95
0 sp 0x7f73e11dd108
READ of size 4294967295 at 0x61d000b4da6f thread T48 (multiqueue1:src)
    #0 0x43b8bd in __interceptor_memcpy _asan_rtl_
    #1 0x7f73e07cfa05 in ?? ??:0
    #2 0x7f73e07cfd3c in ?? ??:0
[...]
Thread T48 (multiqueue1:src) created by T46 (qtdemux0:sink) here:
    #0 0x425e86 in __interceptor_pthread_create _asan_rtl_
    #1 0x7f740f4bc92e in ?? ??:0
This seems to crash all over place in normal Firefox builds for me, from 0x7f1d63800000 in libc and 0x0 in EnterBaseline to 0x7f419c8a8f12 in Interpret.

https://crash-stats.mozilla.com/report/index/7c44fabb-3902-4bc0-ae73-2e9c62141013
https://crash-stats.mozilla.com/report/index/aece920f-aded-43b5-8190-0f05c2141013
https://crash-stats.mozilla.com/report/index/8a9d19f6-3f9e-49b7-997f-eb57f2141013
Anthony, do you know who could look at this?  Thanks.

Maybe we're corrupting some JS memory or something, with that crashing all over the place.
Flags: needinfo?(ajones)
Ralph - can I get you to look into this one?
Flags: needinfo?(ajones) → needinfo?(giles)
I can reproduce a crash on linux nightly from friday. My local debug build reports a crash in gstvideoparsersbad:

#03: ???[/lib64/libc.so.6 +0x93cae]
#04: ???[/usr/lib64/gstreamer-0.10/libgstvideoparsersbad.so +0x8565]
#05: ???[/usr/lib64/gstreamer-0.10/libgstvideoparsersbad.so +0x898a]
#06: ???[/usr/lib64/gstreamer-0.10/libgstvideoparsersbad.so +0x915e]
#07: gst_pad_push[/lib64/libgstreamer-0.10.so.0 +0x5c4c1]
#08: ???[/usr/lib64/gstreamer-0.10/libgstcoreelements.so +0x1e081]
#09: ???[/lib64/libgstreamer-0.10.so.0 +0x82394]

Can reproduce a crash or memory-corruption with gst-launch.
Assignee: nobody → giles
Flags: needinfo?(giles)
Does it reproduce on gstreamer-1.0?
gst-launch-1.0 doesn't seem to crash.

Mike - how do you feel about dropping gstreamer-0.10 support?
Flags: needinfo?(mh+mozilla)
Things to consider:
- We currently can't build gstreamer 1.0 support on the build slaves because they don't have the right -devel packages for that
- We currently can't test gstreamer 1.0 support on the test slaves because they run on Ubuntu 12.04, which only has gstreamer 0.10 support (although, I don't know if we actually have tests for gstreamer)
- At least Ubuntu 12.04 and Debian wheezy come without gstreamer 1.0 support.
Flags: needinfo?(mh+mozilla)
Ralph - can you take a look at where it crashes in gstreamer and see if it is exploitable?
The crashes look like various memory corruption, let's just assume exploitable.

Is it possible to limit what versions of GStreamer we use, similar to the way we block outdated plugins?
Keywords: sec-high, sec-vector
Summary: heap-buffer-overflow (read of size 0xffffffff) when playing a m4v video → heap-buffer-overflow (read of size 0xffffffff) when playing a m4v video (GStreamer)
see comment #7 for the issues with limiting by version. I think our best bet is to get distros shipping gstreamer-plugins-good 0.10 to push a patch.
I can reproduce with:

  gst-launch filesrc location=ff-bigbof.m4v ! qtdemux ! h264parse ! fakesink

built from the most recent tags on the 0.10 branch. The actual crash is in h264parse, where it fails to check for overflow when doing arithmetic with the nal unit size. Here's a quick patch against gstreamer-plugins-bad to (a) crash instead of allocating the wrong size buffer (b) reject large nal units (I picked 20 MB arbitrarily) up front.

There are lots of overflows throughout this code though. :(
Ok, how should we proceed here?

We can:

1) ignore the problem.
2) forward the patch upstream to distros and gstreamer, request they push sec updates.
3) blacklist the h264dec element. fluh264dec seems to work fine without it; not sure about other decoders.
4) blacklist 0.10 versions, or remove support entirely (breaks playback on older linux installs).

Do we have established procedures for 2?
Flags: needinfo?(dveditz)
I'm told on irc ffdec_h264 also works without it, so 3 might be viable.
Discussion with :k17e produced the following proposal:

- Add a pref with a list of blacklisted gstreamer elements.
- Blacklist h264parse for gstreamer-0.10 by default.
- Forward attached gstreamer fix to distros.
- Distros shipping Firefox can flip the pref back when it's safe to do so for their users.

In parallel:

- Fix the pending releng bug blocking our switch to gstreamer-1.0.
Here's a quick hack to block autoplugging the buggy h264parse element. Unfortunately it seems to prevent construction of a simpler h264dec section entirely, even though I can connect qtdemux to fluh264dec directly in manual tests, so we'll have to do something more sophisticated.
Group: media-core-security
If we can't fix this then we should disable gstreamer-0.10 in favour of 1.0. The risk of regression seems lower than the risk of this bug being exploitable.
Updated WIP patch with a version-specific blacklist.

With current m-c we instead hit an IsNULL() assert in the timestamp comparison at MediaDecoderStateMachine.cpp:942 wehre mVideoDecodeStartTime is zero. Issue with the async patch?
Attachment #8517813 - Attachment is obsolete: true
Slight update of the patch to fix an off-by-one in the strlen check. This blocks decoding on Aurora, should work for Beta as well.

Nightly asserts on the problem file before it gets to the problem code, so it may no longer be vulnerable. Still investigating.
Attachment #8526978 - Attachment is obsolete: true
Attachment #8528660 - Flags: review?(edwin)
Comment on attachment 8528660 [details] [diff] [review]
blacklist h264parse prior to 0.10.35

Patch works on Nightly as well, which is still vulnerable. I was testing against a build with ffmpeg enabled when I saw the assert.
Attachment #8528660 - Attachment description: blacklist h264parse prior to 0.10.35 for Beta and Aurora → blacklist h264parse prior to 0.10.35
Comment on attachment 8528660 [details] [diff] [review]
blacklist h264parse prior to 0.10.35

Review of attachment 8528660 [details] [diff] [review]:
-----------------------------------------------------------------

nits, but good otherwise.

::: content/media/gstreamer/GStreamerReader.cpp
@@ +1188,5 @@
> +#if GST_VERSION_MAJOR >= 1
> +  GstPlugin* plugin = gst_plugin_feature_get_plugin(GST_PLUGIN_FEATURE(aFactory));
> +  const gchar* version = plugin ? gst_plugin_get_version(plugin) : "unknown";
> +#else
> +  if (!GST_PLUGIN_FEATURE(aFactory)->plugin_name) {

This whole hunk can be moved to its own function... IsFactoryBlacklisted() or something along those lines.

@@ +1191,5 @@
> +#else
> +  if (!GST_PLUGIN_FEATURE(aFactory)->plugin_name) {
> +    return false;
> +  }
> +  const gchar* plugin_name = GST_PLUGIN_FEATURE(aFactory)->plugin_name;

Should you be using |plugin_name| in the name comparison below?

@@ +1193,5 @@
> +    return false;
> +  }
> +  const gchar* plugin_name = GST_PLUGIN_FEATURE(aFactory)->plugin_name;
> +  GstPlugin* plugin = gst_default_registry_find_plugin(plugin_name);
> +  const gchar* version = plugin ? plugin->desc.version: "unknown";

nit: whitespace around colon

@@ +1203,5 @@
> +  // Reject h264parse <= 0.10.23.
> +  const char badname[] = "h264parse";
> +  const char version_base[] = "0.10.";
> +  const size_t version_min = 7;
> +  static_assert(STRING_LENGTH(version_base) - 1 + 2 <= version_min,

It's not clear where the |- 1 + 2| comes from here. Comment or simplify to |+ 1| (and even then, not sure where it comes from...).

@@ +1206,5 @@
> +  const size_t version_min = 7;
> +  static_assert(STRING_LENGTH(version_base) - 1 + 2 <= version_min,
> +                "template version string too long");
> +  if (strlen(name) >= strlen(badname) &&
> +      strcmp(name, badname) == 0 &&

strcmp makes the strlen check above it redundant.
Attachment #8528660 - Flags: review?(edwin) → review+
Updated patch addressing review comments. Carrying forward r=edwin.


(In reply to Edwin Flores [:eflores] [:edwin] from comment #20)

> This whole hunk can be moved to its own function... IsFactoryBlacklisted()
> or something along those lines.

Good idea.

> Should you be using |plugin_name| in the name comparison below?

No. |plugin_name| is the name of the shared library containing the element we care about. I.e. it's 'gstpluginsbad' not 'h264parse'. A plugin can contain multiple element implementations, so it's the element name in |name| we want to compare against.

We just need the plugin's name to look up the version string on the 0.10 api.

> nit: whitespace around colon

Fixed, thanks.

> > +  static_assert(STRING_LENGTH(version_base) - 1 + 2 <= version_min,
> 
> It's not clear where the |- 1 + 2| comes from here. Comment or simplify to
> |+ 1| (and even then, not sure where it comes from...).

The |- 1| was obsolete, left over from before I added STRING_LENGTH(). The |+ 2| is to make sure there's space for the to tiny revision digits at after the prefix string. I've added a comment and replaced the static assert with calling STRING_LENGTH() directly when initializing version_min.

> strcmp makes the strlen check above it redundant.

I guess so. Removed.
Attachment #8528660 - Attachment is obsolete: true
[Tracking Requested - why for this release]:
Comment on attachment 8530438 [details] [diff] [review]
blacklist h264parse prior to 0.10.35 v2

[Security approval request comment]

> How easily could an exploit be constructed based on the patch?

We read off the end of a buffer, so it's possible there's a way to write file data onto the stack, but it looks like if it doesn't crash it just stirs file data into structures which are already filled in from file data.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Commit mentions blacklisting a component for crashes.

> Which older supported branches are affected by this flaw?

All official builds on Linux systems are vulnerable if the user also has 'gst-plugins-bad' installed on their system.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Patch should apply, modulo path changes, to all supported branches.

> How likely is this patch to cause regressions; how much testing does it need?

This will certainly regress mp4 playback for users on gstreamer 0.10. Playback will start working again if/when their distro ships the other patch attached to this bug.

The alternative here is to disable gstreamer entirely, so I think this is the best we can do. Users of newer distros which build Firefox against gstreamer 1.0 will not be affected with this patch.
Attachment #8530438 - Flags: sec-approval?
Comment on attachment 8508955 [details] [diff] [review]
h264parse-bigbof.patch

This is a patch against third party code we dynamically load if it's on the user's system. Requesting approval to contact the security teams of major Linux distros to ask them to ship the patch as a security update.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

I do mention 'overflow' but it's not obvious how to construct an exploit from that.

> Which older supported branches are affected by this flaw?

gstreamer 0.10 only.

> How likely is this patch to cause regressions; how much testing does it need?

Patch only rejects obviously invalid files, so the risk of regressions is low.
Attachment #8508955 - Flags: sec-approval?
Whiteboard: [checkin approval for 12/16]
Comment on attachment 8530438 [details] [diff] [review]
blacklist h264parse prior to 0.10.35 v2

sec-approval+ for checkin on December 16 or later.

As far as I'm concerned, you can contact the linux distros to fix their own issue but it would be best not to show them the details of the Mozilla end of the flaw for Firefox.
Attachment #8530438 - Flags: sec-approval? → sec-approval+
Attachment #8508955 - Flags: sec-approval? → sec-approval+
Comment on attachment 8530438 [details] [diff] [review]
blacklist h264parse prior to 0.10.35 v2

Approval Request Comment
[Feature/regressing bug #]: Bug 422540
[User impact if declined]: Users will be vulnerable to malicious video files.
[Describe test coverage new/current, TBPL]: Manual testing.
[Risks and why]: This will regress mp4 video playback for many Linux users. Things should start working again once distros update the gstreamer library code which is vulnerable.
[String/UUID change made/needed]: none.

NB: sec-approval for landing 2014 December 16. Asking for landing approval in preparation.
Attachment #8530438 - Flags: approval-mozilla-aurora?
Comment on attachment 8530438 [details] [diff] [review]
blacklist h264parse prior to 0.10.35 v2

Approval Request Comment
[Feature/regressing bug #]: Bug 422540
[User impact if declined]: Users will be vulnerable to malicious video files.
[Describe test coverage new/current, TBPL]: Manual testing.
[Risks and why]: This will regress mp4 video playback for many Linux users. Things should start working again once distros update the gstreamer library code which is vulnerable.
[String/UUID change made/needed]: none.

NB: sec-approval for landing 2014 December 16. Asking for landing approval in preparation.
Attachment #8530438 - Flags: approval-mozilla-beta?
Comment on attachment 8530438 [details] [diff] [review]
blacklist h264parse prior to 0.10.35 v2

Approval Request Comment
[Feature/regressing bug #]: Bug 422540
[User impact if declined]: Users will be vulnerable to malicious video files.
[Describe test coverage new/current, TBPL]: Manual testing.
[Risks and why]: This will regress mp4 video playback for many Linux users. Things should start working again once distros update the gstreamer library code which is vulnerable.
[String/UUID change made/needed]: none.

NB: sec-approval for landing 2014 December 16. Asking for landing approval in preparation.
Attachment #8530438 - Flags: approval-mozilla-esr31?
Comment on attachment 8530438 [details] [diff] [review]
blacklist h264parse prior to 0.10.35 v2

Today is December 16th. Approving.
Attachment #8530438 - Flags: approval-mozilla-esr31?
Attachment #8530438 - Flags: approval-mozilla-esr31+
Attachment #8530438 - Flags: approval-mozilla-beta?
Attachment #8530438 - Flags: approval-mozilla-beta+
Attachment #8530438 - Flags: approval-mozilla-aurora?
Attachment #8530438 - Flags: approval-mozilla-aurora+
I'm going to go out on a limb and guess that bug 973274 will need to be fixed before this can land. Unless we want to disable Linux test coverage for those formats...
Flags: needinfo?(dveditz)
Oops, didn't realize we had positive tests for this. Bug 973274 can't block this; I'll need to disable the tests.
Ralph - can you disable the tests so this can get landed?
Flags: needinfo?(giles)
That's the plan but I won't be able to get to it before next week.
Flags: needinfo?(giles)
Since it's "only" sec-high but also because it was wontfixed for 34 as well, we'll have to wontfix again for 35 and take it up to 36 instead since we're doing our last desktop beta today and this seems like something we shouldn't throw into the RC without any time for user testing.
Attachment #8530438 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
Ouch, so much for that plan. Thanks for the heads up.
Ralph, are you planning to fix the tests for 36? Thanks
Flags: needinfo?(giles)
I haven't gotten a chance to. Given the delay so far, I think targeting 37 might be safer. I'd like to reconsider after I see how invasive the change is.
Flags: needinfo?(giles)
Do you have some time to look at this now?  This bug is about 5 months old now.
Flags: needinfo?(giles)
Sorry, I keep not getting to this and I'm on PTO next week. Edwin, do you mind taking a look? Patch needs rebasing and the issue with tests failing on slaves with the blacklisted h264parse failng addressed. Probably we should just disable the tests.
Flags: needinfo?(giles) → needinfo?(edwin)
We're hoping to ship this in 37, which means we need to fix it within the next week probably.
(In reply to Al Billings [:abillings] from comment #44)
> We're hoping to ship this in 37, which means we need to fix it within the
> next week probably.

Actually, the final 37 Beta build (Beta 7) is tomorrow (Mar 19). At this point we're targeting 38 for the fix.
This is a much simpler patch which uses Edwin's new element filter code to just block the buggy element unconditionally, instead of fiddling with the version. We can remove the block when we drop support for gstreamer 0.10.

I tried porting my version-check code to GStreamerFormatHelper, but querying the version inside the GstPluginFeatureFilter callback deadlocks. This is simpler and probably works just as well.

Avoids the crash and doesn't break vimeo on my linux system with fluendo codes.
Attachment #8530438 - Attachment is obsolete: true
Flags: needinfo?(edwin)
Attachment #8586444 - Flags: review?(kinetik)
Attachment #8586444 - Flags: review?(kinetik) → review+
It looks like gst_plugin_feature_check_version() is available on both 0.10 and 1.x gstreamer releases, so we can try that if we want to pass fixed versions of these elements later.
Fix bug number in commit message. Carrying forward r=kinetik, sec-approval+.
This is simpler than the earlier approved patch which was backed out.
Attachment #8586444 - Attachment is obsolete: true
Comment on attachment 8587053 [details] [diff] [review]
Block h264parse unconditionally v2

Requesting approval to land on 38 and 39 as well. Patch applies cleanly to all versions.

Approval Request Comment
[Feature/regressing bug #]: GStreamer video backend on Linux.
[User impact if declined]: sec-high exploitable by web content.
[Describe test coverage new/current, TreeHerder]: Green on try.
[Risks and why]: The main risk is that this will prevent valid mp4 video from playing on some Linux systems which require this element. I didn't see that in my testing, and given the severity of the problem on long-term-supported systems I think the risk is acceptable.
[String/UUID change made/needed]: None.
Attachment #8587053 - Flags: approval-mozilla-beta?
Attachment #8587053 - Flags: approval-mozilla-aurora?
Comment on attachment 8502981 [details]
ff-bigbof.m4v

May I share the testcase with upstream and linux distro QA so they can verify the fix?
Attachment #8502981 - Flags: sec-approval?
Can this patch be backported to ESR-31? You mention in comment 46 you're using a new feature which makes me worry. We have to support ESR-31 until Firefox 40 (late summer).

Yeah, you can share the testcase if you've shared the upstream patch.
Attachment #8502981 - Flags: sec-approval? → sec-approval+
Does this need to wait to land on m-c at the same time as you land it on aurora and beta? Or can it land on m-c first?  Are there any timing restrictions for this (like waiting longer for us to be closer to the end of the cycle) or sec-high bugs in general or is that more a concern for sec-critical bugs? Thanks.
Flags: needinfo?(abillings)
Comment on attachment 8587053 [details] [diff] [review]
Block h264parse unconditionally v2

Approving for aurora and beta based on discussion with lawrence.
Attachment #8587053 - Flags: approval-mozilla-beta?
Attachment #8587053 - Flags: approval-mozilla-beta+
Attachment #8587053 - Flags: approval-mozilla-aurora?
Attachment #8587053 - Flags: approval-mozilla-aurora+
We always land security bugs on trunk first unless there is an overwhelming reason that we need to land everywhere at the same time (like a 0day). We land on trunk, make sure trunk is green and happy, and then land elsewhere.

I don't give bugs sec-approval+ to land until it is enough in the cycle that I think it is ok. That's where the timing hinges. Once it is in trunk, timing for branches (as long as they land) isn't important because we've checked the fix in publicly and anyone could see it and try to reverse engineer it. This is the whole reason sec-approval exists.
Flags: needinfo?(abillings)
Oh, and sec-approval only applies to high and critical rated security issues. Things that are lower can land whenever and things without ratings need ratings in order to land. 

see https://wiki.mozilla.org/Security/Bug_Approval_Process
OK. Then I think I should not have approved this for uplift until it has been on m-c for a bit. Thanks Al.
Depending on the bug, a day or two is often fine. Some patches are simple and some are wholesale slaughter of guilty code and need more bake time.
Ok. I've pushed to inbound. Ryan or I can uplift once it's cycled to central.
https://hg.mozilla.org/mozilla-central/rev/8c716f35d9ec
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Can we assign a CVE to the gstreamer issue to help coordinate rolling out the upstream fix?
Flags: needinfo?(abillings)
Alias: CVE-2015-0797
Flags: needinfo?(abillings)
Flags: sec-bounty?
Thanks!
I've contacted Red Hat, Debian and Ubuntu. We'll publish my patch to fix the underlying gstreamer issue on Wednesday, April 15th.
Group: media-core-security
Reluctantly paying a bounty because this isn't our code and the distros could have upgraded people to the GStreamer version without the flaw, but this report has made our users safer -- partial bounty.
Flags: sec-bounty? → sec-bounty+
Debian has pushed my patch. Sebastian Dröge is reviewing the patch for inclusion in gstreamer upstream.

https://lists.alioth.debian.org/pipermail/pkg-gstreamer-maintainers/2015-April/011938.html
Whiteboard: [adv-main38+][adv-esr31.7+]
Ralph, are there any straight forward steps to reproduce this issue? I couldn't reproduce with normal Nightly builds from 2014-10-11 on 12.04 x64 nor 13.10 x64?
Flags: needinfo?(giles)
(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #69)
> Ralph, are there any straight forward steps to reproduce this issue? I
> couldn't reproduce with normal Nightly builds from 2014-10-11 on 12.04 x64
> nor 13.10 x64?

Hmm. You do need to have gstreamer elements which can play h.264 video installed. But if your ubuntu system is up to date old firefox builds should be protected from the crash by the fix we submitted upstream. Could that be the issue? Opening the first attachment did reliably crash my fedora 21 system, so no special magic beyond an ubuntu system which hasn't been updated in more that two weeks should be necessary.

You can isolate firefox changes from ubuntu changes by opening a terminal and running the gst-launch (or gst-launch-0.10) command from comment #11. If that crashes, Firefox is not protected by the Ubuntu fix.
Flags: needinfo?(giles)
(In reply to Ralph Giles (:rillian) from comment #70)
> (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #69)
> > Ralph, are there any straight forward steps to reproduce this issue? I
> > couldn't reproduce with normal Nightly builds from 2014-10-11 on 12.04 x64
> > nor 13.10 x64?
> 
> Hmm. You do need to have gstreamer elements which can play h.264 video
> installed. But if your ubuntu system is up to date old firefox builds should
> be protected from the crash by the fix we submitted upstream. Could that be
> the issue? Opening the first attachment did reliably crash my fedora 21
> system, so no special magic beyond an ubuntu system which hasn't been
> updated in more that two weeks should be necessary.
> 
> You can isolate firefox changes from ubuntu changes by opening a terminal
> and running the gst-launch (or gst-launch-0.10) command from comment #11. If
> that crashes, Firefox is not protected by the Ubuntu fix.

Thanks for the reply!
Succeeded to reproduce the crash with Nightly 2014-10-13: bp-65907ea2-112c-432a-a037-0b34a2150505
No crashes encountered with ESR 38.0 (Build ID: 20150505103531), ESR 31.7.0 build 2 (Build ID: 20150504194141) and latest 40.0a1 (from 2015-05-06), under Ubuntu 13.10 x64.
Depends on: 1177363
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.