Last Comment Bug 113351 - XSL Include needs same-origin check
: XSL Include needs same-origin check
Status: VERIFIED FIXED
[ADT2 RTM]
:
Product: Core
Classification: Components
Component: XSLT (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.0
Assigned To: Peter Van der Beken [:peterv]
: Keith Visco
:
Mentors:
: 138672 (view as bug list)
Depends on:
Blocks: 143200
  Show dependency treegraph
 
Reported: 2001-12-03 16:12 PST by Mitchell Stoltz (not reading bugmail)
Modified: 2002-12-23 11:00 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase - xmlempl.html (246 bytes, text/html)
2001-12-03 16:13 PST, Mitchell Stoltz (not reading bugmail)
no flags Details
xmlempl.xsl (220 bytes, text/xml)
2001-12-03 16:14 PST, Mitchell Stoltz (not reading bugmail)
no flags Details
test.xml (172 bytes, text/xml)
2001-12-03 16:15 PST, Mitchell Stoltz (not reading bugmail)
no flags Details
secret.xml (79 bytes, text/xml)
2001-12-03 16:15 PST, Mitchell Stoltz (not reading bugmail)
no flags Details
v1 (10.50 KB, patch)
2002-05-22 17:55 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
remote.xsl (links to http://www.mozilla.org/xmlextras/data.xml) (461 bytes, text/xml)
2002-05-22 18:09 PDT, Peter Van der Beken [:peterv]
no flags Details
remote.xml (links to remote.xsl) (235 bytes, text/xml)
2002-05-22 18:12 PDT, Peter Van der Beken [:peterv]
no flags Details
v1.1 (8.44 KB, patch)
2002-05-22 18:40 PDT, Peter Van der Beken [:peterv]
hjtoi-bugzilla: review+
Details | Diff | Splinter Review
v2.0 (9.79 KB, patch)
2002-05-23 00:06 PDT, Johnny Stenback (:jst, jst@mozilla.com)
jst: review+
jst: superreview+
brendan: approval+
Details | Diff | Splinter Review

Description Mitchell Stoltz (not reading bugmail) 2001-12-03 16:12:24 PST
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) 2001-12-03 16:13:58 PST
Created attachment 60241 [details]
testcase - xmlempl.html
Comment 2 Mitchell Stoltz (not reading bugmail) 2001-12-03 16:14:38 PST
Created attachment 60242 [details]
xmlempl.xsl
Comment 3 Mitchell Stoltz (not reading bugmail) 2001-12-03 16:15:04 PST
Created attachment 60243 [details]
test.xml
Comment 4 Mitchell Stoltz (not reading bugmail) 2001-12-03 16:15:24 PST
Created attachment 60244 [details]
secret.xml
Comment 5 Peter Van der Beken [:peterv] 2001-12-03 23:36:19 PST
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) 2001-12-24 19:50:53 PST
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] 2001-12-28 06:01:12 PST
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) 2002-01-17 10:48:03 PST
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] 2002-02-06 16:31:10 PST
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) 2002-02-07 13:50:16 PST
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) 2002-03-08 16:38:56 PST
Critical Moz1.0 fixes
Comment 12 Mitchell Stoltz (not reading bugmail) 2002-03-08 16:43:39 PST
Mozilla1.1
Comment 13 Mitchell Stoltz (not reading bugmail) 2002-05-21 18:05:25 PDT
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] 2002-05-22 05:42:22 PDT
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] 2002-05-22 05:58:01 PDT
CheckSameOrigin takes a JSContext too. This is not getting called from JS.
Comment 16 Brendan Eich [:brendan] 2002-05-22 13:40:03 PDT
peterv is kindly trying to fix this today.

/be
Comment 17 Peter Van der Beken [:peterv] 2002-05-22 17:55:50 PDT
Created attachment 84706 [details] [diff] [review]
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] 2002-05-22 17:57:57 PDT
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] 2002-05-22 18:09:03 PDT
Created attachment 84707 [details]
remote.xsl (links to http://www.mozilla.org/xmlextras/data.xml)
Comment 20 Peter Van der Beken [:peterv] 2002-05-22 18:12:52 PDT
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.
Comment 21 Peter Van der Beken [:peterv] 2002-05-22 18:40:43 PDT
Created attachment 84710 [details] [diff] [review]
v1.1

Addressed a problem with redirect.
Comment 22 Peter Van der Beken [:peterv] 2002-05-22 19:03:35 PDT
*** Bug 138672 has been marked as a duplicate of this bug. ***
Comment 23 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-05-22 19:52:41 PDT
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, jst@mozilla.com) 2002-05-23 00:06:16 PDT
Created attachment 84725 [details] [diff] [review]
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, jst@mozilla.com) 2002-05-23 00:19:52 PDT
Comment on attachment 84725 [details] [diff] [review]
v2.0

rpotts says sr=rpotts, and mstoltz says r=mstoltz
Comment 26 Brendan Eich [:brendan] 2002-05-23 00:21:35 PDT
Comment on attachment 84725 [details] [diff] [review]
v2.0

a=brendan@mozilla.org, let's get this in NOW.  Thanks to peterv, jst, and the
reviewers.

/be
Comment 27 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-05-23 00:32:33 PDT
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) 2002-05-23 00:48:45 PDT
Fixed on the branch, but not yet on the trunk (should go together with bug
133170 on the trunk).
Comment 29 Brendan Eich [:brendan] 2002-05-23 00:53:52 PDT
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] 2002-05-28 13:48:08 PDT
Adding CC from the duplicates.
Comment 31 Mitchell Stoltz (not reading bugmail) 2002-06-14 19:33:06 PDT
Fix checked in on the trunk.
Comment 32 bsharma 2002-06-27 14:51:49 PDT
Verified on 2002-06-27-trunk and 2002-06-27-branch build on Win2K.

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

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