The default bug view has changed. See this FAQ.

XMLSerializer needs same-origin check

VERIFIED FIXED

Status

()

Core
Security
VERIFIED FIXED
15 years ago
15 years ago

People

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

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt1 rtm], URL)

Attachments

(1 attachment)

2.48 KB, patch
Mitchell Stoltz (not reading bugmail)
: review+
John Bandhauer
: 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

15 years ago
Created attachment 85362 [details] [diff] [review]
jst's untested patch

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

15 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

15 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

15 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

15 years ago
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
Keywords: mozilla1.0+, nsbeta1+
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
(Assignee)

Comment 13

15 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

15 years ago
Fix checked in on the trunk.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Updated

15 years ago
Group: security?

Comment 16

15 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
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.