1158568 - Integer overflow in libstagefright might lead to heap overflow

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

Turned out, bug 1154683 was incorrectly fixed.

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

Comment 1

[Jean-Yves Avenard [:jya]]

The issue was limited to 32 bits systems..

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Fix potential size overflow

Fix potential out of bound writes

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Fix potential size overflow

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

Comment 4

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1b04bf621858

Comment 5

[Jean-Yves Avenard [:jya]]

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

Comment 6

[Jean-Yves Avenard [:jya]]

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.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8597675 [details] [diff] [review]
Fix potential size overflow

[Triage Comment]
Taking it now to make sure it lands for 38

Comment 8

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=9362678&repo=mozilla-inbound

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8597675 [details] [diff] [review]
Fix potential size overflow

Please resubmit when fixed.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

BTW, are you not supported to wait for a sec approval before landing this?

Comment 11

[Jean-Yves Avenard [:jya]]

Attachment: Fix potential size overflow

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

Comment 12

[Jean-Yves Avenard [:jya]]

Only waiting for inbound to re-open.

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

Comment 13

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9dd3a01c6770

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Why is this on inbound without sec-approval+ being given?

Comment 15

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

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

Comment 17

[Jean-Yves Avenard [:jya]]

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? :(

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/9dd3a01c6770

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/81723b7cc67f

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/8a61f534f496

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8a61f534f496
https://hg.mozilla.org/releases/mozilla-esr38/rev/8a61f534f496
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d51cc081453e
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/b7be50195ca2
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/b7be50195ca2
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e8ea484c1efa
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/e8ea484c1efa

Comment 22

[Joshua J. Drake]

What does approval-mozilla-release+ mean exactly?

Comment 23

[Al Billings [:abillings - ex-MoCo]]

It means it is going into 38 though I suspect it is too late now as final builds have been produced.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

oh wait, it did make it in. It is an artifact of 38 versus 38.0.5.

Comment 25

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Jean-Yves, could you provide us some detailed steps to verify this fix on ESR 38?

Comment 26

[Jean-Yves Avenard [:jya]]

Load the MP4 found in bug 1154683. 

It should display immediately, without pause that the file is corrupted.

Comment 27

[Catalin Varga [QA][:VarCat]]

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.

Comment 28

[Jean-Yves Avenard [:jya]]

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

[Joshua J. Drake]

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.

Comment 30

[Al Billings [:abillings - ex-MoCo]]

(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

[Joshua J. Drake]

Sadly even another 6 weeks out puts opening the bug almost a month within the disclosure embargo we agreed upon.

Comment 32

[Al Billings [:abillings - ex-MoCo]]

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

[Joshua J. Drake]

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

Comment 34

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 35

[Joshua J. Drake]

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

Comment 36

[Daniel Veditz [:dveditz]]

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.

Comment 37

[Joshua J. Drake]

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?

Comment 38

[Al Billings [:abillings - ex-MoCo]]

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

[Joshua J. Drake]

Okay thanks! BTW, does the bounty include a t-shirt?!

Comment 40

[chris hofmann]

I'll send you a tshirt the week of June 29.

Comment 41

[Al Billings [:abillings - ex-MoCo]]

It is after July 8. Where are we at in terms of disclosure, Joshua?

Comment 42

[Joshua J. Drake]

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

Comment 43

[Al Billings [:abillings - ex-MoCo]]

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

[Joshua J. Drake]

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

Comment 45

[Al Billings [:abillings - ex-MoCo]]

Works for me. I'll be heading to DEFCON that week.

Comment 46

[Joshua J. Drake]

(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 [e-mail].

Comment 47

[Joshua J. Drake]

Any news on the planned Mozilla Security Advisory?

Comment 48

[Al Billings [:abillings - ex-MoCo]]

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

Comment 49

[Daniel Veditz [:dveditz]]

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.

Comment 50

[Joshua J. Drake]

(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

[Joshua J. Drake]

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.