It is possible to read xml files (as far as I could test any well formed
in xml terms files) residing on arbitrary domains.
The problem is in the xml stylesheets and namely: xsl:import
Find attached several files - open xmlexpl.html which reads
secret.xml which may be anywhere. To change the location of secret.xml
xsl:import acts like #include in C and then the whole document may be accessed
I suggest fixing this.
When fixing have in mind HTTP redirects - some xml components had similar
problems in the past.
IE gives "access denied" on this example.
I think we need to do a same-origin check on XSL Include - IE apparently does.
Created attachment 60241 [details]
testcase - xmlempl.html
Created attachment 60242 [details]
Created attachment 60243 [details]
Created attachment 60244 [details]
Haven't tried the example yet, but I did put in code to try and prevent this.
Maybe it's not right? Look at
It should check if the load is allowed from the main stylesheet.
Peter, the example above doesn't seem to hit the security check you mentioned at
all. Could you please try it out? You can use
http://warp/u/mstoltz/bugs/113351/xmlempl.html inside the firewall.
Mitch, warp has bad mimetypes for .xml files :(. I've finally gotten the example
to work, I've put it up on green, accessing the file you put on grey. You should
try to load http://green.nscp.aoltw.net/peterv/test.xml, that'll use
http://green.nscp.aoltw.net/peterv/xmlexpl.xml to do an XSLT transformation, and
xmlexpl.xml will load http://grey/u/mstoltz/bugs/113351/secret.xml. You'll hit
the breakpoint I mentioned, and finally the security check (CheckLoadURI) will
go through because we end up in
Please advise if I should change the CheckLoadURI check into something else.
Try replacing the call to CheckLoadURI with
nsScriptSecurityManager::CheckConnect(). If you have the current JS context,
pass that in, otherwise pass null. TargetURI is the URI of the included XSL
file. For ClassName and PropertyName, just pass in some descriptive string
literals, like "XSL", "Include" or something like that.
Mitch, should the JSContext be the context of the document which tries to load
the other documents? I tried passing in null, but then CheckConnect returns
immediately because it can't get the JS context, so it assumes the load is OK.
Whoops, I forgot that CheckConnect assumes that it's being called from
this bug, and try to come up with a new security check.
Critical Moz1.0 fixes
Part of my patch to bug 133170 should be applicable here - I created a new
function on nsIScriptSecurityManager called CheckSameOrigin which we should use
in XSLT in addition to CheckLoadURI. It's not checked into the runk yet, but
So we're waiting for bug 133170 to land on the trunk? Is there anything else I
can do? I could probably fix this (and bug 138672) but since I don't have the
CheckSameOrigin takes a JSContext too. This is not getting called from JS.
peterv is kindly trying to fix this today.
Created attachment 84706 [details] [diff] [review]
This should fix it. However, the fix to catch a redirect doesn't seem to work,
+ rv = securityManager->IsCapabilityEnabled("UniversalBrowserRead",
mCrossSiteAccessEnabled is always true (even in a fresh profile).
For people behind the firewall:
http://green.nscp.aoltw.net/peterv/exploit/remote.xml tests reading an xml file
from a different host.
http://green.nscp.aoltw.net/peterv/exploit/redir.xml tests reading an xml file
from a different host through a redirect.
Created attachment 84707 [details]
remote.xsl (links to http://www.mozilla.org/xmlextras/data.xml)
Created attachment 84708 [details]
remote.xml (links to remote.xsl)
If you click the remote.xml testcase and you see "One Two Three" as the second
line there's a security hole. Note that this testcase uses the XSLT document
function, however, xsl:include, xsl:import and the XSLT document() function all
use the exact same code to load external documents so the other two methods
(xsl:include and xsl:import) should be fixed too.
Created attachment 84710 [details] [diff] [review]
Addressed a problem with redirect.
*** Bug 138672 has been marked as a duplicate of this bug. ***
Comment on attachment 84710 [details] [diff] [review]
I don't think we need these includes.
>+ nsIDocument *aLoader,
>+ nsIDOMDocument **aResult)
>+ *aResult = nsnull;
I would like to see NS_ENSURE_ARG_POINTER(aResult) before that.
>+ PRBool mLoading;
>+ PRBool mLoadSuccess;
I would prefer seeing two PRPackedBool's here to save a little space,
but no biggie.
Once I have a build with these changes I'll do some testing, and
might attach a patch that addresses these nits, but r=heikki anyway.
Created attachment 84725 [details] [diff] [review]
So the whole context idea didn't fly at all, we were never able to reach a
context from a document, so we had to add a new call in the caps code that can
do a same origin check on two URI's w/o a context.
Comment on attachment 84725 [details] [diff] [review]
rpotts says sr=rpotts, and mstoltz says r=mstoltz
Comment on attachment 84725 [details] [diff] [review]
firstname.lastname@example.org, let's get this in NOW. Thanks to peterv, jst, and the
This file on green has xml-stylesheet PI pointing to grey (another host), and it
should still be able to load that (initial) stylesheet. As the rest of the
testcases show (earlier), the remote stylesheet can not load other remote
resources, only stuff from the original host where the actual XML file is. This
is useful and desired, same as with CSS (you can reuse stylesheets from W3C for
Fixed on the branch, but not yet on the trunk (should go together with bug
133170 on the trunk).
>>+ nsIDocument *aLoader,
>>+ nsIDOMDocument **aResult)
>>+ *aResult = nsnull;
>I would like to see NS_ENSURE_ARG_POINTER(aResult) before that.
I don't see the point. JS can't pass a null out param pointer, and any C++
luser who does deserves to get a crash, which in a debug build is better than an
Adding CC from the duplicates.
Fix checked in on the trunk.
Verified on 2002-06-27-trunk and 2002-06-27-branch build on Win2K.
The above test case in #7 and #27 work fine.