Closed Bug 147754 Opened 22 years ago Closed 22 years ago

XMLSerializer needs same-origin check

Categories

(Core :: Security, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: security-bugs, Assigned: security-bugs)

References

()

Details

(Whiteboard: [adt1 rtm])

Attachments

(1 file)

From Neal Deakin: Hello, The XMLSerializer object (part of XMLExtras) seems to not to perform a same-origin check. Doesn't seem to let me access local files, but does let me access the content of another domain. This was tested with RC3. <html> <head><title>Test</title></head> <body> <iframe src="http://www.google.com"></iframe> <input type="button" value="Check" onclick="alert(new XMLSerializer().serializeToString(window.frames[0].document));"> </body> </html> Or, see http://www3.sympatico.ca/ndeakin/test/sectest.html where I have uploaded the test case.
So I got word about this security bug in XMLSerializer.serializeToString() and I had a look, looks like it also impacts XMLHttpRequest (XMLHttpRequest.send() can send any document from any origin AFAICT). I have an untested fix that should plug this hole, see attachment.
Using jst's patch I am now getting expections with cross-host access: Error: uncaught exception: [Exception... "Access to property denied" code: "1010" nsresult: "0x805303f2 (NS_ERROR_DOM_PROP_ACCESS_DENIED)" location: "<unknown>"] So that seems good. This check also works with server side redirect from same host to Google (NS internal link): http://green/heikki/xmlserializer.html For regression testing, make sure that you can edit bugzilla attachments as text. Also see http://www.mozilla.org/xmlextras/tests.html and mozilla/content/xml/tests/load/load.html and mozilla/extensions/xmlextras/tests/ (post tests since get tests are covered on mozilla.org). All of these seem to work for me, although on first run I did crash going to mozilla.org testcases (probably unrelated but...)
Comment on attachment 85362 [details] [diff] [review] jst's untested patch Testing edit-attachment-as-comment. >Index: extensions/xmlextras/base/src/nsDOMSerializer.cpp >=================================================================== >RCS file: /cvsroot/mozilla/extensions/xmlextras/base/src/nsDOMSerializer.cpp,v >retrieving revision 1.15.4.1 >diff -u -r1.15.4.1 nsDOMSerializer.cpp >--- extensions/xmlextras/base/src/nsDOMSerializer.cpp 10 Apr 2002 02:44:29 -0000 1.15.4.1 >+++ extensions/xmlextras/base/src/nsDOMSerializer.cpp 29 May 2002 00:28:28 -0000 >@@ -49,6 +49,11 @@ > #include "nsString.h" > #include "nsReadableUtils.h" > >+#include "nsIJSContextStack.h" >+#include "nsIScriptSecurityManager.h" >+#include "nsICodebasePrincipal.h" >+#include "nsIURI.h" >+ > #include "nsLayoutCID.h" // XXX Need range CID > static NS_DEFINE_CID(kRangeCID,NS_RANGE_CID); > >@@ -136,8 +141,64 @@ > > *_retval = nsnull; > >+ // Get JSContext from stack. >+ nsCOMPtr<nsIJSContextStack> stack = >+ do_GetService("@mozilla.org/js/xpc/ContextStack;1"); >+ >+ JSContext *cx = nsnull; >+ nsresult rv = NS_OK; >+ >+ if (stack) { >+ rv = stack->Peek(&cx); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } >+ >+ if (cx) { >+ // We're called from script, make sure the caller and the root are >+ // from the same origin... >+ >+ nsCOMPtr<nsIDOMDocument> owner_doc(do_QueryInterface(root)); >+ >+ if (!owner_doc) { >+ root->GetOwnerDocument(getter_AddRefs(owner_doc)); >+ } >+ >+ nsCOMPtr<nsIDocument> doc(do_QueryInterface(owner_doc)); >+ >+ if (doc) { >+ nsCOMPtr<nsIPrincipal> principal; >+ nsCOMPtr<nsIURI> root_uri; >+ >+ doc->GetPrincipal(getter_AddRefs(principal)); >+ >+ nsCOMPtr<nsICodebasePrincipal> codebase_principal = >+ do_QueryInterface(principal); >+ >+ if (codebase_principal) { >+ codebase_principal->GetURI(getter_AddRefs(root_uri)); >+ } >+ >+ if (root_uri) { >+ nsCOMPtr<nsIScriptSecurityManager> secMan = >+ do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = secMan->CheckSameOrigin(cx, root_uri); >+ >+ if (NS_FAILED(rv)) { >+ // The node that's being serialized comes from a different >+ // origin than the calling script comes from... >+ >+ return rv; >+ } >+ } >+ } >+ } >+ >+ // We're ok security wise... >+ > nsCOMPtr<nsIDocumentEncoder> encoder; >- nsresult rv = SetUpEncoder(root,nsnull,getter_AddRefs(encoder)); >+ rv = SetUpEncoder(root,nsnull,getter_AddRefs(encoder)); > if (NS_FAILED(rv)) > return rv; >
OK, all of Heikki's testcases pass for me as well (I did not see any crashes), and editing attachments in Bugzilla seems to work OK as well (sorry about the spam from the last post). I'm going to try to produce a more descriptive error message when the security check fails.
Comment on attachment 85362 [details] [diff] [review] jst's untested patch r=mstoltz. I'm going to add better error message reporting on the trunk, including i18n of error messages, but that can be checked in separately.
Attachment #85362 - Flags: review+
Comment on attachment 85362 [details] [diff] [review] jst's untested patch sr=jband
Attachment #85362 - Flags: superreview+
I'm taking the liberty to mark this bug nsbeta1+ and mozilla1.0+, so sue me...
Status: NEW → ASSIGNED
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: --- → mozilla1.0
If Johnny can do it so can I. [adt1 rtm]
Whiteboard: [adt1 rtm]
I ran through all the tests that Heikki pointed out and I saw no crash and the exploit does indeed seem fixed, so once we have a= we can check in...
OS: All → Windows 2000
Hardware: All → PC
Target Milestone: mozilla1.0 → ---
Comment on attachment 85362 [details] [diff] [review] jst's untested patch a=dbaron,shaver,scc for 1.0 branch checkin (Please check in to trunk as well. :-)
Attachment #85362 - Flags: approval+
Checked in on the branch.
Hmm, I can't land this on the trunk yet since the CheckSameOrigin() method in nsIScriptSecurityManager doesn't exist on the trunk yet. Over to mstoltz so that he can land this on the trunk once the CheckSameOrigin() method exists on the trunk.
Assignee: jst → mstoltz
Status: ASSIGNED → NEW
Keywords: fixed1.0.0
OS: Windows 2000 → All
Hardware: PC → All
I'll try to get this on the trunk today.
*** Bug 148265 has been marked as a duplicate of this bug. ***
Fix checked in on the trunk.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Group: security?
Verified on 2002-10-11-branch on Win 2000. Attached URL throws an exception.
Status: RESOLVED → VERIFIED
Updating verified keyword so queries of which security bugs were fixed in what release come out right: this was fixed in Mozilla 1.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: