Bug 1094536 (CVE-2014-8637)

Apparent use of uninitialized memory when rendering BMPs on <canvas>

REOPENED
Assigned to

Status

()

Core
ImageLib
REOPENED
4 years ago
3 years ago

People

(Reporter: Michal Zalewski, Assigned: jrmuizel)

Tracking

({csectype-disclosure})

33 Branch
csectype-disclosure
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox33 wontfix, firefox34 wontfix, firefox35 wontfix, firefox36 wontfix, firefox38 wontfix, firefox38.0.5 wontfix, firefox39 affected, firefox40 affected, firefox41 affected, firefox-esr31 unaffected, firefox-esr38 affected, b2g-v1.4 wontfix, b2g-v2.0 affected, b2g-v2.0M affected, b2g-v2.1 affected, b2g-v2.1S affected, b2g-v2.2 affected, b2g-master affected)

Details

(Whiteboard: [adv-main35+])

(Reporter)

Description

4 years ago
See bug 1063733 and bug 1045977 for context. 

I swear that I'm not drip-feeding you these :-) ...but the fuzzer just spewed out another image that appears to render differently on <canvas> across multiple runs in 33.0.2:

http://lcamtuf.coredump.cx/ffbmp/
Component: Security → ImageLib
Product: Firefox → Core
Can't reproduce this on Release or Nightly on Mac and Windows. It always gives me a "You're probably OK" message.
Can't reproduce on 64bit linux Nightly, but can on Release.
Somebody should figure out what fixed this, if in fact it is fixed.
Milan, is there somebody who can look at this?  Thanks.
Flags: needinfo?(milan)
Reverse regression window where good=bad and bad=good.

Last good revision: 2db5b64f6d49 (2014-09-12)
First bad revision: 59d4326311e0 (2014-09-13)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2db5b64f6d49&tochange=59d4326311e0
Looks like here's a narrower range:

Last good revision: cc3e560c8f06
First bad revision: 59d4326311e0
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cc3e560c8f06&tochange=59d4326311e0
The second range actually looks less useful. Hmm. Not sure why mozregression wanted to continue.
This was fixed by bug 1063129:

https://hg.mozilla.org/mozilla-central/rev/cc9e31b9256e

The code has changed quite a bit since then and doesn't contain the above change anymore. The issue somehow still remains fixed on Nightly.
We should at least look into uplifting this to 34.
Between Michael and Seth, we should be able to figure this out, as it's related to bug 1063129.
Flags: needinfo?(seth)
Flags: needinfo?(mwu)
Flags: needinfo?(milan)
(In reply to Tim Taubert [:ttaubert] from comment #8)
> The code has changed quite a bit since then and doesn't contain the above
> change anymore. The issue somehow still remains fixed on Nightly.

It does contain that code; it just moved to imgFrame::UnlockImageData. That change was made in bug 1070426.

Seems like uplifting this is a good idea. Michael, want to request uplift?
Flags: needinfo?(seth)
(As an aside: it's not obvious to me why we were accessing uninitialized memory prior to bug 1063129. Michael, does it make sense to you?)

Comment 13

4 years ago
I requested uplift, but it seems like it's getting late for 34.

dveditz, how do you feel about leaving a fix out of 34? Seems difficult to take advantage of this.
Flags: needinfo?(mwu) → needinfo?(dveditz)
You're talking about uplifting https://hg.mozilla.org/mozilla-central/rev/cc9e31b9256e from bug 1063129, correct? If it's that simple then we should get this in, yes.

What about ESR-31? If the regressing bug mentioned in bug 1063129 comment 12 applies to this security bug then we shouldn't need it. if that's the case please change the status-firefox-esr31 field to "unaffected". If we do need it then please use "affected".
status-firefox33: --- → wontfix
status-firefox34: --- → affected
status-firefox35: --- → fixed
status-firefox36: --- → unaffected
status-firefox-esr31: --- → ?
tracking-firefox-esr31: --- → ?
Depends on: 1063129
Flags: needinfo?(dveditz) → needinfo?(mwu)
Keywords: csectype-disclosure, sec-high

Comment 15

4 years ago
I don't think ESR-31 is affected. To be specific, ESR-31 behaves the same as 35/36 when it comes to handling alpha.

I suspect that the fix for bug 1063129 only covers up the real bug, however, and that we've had this issue in the BMP decoder for a while. The fix in bug 1063129 doesn't make a whole lot of sense to me. The BMP decoder should be filling the alpha channel with 0xFF since it's 4bit RLE encoded.
status-firefox-esr31: ? → unaffected
Flags: needinfo?(mwu)
Group: gfx-core-security
The patch in bug 1063129 bounced and has now missed beta10. In speaking with abillings earlier he told me that this is not a must fix for 34. As the fix for bug 1063129 has already stuck on 35, let's let the fix ride the 35 train.
status-firefox34: affected → wontfix

Updated

4 years ago
tracking-firefox-esr31: ? → ---
Whiteboard: [adv-main35+]

Updated

4 years ago
Alias: CVE-2014-8637

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Group: gfx-core-security

Updated

4 years ago
Flags: sec-bounty?

Updated

4 years ago
Flags: sec-bounty? → sec-bounty+
It seems like this is something that may still need to be addressed on older supported B2G branches given the sec rating?
Assignee: nobody → mwu
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
Target Milestone: --- → mozilla35

Comment 18

3 years ago
Bug 1063129 bounced and I don't know enough about the bmp decoder to fix it. Also not sure why bug 1063129 fixes anything at all.
Assignee: mwu → nobody
I'm not sure what the next step is here, but it seems to me that something needs to be done still.
Flags: needinfo?(continuation)
Seth, is there somebody who knows about the BMP decoder who could look at this?
Flags: needinfo?(continuation) → needinfo?(seth)
(In reply to Andrew McCreight [:mccr8] from comment #20)
> Seth, is there somebody who knows about the BMP decoder who could look at
> this?

As we discussed, I don't have time to fix this in the near term. Jeff, do you have time to take this on right now?
Flags: needinfo?(jmuizelaar)
(In reply to Seth Fowler [:seth] from comment #21)
> As we discussed, I don't have time to fix this in the near term. Jeff, do
> you have time to take this on right now?

Well, I actually emailed you about a different, even older bug, bug 1050342. ;)  But I guess the same thing applies.
Ah, my mistake! The summaries are pretty similar.
No from Jeff either, we're digging out of 37 issues.
Flags: needinfo?(jmuizelaar)
This straight-up reproduces for me on a current trunk build using the link in comment #0. This needs an owner AFAICT.
Status: RESOLVED → REOPENED
status-b2g-v1.4: affected → wontfix
status-b2g-v2.0M: --- → affected
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: fixed → affected
status-b2g-master: --- → affected
status-firefox35: fixed → wontfix
status-firefox36: unaffected → wontfix
status-firefox38: --- → wontfix
status-firefox38.0.5: --- → wontfix
status-firefox39: --- → affected
status-firefox40: --- → affected
status-firefox41: --- → affected
status-firefox-esr38: --- → affected
Flags: needinfo?(milan)
Resolution: FIXED → ---
Target Milestone: mozilla35 → ---
I'll find somebody.
Flags: needinfo?(seth)
Flags: needinfo?(milan)
Kyle, Andrew, can you take a look at this once the other stuff is done?
Assignee: nobody → kfung
Sure, we'll get on it.
(Assignee)

Updated

3 years ago
Assignee: kfung → jmuizelaar
(Assignee)

Comment 29

3 years ago
So it looks like this is caused by the mFormat being different on the images depending on whether there's a an additional lock held at the same time that we would set it to B8G8R8X8. How is this supposed to work Seth? It seems somewhat fragile.
Flags: needinfo?(seth)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #29)
> So it looks like this is caused by the mFormat being different on the images
> depending on whether there's a an additional lock held at the same time that
> we would set it to B8G8R8X8. How is this supposed to work Seth? It seems
> somewhat fragile.

Yep, we can't safely replace the surface at the same time that other threads are accessing it. That's exactly what the raw access lock is designed to prevent.

The BMP decoder should be writing data that can safely be interpreted as B8G8R8A8. It shouldn't rely on the fact that we ignore the alpha data once the format gets changed.
Flags: needinfo?(seth)

Comment 31

3 years ago
So we fixed this in Firefox 35 and then it got unfixed? This is an externally reported sec-high. Can we get this a priority to be fixed in a current release? We've publicly disclosed aspects of this.
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 32

3 years ago
I'm not sure there's actually a security bug here. All that I can confirm is that we take different drawing paths depending on whether the image is B8G8R8A8 vs B8G8R8X8 and that produces different results.
Flags: needinfo?(jmuizelaar)
So, we initialize 24 bits thinking that we're dealing with B8G8R8X8, but the type ends up being left at B8G8R8A8 and then we try to draw using the 8 uninitialized bits?  If that's the case, I'm with Jeff on the "is this a security bug".

Comment 34

3 years ago
If there isn't a security issue here, can we unrate this and open it or do those and resolve it as "won't fix"?
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 35

3 years ago
I think we can do that.
Flags: needinfo?(jmuizelaar)
Group: core-security
Keywords: sec-high
You need to log in before you can comment on or make changes to this bug.