5.33 KB, patch
|Details | Diff | Splinter Review|
2.26 KB, patch
|Details | Diff | Splinter Review|
1.17 KB, text/xml
I thought we already had a bug on this, but I don't see one... I see bugs on the opposite, in fact (bug 236839) and on not-quite-same (bug 277836). In any case, from the discussion on security-group from a few days back: As a short-term fix, we could disallow script in bindings by making nsXBLBinding::AllowScripts() look at the security context of either the bound element or the binding document (in the latter case we may also be able to do this on the XBLDocumentInfo level; see nsXBLDocumentInfo::nsXBLDocumentInfo). The more I think about it, the more I think that the right security context is that of the binding document. If people think otherwise, please let me know.
Another option is to use the security context of the stylesheet that has the 'winning' -moz-binding rule. This way xbl that's added through extensions will still work, but it's not possible for content to bind to chrome-xbl and get scripts running. I know we've talked about using this context to compile the scripts in the xbl. That, of course, is a whole other discussion. But it's where I got the idea.
> Another option is to use the security context of the stylesheet that has the > 'winning' -moz-binding rule. We don't know which sheet that is, as it happens.... (we don't keep track of that in the style system).
That could be fixed, no? Couldn't we work our way back from the rule to the stylesheet whenever we hit an -moz-binding rule? Or attach the stylesheet rule to the -moz-binding property as part of the parsed value?
> Or attach the stylesheet rule to the -moz-binding property as part of the > parsed value? This we could maybe do, but doing it without leaking would be interesting...
Err.. I ment: Attach the stylesheet _uri_ as part of the parsed value. That should be easy to do without leaking since it's just a refcounted nsIURI
Yeah, that we could do. Might not be a bad idea.... David, what do you think?
For this bug it should even be enough to attach a flag indicating if the stylesheet has a chrome uri or not (does userContent.css have a chrome uri?). But maybe the entire uri could be usefull for other things.
> (does userContent.css have a chrome uri?) It has a file:// URI for that file (gotten via the directory service).
This bug might still be exploitable in Thunderbird even though bug 292589 has been fixed, because the XBL could be an attachment (mailbox or imap URL, IIRC) or have a data: URL.
Jesse and I talked about this a bit. Using the bound node security context means that disabling JS will also disable any scripts in chrome bindings attached to content (eg marquee, flashblock, remote XUL, etc, etc). Using the binding security context means that content can try to load chrome bindings and get them to run script for it somehow... doesn't break as much, but is a much bigger attack vector). Note that using the CSS file URI is identical to using the binding URI in this last case -- both are chrome, really. Unless we're worried about the case where content binds a binding to an unexpected node type or something?
(In reply to comment #13) > Note that using the CSS file URI is identical to using the binding URI in this > last case -- both are chrome, really. Unless we're worried about the case > where content binds a binding to an unexpected node type or something? I don't get this comment. Or are you reffering to the fact that you can link to a chrome-css file. Why are we allowing that btw? Seems like no reason to let content link to a chrome-css file.
> Or are you reffering to the fact that you can link to a chrome-css file. That too, but also a lot of the bindings are attached via the UA stylesheet (see xul.css). > Seems like no reason to let content link to a chrome-css file. We want remote XUL to use the current theme.
Created attachment 183756 [details] [diff] [review] Maybe this? Not yet tested.... This is the best I can do given the API the security manager exposes.... Ideas on how to test this are welcome. :(
Created attachment 183759 [details] [diff] [review] Slightly cleaner I filed bug 294381 on a better CanExecuteScripts() api...
Created attachment 183760 [details] [diff] [review] And even compiling I've tested that this stops script execution on the testcases in bug 289074 comment 91 and bug 289074 comment 93. I've also tested that it does NOT stop script execution on the testcase in bug 277836 (since that's a chrome binding).
Comment on attachment 183760 [details] [diff] [review] And even compiling sr=dveditz
Comment on attachment 183760 [details] [diff] [review] And even compiling a=shaver for trunk and branches.
Fixed for 1.8b3, 1.7.9, and aviary 1.0.5.
Created attachment 185575 [details] testcase for QA This works in: Firefox 1.0+ (2005-06-02-18) Firefox 1.0.4 Mozilla 1.7.8 I guess this testcase would be needed for QA. because the old testcases (in bug 289074 comment 91 and bug 289074 comment 93) have been stopped in Firefox 1.0.3 and Mozilla 1.7.7.
It's likely that this caused at least two regressions (the bugs it depends on). We should sort those out before we ship a branch with this fix...
v.fixed on aviary with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.9) Gecko/20050706 Firefox/1.0.5 using test case in comment #23
Security advisories published
This is new MFSA2005-46 advisory; http://www.mozilla.org/security/announce/mfsa2005-46.html now. For 'Alias' section this is SA16043's issue 2). Is it possible to update 'Alias' field?