567511 - Setting document.domain breaks canvas getImageData functionality

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Seppo Vuolteenaho]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a5pre) Gecko/20100521 Minefield/3.7a5pre

// Running script on a domain, for example bugzilla.mozilla.org
// Setting domain breaks script. Commenting out line and it works again.
document.domain = 'bugzilla.mozilla.org';
var ctx = document.getElementById('mycanvas').getContext('2d');
ctx.drawImage(img, 0, 0);  // Draw image that was previously loaded, works

// Following line throws an error if document.domain was altered
var data = ctx.getImageData(20, 20, 1, 1).data;  // Immediately try to read it



Reproducible: Always

Steps to Reproduce:
// Running script on a domain, for example bugzilla.mozilla.org

// Setting the domain breaks script. Commenting out the line and it works again.
document.domain = 'bugzilla.mozilla.org';

var ctx = document.getElementById('mycanvas').getContext('2d');
ctx.drawImage(img, 0, 0);  // Draw image that was previously loaded, works

// Following line throws an error if document.domain was altered
var data = ctx.getImageData(20, 20, 1, 1).data;  // Immediately try to read it


Actual Results:  
Failed with exception: [Exception... "Security error"  code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)"  location: "http://bugzilla.mozilla.org/canvastest.html Line: xx"]


Expected Results:  
Should not fail. Works properly when document.domain is left unchanged. Also should be possible to widen the scope of the script permissions so we can load images from example.com into scripts run at example.com

Code also fails on Firefox 3.6.3

This was tested to work correctly in "Chromium 6.0.401.1 (47018) Ubuntu" and code works as expected in both cases.

Comment 1

[Seppo Vuolteenaho]

Attachment: Testing incorrect behaviour

Comment 2

[Seppo Vuolteenaho]

Attachment: image to be loaded into canvas

Comment 3

[Seppo Vuolteenaho]

Attachment: Testcase for bug

Changed example.png to point to uploaded image

Comment 4

[Seppo Vuolteenaho]

Attachment: Testcase for bug

Comment 5

[Seppo Vuolteenaho]

Attachment: Same code without altering document.domain

Comment 6

[Seppo Vuolteenaho]

Attachment: Setting document.domain to bug567511.bugzilla.mozilla.org doesn't work either

Comment 7

[Seppo Vuolteenaho]

Comment on attachment 446870 [details]
Setting document.domain to bug567511.bugzilla.mozilla.org doesn't work either

In JavaScript console:

Error: Security error = NS_ERROR_DOM_SECURITY_ERR
Source file: https://bug567511.bugzilla.mozilla.org/attachment.cgi?id=446870
Line: 15

Comment 8

[Matt Schemmel]

I've independently confirmed this behavior.

It appears that the Canvas element does not respect the CORS protocol, though the CORS spec specifically uses the canvas element as an example:
http://www.w3.org/TR/cors/

(See 'not taining the canvas')

Comment 9

[Matt Schemmel]

See also:
https://bugzilla.mozilla.org/show_bug.cgi?id=604496

Comment 10

[Matt Schemmel]

See also:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=11203

Comment 11

[Boris Zbarsky [:bzbarsky]]

> It appears that the Canvas element does not respect the CORS protocol

While it doesn't, that's not something tested by the testcases here, since bugzilla doesn't send CORS headers.

That said, the HTML5 draft calls for the value of document.domain to just be ignored when doing the taint check (see the use of "origin" vs "effective script origin" in http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#security-with-canvas-elements ).  We should probably either do that or figure out why we think it's undesirable and communicate that to hixie.

Changing that behavior would involve adding a flag to nsIPrincipal::Subsumes() (and perhaps Equals()) to ignore the document.domain value.  Or using whatever check it is compartments do, since they do something similar?

Is this something we should be trying to fix for 2.0?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We should definitely follow the spec here. We don't want document.domain usage to spread even more.

Don't have a strong opinion on 2.0 or not though.

/ Jonas

Comment 13

[Joe Drew (not getting mail)]

I'd love to have this fixed, but I don't think it needs to happen before 2.0 final.

Comment 14

[renam.passos]

Even "document.domain = document.domain;" causes getImageData to throw the security error.

Comment 15

[Joe Drew (not getting mail)]

I think comment 11 should be enough for a somewhat-experienced contributor to implement this. In particular, in addition to adding a flag to nsIPrincipal::Subsumes and ::Equals, you're going to want to *use* that flag in mozilla::CanvasUtils::DoDrawImageSecurityCheck(), and also in WebGLContext::DOMElementToImageSurface.

Comment 17

[Wojciech Hlibowicki]

Please fix this, the fact that this has been known since 2010 while all other browsers are not affected by this astounds me. 

Does FireFox even adhere to CORs? I have even set "Access-Control-Allow-Origin" to the domain itself with no affect. Seems like setting document.domain just makes all images "origin-clean=false"

Comment 18

[Wojciech Hlibowicki]

This bug still exists in FireFox 14 and 15.

Comment 19

[Joe Drew (not getting mail)]

Firefox does not obey CORS for 2D canvas elements. It should be significantly easier to do so now that bug 662599 is implemented, though (CORS for WebGL textures).

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Firefox does not obey CORS for 2D canvas elements

Sure it does, for drawImage.

Fixing this is not that difficult now.  DoDrawImageSecurityCheck needs to call subsumesIgnoringDomain() instead of subsumes().  I'll be happy to review a patch with a test... ;)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't consider document.domain when deciding whether to taint a canvas.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Now not breaking the harness too

Comment 23

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

Comment on attachment 655850 [details] [diff] [review]
Now not breaking the harness too

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

::: content/canvas/test/file_drawImage_document_domain.html
@@ +17,5 @@
> +      var data = ctx.getImageData(0, 0, 1, 1).data;
> +      parent.postMessage({ msg: "red", data: data[0] }, "*");
> +      parent.postMessage({ msg: "green", data: data[1] }, "*");
> +      parent.postMessage({ msg: "blue", data: data[2] }, "*");
> +      parent.postMessage({ msg: "alpha", data: data[3] }, "*");

I think the test would be a bit simpler and clearer if you just did parent.postMessage("rgba(" + data[0] + "," + data[1] + "," + data[2] + "," + data[3]/255.0 + ")"); and just checked that that's the right result.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Did that and pushed http://hg.mozilla.org/mozilla-central/rev/9e3f2ec9e8f7

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9e3f2ec9e8f7