764389 - Inner window reuse can cause the compartment principal to not match the window

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

This is sort of obvious, but I'm not sure it's something we directly considered. IIRC, one of the blockers for pulling object principals directly off the compartment was that some consumers asked for the principal when the really wanted the URI.

CPG mostly solved this. But inner window reuse can still cause us to alter the principal of a window to something different-but-same-origin without altering the compartment.

We could add an API to update the compartment principal, or we could just declare that we don't care and fix any consumers that get broken by it. bz, what do you think?

Comment 1

[Boris Zbarsky [:bzbarsky]]

I would slightly prefer the former, I think...  ccing some usual culprits.

Comment 2

[Johnny Stenback (:jst)]

I think I'd prefer the former here too.

Comment 3

[Bobby Holley (:bholley)]

So, there are actually two issues here:
1 - Reusing a chrome window for content via the semantics of WouldReuseInnerWindow.
2 - Switching between same-origin principals with document.open.

We could theoretically take care of both in nsGlobalWindow::SetNewDocument, but unfortunately in nsHTMLDocument::Open we call SetNewDocument _before_ calling Reset. Rather than sticking my hand in that pile of rattlesnakes, bz and I decided it would be best to just add an explicit notification method to nsGlobalWindow that we can call when we alter the document principal.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add an API to change the principals of a compartment. v1

Here's the JSAPI part. I'm hoping luke can verify that there's nowhere in the js engine that depends on principals not changing.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Update compartment principals on inner window reuse and on document.write. v1

And here are the gecko bits. Flagging bz.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 633165 [details] [diff] [review]
Part 2 - Update compartment principals on inner window reuse and on document.write. v1

>+++ b/content/base/src/nsDocument.cpp
>+  nsCOMPtr<nsPIDOMWindow> win = GetWindow();

How about:

  nsPIDOMWindow* win = GetInnerWindow();

?

>+++ b/dom/base/nsGlobalWindow.cpp
>+    JS_SetCompartmentPrincipals(js::GetObjectCompartment(currentInner->mJSObject),
>+                                nsJSPrincipals::get(aDocument->NodePrincipal()));

currentInner->RefreshCompartmentPrincipal(), unless there's a strong reason not to.

r=me with those.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> currentInner->RefreshCompartmentPrincipal(), unless there's a strong reason
> not to.

Doesn't work, because mDoc hasn't been switched over yet. :-(

I considered adding an optional parameter to allow the method to be shared, but given that the method was a one-liner (and that the method needed to be declared in two places) it didn't seem worth it.

Comment 8

[Bobby Holley (:bholley)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=84ab2afa0b32

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Doesn't work, because mDoc hasn't been switched over yet. :-(

Even on the curentInner?

If so, please clearly document this so someone doesn't "fix" it...

Comment 10

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #9)
> Even on the curentInner?

Yeah. AFAICT that doesn't happen until here:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2057

> If so, please clearly document this so someone doesn't "fix" it...

ok.

Comment 11

[Bobby Holley (:bholley)]

Landed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/2bcf136fc64c
http://hg.mozilla.org/integration/mozilla-inbound/rev/4176490cd0d2

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4176490cd0d2
https://hg.mozilla.org/mozilla-central/rev/2bcf136fc64c

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 633165 [details] [diff] [review]
Part 2 - Update compartment principals on inner window reuse and on document.write. v1

This fixes the crash over in bug 760745, which is being tracked for aurora.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 739825
User impact if declined: Potential security bugs
Testing completed (on m-c, etc.): On m-c for ~1 week, no problems.
Risk to taking this patch (and alternatives if risky): Not very risky.
String or UUID changes made by this patch: None

Comment 14

[Alex Keybl [:akeybl]]

Beta appears as if it's affected as well. Should we consider uplift?

Comment 15

[Bobby Holley (:bholley)]

(In reply to Alex Keybl [:akeybl] from comment #14)
> Beta appears as if it's affected as well. Should we consider uplift?

I don't think so. The fix here doesn't make any sense pre-CPG.

Comment 16

[Bobby Holley (:bholley)]

Landed to m-a:
http://hg.mozilla.org/releases/mozilla-aurora/rev/34347ff5cd76
http://hg.mozilla.org/releases/mozilla-aurora/rev/9f9876174f1b