Add an assertion ensuring that documents and friends can never end up with an expanded principal

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

(Obviously after the existing cases where this can happen have been fixed.)

Boris, I'm currently thinking of adding the assertion to nsNodeInfoManager::SetDocumentPrincipal().  Can you think of other places where we'd want to assert this as well?
Flags: needinfo?(bzbarsky)
That one location should gate all the document stuff.

I guess you could add something to where a window creates its global JSObject, to catch cases in which a window has a principal that doesn't match its document for some reason.
Flags: needinfo?(bzbarsky)
Comment on attachment 8795884 [details] [diff] [review]
Add assertions ensuring that documents and inner windows cannot end up with an expanded principal

Review of attachment 8795884 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the window stuff removed. If there's a reason for it that I'm missing let's discuss further.

::: dom/base/nsGlobalWindow.cpp
@@ +2664,5 @@
>      bool sameOrigin = false;
>      nsIPrincipal *existing =
>        nsJSPrincipals::get(JS_GetCompartmentPrincipals(compartment));
> +    MOZ_DIAGNOSTIC_ASSERT(!nsContentUtils::IsExpandedPrincipal(existing),
> +                          "The inner window shouldn't have an expanded principal");

This doesn't seem right - we're in an #ifdef DEBUG (so the telemetry will never accumulate), and it seems a bit odd to me to check the _old_ principal in the inner window reuse case (not that it's wrong, but I just don't understand why we're checking it specifically).

I'd think the check to make sure NodePrincipals are never expanded should be sufficient.

::: toolkit/components/telemetry/Histograms.json
@@ +10497,5 @@
> +    "expires_in_version": "58",
> +    "kind": "count",
> +    "description": "Number of documents encountered using an expanded principal."
> +  },
> +  "WINDOW_WITH_EXPANDED_PRINCIPAL": {

Again, it seems like this is subsumed by the document case and should be removed.
Attachment #8795884 - Flags: review?(bobbyholley) → review+
I added the window code based on comment 1.

Boris, are you OK with only a check in nsNodeInfoManager::SetDocumentPrincipal()?
Flags: needinfo?(bzbarsky)
I'm probably fine with that, yes.
Flags: needinfo?(bzbarsky)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93a20b9fe4f9
Add assertions ensuring that documents and inner windows cannot end up with an expanded principal; r=bholley
https://hg.mozilla.org/mozilla-central/rev/93a20b9fe4f9
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
See Also: → 1307730
Depends on: 1307730
See Also: 1307730
Depends on: 1397371
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.