Note: There are a few cases of duplicates in user autocompletion which are being worked on.
Bug 655836 (CVE-2011-2986)

Information leakage between canvases and origins [Windows D2D]

VERIFIED FIXED in Firefox 6

Status

()

Core
Canvas: 2D
--
critical
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: nasalislarvatus3000, Assigned: bas)

Tracking

unspecified
mozilla10
x86
Windows Vista
Points:
---
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox5 affected, firefox6+ fixed, firefox7 affected, firefox8+ fixed, firefox9+ fixed, firefox10+ verified, status2.0 wanted, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:high][qa!] canvas: CVE-2011-2986, azure: CVE-2011-3649 [inbound])

Attachments

(6 attachments, 5 obsolete attachments)

3.41 KB, text/html
Details
517 bytes, text/html
Details
7.23 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
7.58 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
1.71 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.25 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
Created attachment 531137 [details]
repro implementation
(Reporter)

Comment 2

6 years ago
Created attachment 531139 [details]
repro implementation
Attachment #531137 - Attachment is obsolete: true
(Reporter)

Comment 3

6 years ago
Created attachment 531141 [details]
repro implementation

changed the initial attachement from xhtml to html
Attachment #531139 - Attachment is obsolete: true
(Reporter)

Comment 4

6 years ago
Created attachment 531149 [details]
repro implementation

fixed a typo. feel free to remove the auto-comments for obsolete attachements.
Attachment #531141 - Attachment is obsolete: true
(Reporter)

Comment 5

6 years ago
Created attachment 531157 [details]
repro implementation

fixed invalid html that slipped in when converting from xhtml to html. sorry for inconvenience.
Attachment #531149 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #531157 - Attachment mime type: text/plain → text/html
(Reporter)

Comment 6

6 years ago
'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.
Attachment #531137 - Attachment mime type: application/octet-stream → application/xhtml+xml
Attachment #531141 - Attachment mime type: text/plain → application/xhtml+xml
Attachment #531149 - Attachment mime type: text/plain → application/xhtml+xml
Attachment #531139 - Attachment mime type: application/octet-stream → application/xhtml+xml
Attachment #531141 - Attachment mime type: application/xhtml+xml → text/html
Attachment #531149 - Attachment mime type: application/xhtml+xml → text/html
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?
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?
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.
Assignee: nobody → bas.schouten
(Assignee)

Comment 10

6 years ago
I'm not sure why this is security-sensitive?
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
It's security-sensitive because we don't want pages in domain A to read the pixel data of images in domain B.
(Assignee)

Comment 13

6 years ago
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.
(Assignee)

Comment 14

6 years ago
Jeff, what do you think we should do?
(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?
(Assignee)

Comment 16

6 years ago
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.
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.
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?
(Assignee)

Comment 19

6 years ago
(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).
(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.
[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?
Summary: Information leakage between canvases and origins → Information leakage between canvases and origins [Windows D2D]
Whiteboard: [sg:high]
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
status2.0: --- → wanted
status-firefox5: --- → affected
status-firefox6: --- → affected
status-firefox7: --- → affected
tracking-firefox6: --- → ?
(Assignee)

Comment 23

6 years ago
(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.
(Assignee)

Comment 24

6 years ago
I will have a fix for this before the end of wednesday.
(Assignee)

Comment 25

6 years ago
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.
Attachment #538116 - Flags: review?(jmuizelaar)
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.
(Assignee)

Comment 27

6 years ago
Created attachment 538123 [details] [diff] [review]
Flush dependent surfaces when a surface that was used as a source changes v2
Attachment #538116 - Attachment is obsolete: true
Attachment #538116 - Flags: review?(jmuizelaar)
Attachment #538123 - Flags: review?(jmuizelaar)
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.
Attachment #538123 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 29

6 years ago
Created attachment 538331 [details] [diff] [review]
Flush dependent surfaces when a surface that was used as a source changes v3

Updated with review comments.
http://hg.mozilla.org/integration/mozilla-inbound/rev/87406ae57a1b
Whiteboard: [sg:high] → [sg:high][inbound]
(Assignee)

Comment 31

6 years ago
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.
Whiteboard: [sg:high][inbound] → [sg:high]
I would like to have this on Aurora, it's a security bug.
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.
Attachment #538331 - Flags: review+
Attachment #538331 - Flags: approval-mozilla-beta?
Attachment #538331 - Flags: approval-mozilla-beta? → approval-mozilla-aurora?
status-firefox7: affected → fixed
tracking-firefox6: ? → +

Comment 34

6 years ago
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
Attachment #538331 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
(Assignee)

Comment 36

6 years ago
(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.
(Assignee)

Comment 37

6 years ago
Pushed to mozilla-aurora:

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

Updated

6 years ago
status-firefox6: affected → fixed

Comment 38

6 years ago
nasalislarvatus3000,

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

Thanks
(Reporter)

Comment 39

6 years ago
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!
Alias: CVE-2011-2986
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
This still needs a test case. Bas, can you make this into a reftest?
qa?: based on comment 41, is there something QA can do to verify this fix?
Whiteboard: [sg:high] → [sg:high][qa?]
(Assignee)

Comment 43

6 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

6 years ago
Blocks: 659278
(Assignee)

Updated

6 years ago
status-firefox10: --- → affected
status-firefox7: fixed → affected
status-firefox8: --- → affected
status-firefox9: --- → affected
tracking-firefox10: --- → ?
tracking-firefox8: --- → ?
tracking-firefox9: --- → ?
(Assignee)

Comment 44

6 years ago
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!
Attachment #564546 - Flags: review?(roc)
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?
Attachment #564546 - Flags: review?(roc) → review+
BTW you should have filed a new bug for this, not reopened this one.
(Assignee)

Comment 47

6 years ago
(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?
Shouldn't you iterate through all the contexts?
Actually never mind, it's OK because you don't set srcSurf if that check fails. Carry on!
(Assignee)

Comment 50

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a00620f3c3de
(Assignee)

Comment 51

6 years ago
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.
Attachment #564546 - Flags: approval-mozilla-beta?
Attachment #564546 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 52

6 years ago
Merged to mozilla-central:

https://hg.mozilla.org/mozilla-central/rev/a00620f3c3de
(Assignee)

Comment 53

6 years ago
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.
Attachment #564862 - Flags: review?(roc)
Attachment #564862 - Flags: review?(roc) → review+
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Now that this has been re-resolved, is there something QA can do to verify the fix?

Updated

6 years ago
status-firefox10: affected → fixed
tracking-firefox10: ? → +
tracking-firefox8: ? → +
tracking-firefox9: ? → +

Updated

6 years ago
Attachment #564546 - Flags: approval-mozilla-beta?
Attachment #564546 - Flags: approval-mozilla-beta+
Attachment #564546 - Flags: approval-mozilla-aurora?
Attachment #564546 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 55

6 years ago
Landed on aurora:

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

Comment 56

6 years ago
Landed on beta:

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

Updated

6 years ago
status-firefox8: affected → fixed
status-firefox9: affected → fixed
(Assignee)

Comment 57

6 years ago
Pushed follow-up to beta to fix merge error:

https://hg.mozilla.org/releases/mozilla-beta/rev/16b8d68d3505
use CVE-2011-3649 for the Azure version of this bug
Whiteboard: [sg:high][qa?] → [sg:high][qa?] canvas: CVE-2011-2986, azure: CVE-2011-3649

Updated

6 years ago
Target Milestone: --- → mozilla10
Group: core-security
Group: core-security
Flags: in-testsuite?
Reftest checked in on -inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d727c6fb36eb
Whiteboard: [sg:high][qa?] canvas: CVE-2011-2986, azure: CVE-2011-3649 → [sg:high][qa?] canvas: CVE-2011-2986, azure: CVE-2011-3649 [inbound]
(Assignee)

Comment 60

6 years ago
https://hg.mozilla.org/mozilla-central/rev/d727c6fb36eb
Whiteboard: [sg:high][qa?] canvas: CVE-2011-2986, azure: CVE-2011-3649 [inbound] → [sg:high][qa+] canvas: CVE-2011-2986, azure: CVE-2011-3649 [inbound]
Group: core-security
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?
(Assignee)

Comment 62

6 years ago
(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.
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.
Status: RESOLVED → VERIFIED
status-firefox10: fixed → verified
Whiteboard: [sg:high][qa+] canvas: CVE-2011-2986, azure: CVE-2011-3649 [inbound] → [sg:high][qa!] canvas: CVE-2011-2986, azure: CVE-2011-3649 [inbound]
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.