1187099 - User stylesheets loaded from a file inside ~/Library don't apply in the content process

Status: RESOLVED WONTFIX

(Core :: Security: Process Sandboxing, defect)

Description

[Sören Hentzschel]

Attachment: screenshot

The thenextweb.com search does not work for me with e10s and Adblock Plus enabled. It works with e10s enabled and without Adblock Plus and it works with e10s disabled and Adblock Plus.

STR:

1. install Adblock Plus
2. open http://thenextweb.com/?s=mozilla

Expected / Actual:

Please see the screenshot.

Comment 1

[Wladimir Palant]

I can confirm this issue. The website seems to calculate positions incorrectly, the content is positioned way too far at the bottom - likely some timing-sensitive assumptions there. It is somehow related to https://securepubads.g.doubleclick.net/gampad/ads being blocked - adding the filter @@||securepubads.g.doubleclick.net^ to Adblock Plus makes the site work correctly again. I suspect that allowing that script to load simply changes timing again.

Comment 2

[Wladimir Palant]

After wasting way too much time figuring out what the scripts on that website did and why they inserted that huge (blocked) ad there, I realized that element hiding functionality was completely broken - that ad shouldn't be visible in the first place, Adblock Plus should have hidden it.

I'm pretty sure that this is a regression, will try to find a regression range.

Comment 3

[Wladimir Palant]

The actual problem here is that stylesheets registered via nsIStyleSheetService.loadAndRegisterSheet() have no effect, at least on content pages. The weird part: this is dependent on the profile path. On OS X 10.10 only profiles located under ~/Library are broken: ~/test and ~/Downloads/test work correctly, ~/Library/test and ~/Library/Application Support/Firefox/test are broken. It's the same profile in all cases, I've been merely moving it around.

It's a clean profile, I merely installed Adblock Plus 2.6.9.3966 from https://downloads.adblockplus.org/devbuilds/adblockplus/. Reproduced with Firefox 42.0a1 nightly (2015-07-24), same issue in nightly from 2015-06-02 however - I'm not so sure any more that this is a regression.

Comment 4

[Wladimir Palant]

Regression range is https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=436686833af0&tochange=008b3f65a7e0

Comment 5

[Wladimir Palant]

Looking through the list of changes, bug 1083344 looks like the most likely culprit. This would mean that the issue is restricted to OS X 10.10 Yosemite. @Sören Hentzschel: Is that the operating system you used as well?

Comment 6

[Wladimir Palant]

Confirmed: setting security.sandbox.content.level preference to 0 makes the issue go away. Also, moving Adblock Plus data directory outside of ~/Library without moving the entire profile works as well. So the issue is limited to using nsIStyleSheetService.loadAndRegisterSheet() with files inside ~/Library - this will currently fail silently in the content process.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

I can't reproduce this bug, using the STR from comment #0 with today's mozilla-central nightly on OS X 10.10.4.  I used a clean profile, into which I did a fresh install of Adblock Plus (with its default settings).

Wladimir, have you already worked around this? :-)

Comment 8

[Steven Michaud [:smichaud] (Retired)]

I should also mention that I installed Adblock Plus from AMO, after using the Search function in the Add-ons Manager.

Comment 9

[Lars T Hansen [:lth]]

FWIW this also seems to be a problem on Linux.  I have a style sheet for gmail that presents bugzilla mail content in monospace, this is ~/.mozilla/firefox/PROFILE/chrome/userContent.css.  This is not applied with default e10s settings; if e10s is disabled, it is applied as expected.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

For the questions I asked in comment #7.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

FYI Jed: comment #9.

Comment 12

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

Linux content isn't sandboxed at all yet (and we're nowhere near the point where it would be possible to apply policy based on filesystem paths), so whatever's happening in comment #9 is an e10s bug.

Comment 13

[Wladimir Palant]

(In reply to Steven Michaud [:smichaud] (Retired) from comment #7)
> I can't reproduce this bug, using the STR from comment #0 with today's
> mozilla-central nightly on OS X 10.10.4.  I used a clean profile, into which
> I did a fresh install of Adblock Plus (with its default settings).
> 
> Wladimir, have you already worked around this? :-)

No, I didn't, and the issue is still present. However, the website seems to have changed and the content is no longer being pushed down as much. It's very easy to see:

1. Install Adblock Plus.
2. Click the ABP icon and select Filter Preferences.
3. Go to "Custom filters" tab, add a filter group if there is none, select it.
4. Press Ctrl+R (Cmd+R) to expand the right pane, click Add filter, enter "##body" (without quotation marks) and press Enter.

This filter should normally hide the content on all web pages - but it doesn't on OS X if E10S is switched on and the profile folder is located inside ~/Library folder.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Thanks, Wladimir, for your information.

I'm now retired, and will not be doing any further work on this bug.  But with your STR, others can now pick it up.

Comment 15

[Cameron McCormack (:heycam)]

Reinald, since you implemented OS X sandboxing, is this something you can look into?

Comment 16

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

What URL scheme is the content process trying to access?

file:/// is eventually going to break; no, this isn't communicated well, and yes, it would be nice if there were checking deployed at the XPCOM level and there isn't, but the intent was always that content processes wouldn't be able to open arbitrary files.

chrome:// and resource:// and moz-extension:// *should* work, insofar as the set of accessible files is constrained by registrations performed in the parent process, but currently they're resolved in the content process and files are opened there (bug 1109293, bug 1136836).  In my opinion the ~/Library rule in the Mac sandbox policy should be removed until that happens, because as-is it's an undocumented platform-specific add-on compatibility break, but that's what the situation is.

Sending parent-process-constructed File objects through the MessageManager and using createObjectURL in the child should always work, but I don't know if that's applicable/suitable here.

Comment 17

[André Reinald]

(In reply to Cameron McCormack (:heycam) from comment #15)
> Reinald, since you implemented OS X sandboxing, is this something you can
> look into?

It looks very similar to other bugs fixed after bug 1083344 landed.
Adding a special regex to punch a hole in the sandbox is the easiest move.
See:
https://dxr.mozilla.org/mozilla-central/source/security/sandbox/mac/Sandbox.mm#417

bug 1083344 comment 44 explains how to spot what exactly gets blocked by the sandbox.

Comment 18

[Wladimir Palant]

For reference, we are using file:/// URLs with nsIStyleSheetService. Given that nsIStyleSheetService is called from the parent process and messages the content process by itself, blob: URIs won't work - the same URL has to work in all processes. We can fairly easily switch to resource:// URLs once bug 1109293 is resolved. The only option that will work right now however is our own protocol handler I think, that's way too much effort however - so it would be great to see the sandbox rule removed until there are simple solutions.

Comment 19

[Cameron McCormack (:heycam)]

Attachment: patch

ABP wants to access ~/Library/Application Support/Firefox/Profiles/*/adblockplus/elemhide.css.

How about we just enable access to the profile directory, rather than everything under ~/Library?  This patch works for me on OS X 10.11.  (I assume there's no way in the sandbox rules to limit access to the particular profile directory we're using.)

Comment 20

[André Reinald]

(In reply to Cameron McCormack (:heycam) from comment #19)
> How about we just enable access to the profile directory, rather than everything under ~/Library?

There is sensitive personal information in the user profile directory. When implementing the original set of rules, the decision was made to be as restrictive as possible. I'd rather keep the current policy of punching holes on a case by case basis for specific addons subdirs rather than opening the whole Profile directory.

> I assume there's no way in the sandbox rules to limit access to the particular profile directory we're using.

If the profile dir is accessible at the time the sandbox is started, then we could just specify that directory as an additional variable in the ruleset. Maybe someone has the answer to that.

Comment 21

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

Comment on attachment 8700815 [details] [diff] [review]
patch

Review of attachment 8700815 [details] [diff] [review]:
-----------------------------------------------------------------

So, I wouldn't object to this patch, but Andre has, and I'm hesitant to just r+ a patch over the specific objections of one of the people who originally wrote the code.  gcp, you're the relevant module owner — any thoughts?

::: security/sandbox/mac/Sandbox.mm
@@ +410,5 @@
>    "        (iokit-user-client-class \"AppleGraphicsPolicyClient\"))\n"
>    "\n"
> +  "; extensions expect to be able to read files they've stored in the profile\n"
> +  "; directory; until we have a mechanism for them to access their files\n"
> +  "; through the content process, allow reads to profile directories\n"

Also, a nit: should be “through the parent process”, but maybe also specify something like “while running in the content process” or “in/from content-process script” if that was the intent.

Comment 22

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8700815 [details] [diff] [review]
patch

Review of attachment 8700815 [details] [diff] [review]:
-----------------------------------------------------------------

Blanket opening of the profile dir makes a significant part of the sandboxing effort useless (makes login/pw database accessible, etc), so I agree with Andre in this regard. We have to fix it another way (ie. addons have to read in the parent and send it through).

That said, it's clear this is a compatibility issue for many popular plugins. Based on that, I wouldn't object to this being toggled by a pref.

Comment 23

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

(In reply to Gian-Carlo Pascutto [:gcp] from comment #22)
> Blanket opening of the profile dir makes a significant part of the
> sandboxing effort useless (makes login/pw database accessible, etc), so I
> agree with Andre in this regard. We have to fix it another way (ie. addons
> have to read in the parent and send it through).

But one of the ways to fix such an add-on, and the one that seems to be the right solution here (see comment #18; createObjectURL won't work here), is to register resource: or moz-extension: URLs in the parent and then reference them in the child.  This can be supported in the presence of sandboxing; it's not an inherent security problem.  And the Mac sandbox's blacklist is breaking that.

Also, I am not convinced that the Mac content sandbox policy is providing any meaningful security improvement at present (which, to be clear, is not its fault; the Gecko/Firefox platform is just not in a state where that's possible).


(For completeness, the other way I can think of to turn a file into a URI with the same meaning in all processes would be to read it into memory as a data: URI.  We already get enough complaints about Firefox's memory usage being too high without that kind of help.)

Comment 24

[Cameron McCormack (:heycam)]

I guess there is no standard subdirectory within the profile that extensions use?  For example ABP uses $PROFILE/adblockplus/ but I think it just claimed it for itself.  What if we encourage addons to use a directory name like $PROFILE/extensions/$SOMETHING/ (where SOMETHING is based on their extension ID maybe?) and then when an addon is enabled we automatically open up that directory.  Or the addons has to request it.  Or maybe this is all moot once we move to the world of WebExtensions.

For the moment we can whitelist $PROFILE/adblockplus/ explicitly.

Comment 25

[Masatoshi Kimura [:emk]]

Please don't forget $PROFILE/chrome/userContent.css. This is a built-in feature.

Comment 27

[Wladimir Palant]

We've worked around this issue in Adblock Plus as of 2.7.3.4184-beta (will be part of the next release). Rather that writing a file to disk we are now serving the stylesheet via our about: "page." That page is registered in the content process and will message the chrome process in order to retrieve the data. Only issue: this messaging has to happen synchronously, due to bug 1280263 and bug 1280265. Actually, current version of the code still uses nsIStyleSheet.loadAndRegisterSheet() rather than nsIStyleSheet.preloadSheet() but that will change soon - both load data synchronously unfortunately.

Comment 28

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

Now that bug 1279186 is resolved, if I understand correctly, it should be possible to create a blob: URL in one process and send the URL spec to other processes (at least if the consumer of the URL doesn't insist on a real file: URL, like in bug 1221148).

Comment 29

[The 8472]

> (In reply to Jed Davis [:jld] [⏰PDT; UTC-7] from comment #28)
> (at least if the consumer of the URL doesn't insist on a real
> file: URL, like in bug 1221148).

But that bug has been duped to this bug. It probably should be reopened if the solution to this bug doesn't actually cover it.

Comment 30

[Jim Mathies [:jimm]]

(In reply to Jed Davis [:jld] [⏰PDT; UTC-7] from comment #23)
> (In reply to Gian-Carlo Pascutto [:gcp] from comment #22)
> > Blanket opening of the profile dir makes a significant part of the
> > sandboxing effort useless (makes login/pw database accessible, etc), so I
> > agree with Andre in this regard. We have to fix it another way (ie. addons
> > have to read in the parent and send it through).
> 
> But one of the ways to fix such an add-on, and the one that seems to be the
> right solution here (see comment #18; createObjectURL won't work here), is
> to register resource: or moz-extension: URLs in the parent and then
> reference them in the child.  This can be supported in the presence of
> sandboxing; it's not an inherent security problem.  And the Mac sandbox's
> blacklist is breaking that.

Jed, your last sentence here implies the use of resource: or moz-extension: to work around this bug is currently broken by the mac sandbox. Is that a typo? You seemed to be implying earlier that this should work currently.

Comment 31

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

(In reply to Jim Mathies [:jimm] from comment #30)
> Jed, your last sentence here implies the use of resource: or moz-extension:
> to work around this bug is currently broken by the mac sandbox. Is that a
> typo? You seemed to be implying earlier that this should work currently.

No, it's very much broken to the best of my knowledge.  Right now those schemes are just URL shorteners; if they're given a file:/// (or jar:file:, etc.), they'll expand to that file:/// and fail the same way using it directly would fail.  If I said “should” about that, I meant “should” in the sense of “ought to”, and my apologies for the ambiguity.  See also bug 1201935 comment #0, and note that bug 1109293 and bug 1136836 are still open.

Comment 32

[Jim Mathies [:jimm]]

(In reply to Wladimir Palant from comment #27)
> We've worked around this issue in Adblock Plus as of 2.7.3.4184-beta (will
> be part of the next release). Rather that writing a file to disk we are now
> serving the stylesheet via our about: "page." That page is registered in the
> content process and will message the chrome process in order to retrieve the
> data. Only issue: this messaging has to happen synchronously, due to bug
> 1280263 and bug 1280265. Actually, current version of the code still uses
> nsIStyleSheet.loadAndRegisterSheet() rather than
> nsIStyleSheet.preloadSheet() but that will change soon - both load data
> synchronously unfortunately.

We could add async / content process support for this api, however we plan to deprecate support for xul addons in 57 and so we don't see a need to address this. If other major extensions run into problems here we will reconsider.