Closed Bug 135267 Opened 22 years ago Closed 22 years ago

Reading files cross-host using styles

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0.1

People

(Reporter: security-bugs, Assigned: security-bugs)

References

Details

(Whiteboard: [sg])

Attachments

(2 files)

Example
445 bytes, text/html
Details
Patch - block cross-origin DOM access to style rule collections
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
Attached file Example — 

    
    

    
  
Can't this be solved with the javascript security policy by setting
document.styleSheets to sameorigin?
Keywords: nsbeta1+
Whiteboard: [ADT2 RTM]

    
    

    
  
Cc'ing helpers.

/be

    
    

    
  


  
I've verified that this fixes the problem, but I'm not sure what other tests to
run (to look for regressions).

    
    

    
  
Comment on attachment 85380 [details] [diff] [review]
Patch - block cross-origin DOM access to style rule collections

sr=scc

        Attachment #85380 -
        Flags: superreview+

    
    

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

    
    

    
  
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

    
    

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

    
    

    
  
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.

    
    

    
  
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?

    
    

    
  
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?

    
    

    
  
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.

    
    

    
  
Let's try that again...
Status: RESOLVED → VERIFIED

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
> the attacker could just visit the site himself

Not if the site requires authentication (cookies, HTTP auth, etc)...

    
    

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

    
    

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



    
    

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

    
    

    
  
> any feature that could expose a hole should be blocked during review process

Does that include not exposing CSS comments via the CSSOM, ever?

    
  

        Attachment #85380 -
        Flags: approval+

    
    

    
  
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+

    
    

    
  
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.
Blocks: 143047
Keywords: adt1.0.1 → adt1.0.1+
Whiteboard: [ADT2 RTM] → [ADT2 RTM] [ETA 06/24]
Target Milestone: --- → mozilla1.0.1

    
    

    
  
Please check this in today.  If it's already been checked in, please replace the
mozilla1.0.1+ keyword with fixed1.0.1+.

    
    

    
  
checked in on the branch.
Keywords: mozilla1.0.1+ → 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.


    
    

    
  
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
Group: security?

    
  
Keywords: adt1.0.1+
Whiteboard: [ADT2 RTM] [ETA 06/24]

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: