1221148 - Make blob URLs work with mozIJSSubScriptLoader (and anything else that wants a file:///)

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Anthony Lieuallen]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20151015172900

Steps to reproduce:

Context: https://github.com/greasemonkey/greasemonkey/issues/2296

Greasemonkey has received a bug report that turns out to mean: On Firefox Nightly (or at least something newer than stable), on Mac only, Greasemonkey doesn't work.

I've diagnosed it down to not being able to access files inside ~/Library/ -- which contains the Firefox profile, which contains the files Greasemonkey needs to read to serve its primary purpose.  Specifically the Browser Console shows a message like:

    Error opening input stream (invalid filename?): file:///Users/[USER]/Library/Application%20Support/Firefox/Profiles/ol8ivicq.default/gm_scripts/[SCRIPT_NAME]/[SCRIPT_NAME].user.js

If I disable e10s, everything works again.  If I manually open file:///Users/$USER/ in either Stable or Nightly with e10s off, I can click the "Library" link and browse down.  In Nightly with e10s on, I cannot.

Comment 1

[Anthony Lieuallen]

Oh FWIW the Mac platform I was testing on is 10.10.5.

Comment 2

[Markus Stange [:mstange]]

Bug 1083344 looks related. André, is this restriction intentional?

Comment 3

[André Reinald]

(In reply to Markus Stange [:mstange] from comment #2)
> Bug 1083344 looks related. André, is this restriction intentional?

Yes, it's related.

To sum it up, in e10s, the content process currently accesses way too many resources directly.

1. The proper way to do things would be to proxy all those accesses through IPC to the main process.

2. The current sandbox implementation was a tradeoff approach driven by milestones (and goals): so when something breaks because of a denied access, we change the sandbox rules in order to to punch a hole.

For obvious security reasons, files residing in the user profile folder are a typical case where we try to avoid direct access from the content process.

It seems we need to punch another hole for GreaseMonkey. Adding a regex "allow" rule for the specific "gm_scripts/[SCRIPT_NAME]/[SCRIPT_NAME].user.js" profile subfolder is trivial.

If needed I can mentor this bug.

Comment 4

[Anthony Lieuallen]

Well as long as I've got you:

https://github.com/greasemonkey/greasemonkey/pull/2243#issuecomment-134363235
"This page [1] states that file:// will always load in content"
[1] https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Which_URIs_load_where

Is this true?  What you've said above makes me think it's more of a bug than a feature than an e10s child process has a path to loading file contents directly?


If we can rely long term on XHR to a file:// (from child/content) then we've probably got a more appropriate change that we can make, than getting put in a path whitelist.

Comment 5

[Jim Mathies [:jimm]]

blassey, sounds like something your sandboxing team needs to look at?

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Would it be possible to create a DOM File object in the parent, pass it to the content process via MessageManager, and use that?  (Possibly with help from URL.createObjectURL, if a URL is needed.)  That would avoid sandboxing issues entirely, and gives the add-on control over which files it exposes to content processes.

The longer-term plan for sandboxing, as I understand it, is to separate remote Web content and file:/// content into disjoint sets of content processes, so a content process rendering remote content wouldn't be able to read arbitrary file URLs, not even from script running under the system principal in that process.


Something that ought to work but currently doesn't is registering file:///-based URLs as moz-extension:// or resource://; those are opened in the content process (bug 1109293), so they'd run into the Mac sandbox's ~/Library blacklisting the same way as direct file:/// access.  This has been pointed out as a reason why the Mac sandbox shouldn't blacklist filesystem paths like that until those protocols are remoted, but consensus has not yet been reached on this subject.

Comment 7

[The 8472]

> The longer-term plan for sandboxing, as I understand it, is to separate remote Web content and file:/// content into disjoint sets of content processes, so a content process rendering remote content wouldn't be able to read arbitrary file URLs, not even from script running under the system principal in that process.

This contradicts current documentation and statements from developers i've bugged on IRC. If you intend to break file:// URIs for addons in the future you should communicate this more clearly.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to The 8472 from comment #7, about comment #6)
> This contradicts current documentation and statements from developers i've
> bugged on IRC. If you intend to break file:// URIs for addons in the future
> you should communicate this more clearly.

Sorry; I didn't mean to imply that there are concrete plans for this yet, and I'm not the person who'd be making those plans in any case.  But, in order to get Firefox to a point where a compromised content process can't read all of the user's private data (without additional vulnerabilities; there are always more bugs), that's more or less what's going to have to happen.  

Documentation: https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Limitations_of_frame_scripts has a section about not doing file I/O from such scripts, but it's vague about what exactly counts as “file I/O”, and doesn't call out the file: scheme specifically.  Definitely room for improvement here.

Comment 9

[The 8472]

(In reply to Jed Davis [:jld] from comment #6)
> Would it be possible to create a DOM File object in the parent, pass it to
> the content process via MessageManager, and use that?  (Possibly with help
> from URL.createObjectURL, if a URL is needed.)  That would avoid sandboxing
> issues entirely, and gives the add-on control over which files it exposes to
> content processes.

Wouldn't that cause trouble on windows by keeping a file handle for a file, preventing it from being deleted/replaced?

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to The 8472 from comment #9)
> (In reply to Jed Davis [:jld] from comment #6)
> > Would it be possible to create a DOM File object in the parent, pass it to
> > the content process via MessageManager, and use that?  (Possibly with help
> > from URL.createObjectURL, if a URL is needed.)  That would avoid sandboxing
> > issues entirely, and gives the add-on control over which files it exposes to
> > content processes.
> 
> Wouldn't that cause trouble on windows by keeping a file handle for a file,
> preventing it from being deleted/replaced?

That's an excellent point, but I think it's not a problem.  I'm not an expert on the Blob code, but if I'm following it correctly, the Blob/File just holds a filesystem path on the parent side, and reading it from the child side is when the file is opened and a file handle passed to the child.  (In IPDL, PBlob is a File and PBlobStream is a request to open it for reading; BlobParent::RecvPBlobStreamConstructor eventually gets to BlobImplFile::GetInternalStream which opens it.)

Also, <input type="file"> already does this: the file picker UI runs in the parent and passes File objects to the child, which the HTMLInputElement holds references to (mFiles in C++, exposed as the |files| property).  Empirically, this doesn't prevent the file from being deleted on Windows.

Comment 11

[The 8472]

Ok, at least it would be a viable workaround.

Next question: The intent behind passing file URIs to mozIJSSubScriptLoader was memory footprint. The old solution represented the source as a string and eval'd that, which lead to lots of redundant strings. Loading with a file uri did cut down on the string objects. Are blob-URIs properly cached too?

Comment 12

[janekptacijarabaci]

See also https://bugzilla.mozilla.org/show_bug.cgi?id=1196384

Comment 13

[Gian-Carlo Pascutto [:gcp]]

Jed already replied here, he knows more about this issue than I do.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to The 8472 from comment #11)
> Are blob-URIs properly cached too?

What do you mean by this, exactly? Blob URIs were created to avoid having to duplicate large amounts of data (that data: URLs cause), so there should only be one version of the source per blob, per process at any given time.

Comment 15

[The 8472]

I think that answers the question, thanks.

The goal is to have the same memory footprint characteristics as putting file:// URIs into the subscript loader. File URIs didn't consume memory when sent to the child process until they were first used. And when used multiple times there was only a single script-source instance.

Comment 16

[noitidart]

Hi all,
I'm trying to load a simple file uri into the src attribute of an image.  It works on all platforms except Mac (with e10s enabled). This is what it looks like - http://i.imgur.com/u6DLoEW.png

It should look like this - http://i.imgur.com/OThMNtw.png

Is this disallowing of File URI a for sure thing? Do I have to switch to sending it a data: URL?

Here is some more details on the issue - https://discourse.mozilla-community.org/t/loading-file-uri-as-img-src/6954

Comment 17

[The 8472]

(In reply to Jed Davis [:jld] from comment #6)
> Would it be possible to create a DOM File object in the parent, pass it to
> the content process via MessageManager, and use that?  (Possibly with help
> from URL.createObjectURL, if a URL is needed.)  That would avoid sandboxing
> issues entirely, and gives the add-on control over which files it exposes to
> content processes.

I have tried implementing that, are you sure that's supposed to work?

1. I get File and URL via Cu.importGlobalProperties.
2. parent creates a DOM file object from an nsIFile via new File(file.path)
3. file is passed down through the message manager to the child proces
4. child process constructs a blob URI via URL.createObjectURL(fileFromIPC) 
5. child tries to pass the URL to the subscript loader

With e10s on this simply crashes the child process, with e10s off I get 

> Trying to load a non-local URI.: blob:null/24dc6222-c684-44ad-8336-380e36b29517

Comment 18

[The 8472]

since the child process crashes practically immediately, this might be relevant:

the File object is part of a structure that's also passed via the cpmm.initialProcessData

Comment 20

[Jim Mathies [:jimm]]

Work around here for developers is to disable the mac sandbox for nightly builds they test with.

Comment 21

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Hi, old needinfo flag I forgot about.  It might not matter anymore, but if anyone was wondering, the error message in comment #17 seems to reflect comments in js/xpconnect/idl/mozIJSSubScriptLoader.idl:

     * @param url the url of the sub-script, it MUST be either a file:,
     *            resource:, or chrome: url, and MUST be local.

I'd been assuming things would generally follow Web principles and treat URLs abstractly, but in fact there's code in the implementation that checks the scheme and won't accept a blob:.

(On the plus side, as of relatively recently there seems to be a nonzero amount of interest among people who actually understand this XPCOM stuff in engaging with sandboxing, so we'll hopefully be spared the spectacle of a systems programmer trying to give advice about browser add-on APIs in the future.)

Comment 22

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Reopening this because: bug 1187099 should be solvable with blob URLs (now that bug 1279186 landed), but this one can't.

Comment 23

[:shell escalante]

is the reproducable with latest version 3.8?  3.8 passed internal testing across all platforms - but maybe we missed this exact scenario...

Comment 24

[Haik Aftandilian [:haik]]

This should still be reproducible on Nightly only. It should never have affected other trains. On Nightly, on OS X, (limited) content sandboxing is enabled in e10s mode. One of the restrictions is to block the content process from reading or writing files within the profile directory (with some exceptions) which comes into play here. The path here is the "gm_scripts" subdirectory within the profile dir:

  ~/Library/Application Support/Firefox/Profiles/ol8ivicq.default/gm_scripts/

The problem shouldn't occur if the pref security.sandbox.content.level is set to 0.

We're working on figuring out how we'll deal with Addons that do this now.

Comment 25

[Jim Mathies [:jimm]]

read access restrictions milestones

Comment 26

[Jim Mathies [:jimm]]

(In reply to Jed Davis [:jld] [⏰PDT; UTC-7] from comment #22)
> Reopening this because: bug 1187099 should be solvable with blob URLs (now
> that bug 1279186 landed), but this one can't.

Hey Jed, was your intent here to make this bug about getting blob URLs working in the content process with services like nsIStyleSheetService? (We originally duped this over to bug 1187099 so I'm guessing that's the case.)

If so, please updating the title to something more relevant.

Comment 27

[Jim Mathies [:jimm]]

tracy, could you take greasemonkey for a spin on mac/nightly and see if it's currently broken?

Comment 28

[Tracy Walker [:tracy]]

This is still broken on Mac 10.11 with latest Nightly 51.0a1, 20160815030201.  If I set security.sandbox.content.level to 0 user scripts work. With it set to 1 user scripts do not work.

Comment 29

[:shell escalante]

Kris:  have we debated unsandboxing data directories?  this is likely to break a lot of add-ons, benefit versus cost. 

asking Jorge for more details on impact.  when he's done - need info jim mathies

Comment 30

[Gian-Carlo Pascutto [:gcp]]

(In reply to :shell escalante from comment #29)
> Kris:  have we debated unsandboxing data directories?

Unsandboxing the data directories means that if you compromise a content process you now have access to the users entire browsing history, all his logins, and all his passwords. It largely would make sandboxing pointless.

We discussed some workarounds in the sandboxing group, such as protecting the files we know contain sensitive data. But there are significant maintenance burden downsides to this (how do you maintain that list?) and potential security problems (using third party password manager? sorry you're SOL). On top of that, an end goal is to remove all IO from the content process (which should never have been doing file access to begin with). So we would really very strongly prefer to block the profile.

The real question right now is if we have usable workarounds for affected plugins, i.e. if there is some "right thing" they can do to fix themselves.

Comment 31

[Jorge Villalobos [:jorgev] (he/him)]

Shell: since this is about sandboxing, which is the next step after e10s, I don't think we should be tracking this for e10s compatibility.

As for sandboxing, I generally agree with comment #30. We should avoid giving add-ons too much lenience in the content process when it comes to file handling. However, my understanding is that this breaks file:// URLs, which could be useful/necessary in some cases, and bug 1109293 would need to fixed for there to be a suitable workaround for that particular case.

Needinfo Jim Mathies per comment #29.

Comment 32

[Jim Mathies [:jimm]]

This is one of the apis we need to look at in solving our file:// access issues. We'll get to this.

Comment 33

[:shell escalante]

hi Jorge, hi Andreas,

my original understanding of sandboxing is that it would come after e10s, but that was not accurate. Currently the plan is Beta in 50 for Windows and Mac, with the hopes of riding the train to Release in 50.  

With that in mind - how far will bug 1109293 go towards offering an alternative for add-ons?  How large of a change is it that we'd be asking folks to make for Nov 8th?  Are there other areas that you could see an issue?  Should I schedule a talk with Jim or Brad to see if there are other areas sandboxing could impact add-ons

Comment 34

[Andreas Wagner [:TheOne] [use NI]]

I think a couple of hundred to a couple of thousand add-ons make direct references to "file:///" urls. I can't exactly say how many as add-on indexing is incomplete.

I'm not sure whether all handle references can be updated to use "resource:". Using "file:///" is a common pattern when a file outside the extension directory needs to be accessed.

Comment 35

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

To clarify, this is specifically about file:/// access from content processes — which in add-on terms means frame scripts and process scripts, if I understand correctly.  Add-on script running in the parent process wouldn't be affected.

Comment 36

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1221148 - Allow passing blob:// URIs to mozIJSSubScriptLoader;

This is useful for legacy addons as we increasingly lockdown filesystem access
in content processes.

Review commit: https://reviewboard.mozilla.org/r/146726/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146726/

Comment 37

[Alex Gaynor [:Alex_Gaynor]]

I've added the minimal patch to get blob:// URIs working in mozIJSSubScriptLoader. I'm not sure who the correct reviewer is for this.

I'm also not particularly confident this is correct :-) The tests pass so it is minimally functional, but I'd appreciate if someone else can weigh in on whether the origin checking for the blob:// URIs is correct. Normally, loading cross-origin blob:// URIs is disallowed, but since we use the system principal in mozIJSSubScriptLoader I believe that any origin in the blob:// URI will be allowed.

Comment 38

[Andrew McCreight [:mccr8]]

Comment on attachment 8875309 [details]
Bug 1221148 - Allow passing blob:// URIs to mozIJSSubScriptLoader;

Olli, could you take a look at this? I don't really know anything about blob URIs or what the security implications here might be. The actual patch is very simple.

You could also remove the app protocol while you are here (bug 1358585).

Comment 39

[Anthony Lieuallen]

I cared about this when I filed it in 2015.  Now I wonder if browser.tabs.executeScript() will also support blob (URLs)?  I'm much less interested in new features for XPCOM extensions given the looming webext-only Firefox 57.

In my webext, I'm basically looking at storing the full content of every string I ever might want to use as a content script in memory, so I don't have to look them up asynchronously.  (Long term: I have plans for supporting synchronous document_start execution.)  Storing blob references rather than strings would be nice!

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8875309 [details]
Bug 1221148 - Allow passing blob:// URIs to mozIJSSubScriptLoader;

https://reviewboard.mozilla.org/r/146726/#review151574

Caching needs to be addressed somehow. Perhaps revoking should evict the entry from the cache or perhaps blob uris shouldn't be ever cached?

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:633
(Diff revision 1)
>          ReportError(cx, LOAD_ERROR_NOSCHEME, uri);
>          return NS_OK;
>      }
>  
> -    if (!scheme.EqualsLiteral("chrome") && !scheme.EqualsLiteral("app")) {
> +    if (!scheme.EqualsLiteral("chrome") && !scheme.EqualsLiteral("app") &&
> +          !scheme.EqualsLiteral("blob")) {

Nit 2 extra spaces before !scheme.EqualsLiteral("blob")

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp:661
(Diff revision 1)
>      RootedScript script(cx);
>      if (!options.ignoreCache) {
>          if (!options.wantReturnValue)
>              script = ScriptPreloader::GetSingleton().GetCachedScript(cx, cachePath);
>          if (!script && cache)
>              rv = ReadCachedScript(cache, cachePath, cx, mSystemPrincipal, &script);

How does all this caching work with blob uris?
Do we end up caching everything by default, and keep in cache even if blob uri is revoked?
That would be rather bad.

Comment 41

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8875309 [details]
Bug 1221148 - Allow passing blob:// URIs to mozIJSSubScriptLoader;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146726/diff/1-2/

Comment 42

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8875309 [details]
Bug 1221148 - Allow passing blob:// URIs to mozIJSSubScriptLoader;

https://reviewboard.mozilla.org/r/146726/#review151574

> How does all this caching work with blob uris?
> Do we end up caching everything by default, and keep in cache even if blob uri is revoked?
> That would be rather bad.

I've gone ahead and disabled the cache for blob URIs. Based on my reading of the code, the original behavior of this patch would have been to keep them in the cache forever.

Comment 43

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8875309 [details]
Bug 1221148 - Allow passing blob:// URIs to mozIJSSubScriptLoader;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146726/diff/2-3/

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8875309 [details]
Bug 1221148 - Allow passing blob:// URIs to mozIJSSubScriptLoader;

https://reviewboard.mozilla.org/r/146726/#review152860

Comment 45

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8875309 [details]
Bug 1221148 - Allow passing blob:// URIs to mozIJSSubScriptLoader;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146726/diff/3-4/

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32d2d259c161
Allow passing blob:// URIs to mozIJSSubScriptLoader; r=smaug

Comment 47

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/32d2d259c161