113351 - XSL Include needs same-origin check

Status: VERIFIED FIXED

(Core :: XSLT, defect, P1)

Description

[Mitchell Stoltz (not reading bugmail)]

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
edit .xsl.
xsl:import acts like #include in C and then the whole document may be accessed
by script.
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.

Comment 1

[Mitchell Stoltz (not reading bugmail)]

Attachment: testcase - xmlempl.html

Comment 2

[Mitchell Stoltz (not reading bugmail)]

Attachment: xmlempl.xsl

Comment 3

[Mitchell Stoltz (not reading bugmail)]

Attachment: test.xml

Comment 4

[Mitchell Stoltz (not reading bugmail)]

Attachment: secret.xml

Comment 5

[Peter Van der Beken [:peterv]]

Haven't tried the example yet, but I did put in code to try and prevent this.
Maybe it's not right? Look at
http://lxr.mozilla.org/seamonkey/source/extensions/transformiix/source/xml/parser/nsSyncLoader.cpp#182
It should check if the load is allowed from the main stylesheet.

Comment 6

[Mitchell Stoltz (not reading bugmail)]

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.

Comment 7

[Peter Van der Beken [:peterv]]

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
http://lxr.mozilla.org/seamonkey/source/caps/src/nsScriptSecurityManager.cpp#883.
Please advise if I should change the CheckLoadURI check into something else.

Comment 8

[Mitchell Stoltz (not reading bugmail)]

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.

Comment 9

[Peter Van der Beken [:peterv]]

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.
Note that this code is *not* being called from JavaScript.

Comment 10

[Mitchell Stoltz (not reading bugmail)]

Whoops, I forgot that CheckConnect assumes that it's being called from
Javascript.  I think I need to write a new check function for this. I'll take
this bug, and try to come up with a new security check.

Comment 11

[Mitchell Stoltz (not reading bugmail)]

Critical Moz1.0 fixes

Comment 12

[Mitchell Stoltz (not reading bugmail)]

Mozilla1.1

Comment 13

[Mitchell Stoltz (not reading bugmail)]

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
that's coming.

Comment 14

[Peter Van der Beken [:peterv]]

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
right function...

Comment 15

[Peter Van der Beken [:peterv]]

CheckSameOrigin takes a JSContext too. This is not getting called from JS.

Comment 16

[Brendan Eich [:brendan]]

peterv is kindly trying to fix this today.

/be

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: v1

This should fix it. However, the fix to catch a redirect doesn't seem to work,
after:

+    rv = securityManager->IsCapabilityEnabled("UniversalBrowserRead",
+					       &mCrossSiteAccessEnabled);

mCrossSiteAccessEnabled is always true (even in a fresh profile).

Comment 18

[Peter Van der Beken [:peterv]]

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.

Comment 19

[Peter Van der Beken [:peterv]]

Attachment: remote.xsl (links to http://www.mozilla.org/xmlextras/data.xml)

Comment 20

[Peter Van der Beken [:peterv]]

Attachment: 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.

Comment 21

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Addressed a problem with redirect.

Comment 22

[Peter Van der Beken [:peterv]]

*** Bug 138672 has been marked as a duplicate of this bug. ***

Comment 23

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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

>Index: source/xml/parser/Makefile.in
>===================================================================
>Index: source/xml/parser/makefile.win
>===================================================================
>Index: source/xml/parser/nsSyncLoader.cpp
>===================================================================
>+#include "nsIAuthPrompt.h"
>+#include "nsIWindowWatcher.h"

I don't think we need these includes.

>+nsSyncLoader::LoadDocument(nsIURI* aDocumentURI,
>+                           nsIDocument *aLoader,
>+                           nsIDOMDocument **aResult)
> {
>+    *aResult = nsnull;

I would like to see NS_ENSURE_ARG_POINTER(aResult) before that.

>Index: source/xml/parser/nsSyncLoader.h
>===================================================================
>+    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.

Comment 24

[Johnny Stenback (:jst)]

Attachment: v2.0

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 25

[Johnny Stenback (:jst)]

Comment on attachment 84725 [details] [diff] [review]
v2.0

rpotts says sr=rpotts, and mstoltz says r=mstoltz

Comment 26

[Brendan Eich [:brendan]]

Comment on attachment 84725 [details] [diff] [review]
v2.0

a=[e-mail], let's get this in NOW.  Thanks to peterv, jst, and the
reviewers.

/be

Comment 27

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Regression test:

http://green/heikki/remote.xml

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
example).

Comment 28

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Fixed on the branch, but not yet on the trunk (should go together with bug
133170 on the trunk).

Comment 29

[Brendan Eich [:brendan]]

heikki wrote:

>>+nsSyncLoader::LoadDocument(nsIURI* aDocumentURI,
>>+                           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
assert-botch, debugability-wise.

/be

Comment 30

[Peter Van der Beken [:peterv]]

Adding CC from the duplicates.

Comment 31

[Mitchell Stoltz (not reading bugmail)]

Fix checked in on the trunk.

Comment 32

[bsharma]

Verified on 2002-06-27-trunk and 2002-06-27-branch build on Win2K.

The above test case in #7 and #27 work fine.