204140 - [FIX]Allow user style sheets to load XBL from file: URLs

Status: RESOLVED FIXED

(Core :: XBL, enhancement, P3)

Description

[Jesse Ruderman]

The fix for bug 200691 prevents http: pages from loading XBL from file: URLs. 
This also prevents user style sheets from making http: pages load XBL from file:
URLs.  It would be nice if -moz-binding in user style sheets could be exempt
from checkloaduri.

See http://www.squarefree.com/userstyles/xbl.html for an example of using XBL in
a user style sheet.

(Probably related, but not this bug: if a user style sheet mentions an image in
a "content" property value, Mozilla requests the image with the referrer set to
a web page the image appears in.)

Comment 1

[georgi - hopefully not receiving bugspam]

I am not quite sure I understand what "user style sheets" mean in this context?
Are they modified chrome?

Comment 2

[Jesse Ruderman]

"User style sheet" means userContent.css, a file created by users.  Style rules
in userContent.css are applied to all web pages.  The CSS spec specifies what
happens when rules in user style sheets and web page style sheets conflict.

Comment 3

[Mitchell Stoltz (not reading bugmail)]

Couldn't allowing user style sheets to load XBL be abused by an attacker, as in
Georgi's original exploit? If no one's depending on this behavior, I say we
leave it as is.

Comment 4

[Jesse Ruderman]

> Couldn't allowing user style sheets to load XBL be abused by an attacker, as 
> in Georgi's original exploit?

I don't completely understand the exploit in bug 200691, but it seems like the
URL of the XBL matters to the security manager in some way (checkloaduri?
same-origin? both?).  IMO, that's a bug, because it's easy for a web page to
modify what a script does by redefining functions or putting getters and setters
on global variables.

> If no one's depending on this behavior, I say we leave it as is.

A few users e-mailed me regarding http://www.squarefree.com/userstyles/xbl.html
and suggested that I change the instructions to suggest using file: URLs.  I
e-mailed them back saying I didn't recommend that because between bug 200691
being fixed and this bug (which I hadn't yet filed) being fixed, that wouldn't
work.  I think it's safe to assume that some users used file: URLs without
e-mailing me.

Also, it's a lot easier to use a file: URL when setting up flash.xml than it is
to set up a local web server and then use a http://localhost/ URL.  Using
flash.xml at its original location on www.cs.hmc.edu is slower, and since the
purpose is to block Flash, being slower can be bad.

Comment 5

[(not currently active) Ted Mielczarek]

Would a chrome: or resource: url work for this?

Comment 6

[Jiri Znamenacek]

It would be nice at least to allow user_.css to load something like user_.xbl
from the same directory, ie. locale profile. For now it's possible to load XBL
from resource, which is not portable (get discarded during upgrade etc.).

And for security: When somebody can hack to user's computer, i think it's not so
important wheather he should hack Mozilla installation directory or user's
profile directory (speaking of Windows where user without admin privilages can
do practicaly nothing so all users end as admins...).

Comment 7

[Boris Zbarsky [:bzbarsky]]

> speaking of Windows

Let's not break our security model on all OSes because Windows boxes have bad
admins, ok?

I'm a little confused by one thing here -- why are we using the DOCUMENT url for
the security check?  Would it not make more sense to use the URL of the
_stylesheet_ that the rule came from for the security check?  This is not
entirely trivial because it's not obvious from the computed style what sheet
that is, but perhaps the security check could be made at style-resolution time,
with declarations that would fail the security check simply being ignored?  (Not
sure how legal this is from a CSS standpoint....)

Comment 8

[Jiri Znamenacek]

> Let's not break our security model on all OSes because Windows boxes have bad
> admins, ok?

Good point. But when we're not counting single-user instalation, don't we have
the same problem on Linux too? (Sadly, none of my Linux friends have ever tried
to do the same *crazy things* with Mozilla as I did on Windows, so maybe I'm
completely wrong. But I am still convinced userXBL.xml in profile directory
would be nice thing.)

Comment 9

[Mitchell Stoltz (not reading bugmail)]

Boris,
   As a general rule, all content included by reference (images, .js files, css
files, etc.) are considered to have the URL of the page they're included in, for
security checks. We assume that referring to a remote file means the page author
trusts that file enouogh to let it into the domain of the page. I could see
making an exception for userContent.css since it isn't actually referenced in
the page, but I agree that may not be trivial.

Comment 10

[Alan Eliasen]

Has something broken with protocol handling here?  For many moons, I've used the
flash blocking as described at:

http://www.squarefree.com/userstyles/xbl.html

It loads from an http URL because of this bug.  And as of the 2003-10-04 or so
builds, this causes Mozilla to not start with "http is not a registered
protocol" errors.  Removing the userContent.css file fixes things (at the
expense of getting noisy flash ads again.)  See also bug 221324.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Let's keep that discussion in bug 221324, since it has nothing to do with this
bug...

Comment 12

[Vidar Haarr (not reading bugmail)]

I guess the first problem we'll have to solve is how to know that the
-moz-binding instruction comes from user_.css and not the documents CSS?

In nsCSSFrameConstructor::ConstructFrameInternal[1], we have access to the
nsStyleContext, maybe there is some magic we can do there to know what
stylesheet display->mBinding originates from? Then we could check and if it's
user_.css, and the binding's URL points to "[ProfD]/chrome/userXBL.xml", we
modify the call to nsScriptSecurityManager::CheckLoadURI*[2] so that the source
is set to chrome:// (and thereby bypassing the file:// allowance pref checking).

1:
<http://lxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp#7635>
2:
<http://lxr.mozilla.org/seamonkey/source/caps/src/nsScriptSecurityManager.cpp#1201>

Comment 13

[Boris Zbarsky [:bzbarsky]]

> maybe there is some magic we can do there

There is not.

Comment 14

[Jesse Ruderman]

See also bug 310165, "Allow userContent.css to link to local images".

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

David, could you review the style system parts?  Jonas, could you look at the XBL parts?

Comment 16

[Boris Zbarsky [:bzbarsky]]

No idea how to test this automatically...

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 263559 [details] [diff] [review]
Proposed fix

>Index: layout/style/nsStyleStruct.cpp
>+static PRBool EqualURIs(nsCSSValue::URL *aURI1, nsCSSValue::URL *aURI2)
>+{
>+  return aURI1 == aURI2 ||    // handle null==null, and optimize
>+         (aURI1 && aURI2 && *aURI1 == *aURI2);

I think you want to call nsIURI::Equals directly rather than using nsCSSValue::URL::operator==, since nsStyleStruct users shouldn't care whether the string is equal; they should care only about the nsIURI.  This code also duplicates a bunch of unneeded null-checks.

With that, r=dbaron on the changes in layout/style

Comment 18

[Boris Zbarsky [:bzbarsky]]

> since nsStyleStruct users shouldn't care whether
> the string is equal; they should care only about the nsIURI.

They should care about the URI and probably the origin principal (for javascript: URIs), no?

I suppose I could just do the principal equality directly, or expose a FastEquals method on nsCSSValue::URL that skips null-checks and ignores the string (so that we can avoid duplicating this code).

Sound ok?

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Ah, right, I was looking at the current definition of operator== when I wrote that.  That sounds fine.

That said, shouldn't you be changing the definition of nsCSSStruct::URL::operator== as well?  It's not in the patch...

Comment 20

[Boris Zbarsky [:bzbarsky]]

nsCSSValue::URL::operator== looks at the principal as of bug 377091.  Is that what you were asking?

Also, which exactly sounds fine?  Introducing a new method?  Or just using the operator== as the patch does?  Let me know; I can do either one.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Introducing a new method.

(Sorry, my tree doesn't have the previous patch.)

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: With that change

I called the method URIEquals

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 263697 [details] [diff] [review]
With that change

r=dbaron on the layout/style changes

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 263697 [details] [diff] [review]
With that change

Crap.  The patch for bug 379959 bitrotted this really badly.  I'm not even sure how to fix this to work again yet, given the additional security checks that patch added.  :(

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Unbitrotted, I think

Comment 26

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

Upps, forgot to add comment: You're removing the nsContentUtils::CheckSecurityBeforeLoad call from loadBindingDocument. Where does that check live now?

Comment 27

[Boris Zbarsky [:bzbarsky]]

It's in LoadBindingDocumentInfo.  Which is always called by LoadBindingDocument.  Before this patch the check is just done twice.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to trunk

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: Missing file with the URIEquals impl

David, could you give this a once-over?  I forgot to include this file in the earlier diffs...

Comment 30

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 31

[Jason Barnabe (np)]

Does the fix here apply to the stylesheet service as well?

Comment 32

[Boris Zbarsky [:bzbarsky]]

I think it should, yes.  Certainly sync-loaded sheets get principals from their channel, so the rest should just follow.  Try it and let me know?  ;)

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 272869 [details] [diff] [review]
Missing file with the URIEquals impl

r+sr=dbaron