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

RESOLVED FIXED in mozilla9

Status

()

Core
Canvas: 2D
P2
normal
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: Dolske, Assigned: bz)

Tracking

Trunk
mozilla9
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.0.2 -
wanted1.9.0.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
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?
(Reporter)

Comment 3

9 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).
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
Duplicate of this bug: 426691

Comment 6

8 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.
> 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

8 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.
Duplicate of this bug: 522720
Duplicate of this bug: 530928
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.
Duplicate of this bug: 585204
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).
Duplicate of this bug: 666244

Comment 19

6 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.
Duplicate of this bug: 668962
Depends on: 671906
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.
Attachment #546279 - Flags: review?(jst)
Attachment #546279 - Flags: review?(jduell.mcbugs)
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.
Attachment #546280 - Flags: review?(jst)
Attachment #546280 - Flags: review?(dveditz)
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.
Attachment #546282 - Flags: review?(joe)
Attachment #546282 - Flags: review?(dveditz)
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.
Attachment #546283 - Flags: review?(roc)
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]

Updated

6 years ago
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.
Created attachment 546784 [details] [diff] [review]
Part 3 updated to joe's comments
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+
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
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
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 713485

Updated

5 years ago
No longer depends on: 713485

Updated

5 years ago
Depends on: 722037
You need to log in before you can comment on or make changes to this bug.