Closed Bug 147754 Opened 22 years ago Closed 22 years ago

XMLSerializer needs same-origin check

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

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

References

(
URL
)

Details

(Whiteboard: [adt1 rtm])

Attachments

(1 file)

jst's untested patch
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.
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...)
URL: http://www3.sympatico.ca/ndeakin/test...
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
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
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: