Closed Bug 416942 Opened 12 years ago Closed 11 years ago

[FIX]Give agent (and maybe user?) sheets the system principal

Categories

(Core :: CSS Parsing and Computation, defect, P2)

x86
Linux
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: addon-compat, fixed1.9.1, verified1.9.0.9)

Attachments

(3 files)

That way agent sheets can link in chrome images and such even when we start tightening up our CheckLoadURI checks.

We can either do this for all sheets coming through LoadSheetSync(), do it for sheets that are allowed to have unsafe rules (and change the scrollbar sheet load to allow unsafe rules), or add a flag to LoadSheetSync indicating whether this should happen.

Another option is to only _force_ this for sheets that allow unsafe rules, but to change all chrome:// (not just content packages) to return the system principal from the channel; that would give it to the scrollbar sheet, in particular.

The most flexible solution is adding another argument, of course.

Thoughts?
I don't think we should give user stylesheets chrome privileges. Too easy for users to unknowingly shoot themselves in the foot. Especially if we start using stylesheet privileges for more things (thinking about XBL2 and onwards).

We should let them link to local images though IMHO.

what chrome sheets can't have unsafe rules? I think we've said that we should treat skins as extensions and let them do anything (so ideally we should kill all that code that unsuccessfully tries to prevent them from executing script). Or is unsafe rules something else?

Always forcing chrome for syncloaded sheets sounds scary. I'd prefer a flag for that even if it's true for all current callers. Unless it's a lot of effort.
> what chrome sheets can't have unsafe rules? 

Most of them.  The only time unsafe rules are allowed are when:

1)  The sheet is loaded using LoadSheetSync()
2)  The boolean for unsafe rules is passed as true to LoadSheetSync()

Basically, the idea is that the unsafe rules are only allowed in "ua" sheets.

"unsafe" rules are ones that style CSS anonymous boxes; changing those rules will actually lead to spec compliance bugs at best and fun crashes at worst, which is why we don't let random stylesheets do it.
So the current callers of LoadSheetSync are:

1)  nsLayoutStylesheetCache: loads scrollbar sheet, forms.css,
    userContent/userChrome.css.  I think we want the first two to get the
    system principal, but not the latter two.  The scrollbar sheet is
    chrome:// while the others are not.
2)  nsDocumentViewerImpl: loads sheets specified in a "usechromesheets"
    attribute.  I think this always comes from chrome in this case so it
    should be OK to just give these the system principal, but worth checking.
    The sheets that are loaded here, typically, are chrome://
3)  nsStyleSheetService: loads extension-provided user and ua sheets.  I think
    it makes sense to allow the UA ones to load chrome stuff.  URIs are
    arbitrary here.
4)  nsContentDLF: loads ua.css.  This should get the system principal and comes
    from a resource:// URI.
5)  ParseCSS: I think this has no need of system-anything.
6)  nsDocument::EnsureCatalogStyleSheet: not sure about these... probably safe
    to make them system, but not sure they need it.
7)  nsXBLResourceLoader: Only used for chrome:// URIs.  These sheets do need to
    get the system principal.  NOTE: in the chrome: case here we do NOT do a
    CheckLoadURI check.  We would need to do one, probably.
8)  nsXBLPrototypeResources: evil sync reload of sheets on skin flush.  I have
    no idea what should be happening here, to be honest; the current code is
    almost certainly wrong.
9)  nsXMLContentSink: Not sure where these sheets are and all.  Need to sort it
    out.
10) The Chrome registries: Again, I have no clue what the right thing is there. 
    They're loading chrome:// sheets, though.
11) nsHTMLEditor::AddOverrideStyleSheet: no idea what loads come through here;
    this probably needs the system principal.

Making all chrome:// sheets have the system principal would automatically handle  the scrollbar sheet and items 2, 6, 7, 10, and maybe 9 and 11.
Flags: blocking1.9?
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
So I think that if we're going to be able to do any serious security checks on XBL we'll really need this. At least unless we're ok with breaking remote XUL.
Flags: wanted1.9-
Flags: wanted1.9+
Flags: wanted-next+
Attached patch FixSplinter Review
This is against m-c.  I'll try to get 1.9.1 and 1.9.0 patches here ASAP; I'd like to take this for 1.9.0.8 if we take bug 481558.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #367652 - Flags: superreview?(jonas)
Attachment #367652 - Flags: review?(jonas)
Attachment #367652 - Flags: superreview?(jonas)
Attachment #367652 - Flags: superreview+
Attachment #367652 - Flags: review?(jonas)
Attachment #367652 - Flags: review+
Flags: blocking1.9.1?
Pushed http://hg.mozilla.org/mozilla-central/rev/94678a2f5ae9
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Attachment #367708 - Flags: superreview?(jonas)
Attachment #367708 - Flags: review?(jonas)
Attachment #367708 - Flags: superreview?(jonas)
Attachment #367708 - Flags: superreview+
Attachment #367708 - Flags: review?(jonas)
Attachment #367708 - Flags: review+
Summary: Give agent (and maybe user?) sheets the system principal → [FIX]Give agent (and maybe user?) sheets the system principal
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Flags: blocking1.9.0.8+
Comment on attachment 367708 [details] [diff] [review]
1.9.0 branch patch

Approved for 1.9.0.8, a=dveditz for release-drivers
Attachment #367708 - Flags: approval1.9.0.8? → approval1.9.0.8+
Checked into CVS.
Keywords: fixed1.9.0.8
This broke element hiding in Adblock Plus because user sheets no longer trigger content policies. Coming up with a solution before Firefox 3.0.8 release will be hard :-(
Keywords: late-compat
Attachment #367697 - Flags: approval1.9.1?
To explain what the problem is: Adblock Plus allows adding rules that will hide parts of web pages. These rules are translated into a user stylesheet. But Adblock Plus needs a way to detect "hits" (indispensable for tracking down issues) as well as "disable" these rules for some web pages (whitelisting). The current solution is using a -moz-binding rule pointing to no-op XBL - if a selector matches, XBL will be loaded and that can be detected by a content policy (this allows counting a "hit"). The content policy can also decide whether to block the XBL (the element will only be "hidden" if XBL is blocked). Now that content policies are not triggered any more the entire feature stopped working and I don't see any way to fix it right now.
We *could* work around this by calling content policies for user-sheet-linked XBL somehow. But i'm not sure how to detect user-sheet-ness.

The alternative is to back out this patch and find another solution to bug 483170. Shouldn't be too hard.
Can the XBL, instead of being no-op, have a constructor that dispatches an event the extension watches for?  Or do you need this to work also when JS is disabled?
(In reply to comment #15)
> Can the XBL, instead of being no-op, have a constructor that dispatches an
> event the extension watches for?

I thought about this solution already - no, it should work with JS disabled as well (Adblock Plus is often used in conjunction with NoScript).

A solution that I think will work: rather than load that no-op XBL from chrome:, load it from my own protocol. Then I will be able to recognize the hit there. Problem is finding the document where the hit happened, this will have to rely on implementation details (nsIChannel.notificationCallbacks is the only way I know). Now I only need to find time in the next two weeks to implement this...
You should be able to get the channels loadgroup and get the docshell off of that.
For 1.9.0, you would need to look at the notificationCallbacks of the channel's loadgroup.  For 1.9.1 and later, there is some devmo documentation on nsILoadContext, I hope.

I'm sorry about the timetable myself; it wasn't pleasant working on this stuff on a short deadline on our end either.  :(
Everything works fine in 1.9.1/1.9.2 - but in Firefox 3.0.7 I get the message "Security Error: Content at moz-nullprincipal:{...} may not load data from abp-elemhidehit://.../#dummy." regardless of what protocol flags I set. It seems that only loading chrome: or data: is allowed from a user stylesheet, not even resource: works. I guess that it is the same issue with Firefox 2.0, just without a message in the error console. Looking at the source code, this makes no sense to me - nsIScriptSecurity::CheckLoadURI is being called with ALLOW_CHROME flag, it shouldn't be that restrictive.
I realized that the error message doesn't come from nsIScriptSecurity::CheckLoadURI but rather from nsIPrincipal::CheckMayLoad call at the end of nsContentUtils::CheckSecurityBeforeLoad. This call will always cause an error with a null principal, and the only cases where it isn't called are chrome: and data: as I already suspected above. So there is no chance of using custom protocols in user stylesheets for browsers that don't have bug 416942 fixed. Meaning that I somehow have to combine the "new" and the "old" approach without being able to detect which principal is used for user stylesheets.
Can you hardcode a Gecko version check to decide on the approach?
Yes, that's what I will do it seems - even though I don't like hardcoded version checks.
I know; I just can't think of better options here.  :(
Wladimir Palant, can you give specific steps to trigger this bug (and what version of Adblock Plus is in use) so I can verify the fix for 1.9.0.8 (or do you want to verify that it is fixed on the nightly 1.9.0.x build from late 3/18 or early 3/19)?
Al Billings, steps to reproduce:

* Install Adblock Plus 1.0.1 from https://addons.mozilla.org/en-US/firefox/addon/1865
* Cancel the filter subscription selection if you get it after restart
* Go to Tools / Adblock Plus Preferences
* Click "Add filter", enter "mozilla.com#body" in the text field and click OK
* Go to http://www.mozilla.com/

Expected results: page body is hidden, you only see a blank page (this is what you get in Firefox 3.0.7). Instead, the page shows up as usual in Firefox 3.0.8 builds.

I checked in the fix for this issue (http://hg.mozdev.org/adblockplus/rev/719fd8885109), will try to release Adblock Plus 1.0.2 before Firefox 3.0.8.
Thanks!

Verified for 1.9.0.8 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.8pre) Gecko/2009031904 GranParadiso/3.0.8pre.
Adblock Plus 1.0.2 has been released.
You need to log in before you can comment on or make changes to this bug.