47905 - XMLHttpRequest object security vulnerability

Status: VERIFIED FIXED

(Core :: Security, defect, P1)

Description

[Mitchell Stoltz (not reading bugmail)]

Date: Mon, 07 Aug 2000 14:51:42 +0300
   From: Georgi Guninski <[e-mail]>
      To:Mitchell Stoltz <[e-mail]>

The XMLHttpRequest object is not subject to same origin security policy
and allows exploits.

The code is:
--------------------------------------
<SCRIPT>
var h = new XMLHttpRequest();
h.open("GET", "http://joro/joro/mozilla/test/xml.xml", false);
// The XML file must be a valid XML file
h.send(document);
d = h.responseXML;
s = new XMLSerializer()
alert("The xml file is: "+s.serializeToString(d));
</SCRIPT>
--------------------------------------

Comment 1

[Mitchell Stoltz (not reading bugmail)]

nsbeta3 for exploit. 

Comment 2

[Patrick C. Beard]

escalating.

Comment 3

[Mitchell Stoltz (not reading bugmail)]

Future'd because this functionality is not present in the commercial build.
However, Mozilla builds are currently vulnerable; we need to be doing Same
Origin checks on this functionality. Reassigning to Vidur as he is the author of
this code.

Comment 4

[John Unruh]

QA > [e-mail]

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Taking, I am now the owner of most XMLExtras bugs.

Comment 6

[lchiang]

We may need to reconsider this bug due to bugscape:
http://bugscape.netscape.com/show_bug.cgi?id=3734 to have this feature in 
Netscape Commercial builds. 

Comment 7

[Mitchell Stoltz (not reading bugmail)]

We need to add Security Manager hooks to this code (to do a same-origin check
before loading XML content). Heikki, I will work with you on this.

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Mitch is working on this, reassigning. I can review, vidur should probably sr.
This seems to be blocking the Bugscape bug so the sooner the better...

Comment 9

[Mitchell Stoltz (not reading bugmail)]

An ideal fix for this will depend on the new XPConnect security mechanism, and
that's dependent on Jband's forthcoming changes. In the meantime, I'm
implementing a temporary solution; I will have it checked in this week.

Comment 10

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch for review

Comment 11

[Mitchell Stoltz (not reading bugmail)]

This fix is TEMPORARY. Once the DOM moves to XPConnect, security for
XMLHttpRequest will be subsumed by the general DOM security manager.

Comment 12

[Brendan Eich [:brendan]]

Looks ok to me (whitespace nits picked in private email, take 'em or leave 'em),
r=[e-mail].

/be

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

This part doesn't look right to me; you remove braces but you still have two
lines that should be scoped by that if:

 if (!principal ||
         NS_SUCCEEDED(principal->Equals(mSystemPrincipal, &equals)) && equals)
-    {
         // We have native code or the system principal: just allow access
         return NS_OK;
-    }

Here you can avoid operator= if you just use the constructor (hmm, maybe you
don't need the explicit QI at all, it might be automatic):

-nsCOMPtr<nsICodebasePrincipal> codebase = do_QueryInterface(principal);
+nsCOMPtr<nsICodebasePrincipal> codebase(do_QueryInterface(principal));

I have a small concern that the fix would break things if XMLHttpRequest was
used without a script environment, in a command line tool, but I didn't have
time to look at that yet.

Mitch, could you check if the tests in xmlextras/tests work? There is also a
test C++ program you could try. I think there was something that didn't work in
it, but give it whirl. Once I get back from a meeting I'll take another look.

Comment 14

[Mitchell Stoltz (not reading bugmail)]

But there aren't two lines scoped by that if - one is a comment. I'm just trying
to be consistent; elsewhere in the file I don't use braces if there's only one
statement in the if.

Looks like this function already assumes that a script context exists; I didn't
add that. Anyway, if there's no script context or there's no principal in the
contaxt, caps assumes it's being called by native code. I don't know how to test
this, but I think we're OK.

Comment 15

[Mitchell Stoltz (not reading bugmail)]

Your post.html test is broken by this patch, as I would expect - it's trying a
cross-origin load. We can add an enablePrivilege call to the test to make it
work locally; it won't work over the web unless you enable codebase principals.
This is probably OK for testing.

Do we need a security check on the SOAPCall object, too?

Comment 16

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Well, even if one is a comment it looks really weird to me - I didn't even know
this would work!

I checked the command line testprogram in xmlextras/tests and it seems to have
been bitrotted so that it does no longer work.

If you open xmlextras/tests/syncpost.html or post.html in the browser, does the
new security check prevent them from working? They are supposed to send data to
http://blueviper and get the response (echo of what was sent) back, and display
the results in alert boxes. What if you move the html files to blueviper?

Comment 17

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch to fix xmlextras test

Comment 18

[Mitchell Stoltz (not reading bugmail)]

If both the html and the xml are on blueviper the test will work; or if you
apply my patch to the test and run it from file:, that will work too. Do I have
your r=?

Comment 19

[Heikki Toivonen (remove -bugzilla when emailing directly)]

r=heikki

Comment 20

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Hmm, nobody has been interested in the SOAP stuff so far, that I know of. It
should not go into the installer for nightly builds at this point so I don't
think that it is urgent at this point (it is a separate DLL & typelib from
xmlextras). vidur is still the owner of the SOAP stuff so you might want to chat
with him - he might have some plans for SOAP in the web applications group. 

Comment 21

[Mitchell Stoltz (not reading bugmail)]

Temporary fix checked in. Leaving open to remind myself to set a policy for this
call when the new XPConnect security manager becomes active.

Comment 22

[Mitchell Stoltz (not reading bugmail)]

Mass changing milestones to Moz0.9.1. Many of these bugs are dependent on the
XPConnected DOM and its associated security UI changes.

Comment 23

[Mitchell Stoltz (not reading bugmail)]

Attachment: New Patch

Comment 24

[Johnny Stenback (:jst)]

sr=jst

Comment 25

[vidur (gone)]

r=vidur. Mitch is planning on adding a comment to the string funkiness in
nsScriptSecurityManager::Init. The exception related code in
nsXMLHttpRequest::Open will be removed when it's dealt with in
nsIScriptSecurityManager::CheckConnect.

Comment 26

[Christopher Blizzard (:blizzard)]

FYI I backed out the fix for this.

Comment 27

[Ben Bucksch (:BenB)]

> Future'd because this functionality is not present in the commercial build.
> However, Mozilla builds are currently vulnerable

Why the difference? Does Mozilla incude xmlextras by default and Netscape builds
don't, or what? In other words, to protect, can I just delete the xmlextras
component?

Comment 28

[Mitchell Stoltz (not reading bugmail)]

Fix is in. XMLHttpRequest.open() should only be allowed to open connections to
the script's origin.

Comment 29

[ckritzer (gone)]

Annnnnnd does anybody have a testcase?  :-) Should the bit at the beginning of 
the bug be put into a 'standard' xml file?  If yes, should the valid XML file in 
line 3 (mentioned in line 4) of the given example exist on a different or same 
server as the testcase?  Any info would be greatly appreciated!

Comment 30

[Mitchell Stoltz (not reading bugmail)]

Try the URL above for a testcase. This attempts to load and read an XML file
from a different host. It should throw an exception.
 Try http://warp/u/mstoltz/bugs/47905a.html as a counterexample - it does a load
from the same host and should successfully display some data from the target XML
document.

Comment 31

[ckritzer (gone)]

Yep.  That works dandy.
Marking VERIFIED FIXED on:
-MacOS91 2001-05-21-15-trunk MOZILLA
-Win98SE 2001-05-22-06-trunk
-LinRH62 2001-05-22-05-trunk