293244 - canvas drawImage needs sameorigin checks

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

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

See bug 288714 comment 36 and 42. This needs to be fixed before we make any
releases with canvas.

Comment 1

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

Upping severity since this is a security issue if released

Comment 2

[Anne (:annevk)]

This seems to be covered in bug 291218 as well.

Comment 3

[Hixie (not reading bugmail)]

Personally I think a better idea would be to make drawImage() always work but if
it fails a same origin check, flip a bit which then makes .asDataURL() throw a
security exception.

Until we implement .asDataURL() we can ship this without any problems IMHO.

Comment 4

[Hixie (not reading bugmail)]

(er, .toDataURL().)

Comment 5

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

Yeah, i guess as long as we don't have .toDataURL() implemented it isn't as bad. 

It would be usefull to only disable getting the data rather then blocking
drawImage entierly. However we have to make sure to also block things like
getPixel or the ability to submit the canvas as formdata if we ever get that idea.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #5)
> Yeah, i guess as long as we don't have .toDataURL() implemented it isn't as bad. 

Yep, I haven't implemented toDataURL on purpose because of this issue for 1.1
alpha; as soon as the preview release ships we'll revisit toDataURL() and
drawImage().

Comment 7

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

There's a lot of interest in supporting toDataURL or similar in 1.1 --- although
we could just make it chrome-only for 1.1.

Comment 8

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

A sameorigin check is trivial to do so there's no reason this should delay toDataURL

Comment 9

[Hixie (not reading bugmail)]

Note that getPixel() would never work anyway (one coordinate-space pixel can
easily map to multiple pixels on the real bitmap).

Submitting would indeed also need to be blocked if we ever do that. But
personally I would prefer we didn't do that, and just made people get the data
out using .toDataURL(). Having one single point of sale (as it were) makes it
easier to deal with this kind of issue.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Note that this is most likely only a problem once we implement toDataURL() or
whatever the saving mechanism may be (which may not even be in for 1.1), so this
might not be something that needs to block 1.8b4.

Comment 11

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

Now that toDataURL has landed, I think we should get around to adding the security checks. Patch coming up...

Comment 12

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

Attachment: fix

Make the canvas write-only if a) script draws a foreign-origin image into it or b) script draws a write-only canvas into it.

Comment 13

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

Comment on attachment 213539 [details] [diff] [review]
fix

>+nsCanvasRenderingContext2D::DoDrawImageSecurityCheck(nsIURI* aURI)
>+{
>+    nsCOMPtr<nsIScriptSecurityManager> ssm =
>+        do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID);
>+    if (ssm) {
>+        nsCOMPtr<nsIPrincipal> currentPrincipal;
>+        ssm->GetSubjectPrincipal(getter_AddRefs(currentPrincipal));

Please use the mCanvasElement principal (lives on nsINode) instead. I hate us using this global-variable mechanism and I think we should avoid it as much as possible.

Comment 14

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

I guess we could use the canvas element's principal but that's different from what the WHATWG spec currently says. Let's see if Ian has an opinion...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 213539 [details] [diff] [review]
fix

>Index: content/canvas/src/nsCanvasRenderingContext2D.cpp
>+nsCanvasRenderingContext2D::DoDrawImageSecurityCheck(nsIURI* >+    nsCOMPtr<nsIScriptSecurityManager> ssm =
>+        do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID);

nsContentUtils::GetSecurityManager(), maybe?  That's non-null after layout module init and until layout module shutdown....

>+    // XXX ERRMSG we need to report an error to developers here!

File a bug on these, and reference the bug# in the comments?

sr=bzbarsky modulo Ian's response.

Comment 16

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

Well, security trumps spec so we should do whatever we feel is safer here IMHO.

Comment 17

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

Sure. I'm just not sure what's safer.

If we use the canvas element principal, then my worry is that if someone gets hold of a foreign canvas, they can use it to siphon images from the same foreign location. But I suppose that if they get a foreign canvas object they won't actually be able to use it. So I might as well use that principal I guess. New patch coming.

Comment 18

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

(In reply to comment #15)
> >+    // XXX ERRMSG we need to report an error to developers here!
> 
> File a bug on these, and reference the bug# in the comments?

I will file a bug, but I don't really want to reference the bug# in the code.

Comment 19

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

Bug 329026 filed on the error message thing.

Comment 20

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

Attachment: fix

Updated patch to comments, plus using the canvas element principal now. And I mention the bug number in the comments, but I feel oppressed!

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 213653 [details] [diff] [review]
fix

r=me; if canvas/toDataURL is going to go on 1.8.1 we should get that on soon, because I have a rewrite of canvas waiting in the wings when we go all-Thebes...

Comment 22

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

checked in

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

+        rv = imageLoader->GetCurrentURI(uriOut);

why not get a principal from the node instead? That way, you could avoid the codebasePrincipal stuff in the other part of this file.

Comment 24

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

Does that actually do what you want? I would have assumed that in a document loaded from foo.com, an <img src="http://bar.com/baz.png"> still has its principal associated with foo.com.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Yeah, the principal of the node isn't what we want...

Comment 26

[Hixie (not reading bugmail)]

(In reply to comment #14)
> I guess we could use the canvas element's principal but that's different from
> what the WHATWG spec currently says. Let's see if Ian has an opinion...

I'm not sure what the difference is. Could you explain?


(In reply to comment #16)
> Well, security trumps spec so we should do whatever we feel is safer here 
> IMHO.

Sure, but the spec is trying to cover security here too. Please don't consider the spec immutable. If it has problems, then I've made a mistake, and you should tell me, so it can be fixed, and so everyone can agree on how to implement it.

Comment 27

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

(In reply to comment #26)
> (In reply to comment #14)
> > I guess we could use the canvas element's principal but that's different
> > from what the WHATWG spec currently says. Let's see if Ian has an opinion...
> 
> I'm not sure what the difference is. Could you explain?

I believe the principal of the canvas element corresponds roughly to "the domain of the document in which it was loaded" or "the domain of the window of the script that created it". 

Comment 28

[Boris Zbarsky [:bzbarsky]]

> I'm not sure what the difference is. Could you explain?a

The canvas element's principal is the security context of its ownerDocument.

Ian, the problem with using the "calling script" as the principal is that that presupposes that all access to the canvas API is always done in JavaScript.  If we want to require that, that's fine -- we should remove the IDL and implement the whole thing completely in classinfo.  Or leave the IDL and document that non-JS code calling those methods would lead to security issues.  Or whatever.