XSL Include needs same-origin check

VERIFIED FIXED in mozilla1.0

Status

()

Core
XSLT
P1
normal
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: Mitchell Stoltz (not reading bugmail), Assigned: peterv)

Tracking

Trunk
mozilla1.0
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ADT2 RTM])

Attachments

(8 attachments, 1 obsolete attachment)

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.
(Reporter)

Comment 1

16 years ago
Created attachment 60241 [details]
testcase - xmlempl.html
(Reporter)

Comment 2

16 years ago
Created attachment 60242 [details]
xmlempl.xsl
(Reporter)

Comment 3

16 years ago
Created attachment 60243 [details]
test.xml
(Reporter)

Comment 4

16 years ago
Created attachment 60244 [details]
secret.xml
(Assignee)

Comment 5

16 years ago
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.
(Reporter)

Comment 6

16 years ago
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.
Assignee: mstoltz → peterv
(Assignee)

Comment 7

16 years ago
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.
(Reporter)

Comment 8

16 years ago
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.
(Assignee)

Updated

16 years ago
Priority: -- → P1
Target Milestone: --- → mozilla0.9.9
Keywords: nsbeta1+
(Assignee)

Comment 9

16 years ago
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.
Status: NEW → ASSIGNED
(Reporter)

Comment 10

16 years ago
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.
Assignee: peterv → mstoltz
Status: ASSIGNED → NEW
(Reporter)

Comment 11

16 years ago
Critical Moz1.0 fixes
Status: NEW → ASSIGNED
Keywords: nsbeta1
Target Milestone: mozilla0.9.9 → mozilla1.0
(Reporter)

Comment 12

16 years ago
Mozilla1.1
Target Milestone: mozilla1.0 → mozilla1.1
Keywords: nsbeta1
(Reporter)

Updated

16 years ago
Target Milestone: mozilla1.1alpha → mozilla1.0
(Reporter)

Updated

15 years ago
Target Milestone: mozilla1.0 → mozilla1.1alpha
Whiteboard: [ADT2 RTM]
(Reporter)

Comment 13

15 years ago
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.
(Assignee)

Comment 14

15 years ago
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...
(Assignee)

Comment 15

15 years ago
CheckSameOrigin takes a JSContext too. This is not getting called from JS.
peterv is kindly trying to fix this today.

/be
Assignee: mstoltz → peterv
Status: ASSIGNED → NEW
Target Milestone: mozilla1.1alpha → mozilla1.0
(Assignee)

Comment 17

15 years ago
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).
(Assignee)

Comment 18

15 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 19

15 years ago
Created attachment 84707 [details]
remote.xsl (links to http://www.mozilla.org/xmlextras/data.xml)
(Assignee)

Comment 20

15 years ago
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.
(Assignee)

Comment 21

15 years ago
Created attachment 84710 [details] [diff] [review]
v1.1

Addressed a problem with redirect.
Attachment #84706 - Attachment is obsolete: true
(Assignee)

Comment 22

15 years ago
*** Bug 138672 has been marked as a duplicate of this bug. ***
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.
Attachment #84710 - Flags: review+

Updated

15 years ago
Blocks: 143200
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 on attachment 84725 [details] [diff] [review]
v2.0

rpotts says sr=rpotts, and mstoltz says r=mstoltz
Attachment #84725 - Flags: superreview+
Attachment #84725 - Flags: review+
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
Attachment #84725 - Flags: approval+
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).
Fixed on the branch, but not yet on the trunk (should go together with bug
133170 on the trunk).
Keywords: fixed1.0.0
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
(Assignee)

Comment 30

15 years ago
Adding CC from the duplicates.
(Reporter)

Comment 31

15 years ago
Fix checked in on the trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 32

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

The above test case in #7 and #27 work fine.
Status: RESOLVED → VERIFIED
Keywords: fixed1.0.0 → verified1.0.1
(Reporter)

Updated

15 years ago
Group: security?
(Reporter)

Updated

15 years ago
Group: security?
(Reporter)

Updated

15 years ago
Group: security
You need to log in before you can comment on or make changes to this bug.