Last Comment Bug 764389 - Inner window reuse can cause the compartment principal to not match the window
: Inner window reuse can cause the compartment principal to not match the window
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Bobby Holley (busy)
:
Mentors:
Depends on:
Blocks: 754156 760745
  Show dependency treegraph
 
Reported: 2012-06-13 07:47 PDT by Bobby Holley (busy)
Modified: 2012-07-02 16:55 PDT (History)
9 users (show)
bobbyholley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
+
fixed


Attachments
Part 1 - Add an API to change the principals of a compartment. v1 (3.42 KB, patch)
2012-06-14 09:33 PDT, Bobby Holley (busy)
luke: review+
Details | Diff | Review
Part 2 - Update compartment principals on inner window reuse and on document.write. v1 (9.68 KB, patch)
2012-06-14 09:34 PDT, Bobby Holley (busy)
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Bobby Holley (busy) 2012-06-13 07:47:36 PDT
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 [:bz] (Out June 25-July 6) 2012-06-13 08:26:21 PDT
I would slightly prefer the former, I think...  ccing some usual culprits.
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2012-06-13 09:53:59 PDT
I think I'd prefer the former here too.
Comment 3 Bobby Holley (busy) 2012-06-14 09:32:48 PDT
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 (busy) 2012-06-14 09:33:55 PDT
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.
Comment 5 Bobby Holley (busy) 2012-06-14 09:34:17 PDT
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.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-14 10:58:14 PDT
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 (busy) 2012-06-14 16:47:04 PDT
(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 (busy) 2012-06-14 16:51:57 PDT
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=84ab2afa0b32
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-14 18:28:02 PDT
> 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 (busy) 2012-06-14 23:29:01 PDT
(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 13 Bobby Holley (busy) 2012-06-20 03:32:44 PDT
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] 2012-06-24 13:25:14 PDT
Beta appears as if it's affected as well. Should we consider uplift?
Comment 15 Bobby Holley (busy) 2012-06-25 01:39:46 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.