1094536 - (CVE-2014-8637) Apparent use of uninitialized memory when rendering BMPs on <canvas>

Status: RESOLVED WONTFIX

(Core :: Graphics: ImageLib, defect)

Description

[Michal Zalewski]

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/

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Can't reproduce this on Release or Nightly on Mac and Windows. It always gives me a "You're probably OK" message.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Can't reproduce on 64bit linux Nightly, but can on Release.

Comment 3

[Andrew McCreight [:mccr8]]

Somebody should figure out what fixed this, if in fact it is fixed.

Comment 4

[Andrew McCreight [:mccr8]]

Milan, is there somebody who can look at this?  Thanks.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

The second range actually looks less useful. Hmm. Not sure why mozregression wanted to continue.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

We should at least look into uplifting this to 34.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Between Michael and Seth, we should be able to figure this out, as it's related to bug 1063129.

Comment 11

[Seth Fowler [:seth] [:s2h]]

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

Comment 12

[Seth Fowler [:seth] [:s2h]]

(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

[Michael Wu [:mwu]]

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.

Comment 14

[Daniel Veditz [: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".

Comment 15

[Michael Wu [: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.

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

It seems like this is something that may still need to be addressed on older supported B2G branches given the sec rating?

Comment 18

[Michael Wu [:mwu]]

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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

I'm not sure what the next step is here, but it seems to me that something needs to be done still.

Comment 20

[Andrew McCreight [:mccr8]]

Seth, is there somebody who knows about the BMP decoder who could look at this?

Comment 21

[Seth Fowler [:seth] [:s2h]]

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

Comment 22

[Andrew McCreight [:mccr8]]

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

Comment 23

[Seth Fowler [:seth] [:s2h]]

Ah, my mistake! The summaries are pretty similar.

Comment 24

[Milan Sreckovic [:milan] (needinfo for best results)]

No from Jeff either, we're digging out of 37 issues.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

This straight-up reproduces for me on a current trunk build using the link in comment #0. This needs an owner AFAICT.

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

I'll find somebody.

Comment 27

[Milan Sreckovic [:milan] (needinfo for best results)]

Kyle, Andrew, can you take a look at this once the other stuff is done?

Comment 28

[Andrew Comminos [:acomminos]]

Sure, we'll get on it.

Comment 29

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 30

[Seth Fowler [:seth] [:s2h]]

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

Comment 31

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

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.

Comment 32

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 33

[Milan Sreckovic [:milan] (needinfo for best results)]

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

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

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

Comment 35

[Jeff Muizelaar [:jrmuizel]]

I think we can do that.

Comment 36

[Sylvestre Ledru [:Sylvestre]]

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