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

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.9.1 branch patch

Comment 7

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/94678a2f5ae9

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.9.0 branch patch

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 367708 [details] [diff] [review]
1.9.0 branch patch

Approved for 1.9.0.8, a=dveditz for release-drivers

Comment 10

[Boris Zbarsky [:bzbarsky]]

Checked into CVS.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/007e38c25862

Comment 12

[Wladimir Palant]

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 :-(

Comment 13

[Wladimir Palant]

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.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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?

Comment 16

[Wladimir Palant]

(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...

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You should be able to get the channels loadgroup and get the docshell off of that.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 19

[Wladimir Palant]

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.

Comment 20

[Wladimir Palant]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Can you hardcode a Gecko version check to decide on the approach?

Comment 22

[Wladimir Palant]

Yes, that's what I will do it seems - even though I don't like hardcoded version checks.

Comment 23

[Boris Zbarsky [:bzbarsky]]

I know; I just can't think of better options here.  :(

Comment 24

[Al Billings [:abillings - ex-MoCo]]

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)?

Comment 25

[Wladimir Palant]

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.

Comment 26

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 27

[Wladimir Palant]

Adblock Plus 1.0.2 has been released.