Last Comment Bug 147754 - XMLSerializer needs same-origin check
: XMLSerializer needs same-origin check
Status: VERIFIED FIXED
[adt1 rtm]
:
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Mitchell Stoltz (not reading bugmail)
: bsharma
: David Keeler [:keeler] (use needinfo?)
Mentors:
http://www3.sympatico.ca/ndeakin/test...
: 148265 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-05-28 17:56 PDT by Mitchell Stoltz (not reading bugmail)
Modified: 2002-11-05 23:12 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
jst's untested patch (2.48 KB, patch)
2002-05-28 17:58 PDT, Mitchell Stoltz (not reading bugmail)
security-bugs: review+
jband_mozilla: superreview+
dbaron: approval+
Details | Diff | Splinter Review

Description Mitchell Stoltz (not reading bugmail) 2002-05-28 17:56:20 PDT
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.
Comment 1 Mitchell Stoltz (not reading bugmail) 2002-05-28 17:58:05 PDT
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.
Comment 2 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-05-28 18:09:03 PDT
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 3 Mitchell Stoltz (not reading bugmail) 2002-05-28 18:36:20 PDT
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;
>
Comment 4 Mitchell Stoltz (not reading bugmail) 2002-05-28 18:39:54 PDT
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 5 Mitchell Stoltz (not reading bugmail) 2002-05-28 18:43:02 PDT
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.
Comment 6 John Bandhauer 2002-05-28 19:37:03 PDT
Comment on attachment 85362 [details] [diff] [review]
jst's untested patch

sr=jband
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2002-05-28 19:43:54 PDT
I'm taking the liberty to mark this bug nsbeta1+ and mozilla1.0+, so sue me...
Comment 8 Daniel Veditz [:dveditz] 2002-05-28 19:46:26 PDT
If Johnny can do it so can I. [adt1 rtm]
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2002-05-28 19:57:10 PDT
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...
Comment 10 David Baron :dbaron: ⌚️UTC-10 2002-05-28 20:05:31 PDT
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. :-)
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2002-05-28 21:00:39 PDT
Checked in on the branch.
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2002-05-28 21:03:47 PDT
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.
Comment 13 Mitchell Stoltz (not reading bugmail) 2002-05-29 10:56:34 PDT
I'll try to get this on the trunk today.
Comment 14 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-05-30 17:51:22 PDT
*** Bug 148265 has been marked as a duplicate of this bug. ***
Comment 15 Mitchell Stoltz (not reading bugmail) 2002-06-14 19:33:30 PDT
Fix checked in on the trunk.
Comment 16 bsharma 2002-10-14 11:46:06 PDT
Verified on 2002-10-11-branch on Win 2000.

Attached URL throws an exception.
Comment 17 Daniel Veditz [:dveditz] 2002-11-05 23:12:34 PST
Updating verified keyword so queries of which security bugs were fixed in what
release come out right: this was fixed in Mozilla 1.0

Note You need to log in before you can comment on or make changes to this bug.