655836 - (CVE-2011-2986) Information leakage between canvases and origins [Windows D2D]

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[nasalislarvatus3000]

User-Agent:       Mozilla/5.0 (Windows NT 6.0; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: Mozilla/5.0 (Windows NT 6.0; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

Information leakage between canvases and origins.

Reproducible: Always

Steps to Reproduce:
1. have a canvasA which is inserted into DOM tree and visible
2. draw green rect onto canvasA
3. have canvasB, which is not inserted into DOM tree
4. draw canvasA onto canvasB
5. draw red rect and different-origin image onto canvasA

A repro implementation is attached.


Actual Results:  
Red rect and different-origin image on canvasB (which has origin-clean flag true) if accessed 'later than immediately' 


Expected Results:  
Green rect and no different-origin image on canvasB (which has origin-clean flag true)

- GPU: GeForce 9650M GT, Driver Version 270.61
- also reproducable in FF4.0
- bug does not occurs when canvasA meets any of the following conditions:
    . is not in DOM tree
    . has css display: none
    . has css visible: hidden
    . has css opacity less than 1 and the canvasA is not (fully?)
        overlayed by another element

Comment 1

[nasalislarvatus3000]

Attachment: repro implementation

Comment 2

[nasalislarvatus3000]

Attachment: repro implementation

Comment 3

[nasalislarvatus3000]

Attachment: repro implementation

changed the initial attachement from xhtml to html

Comment 4

[nasalislarvatus3000]

Attachment: repro implementation

fixed a typo. feel free to remove the auto-comments for obsolete attachements.

Comment 5

[nasalislarvatus3000]

Attachment: repro implementation

fixed invalid html that slipped in when converting from xhtml to html. sorry for inconvenience.

Comment 6

[nasalislarvatus3000]

'later than immediately' as mentioned in the repro steps can mean up to several seconds later. It appears, that the higher the system load, the longer has to be waited. In the repro attachement, I used a timeout of 1000ms, which in some cases might be to short.

Comment 7

[Daniel Veditz [:dveditz]]

I can't reproduce this (but I'm not on Windows). I end up with a picture of monkeys (canvasA) and a green rectangle (img made from canvasB) which is what I'd expect and what I see in other browsers. When you create canvasB with canvasA as a source that's a snapshot, it should not be affected by future changes to canvasA; since it should never be "dirty" you're allowed to get a dataURL out of it.

If this can be reproduced on Windows I wonder if hardware acceleration has anything to do with it. Or is there some "layers" optimization where the two canvases share a copy-on-write surface and we forget to unhook them when one of them changes?

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Tried on both Mac and Linux, in Nightly, can't reproduce so far. Needs to be tried on windows. Also, I would like to know if you can reproduce this in Nightly?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

On Windows, it only shows up when you're using D2D. I think this is a bug in our D2D backend, presumably some kind of issue with synchronization and snapshotting like dveditz said. Should be fairly easy to analyze.

Comment 10

[Bas Schouten (:bas.schouten)]

I'm not sure why this is security-sensitive?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Reduced testcase

This testcase shows the bug and doesn't require images or timeouts. Compare the results with/without D2D enabled.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's security-sensitive because we don't want pages in domain A to read the pixel data of images in domain B.

Comment 13

[Bas Schouten (:bas.schouten)]

So, the reason for this bug is as follows:

Canvas A receives a drawing command, it executed it.

Canvas A is drawn to Canvas B, Canvas B flushed canvas A and puts a command in the D2D pipeline to draw Canvas A to it.

Canvas A received another drawing command.

Canvas A gets flushed.
Canvas B gets flushed.

When Canvas B gets flushed and the draw of Canvas A to Canvas B actually gets executed, Canvas A's contents will have changed.

There's a couple of things that can be done:

Option A. Flush Canvas B after having executed the Fill call inside the D2D backend from Canvas A. This is super-easy but not great performance wise.

Option B. Create a copy of Canvas A in the D2D backend and draw that to Canvas B. This is also easy, but again not great performance wise.

Option C. Create two lists on each cairo surface, one with surfaces currently having a draw call queued depending on it, and not having flushed. And one list of surfaces it currently has a draw call enqueued for that haven't been flushed yet.

If Canvas A depends on Canvas B for a draw call and Canvas B gets a new drawing call it will flush any surfaces that depend on it (that will include Canvas A). If Canvas A gets flushed first it will tell Canvas B that it no longer needs to flush Canvas A if it gets a drawing call.

Option C is the best for performance, but the hardest to implement.

Comment 14

[Bas Schouten (:bas.schouten)]

Jeff, what do you think we should do?

Comment 15

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #14)
> Jeff, what do you think we should do?

Bas, I'm having a bit of trouble understanding your example. Can you lay it out a little more concretely using the D2D api?

Comment 16

[Bas Schouten (:bas.schouten)]

Imagine I have rt1 and rt2 (render target one and two), backed by bm1 and bm2 (bm1 and bm2). Now think of the following scenario:

rt1->FillRect()
rt1->Flush()
rt2->DrawBitmap(bm1);
rt1->FillRect();
rt1->Flush();
rt2->Flush();

When the DrawBitmap is placed in the command queue, bm1 looks the way we want it. When it is -executed- however (on rt2->Flush()), bm1 has been updated with the second FillRect, so different contents will be drawn than when the DrawBitmap was enqueued.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

I'm surprised D2D doesn't take care of this for us, I can't see a use case for the semantics it gives.

Anyways, I think something like option C is going to be the best thing here. I don't yet understand why we need two lists, but the premise of flushing when things change and someone depends on the old version is good.

Comment 18

[Jeff Muizelaar [:jrmuizel]]

It seems like we should be able to solve this by having a single list on surfaces that lists their users. (surf1->drawImage(surf2) will add surf1 as a user to surf2). Before surf2 is dirtied it flushes all of it's users and removes them from the list of users.

Is there a scenario this doesn't handle?

Comment 19

[Bas Schouten (:bas.schouten)]

(In reply to comment #18)
> It seems like we should be able to solve this by having a single list on
> surfaces that lists their users. (surf1->drawImage(surf2) will add surf1 as
> a user to surf2). Before surf2 is dirtied it flushes all of it's users and
> removes them from the list of users.
> 
> Is there a scenario this doesn't handle?

Yes, the downside is if surf2 is dirtied, it doesn't need to flush users that have already flushed. (i.e. if surf1 is flushed, we want it to remove itself from surf2).

Comment 20

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #19)
> (In reply to comment #18)
> > It seems like we should be able to solve this by having a single list on
> > surfaces that lists their users. (surf1->drawImage(surf2) will add surf1 as
> > a user to surf2). Before surf2 is dirtied it flushes all of it's users and
> > removes them from the list of users.
> > 
> > Is there a scenario this doesn't handle?
> 
> Yes, the downside is if surf2 is dirtied, it doesn't need to flush users
> that have already flushed. (i.e. if surf1 is flushed, we want it to remove
> itself from surf2).

I'm not sure whether this is a good trade off or not.

Comment 21

[Daniel Veditz [:dveditz]]

[sg:high] assuming you can actually read the wrong data back out of the canvas. If an attacker were to sample the supposedly-same-origin canvasB would they get the pixels the user sees or pixels from some internal representation of what should be there?

Comment 23

[Bas Schouten (:bas.schouten)]

(In reply to comment #21)
> [sg:high] assuming you can actually read the wrong data back out of the
> canvas. If an attacker were to sample the supposedly-same-origin canvasB
> would they get the pixels the user sees or pixels from some internal
> representation of what should be there?

If an attacker were to use delayed getImageData on canvasB they would presumably get the pixels the user sees, i.e. the different-origin image.

Comment 24

[Bas Schouten (:bas.schouten)]

I will have a fix for this before the end of wednesday.

Comment 25

[Bas Schouten (:bas.schouten)]

Attachment: Flush dependent surfaces when a surface that was used as a source changes

This patch should fix the issue, the two cases listed here are indeed fixed.

Comment 26

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 538116 [details] [diff] [review]
Flush dependent surfaces when a surface that was used as a source changes

Please use cairo_list_foreach_entry or cairo_list_foreach_entry_safe as appropriate.

Comment 27

[Bas Schouten (:bas.schouten)]

Attachment: Flush dependent surfaces when a surface that was used as a source changes v2

Comment 28

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 538123 [details] [diff] [review]
Flush dependent surfaces when a surface that was used as a source changes v2


>+    // Other d2d surfaces which depend on this one and need to be flushed if
>+    // it is drawn to.
>+    cairo_list_t dependent_surfaces;

It would be nice for this comment to have brief description of the problem that it's trying to solve.

Comment 29

[Bas Schouten (:bas.schouten)]

Attachment: Flush dependent surfaces when a surface that was used as a source changes v3

Updated with review comments.

Comment 30

[Joe Drew (not getting mail)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/87406ae57a1b

Comment 31

[Bas Schouten (:bas.schouten)]

Landed on central.

http://hg.mozilla.org/mozilla-central/rev/1e3af440ce23

I guess a decision still needs to be made if we want to merge this to aurora.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I would like to have this on Aurora, it's a security bug.

Comment 33

[Joe Drew (not getting mail)]

Comment on attachment 538331 [details] [diff] [review]
Flush dependent surfaces when a surface that was used as a source changes v3

Pretend this is roc asking for Aurora in comment 32. This was reviewed by Jeff.

Comment 34

[LegNeato]

Comment on attachment 538331 [details] [diff] [review]
Flush dependent surfaces when a surface that was used as a source changes v3

Approved for mozilla-aurora

Comment 35

[Jonathan Kew [:jfkthame]]

I considered pushing this to aurora, but it doesn't apply cleanly:

patching file gfx/cairo/cairo/src/cairo-d2d-private.h
Hunk #1 FAILED at 43
Hunk #2 FAILED at 75
Hunk #3 FAILED at 133
3 out of 3 hunks FAILED -- saving rejects to file gfx/cairo/cairo/src/cairo-d2d-private.h.rej
patching file gfx/cairo/cairo/src/cairo-d2d-surface.cpp
Hunk #3 FAILED at 2344
1 out of 6 hunks FAILED -- saving rejects to file gfx/cairo/cairo/src/cairo-d2d-surface.cpp.rej

The merge doesn't look difficult, but I think it'd better be done by someone who knows this code, in case there are interactions/dependencies on other changes that have been happening in there.

Comment 36

[Bas Schouten (:bas.schouten)]

(In reply to comment #35)
> I considered pushing this to aurora, but it doesn't apply cleanly:
> 
> patching file gfx/cairo/cairo/src/cairo-d2d-private.h
> Hunk #1 FAILED at 43
> Hunk #2 FAILED at 75
> Hunk #3 FAILED at 133
> 3 out of 3 hunks FAILED -- saving rejects to file
> gfx/cairo/cairo/src/cairo-d2d-private.h.rej
> patching file gfx/cairo/cairo/src/cairo-d2d-surface.cpp
> Hunk #3 FAILED at 2344
> 1 out of 6 hunks FAILED -- saving rejects to file
> gfx/cairo/cairo/src/cairo-d2d-surface.cpp.rej
> 
> The merge doesn't look difficult, but I think it'd better be done by someone
> who knows this code, in case there are interactions/dependencies on other
> changes that have been happening in there.

Likely due to the cairo merge. I'm checking out aurora (takes a while) and will have a look.

Comment 37

[Bas Schouten (:bas.schouten)]

Pushed to mozilla-aurora:

http://hg.mozilla.org/releases/mozilla-aurora/rev/12e254993fd2

Comment 38

[chris hofmann]

nasalislarvatus3000,

if you are interested in a bug bounty for this please contact [e-mail]

Thanks

Comment 39

[nasalislarvatus3000]

chris hofmann,

obviously my initial reply email did not get delivered. I sent again yesterday, using two different mail providers. Sorry for inconvenience and thank you!

Comment 40

[Daniel Veditz [:dveditz]]

Why is the bug still open? Looks like we've got a trunk checkin in the Firefox 7 timeframe (comment 31) and an aurora checkin during the Firefox 6 cycle (comment 37) and I don't see any sign of a reopen -- assuming it just got missed.

Comment 41

[Jeff Muizelaar [:jrmuizel]]

This still needs a test case. Bas, can you make this into a reftest?

Comment 42

[u279076]

qa?: based on comment 41, is there something QA can do to verify this fix?

Comment 43

[Bas Schouten (:bas.schouten)]

We reintroduced this issue when during Azure review processing I agreed to not use the Snapshot() API when using DrawImage between two canvases. This causes Azure's mechanisms to prevent this from happening not to kick in.

Comment 44

[Bas Schouten (:bas.schouten)]

Attachment: Use Snapshot API when drawing one Azure surface to another

This is the fix for the reintroduction in Azure. I'll add a reftest to the bug later today, as this issue reoccuring has proven how important it was for this bug to get a reftest!

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 564546 [details] [diff] [review]
Use Snapshot API when drawing one Azure surface to another

Review of attachment 564546 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +3683,5 @@
>    }
>  
> +  // Special case for Canvas, which could be an Azure canvas!
> +  if (canvas) {
> +    if (canvas->CountContexts() == 1) {

Why this check?

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BTW you should have filed a new bug for this, not reopened this one.

Comment 47

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #45)
> Comment on attachment 564546 [details] [diff] [review] [diff] [details] [review]
> Use Snapshot API when drawing one Azure surface to another
> 
> Review of attachment 564546 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
> @@ +3683,5 @@
> >    }
> >  
> > +  // Special case for Canvas, which could be an Azure canvas!
> > +  if (canvas) {
> > +    if (canvas->CountContexts() == 1) {
> 
> Why this check?

Well, I figured otherwise the AtIndex function might go bad? Just safety really, do you think it doesn't matter?

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Shouldn't you iterate through all the contexts?

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually never mind, it's OK because you don't set srcSurf if that check fails. Carry on!

Comment 50

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a00620f3c3de

Comment 51

[Bas Schouten (:bas.schouten)]

Comment on attachment 564546 [details] [diff] [review]
Use Snapshot API when drawing one Azure surface to another

We should get this fix on Aurora and Beta.

Comment 52

[Bas Schouten (:bas.schouten)]

Merged to mozilla-central:

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

Comment 53

[Bas Schouten (:bas.schouten)]

Attachment: Reftest

This is a reftest for this bug. I don't think we can push it to a public repo yet though, since this pretty much also demos the exploit.

Comment 54

[u279076]

Now that this has been re-resolved, is there something QA can do to verify the fix?

Comment 55

[Bas Schouten (:bas.schouten)]

Landed on aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/cf8c888e1f0e

Comment 56

[Bas Schouten (:bas.schouten)]

Landed on beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/ebb738ca552e

Comment 57

[Bas Schouten (:bas.schouten)]

Pushed follow-up to beta to fix merge error:

https://hg.mozilla.org/releases/mozilla-beta/rev/16b8d68d3505

Comment 58

[Daniel Veditz [:dveditz]]

use CVE-2011-3649 for the Azure version of this bug

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Reftest checked in on -inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d727c6fb36eb

Comment 60

[Bas Schouten (:bas.schouten)]

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

Comment 61

[Virgil Dicu [:virgil] [QA]]

Can this be verified using the reduced test case attachment or is that obsolete since the bug was opened and fixed again? Would this be enough to verify this?

Comment 62

[Bas Schouten (:bas.schouten)]

(In reply to Virgil Dicu [QA] from comment #61)
> Can this be verified using the reduced test case attachment or is that
> obsolete since the bug was opened and fixed again? Would this be enough to
> verify this?

The symptoms of the bug remained identical, the reduced testcase is still valid.

Comment 63

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (Windows NT 6.0; rv:10.0) Gecko/20100101 Firefox/10.0 beta 6

Verified on Firefox 10 beta 6 on Windows Vista using reduced test case from comment 11.

Red-green rectangle are displayed with/without 2d.disabled-same as other browsers.

Comment 64

[Raymond Forbes[:rforbes]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719