Closed Bug 1149605 (CVE-2015-4496) Opened 5 years ago Closed 5 years ago

Security Vulnerability in StageFright MP4 Processing

Categories

(Core :: Audio/Video, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox37 --- wontfix
firefox38 + fixed
firefox39 + fixed
firefox38.0.5 --- fixed
firefox40 + fixed
firefox-esr31 --- unaffected
firefox-esr38 --- fixed
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: mozilla-bugs, Assigned: jya)

Details

(Keywords: sec-critical, Whiteboard: [Android and B2G] Embargo until July 8, 2015 (needs a fix in Firefox 39) [adv-main38-])

Attachments

(3 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2272.101 Safari/537.36
Firefox for Android

Steps to reproduce:

Audited source and discovered an issue in the StageFright code shipping with Firefox. Developed proof-of-concept code.

I apologize for the brief details here, I will attach proof-of-concept code and an advisory with much more detail soon.


Actual results:

Crashed with the potential for arbitrary code execution


Expected results:

Failed to render.
Component: Untriaged → Video/Audio
Product: Firefox → Core
Would love the details when you have them.
Flags: needinfo?(mozilla-bugs)
Reporter says the mac and windows crashes were benign, but there's a problem on Android (and involves coordinated disclosure with Google). We're still waiting on details.
Flags: needinfo?(mozilla-bugs)
I've just added an advisory with embedded PoC code.

This is the first of several vulnerabilities to report. Should I create new tickets for the additional ones?
(In reply to Joshua J. Drake from comment #4)
> This is the first of several vulnerabilities to report. Should I create new
> tickets for the additional ones?

Yes, please.
Okay! Will do.
PS. I requested a 90 day embargo from Daniel on twitter and he said that shouldn't be a problem. How do y'all feel about scheduling public release for ~ July 8th?

The main reason for this request is to give the Android Security Team (also affected by these issues) some time to work within their normal back-channels (OHA, etc).
That's probably a question for Al.
Flags: needinfo?(abillings)
Why July 8?

We ship Firefox releases on June 30 and August 11. Normally, assuming we're fixing something, we'd want dates to line up with a release.
Flags: needinfo?(abillings)
Because it's 90 days from today. I'm fine with June 30 but August 11 is too late. I mainly wanted to give the Android guys a solid 90 days to do their magic.
(In reply to Joshua J. Drake from comment #10)
> Because it's 90 days from today. I'm fine with June 30 but August 11 is too
> late. I mainly wanted to give the Android guys a solid 90 days to do their
> magic.

Our philosophy is that the reporter is in control so whatever you set we'll have to live with. 90 days is reasonable but it's a little stressful having the countdown clock running with no vulnerability information to work with.
Flags: needinfo?(mozilla-bugs)
Whiteboard: Embargo until July 8, 2015 (?)
Daniel, please accept my apologies for not sending more details sooner. I'm in the midst of switching jobs (and computers and so on) and tax time so my time is a bit stretched thin. I should have some time very soon.
Flags: needinfo?(mozilla-bugs)
Ok, we'll wait a bit longer. If you don't mind please leave the "needinfo?" request for you on the bug until you've actually got the details. Otherwise it keeps slipping into our actionable security bug queue and there's nothing we can really do with it right now.
Flags: needinfo?(mozilla-bugs)
Jean-Yves - can you look into this issue?
Flags: needinfo?(jyavenard)
Flags: needinfo?(mozilla-bugs)
Keywords: sec-critical
Whiteboard: Embargo until July 8, 2015 (?) → [Android and B2G] Embargo until July 8, 2015
I tried extracting the PoC from the writeup for convenience, but I'm not seeing a crash on Mac (Nightly) or Android (Beta).
On mac I'm seeing this message from a debug build:
W/MPEG4Extractor( 7106): Error -1004 parsing chuck at offset 3028 looking for first track

I don't know if that's reporting an expected error from this intentionally corrupt video, or if that's something preventing us from getting to the reported bug.
Whiteboard: [Android and B2G] Embargo until July 8, 2015 → [Android and B2G] Embargo until July 8, 2015 (needs a fix in Firefox 39)
Remove this vulnerability, and 5 others that could be similarly exploited. With bug 1158568, we ensure that an atom size never exceeds UINT32_MAX, we use that fact so we don't have to specifically test for overflow.
Attachment #8597674 - Flags: review?(ajones)
Assignee: nobody → jyavenard
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(In reply to Jean-Yves Avenard [:jya] from comment #18)
> Created attachment 8597674 [details] [diff] [review]
> Avoid potential integers overflow
> 
> Remove this vulnerability, and 5 others that could be similarly exploited.
> With bug 1158568, we ensure that an atom size never exceeds UINT32_MAX, we
> use that fact so we don't have to specifically test for overflow.

Jean-Yves: Your changes look good although there are quite a few spurious ones (not security related). I'm unable to see bug 1158568 so I cannot comment on that. I still have backlog of issues to report. I'll do my best to get that done before Wednesday.

PS. have these changes been pushed to any public repository yet?
(In reply to Joshua J. Drake from comment #19)
> PS. have these changes been pushed to any public repository yet?

not this one...
We never push to the public repository until changes are peer-reviewed.

I've added you as CC on 1158568 so you'll be able to see it.

Note that we are going to get rid of libstagefright "soon" ((c) wishful thinking), and we now only use it to read the MP4 metadata.
Flags: needinfo?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #20)
> (In reply to Joshua J. Drake from comment #19)
> > PS. have these changes been pushed to any public repository yet?
> 
> not this one...

I see there was a change for bug 1154683 pushed publicly though. That was one of the other issues I discovered but haven't yet reported :-/

> We never push to the public repository until changes are peer-reviewed.
 
> I've added you as CC on 1158568 so you'll be able to see it.

Thanks

> Note that we are going to get rid of libstagefright "soon" ((c) wishful
> thinking), and we now only use it to read the MP4 metadata.

The sooner the better. That code was written without any regard for security or stability. I'd argue it's actually dangerous even with all of the fixes I've been putting in in the last month.
(In reply to Joshua J. Drake from comment #21)
> The sooner the better. That code was written without any regard for security
> or stability. I'd argue it's actually dangerous even with all of the fixes
> I've been putting in in the last month.

You don't need to convince me there... However, for our now limited use of it and its scope, it's okay.
We don't use it to the same extent Android does.
(In reply to Jean-Yves Avenard [:jya] from comment #22)
> You don't need to convince me there... However, for our now limied use of
> it and its scope, it's okay.
> We don't use it to the same extent Android does.

I'm not sure what you mean. All of the RCE vulns I've found thus far are in the MetdataExtractor related issues. This are the most dangerous as they happen before anything else and thus can be exploited silently. There's no user interaction required beyond getting the media processed somehow. It just so happens in Firefox that happens only when assigning the media file to a video element.
(In reply to Jean-Yves Avenard [:jya] from comment #22)
> 
> You don't need to convince me there... However, for our now limited use of
> it and its scope, it's okay.
> We don't use it to the same extent Android does.

(trying again, broken english)

I'm not sure what you mean. All of the RCE vulns I've found thus far are MetdataExtractor related issues. These are the most dangerous. Metadata extraction happens before anything else and thus can be exploited silently. There's no user interaction required beyond getting the media processed. AFAIK, in Firefox, that happens only when assigning the media file to a video element.
Does this report qualify for a Mozilla bug bounty?
(In reply to Joshua J. Drake from comment #25)
> Does this report qualify for a Mozilla bug bounty?

It may and I've marked it. The normal process for that is to write to security@mozilla.org when you have bugs you want to nominate for bounty consideration (as an FYI).
Flags: sec-bounty?
Attachment #8597674 - Flags: review?(ajones) → review+
Comment on attachment 8597674 [details] [diff] [review]
Avoid potential integers overflow

Copying from bug 1154683, as it's similar just different MP4 atoms

[Security approval request comment]
How easily could an exploit be constructed based on the patch? You'll need to craft an MP4 file with bogus atom's size offset. If you set the offset to be INT32_MAX - [1-8] ; you could end up allocating only [1-8] bytes and write in this allocated buffer INT32_MAX bytes.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no. Only mentions that we prevent an overflow from happening (and reject the entire mp4 as being invalid)

Which older supported branches are affected by this flaw? anything including libstagefright. The flow was in this 3rd party library/

If not all supported branches, which bug introduced the flaw? bug 908503

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch should apply on all branches with stagefright

How likely is this patch to cause regressions; how much testing does it need? Little, we just ensure that no integer overflow can occur.

As a note, all android devices ship with stagefright of which we have no control. They will have that flaw too. Firefox on Android makes use of the system's stagefright. Hopefully google will fix it.
Attachment #8597674 - Flags: sec-approval?
Comment on attachment 8597674 [details] [diff] [review]
Avoid potential integers overflow

[Feature/regressing bug #]:1154683
[User impact if declined]: Potential overflow and out of bound read.
[Describe test coverage new/current, TreeHerder]: Tested locally.
[Risks and why]: Low. Just extra checks that no size calculation will ever overflow
[String/UUID change made/needed]: None
Attachment #8597674 - Flags: approval-mozilla-beta?
Attachment #8597674 - Flags: approval-mozilla-aurora?
Comment on attachment 8597674 [details] [diff] [review]
Avoid potential integers overflow

[Triage Comment]
Taking it now to make sure it lands for 38
Attachment #8597674 - Flags: approval-mozilla-release+
Attachment #8597674 - Flags: approval-mozilla-beta?
Attachment #8597674 - Flags: approval-mozilla-aurora?
Attachment #8597674 - Flags: approval-mozilla-aurora+
Sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=9362678&repo=mozilla-inbound
Flags: needinfo?(jyavenard)
Comment on attachment 8597674 [details] [diff] [review]
Avoid potential integers overflow

Please resubmit when fixed.
Attachment #8597674 - Flags: approval-mozilla-release?
Attachment #8597674 - Flags: approval-mozilla-release+
Attachment #8597674 - Flags: approval-mozilla-aurora?
Attachment #8597674 - Flags: approval-mozilla-aurora+
(In reply to Jean-Yves Avenard [:jya] from comment #28)
> Comment on attachment 8597674 [details] [diff] [review]
> Avoid potential integers overflow
> 
> Copying from bug 1154683, as it's similar just different MP4 atoms

> As a note, all android devices ship with stagefright of which we have no
> control. They will have that flaw too. Firefox on Android makes use of the
> system's stagefright. Hopefully google will fix it.

Google already accepted the 7 patches I sent them. Those patches contain over 100 changes. However, bug 1154683 (which I still cannot access) was not yet reported to them. Was bug 1154683 externally reported? If so that is a very troubling development. Especially for Android security.

I am back to focus with reporting that and other problems to all affected parties. Hang on to your britches.
(In reply to Jean-Yves Avenard [:jya] from comment #28)
> If not all supported branches, which bug introduced the flaw? bug 908503

That would make it 32+. Changing the flags around to reflect accordingly.
btw Jean i guess this needs security approval before landing again
It probably should have had security approval BEFORE LANDING THE FIRST TIME. Now the issue has been partially publicly disclosed.
(In reply to Joshua J. Drake from comment #36)
> It probably should have had security approval BEFORE LANDING THE FIRST TIME.
> Now the issue has been partially publicly disclosed.

Yes, this is correct.  sec-high and sec-critical bugs that affect more than trunk need sec-approval+ before landing.

sec-approval is kind of formality at this point, as disclosure has occurred, though it is still good to let somebody make sure flags and set or whatever.
(In reply to Jean-Yves Avenard [:jya] from comment #28)
> As a note, all android devices ship with stagefright of which we have no
> control. They will have that flaw too. Firefox on Android makes use of the
> system's stagefright. Hopefully google will fix it.

BTW, my testing shows that Android does *NOT* use Android's system stagefright. The crash backtrace shows it's using the code within Mozilla itself. Did I make some mistake?
(In reply to Joshua J. Drake from comment #38)
> BTW, my testing shows that Android does *NOT* use Android's system
> stagefright. The crash backtrace shows it's using the code within Mozilla
> itself. Did I make some mistake?

LOL, Firefox for Android does *NOT* use... One day I'll learn English. I swear.
(In reply to Joshua J. Drake from comment #39)
> (In reply to Joshua J. Drake from comment #38)
> > BTW, my testing shows that Android does *NOT* use Android's system
> > stagefright. The crash backtrace shows it's using the code within Mozilla
> > itself. Did I make some mistake?
> 
> LOL, Firefox for Android does *NOT* use... One day I'll learn English. I
> swear.

I wasn't referring to Firefox for Android..

Running a new try: that itself makes it public too (sorry I wasn't aware of the process).
https://treeherder.mozilla.org/#/jobs?repo=try&revision=594c1f8183f1

Not sure why the static analyser chokes on (size_t)-1. This is a very common trick to determine the maximum value of size_t (which isn't always UINT32_T and can be less: I know of one system where it's 24 bits, though likely never going to run firefox :) )
Considering that warning fatal is totally idiotic.
Of course it's going to always be false on 64 bits systems as it would never overflow there. That's the whole point of testing for overflow !
Flags: needinfo?(jyavenard)
Comment on attachment 8597674 [details] [diff] [review]
Avoid potential integers overflow

*sigh* sec-approval+ after the fact.
Attachment #8597674 - Flags: sec-approval?
Attachment #8597674 - Flags: sec-approval+
Attachment #8597674 - Flags: approval-mozilla-release?
Attachment #8597674 - Flags: approval-mozilla-release+
Attachment #8597674 - Flags: approval-mozilla-aurora?
Attachment #8597674 - Flags: approval-mozilla-aurora+
Limit size of memory allocation to 2GB max (same limit as a nsTArray). This means we won't be able to play a 60Hz MP4 video longer than 51.78 days :)
Attachment #8597674 - Attachment is obsolete: true
Comment on attachment 8599013 [details] [diff] [review]
Avoid potential integers overflow

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8d18fdd65f8

I can't foresee issues now.

The artificial size limitation (though extremely reasonable) is only to bypass our build system's own limitation that doesn't want a test to always be false (as intended)
Attachment #8599013 - Flags: approval-mozilla-beta?
Attachment #8599013 - Flags: approval-mozilla-aurora?
I'm confused. Doesn't sec-approval+ mean you're going to violate the embargo even worse than it already is?
Sec-approval+ means that the fix can be checked in. That doesn't violate any embargoes. Those are about disclosure, not fixing the bug.
(In reply to Al Billings [:abillings] from comment #46)
> Sec-approval+ means that the fix can be checked in. That doesn't violate any
> embargoes. Those are about disclosure, not fixing the bug.

Okay. I guess that is a bit of a grey area since checking things in to a public repository constitutes a form of public disclosure. It's well known in security researcher circles that various parties monitor the code repositories for security fixes for their own purposes -- both offensive and defensive focused organizations.

If it weren't for the severity of these issues and the complications with cross-disclosure I'd be inclined to publish my own advisories once fixes hit the public tree. Alas, it shouldn't be long until everything is public anyway but I wouldn't be surprised to see additional external reports or even malicious actors becoming more active around these and related issues in the meantime. It's a shame that the process doesn't protect this type of information better.
Attachment #8599013 - Flags: approval-mozilla-beta?
Attachment #8599013 - Flags: approval-mozilla-beta+
Attachment #8599013 - Flags: approval-mozilla-aurora?
Attachment #8599013 - Flags: approval-mozilla-aurora+
(In reply to Joshua J. Drake from comment #47)

> Okay. I guess that is a bit of a grey area since checking things in to a
> public repository constitutes a form of public disclosure. 

 1) It isn't a gray area.

 20 We don't have a non-public repository.

If you want Mozilla to fix a bug, we have to check it in.

> It's well known in security researcher circles that various parties monitor the code
> repositories for security fixes for their own purposes -- both offensive and
> defensive focused organizations.

 Yes, we know this. This is why we have sec-approval as a flag. I and one other person (Dan Veditz) approve checkins for any critical or high rated security bugs that affect more than trunk during our six week dev cycle taking the fact that our repositories are watched into account. See https://wiki.mozilla.org/Security/Bug_Approval_Process for more information.

> If it weren't for the severity of these issues and the complications with
> cross-disclosure I'd be inclined to publish my own advisories once fixes hit
> the public tree.

 If you did this you'd burn any good will and we'd be unlikely to work with you in a friendly manner in the future. That is not an appropriate response to a checkin to fix an issue by us and would damage your relationship to us. We are well aware of the risks of checkin. We fixing it in our trunk, which is Firefox 40, and we'll also be taking it in 39 and 38 (Aurora and Beta) *specifically* because it is a sec-critical bug that we've just checked a fix in for.
I'd also point out that people that zero day Firefox users are not people that we have participate in our bounty program. Publishing an advisory before we ship a release with this fix is a zero day disclosure against us.
Al,

Your points are well placed and understood. Perhaps in the future the situation can improve. I will adjust my coordination efforts with Mozilla accordingly. Maybe it makes more sense to notify Mozilla later in the process so that they can ship faster and expose other (non-Firefox users) less.

This is no small number of users btw. These issues affect 95% of Android devices today, which amounts to around 950 million devices. It's critically exploitable on those systems via MMS. I hope you can understand my point of view.
This bug was disclosed to us on March 31. We're likely fixing this in our Firefox release on May 12. That's six weeks. I'm not sure how much faster you would expect this to be fixed than six weeks from report when we have (coincidentally this time) a six week release cycle.
In fact, we didn't get the PoC until April 9. How fast, exactly, do you expect Firefox to ship a fix if five weeks from full disclosure to us is not acceptable? Aren't you the one asking for an embargo until July 8?
I'm not sure where the confusion came in, but let me attempt to clarify.

I'm not arguing that you need to deliver a fix faster. You guys do great there and it's well known that's the case.

My issue about properly honoring the embargo. You guys have effectively 0day'd all Android users by pushing a fix publicly ahead of the embargo. I understand you care about Firefox users first and foremost, but they are not the only people at risk and don't even come close to being the majority.

So my proposal is that in order to keep things embargoed by Mozilla (not pushed to a public repository) people should notify Mozilla around 6 weeks prior to the embargo date instead of 90 days ahead.
I think you misunderstand what "embargo" means at Mozilla. It doesn't mean we don't fix the bug in our repositories. It means we don't disclose the fix and write an advisory discussing it until the embargo date, giving other parties time to do the same. Neither we nor anyone else is coordinating the day that fixes are checked into a repository. We coordinate when we *talk* in *public* about such fixes.

If you notify Mozilla six weeks ahead of when you think code should be checked in, I'm afraid there is no guarantee that any code will be checked in on your specified date. Our developers work on code at best possible speed balancing the calls on their time, their expertise, and the difficulties of fixes. 

If you wanted no *checkin* until a specific date, you needed to be clear about that up front.

In any case, the code is checked in now and the only violation of our standard procedure was that the dev did not get sec-approval+ before checkin. We don't consider fixing a bug in a repository a public disclosure for the purposes of coordination.
The Android guys actually can guarantee an embargo date. They have a private repository from which they do not push fixes until when they release. That said, it's unlikely that they'll make the July 8th date anyway. We're likely just splitting hairs about how early Android get's 0day'd at this point :-/

I understand the complexities of balancing developing against security. While I don't necessarily agree with Mozilla's policies, I do understand them. In fact I understand them better than before, thanks to this discussion. Thank you for clarifying these details. I'll be sure to request *no-checkin-until* on future reports. Sorry for the misunderstanding :-/
*developing openly*
I personally think that you're over estimating the exploitability of this bug (other than crashing that is).

Let's also keep in mind that we've been fixing vulnerabilities in stagefright ever since we included in our source tree (over 2 years ago) The code isn't big, that it contains such a high ratio of bugs per line count is a remarkable achievement in itself.
What I mean to say by this, is that the people you claim would monitor for new exploits would likely have noticed already. Of course this is my personal opinion...

(Should I state my opinion of android? :) )

Now, this isn't about lessening the impact of my foolishness about what sec-approval meant. It's the second times I've pushed this week without such approval. I should have asked first on what to do rather than assume things. For that I apologise (though I will accept that this doesn't make much of a difference)
Thanks. I'm being genuine here and not *trying* to be antagonistic if it seems like I am. We clearly had a misunderstanding about the meaning of terminology.

Please be aware of our (normally) six week cycle for releases when discussing dates: https://wiki.mozilla.org/RapidRelease/Calendar

We check all security fixes into trunk (nightly) first and then check them into Aurora and Beta branches if they are high or critical rated issues and there is time to do so without destabilizing those branches for a release. For example, you're unlikely to be see too many fixes in the last two weeks before a release into the Beta branch (which is the next release always) because we could jeopardize the release's stability. We have a window of about three weeks in each cycle where most fixes can go in. We also have to take our Extended Support Release (ESR) branch or branches (during the overlap between two ESR branches) into account as well if the issues affects ESR.

So, there is a lot of coordination to do. We try to get any high or critical rated issues fixed as soon as possible though.
(In reply to Jean-Yves Avenard [:jya] from comment #57)
> I personally think that you're over estimating the exploitability of this
> bug (other than crashing that is).

Exploitability is secondary to getting things fixed in my opinion. Assuming that memory corruption can only result in a crash is a mistake that many have made in the past. Let's not repeat their folly.

I've personally examined the resulting crash and various other relevant parameters, both in Firefox and Android and believe this issue to be exploitable. I will invest more research time in this area soon and prove it to myself (and others) one way or the other.

> Let's also keep in mind that we've been fixing vulnerabilities in
> stagefright ever since we included in our source tree (over 2 years ago) The
> code isn't big, that it contains such a high ratio of bugs per line count is
> a remarkable achievement in itself.

I agree that the number of bugs per line count is a remarkable achievement. However, rather than laugh about it I'm attempting to fix these issues.

> What I mean to say by this, is that the people you claim would monitor for
> new exploits would likely have noticed already. Of course this is my
> personal opinion...

That's a fair point. They likely have taken notice, which is perhaps the main reason I am working to get these fixed.

> (Should I state my opinion of android? :) )

Despite what you think about Android, it has grown to become the most widely deployed operating system globally. In fact, isn't B2G based heavily on Android? I think that deserves some respect and increased attention on securing the platform. I'm happy to spend my time trying to improve it at least.

> Now, this isn't about lessening the impact of my foolishness about what
> sec-approval meant. It's the second times I've pushed this week without such
> approval. I should have asked first on what to do rather than assume things.
> For that I apologise (though I will accept that this doesn't make much of a
> difference)

Thanks for apologizing. It takes a strong person to admit their mistakes and apologize. You've gained my respect for that, for whatever that is worth.
(In reply to Al Billings [:abillings] from comment #58)
> Thanks. I'm being genuine here and not *trying* to be antagonistic if it
> seems like I am. We clearly had a misunderstanding about the meaning of
> terminology.

Not at all Al. I've not worked with Mozilla in this capacity before, certainly not on coordinating issues as complex is this. This is very much a learning process for me. I genuinely appreciate your elaborations and clarifications.
https://hg.mozilla.org/mozilla-central/rev/c99d19bfd6a0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Flags: sec-bounty? → sec-bounty+
Whiteboard: [Android and B2G] Embargo until July 8, 2015 (needs a fix in Firefox 39) → [Android and B2G] Embargo until July 8, 2015 (needs a fix in Firefox 39) [adv-main38-]
Hey all. I've revisited my remaining fixes and found that Jean-Yves seems to have fixed all of the critical issues with his changes made between this and bug 1158568. I still have a significant number of lesser severity or non-security related changes that I've submitted upstream (and were accepted). If Mozilla is interested in carrying these changes forward as well please say so and I will build a patch including all of those changes on a new ticket.
Alias: CVE-2015-4496
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.