canvas drawImage needs sameorigin checks

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: sicking, Assigned: roc)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

See bug 288714 comment 36 and 42. This needs to be fixed before we make any
releases with canvas.
Flags: blocking1.8b2?
Flags: blocking-aviary1.1?
Assignee: nobody → vladimir
Upping severity since this is a security issue if released
Severity: normal → blocker

Comment 2

14 years ago
This seems to be covered in bug 291218 as well.
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.
(er, .toDataURL().)
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.
Blocks: 291218
Severity: blocker → normal
Flags: blocking1.8b2?
(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().
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.
A sameorigin check is trivial to do so there's no reason this should delay toDataURL
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.

Updated

14 years ago
Flags: blocking-aviary1.1? → blocking1.8b4?

Updated

14 years ago
Flags: blocking1.8b4? → blocking1.8b4+
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.

Updated

14 years ago
Flags: blocking1.8b4+ → blocking1.8b4-
Now that toDataURL has landed, I think we should get around to adding the security checks. Patch coming up...
Assignee: vladimir → roc
Posted patch fix (obsolete) — Splinter Review
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.
Attachment #213539 - Flags: superreview?(bzbarsky)
Attachment #213539 - Flags: review?(vladimir)
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.
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 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.
Attachment #213539 - Flags: superreview?(bzbarsky) → superreview+
Well, security trumps spec so we should do whatever we feel is safer here IMHO.
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.
(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.
Bug 329026 filed on the error message thing.
Posted patch fixSplinter Review
Updated patch to comments, plus using the canvas element principal now. And I mention the bug number in the comments, but I feel oppressed!
Attachment #213539 - Attachment is obsolete: true
Attachment #213653 - Flags: superreview?(bzbarsky)
Attachment #213653 - Flags: review?(vladimir)
Attachment #213539 - Flags: review?(vladimir)
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...
Attachment #213653 - Flags: review?(vladimir) → review+
Attachment #213653 - Flags: superreview?(bzbarsky) → superreview+
checked in
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
+        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.
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.
Yeah, the principal of the node isn't what we want...
(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.
(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". 
> 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.
You need to log in before you can comment on or make changes to this bug.