Closed Bug 403168 Opened 14 years ago Closed 14 years ago

XSS by using XMLHttpRequest and event handler

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: moz_bug_r_a4, Assigned: smaug)

References

Details

(Keywords: testcase, verified1.8.1.13, Whiteboard: [sg:high])

Attachments

(5 files, 4 obsolete files)

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.
Flags: blocking1.8.1.10?
Whiteboard: [sg:critical]
Assignee: dveditz → nobody
Component: Security → DOM
Flags: blocking1.9?
QA Contact: toolkit → general
Smaug, this looks a lot like bug 403167.
Assignee: nobody → Olli.Pettay
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
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.
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.
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.
Blocks: 386823
Attached patch possible patch, v1-simple (obsolete) — Splinter Review
This patch fixes the bug, but is still not quite right. But perhaps enough for 1.8?
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.
Attachment #288237 - Flags: superreview?(jonas)
Attachment #288237 - Flags: review?(jonas)
Status: NEW → ASSIGNED
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)
Flags: blocking1.8.1.10? → blocking1.8.1.11+
Whiteboard: [sg:critical] → [sg:high]
This is actually quite different problem comparing to bug 403167.
The proposed fix can be circumvented, since it's possible to call send() in the
subframe's context.
Comment on attachment 288237 [details] [diff] [review]
possible patch, v1-simple
Attachment #288237 - Attachment is obsolete: true
Attachment #288237 - Flags: superreview?(jonas)
Attachment #288237 - Flags: review?(jonas)
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.
moz_bug_r_a4@yahoo.com can you break this one ;)
Attachment #288680 - Flags: review?(jonas)
The second patch can be circumvented by calling send() on the subframe's
context after loading a target site in the subframe.
Attachment #288680 - Flags: review?(jonas)
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.
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.
No, the 4th bug is still there...just very hard to reproduce.
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...
Attached patch Bind XHR to a window (obsolete) — Splinter Review
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.

moz_bug_r_a4@yahoo.com, still able to break this?
Attachment #289026 - Flags: review?(jonas)
Why are the calls to CheckInnerWindowCorrectness needed?
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.)
Note, when a normal document is going away (unloaded/bfcached) its scriptglobalobject is cleared. But documentviewer, nor globalobject knows nothing 
about XHRs.
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?
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 :)
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.
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.
(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
(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.)
Could you upload that testcase? Would be easier to confirm this fixed (when this eventually lands).
This is needed to fix testcase 4&5 in Bug 403167, and makes DOMParser and XMLHttpRequest work consistently; bind them to the owning window.
Attachment #289026 - Attachment is obsolete: true
Attachment #289289 - Flags: review?(jonas)
Attachment #289026 - Flags: review?(jonas)
Attachment #289289 - Attachment is obsolete: true
Attachment #290733 - Flags: review?(jonas)
Attachment #289289 - Flags: review?(jonas)
Testcase 4 tests CheckInnerWindowCorrectness in Send
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.
Attachment #290733 - Flags: review?(jonas) → review+
 > 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?
(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.
(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

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 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.
Attachment #290733 - Flags: superreview?(jst)
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 on attachment 290733 [details] [diff] [review]
+CheckInnerWindowCorrectness in Open and Send

clearing sr request until it is clear what we want here.
Attachment #290733 - Flags: superreview?(jst)
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.
> 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

Attachment #290733 - Flags: superreview?(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.
Attachment #290733 - Flags: superreview?(jst) → superreview+
(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.
Attached patch to be checked inSplinter Review
I'll check this in once tree is open
Checked in finally.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Whiteboard: [sg:high] → [sg:high][need 1.8-branch patch]
Flags: wanted1.8.1.x+
Attached patch for 1.8 (obsolete) — Splinter Review
I tested this for example using http://www.mnot.net/javascript/xmlhttprequest/,
gmail, gmaps, search bar, etc.
Attachment #296842 - Flags: review?(jonas)
Whiteboard: [sg:high][need 1.8-branch patch] → [sg:high][need 1.8-branch r=sicking]
Review for the patch would be great.
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?
uh, right.
Will do a new patch immediately!
Sorry about not thinking nsDOMParser. (The testcases were ok in linux.)
Attachment #296842 - Attachment is obsolete: true
Attachment #299223 - Flags: superreview?(jst)
Attachment #299223 - Flags: review?(jonas)
Attachment #296842 - Flags: review?(jonas)
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.
Attachment #299223 - Flags: superreview?(jst) → superreview+
Comment on attachment 299223 [details] [diff] [review]
for 1.8, with nsDOMParser

Looking good to me.
Attachment #299223 - Flags: review?(jonas) → review+
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?
Flags: blocking1.8.1.13+
Flags: blocking1.8.1.12-
Flags: blocking1.8.1.12+
Whiteboard: [sg:high][need 1.8-branch r=sicking] → [sg:high][need 1.8-branch]
Whiteboard: [sg:high][need 1.8-branch] → [sg:high]
This fixes bug 403167 too.
Attachment #299223 - Flags: approval1.8.1.13?
Comment on attachment 299223 [details] [diff] [review]
for 1.8, with nsDOMParser

approved for 1.8.1.13, a=dveditz for release-drivers
Attachment #299223 - Flags: approval1.8.1.13? → approval1.8.1.13+
Depends on: 417617
Depends on: 421622
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.
Attached patch for 1.8.0Splinter Review
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!
Attachment #311272 - Flags: review?(Olli.Pettay)
Flags: blocking1.8.0.15+
Comment on attachment 311272 [details] [diff] [review]
for 1.8.0

Remember to fix also the regressions this caused: 	bug 417617 and bug 421622
Attachment #311272 - Flags: review?(Olli.Pettay) → review+
(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.
(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.
Group: security
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)
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|.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.