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

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
XBL
P3
enhancement
RESOLVED FIXED
14 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: bz)

Tracking

Trunk
mozilla1.9alpha8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

14 years ago
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.)
I am not quite sure I understand what "user style sheets" mean in this context?
Are they modified chrome?
(Reporter)

Comment 2

14 years ago
"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.
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.
(Reporter)

Comment 4

14 years ago
> 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.
Would a chrome: or resource: url work for this?

Comment 6

14 years ago
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...).
> 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

14 years ago
> 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.)
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

14 years ago
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.
Let's keep that discussion in bug 221324, since it has nothing to do with this
bug...
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>
> maybe there is some magic we can do there

There is not.
(Reporter)

Comment 14

12 years ago
See also bug 310165, "Allow userContent.css to link to local images".
Depends on: 221428
Depends on: 377091
Created attachment 263559 [details] [diff] [review]
Proposed fix

David, could you review the style system parts?  Jonas, could you look at the XBL parts?
Assignee: hyatt → bzbarsky
Status: NEW → ASSIGNED
Attachment #263559 - Flags: superreview?(jonas)
Attachment #263559 - Flags: review?(jonas)
Attachment #263559 - Flags: review?(dbaron)
No idea how to test this automatically...
Flags: in-testsuite?
OS: Windows XP → All
Priority: -- → P3
Hardware: PC → All
Summary: Allow user style sheets to load XBL from file: URLs → [FIX]Allow user style sheets to load XBL from file: URLs
Target Milestone: --- → mozilla1.9alpha5
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
Attachment #263559 - Flags: review?(dbaron) → review+
> 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?
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...
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.
Introducing a new method.

(Sorry, my tree doesn't have the previous patch.)
Created attachment 263697 [details] [diff] [review]
With that change

I called the method URIEquals
Attachment #263559 - Attachment is obsolete: true
Attachment #263697 - Flags: superreview?(jonas)
Attachment #263697 - Flags: review?(jonas)
Attachment #263697 - Flags: review?(dbaron)
Attachment #263559 - Flags: superreview?(jonas)
Attachment #263559 - Flags: review?(jonas)
Comment on attachment 263697 [details] [diff] [review]
With that change

r=dbaron on the layout/style changes
Attachment #263697 - Flags: review?(dbaron) → review+
Target Milestone: mozilla1.9alpha5 → mozilla1.9alpha6
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.  :(
Attachment #263697 - Flags: superreview?(jonas)
Attachment #263697 - Flags: superreview-
Attachment #263697 - Flags: review?(jonas)
Attachment #263697 - Flags: review-
Created attachment 268602 [details] [diff] [review]
Unbitrotted, I think
Attachment #263697 - Attachment is obsolete: true
Attachment #268602 - Flags: superreview?(jonas)
Attachment #268602 - Flags: review?(jonas)
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Blocks: 388597
Attachment #268602 - Flags: superreview?(jonas)
Attachment #268602 - Flags: superreview+
Attachment #268602 - Flags: review?(jonas)
Attachment #268602 - Flags: review+
Upps, forgot to add comment: You're removing the nsContentUtils::CheckSecurityBeforeLoad call from loadBindingDocument. Where does that check live now?
It's in LoadBindingDocumentInfo.  Which is always called by LoadBindingDocument.  Before this patch the check is just done twice.
Created attachment 272868 [details] [diff] [review]
Merged to trunk
Attachment #268602 - Attachment is obsolete: true
Created attachment 272869 [details] [diff] [review]
Missing file with the URIEquals impl

David, could you give this a once-over?  I forgot to include this file in the earlier diffs...
Attachment #272869 - Flags: review?(dbaron)
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 31

10 years ago
Does the fix here apply to the stylesheet service as well?
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 on attachment 272869 [details] [diff] [review]
Missing file with the URIEquals impl

r+sr=dbaron
Attachment #272869 - Flags: superreview+
Attachment #272869 - Flags: review?(dbaron)
Attachment #272869 - Flags: review+
Blocks: 379959
Blocks: 387971
You need to log in before you can comment on or make changes to this bug.