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

RESOLVED FIXED in Firefox 15

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox14-, firefox15+ fixed)

Details

Attachments

(2 attachments)

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.
Created attachment 633164 [details] [diff] [review]
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.
Attachment #633164 - Flags: review?(luke)
Created attachment 633165 [details] [diff] [review]
Part 2 - Update compartment principals on inner window reuse and on document.write. v1

And here are the gecko bits. Flagging bz.
Attachment #633165 - Flags: review?(bzbarsky)
Blocks: 760745

Updated

5 years ago
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.
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=84ab2afa0b32
> 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...

Updated

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/4176490cd0d2
https://hg.mozilla.org/mozilla-central/rev/2bcf136fc64c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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?

Updated

5 years ago
tracking-firefox15: --- → +

Updated

5 years ago
tracking-firefox14: --- → +
Beta appears as if it's affected as well. Should we consider uplift?

Updated

5 years ago
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.
Landed to m-a:
http://hg.mozilla.org/releases/mozilla-aurora/rev/34347ff5cd76
http://hg.mozilla.org/releases/mozilla-aurora/rev/9f9876174f1b
status-firefox15: --- → fixed

Updated

5 years ago
tracking-firefox14: + → -
You need to log in before you can comment on or make changes to this bug.