Closed Bug 1094536 (CVE-2014-8637) Opened 5 years ago Closed 4 months ago

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

Categories

(Core :: ImageLib, defect)

33 Branch
defect
Not set

Tracking

()

RESOLVED WONTFIX
Tracking Status
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

People

(Reporter: lcamtuf, Assigned: jrmuizel)

References

Details

(Keywords: csectype-disclosure, Whiteboard: [adv-main35+])

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?)
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".
Depends on: 1063129
Flags: needinfo?(dveditz) → needinfo?(mwu)
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.
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.
Whiteboard: [adv-main35+]
Alias: CVE-2014-8637
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Group: gfx-core-security
Flags: sec-bounty?
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
Target Milestone: --- → mozilla35
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
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: kfung → jmuizelaar
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)
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)
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".
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)
I think we can do that.
Flags: needinfo?(jmuizelaar)
Group: core-security
Keywords: sec-high

Closing bugs with b2g-master=affected as it is likely to be out dated.

Status: REOPENED → RESOLVED
Closed: 5 years ago4 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.