Last Comment Bug 655836 - (CVE-2011-2986) Information leakage between canvases and origins [Windows D2D]
(CVE-2011-2986)
: Information leakage between canvases and origins [Windows D2D]
Status: VERIFIED FIXED
[sg:high][qa!] canvas: CVE-2011-2986,...
:
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: unspecified
: x86 Windows Vista
: -- critical (vote)
: mozilla10
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
Depends on:
Blocks: CVE-2011-3649
  Show dependency treegraph
 
Reported: 2011-05-09 14:27 PDT by nasalislarvatus3000
Modified: 2014-06-26 06:05 PDT (History)
13 users (show)
rforbes: sec‑bounty+
dveditz: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
fixed
affected
+
fixed
+
fixed
+
verified
wanted
unaffected
unaffected


Attachments
repro implementation (3.49 KB, application/xhtml+xml)
2011-05-09 14:29 PDT, nasalislarvatus3000
no flags Details
repro implementation (3.49 KB, application/xhtml+xml)
2011-05-09 14:32 PDT, nasalislarvatus3000
no flags Details
repro implementation (3.38 KB, text/html)
2011-05-09 14:36 PDT, nasalislarvatus3000
no flags Details
repro implementation (3.41 KB, text/html)
2011-05-09 14:45 PDT, nasalislarvatus3000
no flags Details
repro implementation (3.41 KB, text/html)
2011-05-09 15:04 PDT, nasalislarvatus3000
no flags Details
Reduced testcase (517 bytes, text/html)
2011-05-24 21:40 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
Flush dependent surfaces when a surface that was used as a source changes (7.48 KB, patch)
2011-06-08 14:26 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Flush dependent surfaces when a surface that was used as a source changes v2 (7.23 KB, patch)
2011-06-08 15:19 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review
Flush dependent surfaces when a surface that was used as a source changes v3 (7.58 KB, patch)
2011-06-09 13:17 PDT, Bas Schouten (:bas.schouten)
joe: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Use Snapshot API when drawing one Azure surface to another (1.71 KB, patch)
2011-10-04 07:16 PDT, Bas Schouten (:bas.schouten)
roc: review+
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Reftest (2.25 KB, patch)
2011-10-05 08:28 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review

Description nasalislarvatus3000 2011-05-09 14:27:06 PDT
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 2011-05-09 14:29:14 PDT
Created attachment 531137 [details]
repro implementation
Comment 2 nasalislarvatus3000 2011-05-09 14:32:10 PDT
Created attachment 531139 [details]
repro implementation
Comment 3 nasalislarvatus3000 2011-05-09 14:36:04 PDT
Created attachment 531141 [details]
repro implementation

changed the initial attachement from xhtml to html
Comment 4 nasalislarvatus3000 2011-05-09 14:45:28 PDT
Created attachment 531149 [details]
repro implementation

fixed a typo. feel free to remove the auto-comments for obsolete attachements.
Comment 5 nasalislarvatus3000 2011-05-09 15:04:28 PDT
Created attachment 531157 [details]
repro implementation

fixed invalid html that slipped in when converting from xhtml to html. sorry for inconvenience.
Comment 6 nasalislarvatus3000 2011-05-10 05:32:20 PDT
'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] 2011-05-24 12:27:32 PDT
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) 2011-05-24 12:33:32 PDT
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) (Exited; email my personal email if necessary) 2011-05-24 21:10:05 PDT
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) 2011-05-24 21:25:09 PDT
I'm not sure why this is security-sensitive?
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-24 21:40:01 PDT
Created attachment 534982 [details]
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) (Exited; email my personal email if necessary) 2011-05-24 21:40:38 PDT
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) 2011-05-24 22:09:57 PDT
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) 2011-05-24 22:12:45 PDT
Jeff, what do you think we should do?
Comment 15 Jeff Muizelaar [:jrmuizel] 2011-05-25 15:50:59 PDT
(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) 2011-05-25 20:27:16 PDT
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] 2011-05-26 10:58:08 PDT
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] 2011-05-26 12:42:25 PDT
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) 2011-05-26 19:33:31 PDT
(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] 2011-06-02 07:54:26 PDT
(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] 2011-06-06 16:31:44 PDT
[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) 2011-06-06 16:38:25 PDT
(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) 2011-06-06 16:39:05 PDT
I will have a fix for this before the end of wednesday.
Comment 25 Bas Schouten (:bas.schouten) 2011-06-08 14:26:13 PDT
Created attachment 538116 [details] [diff] [review]
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] 2011-06-08 14:43:42 PDT
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) 2011-06-08 15:19:59 PDT
Created attachment 538123 [details] [diff] [review]
Flush dependent surfaces when a surface that was used as a source changes v2
Comment 28 Jeff Muizelaar [:jrmuizel] 2011-06-09 07:26:08 PDT
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) 2011-06-09 13:17:38 PDT
Created attachment 538331 [details] [diff] [review]
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) 2011-06-09 14:30:37 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/87406ae57a1b
Comment 31 Bas Schouten (:bas.schouten) 2011-06-10 15:17:30 PDT
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) (Exited; email my personal email if necessary) 2011-06-10 23:26:44 PDT
I would like to have this on Aurora, it's a security bug.
Comment 33 Joe Drew (not getting mail) 2011-06-13 13:38:54 PDT
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 christian 2011-06-15 12:00:43 PDT
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) 2011-06-25 05:09:37 PDT
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) 2011-06-25 08:25:06 PDT
(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) 2011-06-27 17:44:07 PDT
Pushed to mozilla-aurora:

http://hg.mozilla.org/releases/mozilla-aurora/rev/12e254993fd2
Comment 38 chris hofmann 2011-07-19 12:55:49 PDT
nasalislarvatus3000,

if you are interested in a bug bounty for this please contact chofmann@mozilla.com

Thanks
Comment 39 nasalislarvatus3000 2011-07-25 06:10:53 PDT
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] 2011-08-10 16:16:09 PDT
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] 2011-08-17 12:23:23 PDT
This still needs a test case. Bas, can you make this into a reftest?
Comment 42 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 13:47:57 PDT
qa?: based on comment 41, is there something QA can do to verify this fix?
Comment 43 Bas Schouten (:bas.schouten) 2011-10-04 06:51:46 PDT
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) 2011-10-04 07:16:08 PDT
Created attachment 564546 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2011-10-04 11:15:41 PDT
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) (Exited; email my personal email if necessary) 2011-10-04 11:16:07 PDT
BTW you should have filed a new bug for this, not reopened this one.
Comment 47 Bas Schouten (:bas.schouten) 2011-10-04 11:40:41 PDT
(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) (Exited; email my personal email if necessary) 2011-10-04 12:37:39 PDT
Shouldn't you iterate through all the contexts?
Comment 49 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-10-04 14:57:22 PDT
Actually never mind, it's OK because you don't set srcSurf if that check fails. Carry on!
Comment 50 Bas Schouten (:bas.schouten) 2011-10-04 16:14:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a00620f3c3de
Comment 51 Bas Schouten (:bas.schouten) 2011-10-05 04:27:36 PDT
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) 2011-10-05 05:19:21 PDT
Merged to mozilla-central:

https://hg.mozilla.org/mozilla-central/rev/a00620f3c3de
Comment 53 Bas Schouten (:bas.schouten) 2011-10-05 08:28:31 PDT
Created attachment 564862 [details] [diff] [review]
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-05 14:16:46 PDT
Now that this has been re-resolved, is there something QA can do to verify the fix?
Comment 55 Bas Schouten (:bas.schouten) 2011-10-10 05:44:17 PDT
Landed on aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/cf8c888e1f0e
Comment 56 Bas Schouten (:bas.schouten) 2011-10-10 07:03:33 PDT
Landed on beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/ebb738ca552e
Comment 57 Bas Schouten (:bas.schouten) 2011-10-10 08:21:37 PDT
Pushed follow-up to beta to fix merge error:

https://hg.mozilla.org/releases/mozilla-beta/rev/16b8d68d3505
Comment 58 Daniel Veditz [:dveditz] 2011-11-07 20:49:32 PST
use CVE-2011-3649 for the Azure version of this bug
Comment 59 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-24 18:19:22 PST
Reftest checked in on -inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d727c6fb36eb
Comment 60 Bas Schouten (:bas.schouten) 2011-11-25 04:43:20 PST
https://hg.mozilla.org/mozilla-central/rev/d727c6fb36eb
Comment 61 Virgil Dicu [:virgil] [QA] 2012-01-24 07:31:28 PST
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) 2012-01-24 07:39:01 PST
(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] 2012-01-25 08:43:52 PST
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] 2013-07-19 17:35:36 PDT
rforbes-bugspam-for-setting-that-bounty-flag-20130719

Note You need to log in before you can comment on or make changes to this bug.