Closed
Bug 147754
Opened 22 years ago
Closed 22 years ago
XMLSerializer needs same-origin check
Categories
(Core :: Security, defect)
Core
Security
Tracking
()
VERIFIED
FIXED
People
(Reporter: security-bugs, Assigned: security-bugs)
References
()
Details
(Whiteboard: [adt1 rtm])
Attachments
(1 file)
|
2.48 KB,
patch
|
security-bugs
:
review+
jband_mozilla
:
superreview+
dbaron
:
approval+
|
Details | Diff | Splinter Review |
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.
|
Assignee | |
Comment 1•22 years ago
|
||
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...)
|
|
Assignee | |
Comment 3•22 years ago
|
||
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;
>
|
|
Assignee | |
Comment 4•22 years ago
|
||
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.
|
|
Assignee | |
Comment 5•22 years ago
|
||
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 6•22 years ago
|
||
Comment on attachment 85362 [details] [diff] [review]
jst's untested patch
sr=jband
Attachment #85362 -
Flags: superreview+
|
||
Comment 7•22 years ago
|
||
I'm taking the liberty to mark this bug nsbeta1+ and mozilla1.0+, so sue me...
Status: NEW → ASSIGNED
Keywords: mozilla1.0+,
nsbeta1+
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: --- → mozilla1.0
|
|
||
Comment 9•22 years ago
|
||
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+
|
|
||
Comment 11•22 years ago
|
||
Checked in on the branch.
|
|
||
Comment 12•22 years ago
|
||
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
|
|
Assignee | |
Comment 13•22 years ago
|
||
I'll try to get this on the trunk today.
*** Bug 148265 has been marked as a duplicate of this bug. ***
|
|
Assignee | |
Comment 15•22 years ago
|
||
Fix checked in on the trunk.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
|
|
Assignee | |
Updated•22 years ago
|
Group: security?
|
||
Comment 16•22 years ago
|
||
Verified on 2002-10-11-branch on Win 2000.
Attached URL throws an exception.
Status: RESOLVED → VERIFIED
Keywords: fixed1.0.0 → verified1.0.2
|
||
Comment 17•22 years ago
|
||
Updating verified keyword so queries of which security bugs were fixed in what
release come out right: this was fixed in Mozilla 1.0
Keywords: verified1.0.2 → verified1.0.0
You need to log in
before you can comment on or make changes to this bug.
Description
•