Closed
Bug 113351
Opened 23 years ago
Closed 22 years ago
XSL Include needs same-origin check
Categories
(Core :: XSLT, defect, P1)
Core
XSLT
Tracking
()
VERIFIED
FIXED
mozilla1.0
People
(Reporter: security-bugs, Assigned: peterv)
References
Details
(Whiteboard: [ADT2 RTM])
Attachments
(8 files, 1 obsolete file)
246 bytes,
text/html
|
Details | |
220 bytes,
text/xml
|
Details | |
172 bytes,
text/xml
|
Details | |
79 bytes,
text/xml
|
Details | |
461 bytes,
text/xml
|
Details | |
235 bytes,
text/xml
|
Details | |
8.44 KB,
patch
|
hjtoi-bugzilla
:
review+
|
Details | Diff | Splinter Review |
9.79 KB,
patch
|
jst
:
review+
jst
:
superreview+
brendan
:
approval+
|
Details | Diff | Splinter Review |
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•23 years ago
|
||
Reporter | ||
Comment 2•23 years ago
|
||
Reporter | ||
Comment 3•23 years ago
|
||
Reporter | ||
Comment 4•23 years ago
|
||
Assignee | ||
Comment 5•23 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•23 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•23 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•23 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•23 years ago
|
Priority: -- → P1
Target Milestone: --- → mozilla0.9.9
Assignee | ||
Comment 9•23 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•23 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•22 years ago
|
||
Critical Moz1.0 fixes
Reporter | ||
Updated•22 years ago
|
Target Milestone: mozilla1.1alpha → mozilla1.0
Reporter | ||
Updated•22 years ago
|
Target Milestone: mozilla1.0 → mozilla1.1alpha
Updated•22 years ago
|
Whiteboard: [ADT2 RTM]
Reporter | ||
Comment 13•22 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•22 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•22 years ago
|
||
CheckSameOrigin takes a JSContext too. This is not getting called from JS.
Comment 16•22 years ago
|
||
peterv is kindly trying to fix this today. /be
Assignee: mstoltz → peterv
Status: ASSIGNED → NEW
Target Milestone: mozilla1.1alpha → mozilla1.0
Assignee | ||
Comment 17•22 years ago
|
||
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•22 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•22 years ago
|
||
Assignee | ||
Comment 20•22 years ago
|
||
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•22 years ago
|
||
Addressed a problem with redirect.
Attachment #84706 -
Attachment is obsolete: true
Assignee | ||
Comment 22•22 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+
Comment 24•22 years ago
|
||
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•22 years ago
|
||
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 26•22 years ago
|
||
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
Comment 29•22 years ago
|
||
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•22 years ago
|
||
Adding CC from the duplicates.
Reporter | ||
Comment 31•22 years ago
|
||
Fix checked in on the trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 32•22 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•22 years ago
|
Group: security?
Reporter | ||
Updated•22 years ago
|
Group: security?
Reporter | ||
Updated•22 years ago
|
Group: security
You need to log in
before you can comment on or make changes to this bug.
Description
•