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?

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

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

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

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

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.

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

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

No idea how to test this automatically...

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

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

I called the method URIEquals

Comment on attachment 263697 [details] [diff] [review] With that change r=dbaron on the layout/style changes

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

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.

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

Fixed on trunk.

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