Last Comment Bug 444641 - Canvas's .getImageData throws when accessing <img src="file://...">
: Canvas's .getImageData throws when accessing <img src="file://...">
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla9
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Milan Sreckovic [:milan]
Mentors:
: 426691 522720 530928 585204 666244 668962 (view as bug list)
Depends on: 417836 671906 722037
Blocks: 230606
  Show dependency treegraph
 
Reported: 2008-07-10 12:22 PDT by Justin Dolske [:Dolske]
Modified: 2012-01-30 08:20 PST (History)
16 users (show)
samuel.sidler+old: blocking1.9.0.2-
samuel.sidler+old: wanted1.9.0.x+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1. Factor out IsAboutBlank into nsNetUtil.h. (10.98 KB, patch)
2011-07-15 21:48 PDT, Boris Zbarsky [:bz] (still a bit busy)
jduell.mcbugs: review+
jst: review+
Details | Diff | Splinter Review
part 2. Factor out the channel owner setting from docshell so other consumers can use it too. (14.93 KB, patch)
2011-07-15 21:50 PDT, Boris Zbarsky [:bz] (still a bit busy)
jst: review+
dveditz: review+
Details | Diff | Splinter Review
part 3. Propagate the loading principal to the image channel as needed. (14.62 KB, patch)
2011-07-15 21:52 PDT, Boris Zbarsky [:bz] (still a bit busy)
joe: review+
Details | Diff | Splinter Review
part 4. Remove the data: special-casing for images in canvas, since we now set the right principal for data: images. (6.89 KB, patch)
2011-07-15 21:53 PDT, Boris Zbarsky [:bz] (still a bit busy)
roc: review+
Details | Diff | Splinter Review
Part 3 updated to joe's comments (14.96 KB, patch)
2011-07-19 07:57 PDT, Boris Zbarsky [:bz] (still a bit busy)
dveditz: review+
Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2008-07-10 12:22:11 PDT
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.
Comment 1 Samuel Sidler (old account; do not CC) 2008-08-14 17:04:17 PDT
Not blocking, but wanted.

Vlad, who can own this?
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2008-08-14 20:29:10 PDT
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?
Comment 3 Justin Dolske [:Dolske] 2008-08-14 21:57:14 PDT
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).
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2008-08-17 18:10:08 PDT
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.
Comment 5 Vladimir Vukicevic [:vlad] [:vladv] 2008-08-17 21:24:52 PDT
*** Bug 426691 has been marked as a duplicate of this bug. ***
Comment 6 Daniel Kirsch 2009-02-13 08:31:52 PST
(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.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2009-02-13 08:48:29 PST
> 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.
Comment 8 Daniel Kirsch 2009-02-16 04:39:24 PST
(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.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2009-10-16 10:16:42 PDT
*** Bug 522720 has been marked as a duplicate of this bug. ***
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2009-11-24 19:22:01 PST
*** Bug 530928 has been marked as a duplicate of this bug. ***
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2009-11-24 20:08:45 PST
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?
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2009-11-28 01:19:49 PST
Joe knows more about this stuff, so I'll defer to him.

Joe?
Comment 13 Joe Drew (not getting mail) 2009-12-10 14:52:24 PST
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?
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2009-12-10 16:07:25 PST
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.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2010-08-31 19:35:22 PDT
*** Bug 585204 has been marked as a duplicate of this bug. ***
Comment 16 John Drinkwater (:beta) 2011-02-17 15:45:25 PST
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)
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2011-02-17 17:33:23 PST
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).
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2011-06-22 12:58:04 PDT
*** Bug 666244 has been marked as a duplicate of this bug. ***
Comment 19 Julian Adams 2011-06-22 13:35:58 PDT
(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.
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2011-07-02 07:25:32 PDT
*** Bug 668962 has been marked as a duplicate of this bug. ***
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2011-07-15 21:48:49 PDT
Created attachment 546279 [details] [diff] [review]
part 1.  Factor out IsAboutBlank into nsNetUtil.h.

The other option is to put an IsAboutBlank on nsContentUtils.  Let me know if you prefer that.
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2011-07-15 21:50:31 PDT
Created attachment 546280 [details] [diff] [review]
part 2.  Factor out the channel owner setting from docshell so other consumers can use it too.

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.
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2011-07-15 21:52:42 PDT
Created attachment 546282 [details] [diff] [review]
part 3.  Propagate the loading principal to the image channel as needed.

This is the actual meat of the change.  Daniel, this also needs a security-perspective look.
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2011-07-15 21:53:28 PDT
Created 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.
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2011-07-15 21:55:03 PDT
Daniel, let me know if you think I should just try to schedule a security review here.
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-07-16 04:12:04 PDT
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]:
-----------------------------------------------------------------
Comment 27 Joe Drew (not getting mail) 2011-07-19 00:01:29 PDT
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.
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2011-07-19 06:48:01 PDT
> 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.
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2011-07-19 07:57:01 PDT
Created attachment 546784 [details] [diff] [review]
Part 3 updated to joe's comments
Comment 30 Johnny Stenback (:jst, jst@mozilla.com) 2011-07-19 23:07:17 PDT
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
Comment 31 Boris Zbarsky [:bz] (still a bit busy) 2011-07-20 12:26:07 PDT
> Wanna s/Is/It/ here while you're moving this around?

Done.
Comment 32 Jason Duell [:jduell] (needinfo me) 2011-07-25 13:24:56 PDT
Comment on attachment 546279 [details] [diff] [review]
part 1.  Factor out IsAboutBlank into nsNetUtil.h.

Review of attachment 546279 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 33 Daniel Veditz [:dveditz] 2011-09-20 00:51:48 PDT
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

Note You need to log in before you can comment on or make changes to this bug.