Closed Bug 444641 Opened 16 years ago Closed 13 years ago

Canvas's .getImageData throws when accessing <img src="file://...">

Categories

(Core :: Graphics: Canvas2D, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: Dolske, Assigned: bzbarsky)

References

Details

Attachments

(4 files, 1 obsolete file)

I have a HTML testcase for canvas which includes an image with a relative path: <img src="blah.png">.

When the HTML and image are served over HTTP, all works as expected. But when the testcase+image are loaded from my Desktop, getImageData throws.

Specifically, http://people.mozilla.com/~dolske/tmp/test.html works, but if you save test.html and test-my30.png locally it fails

Between this and bug 417836, canvas testcases are a PITA.
Flags: blocking1.9.0.2?
Not blocking, but wanted.

Vlad, who can own this?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.2?
Flags: blocking1.9.0.2-
This is likely a WONTFIX -- while http://people.mozilla.com/~dolske/tmp/test.html and http://people.mozilla.com/~dolske/tmp/blah.png are same-origin, file:///test.html and file:///blah.png are two different origins.  bz, can you confirm?  This was done to tighten up file:/// security; it makes developing a pain though.

Perhaps a good fix would be to add a hidden pref to relax the file:/// origin policy to either be same-dir or everything, so that developers can set it to avoid having to set up a local http server?
Ugh, I thought the file:// same-origin stuff was ok as long as the directories were the same.  But I suppose that usefully prevents a saved ~/Desktop/evilpage.html from accessing ~/Desktop/secret-sexy-lolcat.jpg (ie, where ~/Desktop is a default download location).
file:// origins are per-file.  When doing a load of a file in the same directory as another file from said other file, we set up a special principal for the loadee that is same-origin with the loader, which is what allows the same-directory stuff to work when it needs to.

This magic only happens for document loads, though, not for image loads.  We should do it all places where we inherit the principal (as for data:, etc), but that wouldn't help this situation for the reasons described in bug 417836.

> Perhaps a good fix would be to add a hidden pref

We've had the boolean security.fileuri.strict_origin_policy pref in ever since the new-world file:// stuff landed.
Blocks: 230606
Depends on: 417836
(In reply to comment #2)
> file:///test.html and file:///blah.png are two different origins.

Sorry, I don't understand that. I run into the same error and it's hard to test applications when they don't work locally as well.

If I have a local file and load another local file even with a relative path, I expect this to be handled like it is: from the same origin.
> Sorry, I don't understand that.
...
> it's hard to test applications when they don't work locally as well.

You DID read comment 4, right?  It explains everything in detail.
(In reply to comment #7)
> You DID read comment 4, right?

Hmmm... yes. But i didn't fully got it the first time. Thanks for pointing to that again.

BTW. Setting the "security.fileuri.strict_origin_policy" pref does the trick.
So I was just thinking...  I think what we should do is to store the loader principal in the imgRequestProxy handed out by loadImage.  Then when the imgRequestProxy computes its actual principal, instead of just grabbing the one from the imgRequest's channel it should grab, then do the file/load check and if it passes use the loader principal.  That should allow us to keep coalescing imgRequests across loader principals sanely.  It won't help with javascript: much, but I don't think we plan to unsandbox those anyway.  It'll help with data: images.

Joe, Bobby, thoughts?
Joe knows more about this stuff, so I'll defer to him.

Joe?
Boris, is there a problem with just using the loader's principal all the time? Or is this a case of two potentially disjoint principals, and we want to allow things in the union of their capabilities?
Target Milestone: --- → mozilla1.9.1b4
This is a case of some entity (the loader) loading some image, possibly from a different origin.  We then want the image to have the image's principal, except put in a special case if it's a file:// image and the loader is also file:// and the CheckMayLoad check passes; in that case use the loader's principal.

If we just always use the loader principal, you could link to an image on a different site and then read it.  We don't want that.
I came across some example code that used
netscape.security.PrivilegeManager.enablePrivilege("UniversalBrowserRead") to allow the page to locally open an image and access its contents with failing, though it does handily ask the user if its ok. Avoids user having to adjust their own settings obviously.

Sensible? Mad? Is there a better method coming?

(platform could expand to all)
John, that will work for now, but that functionality will likely go away sometime in the not too distant future (12-18 months at most, I'd hope).
(In reply to comment #18)
> *** Bug 666244 has been marked as a duplicate of this bug. ***

Fair enough, but it would be good to see the detail in comment 4 worked into the documentation: http://kb.mozillazine.org/Security.fileuri.strict_origin_policy. As I currently read that page it suggests that images from the current directory or subdirectory may be accessed from file:// links when it is true.
Depends on: 671906
The other option is to put an IsAboutBlank on nsContentUtils.  Let me know if you prefer that.
Attachment #546279 - Flags: review?(jst)
Attachment #546279 - Flags: review?(jduell.mcbugs)
Daniel, please look this over from a security perspective.  In particular, there's a functional change to the way file:// is handled for <object> elements to make them more like <iframe>s.
Attachment #546280 - Flags: review?(jst)
Attachment #546280 - Flags: review?(dveditz)
This is the actual meat of the change.  Daniel, this also needs a security-perspective look.
Attachment #546282 - Flags: review?(joe)
Attachment #546282 - Flags: review?(dveditz)
Daniel, let me know if you think I should just try to schedule a security review here.
Assignee: nobody → bzbarsky
OS: Mac OS X → All
Priority: -- → P2
Hardware: x86 → All
Target Milestone: mozilla1.9.1b4 → ---
Comment on attachment 546283 [details] [diff] [review]
part 4.  Remove the data: special-casing for images in canvas, since we now set the right principal for data: images.

Review of attachment 546283 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #546283 - Flags: review?(roc) → review+
Whiteboard: [need review]
Attachment #546279 - Flags: review?(jst) → review+
Comment on attachment 546282 [details] [diff] [review]
part 3.  Propagate the loading principal to the image channel as needed.

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

I'd prefer that in this new code you're adding, you use bool instead of PRBool wherever possible.

::: modules/libpr0n/src/imgLoader.cpp
@@ +430,5 @@
>  // given loading principal, and false if the request may not be reused due
>  // to CORS.
>  static bool
> +ValidateCORS(imgRequest* request, PRBool forcePrincipalCheck, PRInt32 corsmode,
> +             nsIPrincipal* loadingPrincipal)

Maybe we should change the name - ValidateCORSAndPrincipal?

@@ +460,5 @@
>  static nsresult NewImageChannel(nsIChannel **aResult,
> +                                // If aForcePrincipalCheckForCacheEntry is
> +                                // true, then we force a principal check even
> +                                // when not using CORS before assuming we have
> +                                // a cache hit.

This implies that the incoming value of aForcePrincipalCheckForCacheEntry matters, which it seems is not the case.
Attachment #546282 - Flags: review?(joe) → review+
> I'd prefer that in this new code you're adding, you use bool 

OK.

> Maybe we should change the name - ValidateCORSAndPrincipal?

OK.

> This implies that the incoming value of aForcePrincipalCheckForCacheEntry
> matters

I'll make it clearer that this is an out param.
Attachment #546784 - Flags: review?(dveditz)
Attachment #546282 - Attachment is obsolete: true
Attachment #546282 - Flags: review?(dveditz)
Comment on attachment 546280 [details] [diff] [review]
part 2.  Factor out the channel owner setting from docshell so other consumers can use it too.

Looks good, one nit:

- In nsContentUtils::SetUpChannelOwner():

+  // XXX: Is seems wrong that the owner is ignored - even if one is

Wanna s/Is/It/ here while you're moving this around?

r=jst
Attachment #546280 - Flags: review?(jst) → review+
> Wanna s/Is/It/ here while you're moving this around?

Done.
Comment on attachment 546279 [details] [diff] [review]
part 1.  Factor out IsAboutBlank into nsNetUtil.h.

Review of attachment 546279 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #546279 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 546280 [details] [diff] [review]
part 2.  Factor out the channel owner setting from docshell so other consumers can use it too.

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

r=dveditz
Attachment #546280 - Flags: review?(dveditz) → review+
Attachment #546784 - Flags: review?(dveditz) → review+
Depends on: 713485
No longer depends on: 713485
Depends on: 722037
You need to log in before you can comment on or make changes to this bug.