Closed Bug 764389 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16
Tracking Status
firefox14 - ---
firefox15 + fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(2 files)

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?
I would slightly prefer the former, I think...  ccing some usual culprits.
I think I'd prefer the former here too.
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.
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.
Attachment #633164 - Flags: review?(luke)
And here are the gecko bits. Flagging bz.
Attachment #633165 - Flags: review?(bzbarsky)
Blocks: 760745
Attachment #633164 - Flags: review?(luke) → review+
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.
Attachment #633165 - Flags: review?(bzbarsky) → review+
(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.
> 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...
Blocks: 754156
(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.
Landed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/2bcf136fc64c
http://hg.mozilla.org/integration/mozilla-inbound/rev/4176490cd0d2
Assignee: nobody → bobbyholley+bmo
Flags: in-testsuite+
Target Milestone: --- → mozilla16
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
Attachment #633165 - Flags: approval-mozilla-aurora?
Beta appears as if it's affected as well. Should we consider uplift?
Attachment #633165 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
You need to log in before you can comment on or make changes to this bug.