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)
Core
Graphics: Canvas2D
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: Dolske, Assigned: bzbarsky)
References
Details
Attachments
(4 files, 1 obsolete file)
(deleted),
patch
|
jduell.mcbugs
:
review+
jst
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jst
:
review+
dveditz
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dveditz
:
review+
|
Details | Diff | Splinter Review |
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?
Comment 1•16 years ago
|
||
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?
Reporter | ||
Comment 3•16 years ago
|
||
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).
Assignee | ||
Comment 4•16 years ago
|
||
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 6•16 years ago
|
||
(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.
Assignee | ||
Comment 7•16 years ago
|
||
> 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•16 years ago
|
||
(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.
Assignee | ||
Comment 11•15 years ago
|
||
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•15 years ago
|
||
Joe knows more about this stuff, so I'll defer to him.
Joe?
Comment 13•15 years ago
|
||
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
Assignee | ||
Comment 14•15 years ago
|
||
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 16•14 years ago
|
||
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)
Assignee | ||
Comment 17•14 years ago
|
||
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 19•13 years ago
|
||
(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.
Assignee | ||
Comment 21•13 years ago
|
||
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)
Assignee | ||
Comment 22•13 years ago
|
||
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)
Assignee | ||
Comment 23•13 years ago
|
||
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)
Assignee | ||
Comment 24•13 years ago
|
||
Attachment #546283 -
Flags: review?(roc)
Assignee | ||
Comment 25•13 years ago
|
||
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+
Assignee | ||
Updated•13 years ago
|
Whiteboard: [need review]
Updated•13 years ago
|
Attachment #546279 -
Flags: review?(jst) → review+
Comment 27•13 years ago
|
||
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+
Assignee | ||
Comment 28•13 years ago
|
||
> 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.
Assignee | ||
Comment 29•13 years ago
|
||
Attachment #546784 -
Flags: review?(dveditz)
Assignee | ||
Updated•13 years ago
|
Attachment #546282 -
Attachment is obsolete: true
Attachment #546282 -
Flags: review?(dveditz)
Comment 30•13 years ago
|
||
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+
Assignee | ||
Comment 31•13 years ago
|
||
> Wanna s/Is/It/ here while you're moving this around?
Done.
Comment 32•13 years ago
|
||
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 33•13 years ago
|
||
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+
Updated•13 years ago
|
Attachment #546784 -
Flags: review?(dveditz) → review+
Assignee | ||
Comment 34•13 years ago
|
||
Daniel, thanks!
Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ca75ce160b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6a3d724bcf5
https://hg.mozilla.org/integration/mozilla-inbound/rev/c237a8550070
https://hg.mozilla.org/integration/mozilla-inbound/rev/68928bdabfd7
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla9
Comment 35•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2ca75ce160b0
https://hg.mozilla.org/mozilla-central/rev/a6a3d724bcf5
https://hg.mozilla.org/mozilla-central/rev/c237a8550070
https://hg.mozilla.org/mozilla-central/rev/68928bdabfd7
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•