Integer overflow in libstagefright might lead to heap overflow

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

({csectype-bounds, sec-high})

Trunk
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [b2g-adv-main-2.5+][adv-main38+] fixes CVE-2015-3864)

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

4 years ago
Turned out, bug 1154683 was incorrectly fixed.

while it fixes one problem, the vulnerability wasn't patched for all cases.
Assignee

Comment 1

4 years ago
The issue was limited to 32 bits systems..
Assignee

Updated

4 years ago
Assignee

Comment 2

4 years ago
Posted patch Fix potential size overflow (obsolete) — Splinter Review
Fix potential out of bound writes
Attachment #8597673 - Flags: review?(ajones)
Assignee

Updated

4 years ago
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Assignee

Comment 3

4 years ago
In theory, size_t may be less than sizeof(uint32_t). So handle that case, and use a fallible array instead of mallocing a big array and simply ignore the metadata alltogether if we couldn't allocate memory. Slightly more elegant solution
Attachment #8597675 - Flags: review?(ajones)
Assignee

Updated

4 years ago
Attachment #8597673 - Attachment is obsolete: true
Attachment #8597673 - Flags: review?(ajones)
Attachment #8597675 - Flags: review?(ajones) → review+
Assignee

Comment 5

4 years ago
Comment on attachment 8597675 [details] [diff] [review]
Fix potential size 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 #8597675 - Flags: approval-mozilla-beta?
Attachment #8597675 - Flags: approval-mozilla-aurora?
Assignee

Comment 6

4 years ago
Comment on attachment 8597675 [details] [diff] [review]
Fix potential size 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 #8597675 - Flags: sec-approval?
Comment on attachment 8597675 [details] [diff] [review]
Fix potential size overflow

[Triage Comment]
Taking it now to make sure it lands for 38
Attachment #8597675 - Flags: approval-mozilla-release+
Attachment #8597675 - Flags: approval-mozilla-beta?
Attachment #8597675 - Flags: approval-mozilla-aurora?
Attachment #8597675 - 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 8597675 [details] [diff] [review]
Fix potential size overflow

Please resubmit when fixed.
Attachment #8597675 - Flags: approval-mozilla-release?
Attachment #8597675 - Flags: approval-mozilla-release+
Attachment #8597675 - Flags: approval-mozilla-aurora?
Attachment #8597675 - Flags: approval-mozilla-aurora+
BTW, are you not supported to wait for a sec approval before landing this?
Assignee

Comment 11

4 years ago
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
Assignee

Comment 12

4 years ago
Only waiting for inbound to re-open.

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

Updated

4 years ago
Flags: needinfo?(jyavenard)
Why is this on inbound without sec-approval+ being given?
Comment on attachment 8597675 [details] [diff] [review]
Fix potential size overflow

Even though this is a patch on an incomplete fix, it shouldn't have gone in without sec-approval+. Giving it now.
Attachment #8597675 - Flags: sec-approval?
Attachment #8597675 - Flags: sec-approval+
Attachment #8597675 - Flags: approval-mozilla-aurora?
Attachment #8597675 - Flags: approval-mozilla-aurora+
(This is why we're working on blocking checkins that are missing sec-approval as an HG hook, by the way. This is happening too often.)
Assignee

Comment 17

4 years ago
It was originally committed with bug 1149605 (and I asked to have it backed out) which too went without sec-approval. And seeing I created that bug when I realised that bug 1154683 fix was incorrect. 

I can't do it right can I? :(
https://hg.mozilla.org/mozilla-central/rev/9dd3a01c6770
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Attachment #8597675 - Flags: approval-mozilla-release? → approval-mozilla-release+

Comment 22

4 years ago
What does approval-mozilla-release+ mean exactly?
It means it is going into 38 though I suspect it is too late now as final builds have been produced.
oh wait, it did make it in. It is an artifact of 38 versus 38.0.5.
Whiteboard: [adv-main38+]
Jean-Yves, could you provide us some detailed steps to verify this fix on ESR 38?
Flags: needinfo?(jyavenard)
Assignee

Comment 26

4 years ago
Load the MP4 found in bug 1154683. 

It should display immediately, without pause that the file is corrupted.
Flags: needinfo?(jyavenard)
I've tried reproducing the issue using:

FF 38.0b9
Build Id: 20150429135941
OS: Win 8.1 x86

and the mp4 file from https://bug1154683.bugzilla.mozilla.org/attachment.cgi?id=8592760&t=y04KY4scwe ,
but I didn't manage to reproduce it, the error that the file is corrupted is displayed immediately with and without the fix.
Assignee

Comment 28

4 years ago
The file will be detected as corrupted regardless. The time for it make take to be seen as corrupted without the fix may vary however. It could go from very quick, to several seconds (it will be parsing random area of the memory).

Unfortunately, the only proper way to tell it's actually fixed would be to use a debugger and show that we properly stop as soon as the corrupted field is encountered.

Comment 29

4 years ago
FYI, this, bug 1149605 that I reported, and bug 1154683 were all part of the initial patch set I sent upstream to the Android Security Team. They are in the process of fixing them and making a release. They may release a new Android version carrying these fixes in mid-to-late May, but I plan to continue to hold disclosure until the 90 day embargo is up (July 8th). Please do give the Android security team (and the remaining members of the mobile ecosystem) until July 8th before disclosing these issues publicly.
(In reply to Joshua J. Drake from comment #29)
> FYI, this, bug 1149605 that I reported, and bug 1154683 were all part of the
> initial patch set I sent upstream to the Android Security Team. They are in
> the process of fixing them and making a release. They may release a new
> Android version carrying these fixes in mid-to-late May, but I plan to
> continue to hold disclosure until the 90 day embargo is up (July 8th).
> Please do give the Android security team (and the remaining members of the
> mobile ecosystem) until July 8th before disclosing these issues publicly.

Bug 1154683, reported by a different reporter to us, has an advisory in the upcoming release but we aren't opening the bug up for at least another six weeks. We generally don't open bugs for at least six weeks (a release cycle) following their public fixing.

Comment 31

4 years ago
Sadly even another 6 weeks out puts opening the bug almost a month within the disclosure embargo we agreed upon.
I'm not sure what you're trying to say, Joshua. I was letting you know that we wrote an advisory for today's release on another bug by another reporter that was in a related area. That advisory is now up and that release is now out. Their bug is not your bug, though, so I was mentioning it as a courtesy.

Comment 33

4 years ago
It actually was one of the many bugs that I had started queuing up to report. Unfortunately, things changed so fast after my first report I had to go back and revalidate my results.

On that note, there are quite a few more changes that have lesser/no security impact that I reported upstream. I have been trying to get guidance from Mozilla on whether or not they are desired. If they are wanted, would it be better to send one big patch or break it down into several?

I understand that the other bug was reported independently. I still think it's unfortunate that Mozilla decided to go ahead and publish while simultaneously agreeing to an embargo on extremely an related issue. Especially being informed that these issues were already being coordinated with upstream (Android) with a reasonable timeline. I made to drop a note to the Android security team letting them know that you guys dropped 0day on their 950 million customers today. 

Anyway it's all blah blah and boo hoo at this point. What is done is done. Let's discuss what happens from here on out.

My plan, unless more information surfaces publicly, will be to stay the course and continue honor my embargo with the Android team. Hopefully some good comes from Mozilla's (perhaps somewhat selfish) actions and the Android guys will accelerate their schedule.

Would Mozilla be willing to keep the other bug closed until at least after the embargo period for expires (rather than opening it in June)?
To be clear, we didn't zero day anyone as the advisory isn't details on how to do the exploit and would take work to figure out.

The problem here is that the reporter of that bug was not you and they had expectations as well to have their issue reported when it was fixed. If you had reported it earlier when you found it, we wouldn't have that as an issue.

We can keep the bug closed for a while. Dan Veditz opens them up and isn't in a rush to do so necessarily, especially if there are reasons to not do so.

As to other bugs/fixes, we prefer that bugs contain single issues. If you have a series of interrelated issues covered by one patch, that might be one bug but if you've found a variety of issues that affect us and which need separate patches, they should be opened as their own issues.
Flags: needinfo?(dveditz)

Comment 35

4 years ago
(In reply to Al Billings [:abillings] from comment #34)
> To be clear, we didn't zero day anyone as the advisory isn't details on how
> to do the exploit and would take work to figure out.

I think this is subjective, similar to the way embargo is subjectively interpreted. You certainly didn't point a big red finger at Android and call them out, so that's good.

> The problem here is that the reporter of that bug was not you and they had
> expectations as well to have their issue reported when it was fixed. If you
> had reported it earlier when you found it, we wouldn't have that as an issue.

Yep. I totally understand. I've definitely learned a lesson from this experience.

> We can keep the bug closed for a while. Dan Veditz opens them up and isn't
> in a rush to do so necessarily, especially if there are reasons to not do so.

Thanks!

> As to other bugs/fixes, we prefer that bugs contain single issues. If you
> have a series of interrelated issues covered by one patch, that might be one
> bug but if you've found a variety of issues that affect us and which need
> separate patches, they should be opened as their own issues.

Perfect! Thanks for the guidance. The patches currently apply cleanly to AOSP master, so I'll need get them rebased to the inbound branch. I'll get this done in the coming days and post them up under new sec tickets.
bug 1154683 is tagged with the embargo date. Since libstagefright is shipped with Firefox OS devices and we have the same problems Android has getting carriers to ship updates I'm not in any hurry to unhide these bugs.
Flags: needinfo?(dveditz)
Whiteboard: [adv-main38+] → [adv-main38+][don't un-hide until at least July 8]

Comment 37

4 years ago
Hey guys. Just an update.

I am planning to move the expected disclosure date out until the day of my presentation at BlackHat USA. Let's call it August 5th-ish. There will be some partial disclosure happening in the way of marketing stuff starting shortly after the original embargo period expires (July 9th). What dates work best for you guys to publish?
Since it has been embargoed past the release in which it is fixed, we aren't time sensitive. Other than writing an "after the fact" advisory, we're done. There isn't going to be anything published with fanfare. I'll put an advisory together when it is public for all parties.

Comment 39

4 years ago
Okay thanks! BTW, does the bounty include a t-shirt?!
Whiteboard: [adv-main38+][don't un-hide until at least July 8] → [adv-main38+][don't un-hide until at least July 8. see comment 37]

Comment 40

4 years ago
I'll send you a tshirt the week of June 29.
It is after July 8. Where are we at in terms of disclosure, Joshua?
Flags: needinfo?(mozilla-bugs)

Comment 42

4 years ago
(In reply to chris hofmann from comment #40)
> I'll send you a tshirt the week of June 29.

I got the shirt! It looks great, thanks!

(In reply to Al Billings [:abillings] from comment #41)
> It is after July 8. Where are we at in terms of disclosure, Joshua?

I opted to delay a bit longer for the sake of OEMs out there. The ultimate disclosure date will be August 5th when I present the research at BlackHat USA. When is the next release planned?
Flags: needinfo?(mozilla-bugs)
August 11 but the release date doesn't matter since this was fixed two releases ago... This is about when to disclose that it was fixed.

Comment 44

4 years ago
(In reply to Al Billings [:abillings] from comment #43)
> August 11 but the release date doesn't matter since this was fixed two
> releases ago... This is about when to disclose that it was fixed.

Does August 5th work for you guys then?
Works for me. I'll be heading to DEFCON that week.

Comment 46

4 years ago
(In reply to Al Billings [:abillings] from comment #45)
> Works for me. I'll be heading to DEFCON that week.

Excellent. Hopefully we'll get a chance to meet while we're out there. If you guys end up having an event and would like me to drop by, ping me at josh@zimperium.com.

Comment 47

4 years ago
Any news on the planned Mozilla Security Advisory?
(In reply to Joshua J. Drake from comment #47)
> Any news on the planned Mozilla Security Advisory?

As I mentioned in email, I expect to get to it this week. We had other releases to do over the last week first and there isn't specific time pressure on a bug from two releases ago (that you asked us not to disclose until now).
This fixes the bug blogged about at
https://blog.exodusintel.com/2015/08/13/stagefright-mission-accomplished/

Google assigned CVE-2015-3864 to the issue in that blog. I don't know if our version of stagefright has forked too much to reuse that CVE, and I believe this patch fixes more cases than just the one mentioned in that blog so it's probably inappropriate to re-use that label.
Whiteboard: [adv-main38+][don't un-hide until at least July 8. see comment 37] → [adv-main38+] fixes CVE-2015-3864

Comment 50

4 years ago
(In reply to Daniel Veditz [:dveditz] from comment #49)
> This fixes the bug blogged about at
> https://blog.exodusintel.com/2015/08/13/stagefright-mission-accomplished/
> 
> Google assigned CVE-2015-3864 to the issue in that blog. I don't know if our
> version of stagefright has forked too much to reuse that CVE, and I believe
> this patch fixes more cases than just the one mentioned in that blog so it's
> probably inappropriate to re-use that label.

Daniel,

Forgive me if I'm wrong, but I don't believe Mozilla's Stagefright fork is affected at all (after the fix for the issue I reported in this ticket). IIRC JYA limited the max chunk_data_size to 2^32 - 128 or so. If you're interested to test there's a PoC file inside our Stagefright Detector Android app.

Comment 51

4 years ago
Sorry, I meant "chunk_size". See the link in comment #18. chunk_size is bounded by kMAX_ALLOCATION, which is set to the lesser of SIZE_MAX and INT32_MAX less 128.
Whiteboard: [adv-main38+] fixes CVE-2015-3864 → [b2g-adv-main-2.5+][adv-main38+] fixes CVE-2015-3864

Updated

4 years ago
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.