Closed Bug 1221148 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Security: Process Sandboxing, defect)

41 Branch
Unspecified
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
e10s + ---
firefox56 --- fixed

People

(Reporter: arantius, Assigned: Alex_Gaynor)

References

Details

(Whiteboard: sbwc3, sbmc2, sblc3)

Attachments

(1 file)

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.
Oh FWIW the Mac platform I was testing on is 10.10.5.
Bug 1083344 looks related. André, is this restriction intentional?
Flags: needinfo?(areinald.bug)
Blocks: 1149706
(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.
Flags: needinfo?(areinald.bug)
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.
blassey, sounds like something your sandboxing team needs to look at?
Flags: needinfo?(blassey.bugs)
Status: UNCONFIRMED → NEW
Component: Untriaged → Security: Process Sandboxing
Ever confirmed: true
Product: Firefox → Core
OS: Unspecified → Mac OS X
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.
> 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.
(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.
Flags: needinfo?(blassey.bugs) → needinfo?(gpascutto)
(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?
(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.
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?
Jed already replied here, he knows more about this issue than I do.
Flags: needinfo?(gpascutto)
(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.
Flags: needinfo?(bugzilla.mozilla.org)
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.
Flags: needinfo?(bugzilla.mozilla.org)
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
(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
Flags: needinfo?(jld)
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
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1187099
Work around here for developers is to disable the mac sandbox for nightly builds they test with.
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.)
Flags: needinfo?(jld)
Reopening this because: bug 1187099 should be solvable with blob URLs (now that bug 1279186 landed), but this one can't.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
is the reproducable with latest version 3.8?  3.8 passed internal testing across all platforms - but maybe we missed this exact scenario...
Flags: needinfo?(jld)
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.
Flags: needinfo?(jld)
read access restrictions milestones
Whiteboard: sbmc2
Whiteboard: sbmc2 → sbmc2, sblc3
Whiteboard: sbmc2, sblc3 → sbwc2, sbmc2, sblc3
(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.
Flags: needinfo?(jld)
Flags: needinfo?(jld)
Summary: Mac Nightly with e10s on cannot open some files → Make blob URLs work with mozIJSSubScriptLoader (and anything else that wants a file:///)
Whiteboard: sbwc2, sbmc2, sblc3 → sbwc2, sbmc2, sblc3 [sandboxing]
tracy, could you take greasemonkey for a spin on mac/nightly and see if it's currently broken?
Flags: needinfo?(twalker)
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.
Flags: needinfo?(twalker)
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
Flags: needinfo?(jorge)
(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.
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.
Flags: needinfo?(jorge) → needinfo?(jmathies)
This is one of the apis we need to look at in solving our file:// access issues. We'll get to this.
Flags: needinfo?(jmathies)
Whiteboard: sbwc2, sbmc2, sblc3 [sandboxing] → sbwc2, sbmc2, sblc3 [sandboxing] triaged
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
Flags: needinfo?(jorge)
Flags: needinfo?(awagner)
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.
Flags: needinfo?(awagner)
Flags: needinfo?(jorge)
Blocks: 1300829
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.
Whiteboard: sbwc2, sbmc2, sblc3 [sandboxing] triaged → sbwc3, sbmc2, sblc3 [sandboxing] triaged
Whiteboard: sbwc3, sbmc2, sblc3 [sandboxing] triaged → sbwc3, sbmc2, sblc3
Blocks: sandbox-fs
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.
Assignee: nobody → agaynor
Attachment #8875309 - Flags: review?(continuation)
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).
Attachment #8875309 - Flags: review?(continuation) → review?(bugs)
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 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.
Attachment #8875309 - Flags: review?(bugs) → review-
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.
Attachment #8875309 - Flags: review?(continuation) → review?(bugs)
Comment on attachment 8875309 [details]
Bug 1221148 - Allow passing blob:// URIs to mozIJSSubScriptLoader;

https://reviewboard.mozilla.org/r/146726/#review152860
Attachment #8875309 - Flags: review?(bugs) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/32d2d259c161
Allow passing blob:// URIs to mozIJSSubScriptLoader; r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/32d2d259c161
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.