Closed
Bug 135267
Opened 22 years ago
Closed 22 years ago
Reading files cross-host using styles
Categories
(Core :: Security, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.0.1
People
(Reporter: security-bugs, Assigned: security-bugs)
References
Details
(Whiteboard: [sg])
Attachments
(2 files)
445 bytes,
text/html
|
Details | |
2.02 KB,
patch
|
security-bugs
:
review+
scc
:
superreview+
jud
:
approval+
|
Details | Diff | Splinter Review |
Got this for IE on bugtraq this morning: Reading portions of local files, depending on structure http://security.greymagic.com/adv/gm004-ie/ Mozilla is partially affected by a modification of this. Thru stylesheets it is possible to read parts of remote files (does not work with local files) in case the file contains curley bracket - "{". The attached file "styleread.html" reads the stylesheets from netscape.com
Assignee | ||
Comment 1•22 years ago
|
||
Comment 2•22 years ago
|
||
Can't this be solved with the javascript security policy by setting document.styleSheets to sameorigin?
Comment 3•22 years ago
|
||
Cc'ing helpers. /be
Assignee | ||
Comment 4•22 years ago
|
||
I've verified that this fixes the problem, but I'm not sure what other tests to run (to look for regressions).
Comment 5•22 years ago
|
||
Comment on attachment 85380 [details] [diff] [review] Patch - block cross-origin DOM access to style rule collections sr=scc
Attachment #85380 -
Flags: superreview+
Assignee | ||
Comment 6•22 years ago
|
||
Comment on attachment 85380 [details] [diff] [review] Patch - block cross-origin DOM access to style rule collections r=dveditz. We still need testing, though, and I'm not sure what to test.
Attachment #85380 -
Flags: review+
My testing thoughts, from email: * Visit a page with multiple stylesheets (alternate, etc.) and make sure that they are switchable with View->Use Style. http://www.alistapart.com/stories/alternate/ is such a page. * DOM inspector, visit a page with multiple stylesheets and make sure that they all appear in the "Stylesheets" version of the left view. * content/xml/tests/toc and content/xml/tests/books both seem to use the .styleSheets property, give them a spin. * test16 from viewer, too. * chatzilla uses it too, if you want to be really thorough.
Actually, I don't think this code should be triggered by any of those things. It should only be triggered by CSSOM usage, which is somewhat rare (since the CSSOM is horribly designed, etc., etc.). bzbarsky would know exactly what would use this, and probably be able to point to testcases. He'd also know if there were other access points for the same or similar information (although I suspect you've already checked, to some extent). I don't *think* (off the top of my head, without looking at the code) that this bug is as serious as the vulnerablility in IE, since our CSS stylesheet data structures only store what's valid CSS (and we then reconstruct .cssText from our data structures), at least for the most part. That may not be true for everything, though (CSSUnknownRule?).
Assignee | ||
Comment 9•22 years ago
|
||
That's what I figured too. This can only be used to steal data that looks something like valid CSS. Georgi's testcase, above, only reads style data from netscape.com, not scripts or any other sort of content. I tried Shaver's recommended tests, just for the halibut, and everything appears to work fine. So, I need to know if Mozilla wants this patch for 1.0 or not. Do you think an attacker could glean valuable data from inline CSS on a bank's page, for example? I'll check this in if you want it, just let me know within a few hours.
Status: NEW → ASSIGNED
Comment 10•22 years ago
|
||
We don't implement CSSUnknownRule and I hope we never do. There are no scriptable access points for the rules list other than this that I know of, and there are no scriptable access points for rules other than the rules list. So the patch looks good to me.... Things to test, possibly: Chrome: 1) The stylesheet list in Inspector (click on a sheet; I _think_ it should somehow make the rules visible....) 2) Switching between beginner/intermediate/advanced user levels in XMLTerm (easy way to exercise this code). Content: 1) http://lxr.mozilla.org/seamonkey/source/extensions/xmlterm/ui/content/styletest. html (uses dump() to dump out the sheet). 2) http://bugzilla.mozilla.org/attachment.cgi?id=56048&action=view 3) http://bugzilla.mozilla.org/attachment.cgi?id=84012&action=view 4) http://www.xs4all.nl/~ppk/js/changess.html (the "Try changing the colour of all PRE's to blue" link)
Comment 11•22 years ago
|
||
Comment on attachment 85380 [details] [diff] [review] Patch - block cross-origin DOM access to style rule collections >+ JSContext *cx = nsnull; >+ nsresult rv = NS_OK; >+ >+ rv = stack->Peek(&cx); Nit: no need to initialize rv if it's immediately set to another value. /be
Comment on attachment 85380 [details] [diff] [review] Patch - block cross-origin DOM access to style rule collections >+ if (NS_FAILED(rv)) { >+ return rv; >+ } While we're nitting, this is inconsistent with the other the uses of NS_ENSURE_TRUE, and the brace style of the rest of the patch for single-line then clauses.
Assignee | ||
Updated•22 years ago
|
Keywords: adt1.0.1,
mozilla1.0.1
Given the fact that we don't have CSSUnknownRule, that cssText can only disclose CSS valid rules, that it is unlikely that a class name in a stylesheet will contain a super-important information like a credit card number or a password, that CSS files do not store OS-related information, I do believe that we have no super-urgent mega-important issue here (to say the least). There is no possible comparison between the way we could be hit by this problem and the way IE is currently hit by it.
Comment 14•22 years ago
|
||
Assuming the patch is done, would it be possible to land this in 1.1alpha and get some of the test cases mentioned in the bug verified before landing on the branch?
Assignee | ||
Comment 15•22 years ago
|
||
Fix checked in on the trunk - please verify this ASAP.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
IMPORTANT : Am I correct if I say that if I create a document using a W3C stylesheet, publish this document on my personal web site, the CSS OM won't be able to list the rules in the attached stylesheet with this patch in ? If I am correct, this is a too strong restriction and I don't think this should land. Correction : I am sure this should not land.
Daniel, I am not sure what you mean (CSS OM?). I think this does the same thing as the fix for XSLT (although I have not checked this patch yet). The XSLT patch allows (I think, I should check again) a document on site A to read in a stylesheet from site B, but the stylesheet at site B can only read additional files from site A. Do you want to: 1) have site B stylesheet only be able to read in files at site B, 2) have site B stylesheet be able to read in files from sites A and B?
Assignee | ||
Comment 18•22 years ago
|
||
From Bindu: Verified the http://bugzilla.mozilla.org/show_bug.cgi?id=135267 on today's trunk build. Steps: 1. Clciked on the attachment example. 2. Wait for page to load. 3. Saw an exception in the console and no alert. Exception: Error: uncaught exception: [Exception... "Access to restricted URI denied" code: "1012" nsresult: "0x805303f4 (NS_ERROR_DOM_BAD_URI)" location: "http://bugzilla.mozilla.org/attachment.cgi?id=77527&action=view Line: 15"] Marking Verified.
Comment 20•22 years ago
|
||
Daniel, your statement in comment #16 is exactly correct. Consider the following: If you have an <iframe> on your site in which you include the W3C site, you will not be able to access the DOM of the W3C site. This patch merely applies a similar restriction to stylesheets.
Comment 21•22 years ago
|
||
But those aren't exactly equivalent. In the foreign <iframe> the user could start inputting data or otherwise interacting with the site, and we want to prevent eavesdropping (the initial DOM state is uninteresting, the attacker could just visit the site himself). Granted I'm not sure why you'd want to walk the style rules in some foreign sheet you've applied to your document, but I don't see how it's leaking any information. Maybe a CheckLoadURI to block file:// sheets would be sufficient.
Comment 22•22 years ago
|
||
> the attacker could just visit the site himself
Not if the site requires authentication (cookies, HTTP auth, etc)...
Comment 23•22 years ago
|
||
Exactly, that's what I was trying to say. The page as viewable by the attacker is obviously uninteresting or he would just go view it; something about adding the user into the mix makes it worth trying to eavesdrop. I don't see how this carries into the CSS case. If the "attacker" can style his page using the foreign sheet then he obviously has access to the sheet. If we're worried that he only has access because he's borrowing the user's cookie auth then maybe we should block his ability to style using that sheet entirely, but I don't see the point of applying the style yet blocking script ability to see them. If you apply the sheet you can do things like getComputedStyle() to try to figure out what was in it. (We already block the ability to style using local file:// sheets from remote content, sorry for not checking before suggesting it.)
Comment 24•22 years ago
|
||
> If the "attacker" can style his page using the foreign sheet then he obviously
> has access to the sheet.
No... All that means is that at pageload we put up a password dialog
_for_the_sheet_ and the user logged in to the site the sheet is being gotten
from via this dialog. Or that the user has a stored cookie that allows access
to this sheet that we send with the request.
This is really no different from the <iframe> case, as far as I can tell...
Oh, and getComputedStyle is a very imprecise tool for getting the original
stylesheet data. It's a darned crappy one in fact.
Dan and all : in a former professionnal life at EDF, I had to deal with a dozen of corporate web sites where the pages had to be presented with a corporate stylesheet available from a thirtheenth web site. No local copy of the sheet was allowed to be sure that an update of the sheet would affect all pages. In such a environment, no web page will be able to browse the contents of the style sheet using the CSS OM because of the fix for the current bug. I am saying that this restriction is (a) too strong (b) not in the spirit of CSS (b) almost useless because all that can be retrieved by this mean is CSS style rules that have been validated by the CSS parser and re-generated. There is no way in Mozilla the document can access the SOURCE of the file. Conclusion : only CSS rules in the file will be made available through CSS OM. In particular, if the file is *not* a CSS rule, the CSS OM will show nothing at all. This is clearly different from an IFRAME and DOM because a sheet does not contain **content**, only stylistic rules. One does not store a password in a CSS rule, nor a credit card number. Let me rephrase with perhaps stronger words, but more perhaps also more accurate : Ok, I can build a document here able to detect that www.netscape.com has in itself a CSS rule defining yellow color for a div ! So what ? Isn't the solution worse than the problem ?
Assignee | ||
Comment 26•22 years ago
|
||
Daniel, You're beginning to convince me. However, I'm worried about future changes to CSS or the CSSOM opening up security holes. Currently, as you say, the CSSOM only exposes parsed style rules, not source, and no other information about the document. Will this always be so? Will a future version of CSS possibly allow stylesheets to contain more information that might be of interest to an attacker? If so, I can easily see someone adding additional functionality to CSS without being aware of the security consequences. This patch makes us future-proof. Also, are we absolutely sure that the CSSOM won't pick up other content in the page that looks like CSS but isn't? Like scripts, maybe? Is that possible?
> You're beginning to convince me. However, I'm worried about future changes to > CSS or the CSSOM opening up security holes. Currently, as you say, the CSSOM > only exposes parsed style rules, not source, and no other information about the > document. Will this always be so? Mitch, I am now the editor of the future versions of the CSS OM, in the CSS WG. We have in the CSS WG two "keepers of the temple", the two inventors of CSS. Vidur can confirm that these two guys will never let CSS include anything else but declarative stylistic stuff. When we tried together a few years ago to add scripts to CSS sheets, we faced a so strong opposition that it went fast into /dev/null. > Will a future version of CSS possibly allow stylesheets to contain more > information that might be of interest to an > attacker? No. The CSS WG also cares for security issues and any feature that could expose a hole should be blocked during review process. Again, I am the editor of the document, we should be able to do something here ;-) If we all agree here, I could raise the issue of this bug to the CSS WG and I am pretty sure that the WG will come up with a clarification about CSSUnknownRule. > If so, I can easily see someone adding additional functionality to CSS > without being aware of the security consequences. This patch makes us > future-proof. Frm my point of view, the fact that we don't implement CSSUnkownRule is far enough. We are proof to all non-CSS content that a malicious document would try to reach through stylesheet linking. > Also, are we absolutely sure that the CSSOM won't pick up other content in the > page that looks like CSS but isn't? Like scripts, maybe? Is that possible? Well, since our implementation of the CSS OM will just not keep a rule or selector or declaration if it's not a valid CSS rule, I guess that I can answer without any fear "yes, we are sure it can't pick up other content". I recommend (a) backing out this patch (b) find a less intrusive fix or explain, perhaps through an offical PR, that the security warning does not apply to Mozilla/Netscape because we don't implement CSSUnkownRule (explanations above). (I will be away from the office during the following weeks and will not be able to contribute to this discussion...)
Comment 28•22 years ago
|
||
> any feature that could expose a hole should be blocked during review process
Does that include not exposing CSS comments via the CSSOM, ever?
Updated•22 years ago
|
Attachment #85380 -
Flags: approval+
Comment 29•22 years ago
|
||
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+" keyword and add the "fixed1.0.1" keyword.
Keywords: mozilla1.0.1 → mozilla1.0.1+
Comment 30•22 years ago
|
||
adt1.0.1 (on ADT's behalf) approval for checkin into the 1.0 branch. pls check this in asap, then add the "fixed1.0.1" keyword.
Comment 31•22 years ago
|
||
Please check this in today. If it's already been checked in, please replace the mozilla1.0.1+ keyword with fixed1.0.1+.
So the answer to my comment #27 was "check that in everywhere" ?!?!? Sorry to say so, but we took the worst direction here IMHO.
Comment 34•22 years ago
|
||
Verified on 2002-07-23-branch build on Win 2000. Steps: 1. Clciked on the attachment example. 2. Wait for page to load. 3. Saw an exception in the console and no alert. Exception: Error: uncaught exception: [Exception... "Access to restricted URI denied" code: "1012" nsresult: "0x805303f4 (NS_ERROR_DOM_BAD_URI)" location: "http://bugzilla.mozilla.org/attachment.cgi?id=77527&action=view Line: 15"] Also loaded the styles pointed out in comment #7 * Visit a page with multiple stylesheets (alternate, etc.) and make sure that they are switchable with View->Use Style. http://www.alistapart.com/stories/alternate/ is such a page. * content/xml/tests/toc and content/xml/tests/books both seem to use the .styleSheets property, give them a spin.
Keywords: fixed1.0.1 → verified1.0.1
Assignee | ||
Updated•22 years ago
|
Group: security?
Just adding an annotation so I can find this bug again. FTR I agree with the resolution. The content of stylesheets could be sensitive in some bizarre cases. E.g., monitor the classes used in the style sheet for Company X's intranet home page, and notice when someone adds the class "earnings-alert" or changes its style. Ideally we would turn off cross-domain style sheets completely, but since that's too restrictive, this is the best we can do, and it's a lot better than nothing. It also protects us against future CSSOM changes --- or bugs in our CSS implementation.
Whiteboard: [sg]
You need to log in
before you can comment on or make changes to this bug.
Description
•