403168 - XSS by using XMLHttpRequest and event handler

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[moz_bug_r_a4]

1. Create an XMLHttpRequest (R) with a subframe's context, and attach an event
   handler to R.
2. Load a target site in the subframe.
3. Call R.send().

The event handler is called with the target site's principal.

Comment 2

[Johnny Stenback (:jst)]

Smaug, this looks a lot like bug 403167.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Also here it would be great to have a testcase which shows that the mainpage can
actually steal something from pages loaded from different domain.
So not just javascript:alert.

Comment 4

[Johnny Stenback (:jst)]

If a page can alert something it means it can read the sensitive data, after that there's endless ways to dial home and steal the data etc. The fact that the data is readable is the problem here.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

This doesn't really block bug 386823, but I'm going to upload a patch which
could be simplified after bug 386823.
The coming patch is however something which could be used on 1.8, I think.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch, v1-simple

This patch fixes the bug, but is still not quite right. But perhaps enough for 1.8?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 288237 [details] [diff] [review]
possible patch, v1-simple

Actually, because mScriptContext is updated for example when adding event listener, this makes behavior more consistent - not perfect but consistent.

Then for 1.9 bounding XHR to a DOMWindow and that way cleaning up mPrincipal etc... But first this patch to 1.9 too, IMO.

Comment 8

[Daniel Veditz [:dveditz]]

I think these need to wait for 2.0.0.11 and more testing of these patches to flush out further problems (see bug 403167 comment 10)

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

This is actually quite different problem comparing to bug 403167.

Comment 10

[moz_bug_r_a4]

The proposed fix can be circumvented, since it's possible to call send() in the
subframe's context.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 288237 [details] [diff] [review]
possible patch, v1-simple

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: first patch + mInnerWindow

The size of the patch is bigger than in the first patch, but in principle this
is the first one + inner window check in NotifyEventListeners.
Code wise this is perhaps not perfect, but for 1.8...
The second testcase shows sort-of-similar problem what we have with data 
documents. When window is teared down, the main document's scriptglobalobject 
is cleared, but scriptobject in datadocuments or in this case in XHR points 
still to somewhere.
[e-mail] can you break this one ;)

Comment 14

[moz_bug_r_a4]

The second patch can be circumvented by calling send() on the subframe's
context after loading a target site in the subframe.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

I have a patch which fixes all the 3 testcases, but I've found still one problem.
It is a bit hard to reproduce. Using testcase 2, let it finish.
Click forward, then twice backward (you should be on bugzilla page), then
click fast forward, and doubleclick forward. What you may get is an alert box, 
which says that location.href is about:blank, but cookie is the cookie from bugzilla page.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

But making checks even more strict, that problem goes away.
Would be great to find a testcase though.
I'll clean up the patch before uploading.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

No, the 4th bug is still there...just very hard to reproduce.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Hmm, that cookie information is ofc coming from the current domain, so
that is not XSS. It is just a bug that
location.href is about:blank and cookie is not about that.
And cookie is get based on principal...

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bind XHR to a window

This takes part of the bug 372964 to bind XHR to DOMWindow when |new XMLHttpRequest| is used. That way testcase 3 can be fixed.
mScriptContext (and mPrincipal) is set only when initializing, not every time 
when an event listener is set etc.

When using the testcases CheckInnerWindowCorrectness() returns always NS_OK,
but I'd like to keep it there to ensure that inner-outer -window are really
still properly connected to each other.

This doesn't fix Bug 386823, but that should be pretty easy to fix after this.

Jonas, what do you think about the approach? The biggest change is to use
script context of the bound window, not script context from stack.
That is how it should be according to current XHR draft, AFAIK.

[e-mail], still able to break this?

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why are the calls to CheckInnerWindowCorrectness needed?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Currently they aren't, but I really want to make sure that if there is some
problem elsewhere or XHR's script handling has some bug, we at least don't
execute event handlers in that case. Patch 2 actually did need that kind of check.
(One question is ofc, that whether the need for that kind of check means a bug
somewhere in inner-outer window handling, or not maybe bug, but wrong kind of 
assumption.)

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Note, when a normal document is going away (unloaded/bfcached) its scriptglobalobject is cleared. But documentviewer, nor globalobject knows nothing 
about XHRs.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

But if the inner and outer window don't match up, which could happen if the user has navigated away from the page, shouldn't we simply execute things on the inner window?

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

We don't want to execute anything on windows/documents which are in bfcache.
Inner window has the connection to outer. That is maybe a bad thing. And that is
perhaps a bug we should fix elsewhere, not in this bug.
For the 3rd testcase I couldn't find any other way than to bind XHR to a window, and that also fixed other testcases.
For the inner-outer thing I'd certainly need comments/help from jst, but that is 
anyway, as I said, a different bug, if bug at all (depends on how things are 
designed to work).

All comments welcome here :)

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

First of all, why do we not want to execute scripts on documents in the bfcache? 

Also, this can happen on documents that are held alive by simple references to them from other windows. And I definitely think we want to make them behave as normal as we can.

I agree it behaves weird when the inner window points to a different outer, unfortunately that's not something we can do much about. But we do still execute scripts in such situations.

Comment 27

[Johnny Stenback (:jst)]

Generally speaking executing script in a window in the bfcache is something we try to avoid (we suspend timer etc). If scripts do execute, their "window" property will refer to the current window, which could be from a different origin etc.

There's basically 3 options on the table for dealing with what we've got here:

1) Execut event handlers (bad per above paragraph).
2) Don't execute event handlers at all, drop them on the floor
3) Queue them up and fire them once the page becomes the current one again (i.e. the user hits back/forward).

Option 3 is the one that most likely breaks the least pages that end up in this situation, but it's also hard, if even possible at all to implement. IMO we should try to change as little as possible here in this bug re what event handlers do or don't fire, unless we need to do that to block exploits, and then file a followup bug on queueing XHR events and dispatching them once it's ok to do so. Dave Camp did that for some of the offline stuff recently, but whether it's possible to do that here (depending on what needs to be reachable off of the event objects etc).

Another thing that comes to mind is to walk all XHRs in a window when we stick it in the bfcache and suspend any open requests, that should minimize the number of events that do fire in XHRs that are in the bfcache. Also material for a different bug IMO.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #27)
> 1) Execut event handlers (bad per above paragraph).
That is for example testcase 2 (and the second patch). bad.

> 2) Don't execute event handlers at all, drop them on the floor
That is what the patch does and why I even left 
CheckInnerWindowCorrectness()

> 3) Queue them up and fire them once the page becomes the current one again
This is quite difficult to do properly with XHRs. Those are anyway doing some 
networking. And IMO it is questionable if this is needed. Do pages really expect
that XHR does something still after pagehide/unload

Comment 29

[moz_bug_r_a4]

(In reply to comment #21)
> Why are the calls to CheckInnerWindowCorrectness needed?

(In reply to comment #22)
> Currently they aren't, but I really want to make sure that if there is some
> problem elsewhere or XHR's script handling has some bug, we at least don't
> execute event handlers in that case. Patch 2 actually did need that kind of
> check.
> (One question is ofc, that whether the need for that kind of check means a bug
> somewhere in inner-outer window handling, or not maybe bug, but wrong kind of 
> assumption.)

The third patch actually needs CheckInnerWindowCorrectness.  If it does not
exist, it's possible to break the fix by using frames[0].XMLHttpRequest.  (I've
confirmed it after removing the calls to CheckInnerWindowCorrectness.)

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Could you upload that testcase? Would be easier to confirm this fixed (when this eventually lands).

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Use inner window when creating documents, bind nsDOMParser to window

This is needed to fix testcase 4&5 in Bug 403167, and makes DOMParser and XMLHttpRequest work consistently; bind them to the owning window.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +CheckInnerWindowCorrectness in Open and Send

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Testcase 4 tests CheckInnerWindowCorrectness in Send

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 290733 [details] [diff] [review]
+CheckInnerWindowCorrectness in Open and Send

> NS_IMETHODIMP
> nsXMLHttpRequest::Open(const nsACString& method, const nsACString& url)
> {
>-  nsresult rv = NS_OK;
>+  nsresult rv = CheckInnerWindowCorrectness();
>+  NS_ENSURE_SUCCESS(rv, rv);

No need to check here since Open calls OpenRequest.

>+  // nsIJSNativeInitializer
>+  NS_IMETHOD Initialize(nsISupports* aOwner, JSContext* cx, JSObject* obj,
>+                       PRUint32 argc, jsval* argv);
>+
>+  // This is called by the factory constructor.
>+  nsresult Init();

What factory constructor? I don't see any callers in the current code. I do think we should make XHR work the way that nsDOMParser does and have a scriptable Init() function (living on a non-flattened interface) that non-content script-users can call to initialize with the right principal and context and such. Possibly also a non-scripted function that takes all necessary parameters for use for C++ callers.

Actually, that probably needs to happen in this bug because otherwise we're hosing chrome users since you're removing all those GetCurrentContext() calls.

r=me with that fixed. Would like jst to sr though since I don't know the initializer stuff well enough.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

 > What factory constructor?
NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsXMLHttpRequest, Init)
 
> Actually, that probably needs to happen in this bug because otherwise we're
> hosing chrome users since you're removing all those GetCurrentContext() calls.
What you mean here?

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #36)
>  > What factory constructor?
> NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsXMLHttpRequest, Init)

And that means that if "@mozilla.org/xmlextras/xmlhttprequest;1" is created in JS, principal is set to the current subject principal.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #35)
> Actually, that probably needs to happen in this bug because otherwise we're
> hosing chrome users since you're removing all those GetCurrentContext() calls.
So Init gets the current context.

>Possibly also a non-scripted function that takes all
>necessary parameters for use for C++ callers.
I wouldn't want to start fixing C++ callers here, because this is something that
should probably go to 1.8 at some point

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, the C++ side can happen elsewhere.

Hmm.. I think i'd prefer to have script factory users call Init() explicitly. Otherwise it will be hard to support C++ users in the future as they will always pick up whatever script happen to be running. That would also allow script-users to use such C++ apis if they need to use some other principal. See the DOMParser. 

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 290733 [details] [diff] [review]
+CheckInnerWindowCorrectness in Open and Send

So based on those things, I think sr'ing this should be ok too. I'll remove the extra CheckInnerWindowCorrectness() though.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Oops, didn't notice sicking comment.
Adding init call breaks some extensions I guess. and even ff UI.
For C++ we could still add Init(principal), that would override the script principal.
I've looked at domparser, and actually I don't like that too much

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 290733 [details] [diff] [review]
+CheckInnerWindowCorrectness in Open and Send

clearing sr request until it is clear what we want here.

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, requiring the init() call even from script will break extensions, which is why it would be good to get done for beta2.

The reason I like the explicit init() calls is that it makes it clear what is happening, and it's much less likely that C++ code will do the wrong thing. Explicitness is always a good idea for security sensitive stuff.

I realize we're running short on time for beta2 though as we'd need to fix all internal callers, and possibly also give extensions time to fix their code before we release.

So given that, it seems like a good idea to use the current patch for beta2, and figure out what we want to do for beta3.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

> The reason I like the explicit init() calls is that it makes it clear what is
> happening
Though on the other hand calling init implicitly should give reasonable results
for example for extensions.

> So given that, it seems like a good idea to use the current patch for beta2,
> and figure out what we want to do for beta3.
So will ask sr

Comment 45

[Johnny Stenback (:jst)]

Comment on attachment 290733 [details] [diff] [review]
+CheckInnerWindowCorrectness in Open and Send

nsDOMParser::Initialize():

+    nsCOMPtr<nsIDocument> doc;
+    nsCOMPtr<nsIDOMWindow> window = do_QueryInterface(aOwner);
+    if (aOwner) {
+      nsCOMPtr<nsIDOMDocument> domdoc;
+      window->GetDocument(getter_AddRefs(domdoc));
+      doc = do_QueryInterface(domdoc);
+    }

It might be a good idea to make this use nsPIDOMWindow::GetExtantDocument() instead of GetDocument() as the latter will create one if one doesn't exist. I'm not sure we want that, as it's for sure not something that would be needed in the expected cases. So I'd rather throw if someone tries to create a DOMParser for a window that has no document than create a new one etc...

- In CheckInnerWindowCorrectness():

+  {
+    if (mOwner) {
+      NS_ASSERTION(mOwner->IsInnerWindow(), "Should have inner window here!\n");
+      nsPIDOMWindow* outer = mOwner->GetOuterWindow();
+      if (!outer || outer->GetCurrentInnerWindow() != mOwner) {
+        return NS_ERROR_FAILURE;
+      }
+    }
+    return NS_OK;

Should this return an error if there is no owner?

sr=jst with that.

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #45)
> Should this return an error if there is no owner?
No, because owner is set only when using the contructor from a |window|, 
so basically when doing what is done in content pages.
Though, now that I think about it, perhaps mOwner should be set also in Init() if just possible. That would make sense. More consistent behavior.
But still, no error in that method. What if xhr is created in non DOMWindow context.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Attachment: to be checked in

I'll check this in once tree is open

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

Checked in finally.

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for 1.8

I tested this for example using http://www.mnot.net/javascript/xmlhttprequest/,
gmail, gmaps, search bar, etc.

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

Review for the patch would be great.

Comment 51

[moz_bug_r_a4]

I've tested with 1.8 patch on Linux.  The testcases in this bug and testcase 6
in bug 403167 are fixed, but a new version of testcase 5 (please see bug 403167
comment #24,#25) still works.  "bind nsDOMParser to window" stuff is needed
also on 1.8 branch?

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

uh, right.

Comment 53

[Olli Pettay [:smaug][[e-mail]]]

Will do a new patch immediately!

Comment 54

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for 1.8, with nsDOMParser

Sorry about not thinking nsDOMParser. (The testcases were ok in linux.)

Comment 55

[Johnny Stenback (:jst)]

Comment on attachment 299223 [details] [diff] [review]
for 1.8, with nsDOMParser

- In nsDOMParser::ParseFromStream():

+    nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mOwner);
+    NS_ENSURE_STATE(win);
+    nsPIDOMWindow* outer = win->GetOuterWindow();
+    NS_ENSURE_STATE(outer && outer->GetCurrentInnerWindow() == win);
+    nsCOMPtr<nsIDOMWindow> window = do_QueryInterface(win);

Pointless QI, nsPIDOMWindow inherits nsIDOMWindow (indirectly).

sr=jst with that.

Comment 56

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 299223 [details] [diff] [review]
for 1.8, with nsDOMParser

Looking good to me.

Comment 57

[Samuel Sidler (old account; do not CC)]

Sadly, we decided to push this to the next release. It should be one of the first things that lands.

Olli, does this fix bug 403167 as well or does that one need a patch too?

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

This fixes bug 403167 too.

Comment 59

[Daniel Veditz [:dveditz]]

Comment on attachment 299223 [details] [diff] [review]
for 1.8, with nsDOMParser

approved for 1.8.1.13, a=dveditz for release-drivers

Comment 60

[Al Billings [:abillings - ex-MoCo]]

Verified fixed in 1.8.1.13 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.13) Gecko/2008031115 Firefox/2.0.0.13.

Comment 61

[Alexander Sack]

Attachment: for 1.8.0

This came out after some shuffling. Appears to fix the testcases and didn't break any xmlhttprequest site i tested.

Please check whether I forgot something. Thanks!

Comment 62

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 311272 [details] [diff] [review]
for 1.8.0

Remember to fix also the regressions this caused: 	bug 417617 and bug 421622

Comment 63

[Alexander Sack]

(In reply to comment #62)
> (From update of attachment 311272 [details] [diff] [review])
> Remember to fix also the regressions this caused:       bug 417617 and bug
> 421622
> 

thanks. looks like bug #403167 was wrapped into this patch for the branches?

bug #421622 hasn't landed yet on 1.8.1 branch afaict. Please request blocking1.8.0.X when that happens.

Comment 64

[Alexander Sack]

(In reply to comment #63)
> (In reply to comment #62)
> > (From update of attachment 311272 [details] [diff] [review] [details])
> > Remember to fix also the regressions this caused:       bug 417617 and bug
> > 421622
> > 
> 
> thanks. looks like bug #403167 was wrapped into this patch for the branches?
> 

sorry i misread the bug id. bug #417617 now blocks 1.8.0.15.

Comment 65

[Martin Stránský [:stransky] (ni? me)]

CheckInnerWindowCorrectness() test breaks orders at www.salesforce.com
(see https://bugzilla.redhat.com/show_bug.cgi?id=441227)

The current inner window of the outer is a different one...
(see comment 4)

Comment 66

[Olli Pettay [:smaug][[e-mail]]]

Well I guess that is an evangelism bug. Are they loading a new page but still
using (via .connection) XHR object from some old |window|.