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

RESOLVED WONTFIX

Status

()

defect
RESOLVED WONTFIX
4 years ago
2 years ago

People

(Reporter: soeren.hentzschel, Unassigned)

Tracking

({regression})

Trunk
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-, firefox42 affected)

Details

(Whiteboard: [STR in comment #13], sbmc2, )

Attachments

(2 attachments)

Reporter

Description

4 years ago
Posted image 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

4 years ago
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

4 years ago
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

4 years ago
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.
Summary: thenextweb.com search does not work with e10s + Adblock Plus → User stylesheets registered in Chrome process don't apply to the content process

Comment 5

4 years ago
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?
Blocks: 1083344

Comment 6

4 years ago
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.
Component: Extension Compatibility → Security: Process Sandboxing
OS: Unspecified → Mac OS X
Product: Firefox → Core
Hardware: Unspecified → x86_64

Updated

4 years ago
Summary: User stylesheets registered in Chrome process don't apply to the content process → [e10s] User stylesheets loaded from a file inside ~/Library don't apply in the content process
Blocks: e10s-addons
tracking-e10s: --- → +
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? :-)
I should also mention that I installed Adblock Plus from AMO, after using the Search function in the Add-ons Manager.
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.
For the questions I asked in comment #7.
Flags: needinfo?(trev.moz)
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

4 years ago
(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.
Flags: needinfo?(trev.moz)
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.
Whiteboard: [STR in comment #13]
Reinald, since you implemented OS X sandboxing, is this something you can look into?
Flags: needinfo?(areinald.bug)
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

4 years ago
(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.
Flags: needinfo?(areinald.bug)
See Also: → 1083344

Comment 18

4 years ago
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.
Posted patch patchSplinter Review
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.)
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8700815 - Flags: review?(jld)

Comment 20

4 years ago
(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 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.
Attachment #8700815 - Flags: review?(jld) → feedback?(gpascutto)
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.
Attachment #8700815 - Flags: feedback?(gpascutto) → feedback-
(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.)
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.
Please don't forget $PROFILE/chrome/userContent.css. This is a built-in feature.
See Also: → 1046166

Updated

3 years ago
Whiteboard: [STR in comment #13] → [STR in comment #13], sbmc1

Updated

3 years ago
No longer blocks: e10s-addons, e10s-abp
Summary: [e10s] User stylesheets loaded from a file inside ~/Library don't apply in the content process → User stylesheets loaded from a file inside ~/Library don't apply in the content process

Updated

3 years ago
Duplicate of this bug: 1221148

Comment 27

3 years ago
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.
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

3 years ago
> (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.

Updated

3 years ago
Assignee: cam → nobody
Status: ASSIGNED → NEW
(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.
Flags: needinfo?(jld)
(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.
Flags: needinfo?(jld)

Updated

3 years ago
Whiteboard: [STR in comment #13], sbmc1 → [STR in comment #13], sbmc2
(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.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.