1587743 - Moving GetPermission to parent to get correct top level principal when fission enabled

Status: RESOLVED FIXED

(Core :: DOM: Security, task, P3)

Description

[Thomas Nguyen (:tnguyen)]

AFAIK, in several places, we may use top principal from subframe cross process. Unfortunately, the top principal we are getting could be null. It would be ok if we only use the result of getTopLevelPrincipal to compute cross-origin (we got null means we are in cross-origin cross-process). However, if we use that top-level principal, for example do security/permission check, the result will be wrong.

We probably have to do some IPC with getTopLevelPrincipal and need to audit all the usage of getTopLevelPrincipal

Comment 1

[Thomas Nguyen (:tnguyen)]

One false case
https://searchfox.org/mozilla-central/rev/05a22d864814cb1e4352faa4004e1f975c7d2eb9/extensions/permissions/PermissionDelegateHandler.cpp#91

Comment 2

[(no longer active)]

(In reply to Thomas Nguyen (:tnguyen) ni? plz from comment #0)

We probably have to do some IPC with getTopLevelPrincipal and need to audit all the usage of getTopLevelPrincipal

I believe the rest of the usages are for bug 1576291, FWIW...

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

It seems that loadinfo->GetTopLevelPrincipal() should still work because we don't compute the information but rather pass the information down the inheritance tree whenever we create a new loadinfo object.

Comment 4

[Rob Wu [:robwu]]

The line from comment 1 also caused an issue without Fission ( bug 1589754 ) because it is null for top-level documents.

Currently, some "GetTopLevelPrincipal" methods return a principal of the top-level documents, others return null, and some others have different return values depending on whether Fission is enabled.

At least for permission-related code, "topLevelPrincipal" is expected to be non-null, e.g. at
https://searchfox.org/mozilla-central/rev/55aa17110091deef24b913d033ccaf58f9c6d337/dom/base/nsContentPermissionHelper.cpp#518,521,549

Here is an example where the value is expected to be non-null, but would be null with Fission enabled:
https://searchfox.org/mozilla-central/rev/55aa17110091deef24b913d033ccaf58f9c6d337/dom/html/HTMLMediaElement.cpp#6829,6837
(this example is dead code, by the way. I'll remove it in bug 1590417 ).

Is it in scope of this bug to ensure that topLevelPrincipal is always non-null, without exceptions?

Comment 5

[Thomas Nguyen (:tnguyen)]

Hi Nika, from bug 1589754 comment 7 I got topLevelPrincipal from loadinfo and it seems to be correct from cross-process document. Is that something unexpected and we need to fix it? Or I can rely on this to do further check

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Any code which exposes the principal of a cross-process document into your process is buggy, and we should be fixing it at some point. We don't want to be leaking that information into content processes. It would be good to file a bug about topLevelPrincipal being non-null for cross-process loadInfos.

This method, in general, is quite flawed. We cannot provide an implementation of it in the content process in a post fission world. Code which wants to do checks like this should do one of a couple of things:

1. If the principal which we want to compare with the TopLevelPrincipal is for an in-process document, the absence of a toplevel principal indicates that they don't match
2. If the principals we want to compare don't correspond to resources in this process, the parent process needs to handle doing the comparison for us.

Comment 7

[Thomas Nguyen (:tnguyen)]

(In reply to :Nika Layzell (ni? for response) from comment #6)

Any code which exposes the principal of a cross-process document into your process is buggy, and we should be fixing it at some point. We don't want to be leaking that information into content processes. It would be good to file a bug about topLevelPrincipal being non-null for cross-process loadInfos.

This method, in general, is quite flawed. We cannot provide an implementation of it in the content process in a post fission world. Code which wants to do checks like this should do one of a couple of things:

1. If the principal which we want to compare with the TopLevelPrincipal is for an in-process document, the absence of a toplevel principal indicates that they don't match
2. If the principals we want to compare don't correspond to resources in this process, the parent process needs to handle doing the comparison for us.

Thanks for confirming. So in this case of permission delegation (using topPrincipal to do permission check, store and retrieve), I will move all these tasks into parent.

Comment 8

[Chris Peterson [:cpeterson]]

Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Comment 9

[Thomas Nguyen (:tnguyen)]

I will take this bug, but I would narrow down the bug to fix top level principal in permision delegation, by moving the getPermission to parent.
Another moving of getting top principal to parent is in patch https://phabricator.services.mozilla.com/D53261

Comment 10

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1587743 - Move getPermission from top level principal to parent process when fission enabled

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:tnguyen, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 12

[Thomas Nguyen (:tnguyen)]

I tried to land but failed because it requires IPC peer to review. Asking for review in IPC modules

Comment 13

[Chris Peterson [:cpeterson]]

Temporarily reassigning these DOM Security Fission bugs to ckerschb for re-triage.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

I just checked in it seems e.g. test browser_permission_delegate_geo.js which makes this a valid bug. I guess this one needs an owner.

Comment 15

[Chris Peterson [:cpeterson]]

Thomas Nguyen said (in bug 1582999 comment 15) that his fix for this bug will also fix test_cross_origin_iframe.html bug 1582999.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Tim said he can pick that one up once he is done with some ETP fission work.

Comment 18

[Tim Huang[:timhuang]]

I think ETP is no longer using the top-level principal in the content processes. So, we can focus on the permission delegation that Thomas pointed out in comment 1.

I believe that we don't need to move the checking of the delegated permission to the parent process. It would be hard to do that since some delegated permissions do checks in a sync manner, such as the vibration permission. Given the fact that we prefer not to have a sync IPC, it would be very difficult to implement.

As an alternative, I proposed that we pre-compute all delegated permissions in the top-level window and store the results as a flag in the WindowContext of the top-level window. When we need to do the permission delegation, we can directly check the flag in the top-level WindowContext. And we keep the flag up-to-date once the permissions get updated. By doing this, we don't need the top-level principal and the permission delegation can still work for those requiring a sync operation.

Comment 19

[Tim Huang[:timhuang]]

Attachment: Bug 1587743 - Part 1: Pre-compute the delegated permissions for the top-level content and store it in the WindowContext. r?baku!,nika!

In order to delegate the permission to the top-level window, in this
patch, we pre-compute the permissions of the top-level context and set
them to the top-level WindowContext. So, the cross-origin iframe can
know the permission of the top-level window through the WindowContext.
Thus, the permission can be delegated in Fission.

Comment 20

[Tim Huang[:timhuang]]

Attachment: Bug 1587743 - Part 2: Removing mTopLevelPrincipal from the PermissionDelegateHandler. r?baku!

Depends on D79132

Comment 21

[Tim Huang[:timhuang]]

Attachment: Bug 1587743 - Part 3: Using WindowContext to test delegated permissions. r?baku!

Depends on D79133

Comment 22

[Tim Huang[:timhuang]]

Attachment: Bug 1587743 - Part 4: Enable test browser_permission_delegate_geo.js in Fission. r?baku!

Depends on D79134

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44e715808901
Part 1: Pre-compute the delegated permissions for the top-level content and store it in the WindowContext. r=baku,nika
https://hg.mozilla.org/integration/autoland/rev/a2bec965260d
Part 2: Removing mTopLevelPrincipal from the PermissionDelegateHandler. r=baku
https://hg.mozilla.org/integration/autoland/rev/4e6d8f786f87
Part 3: Using WindowContext to test delegated permissions. r=baku
https://hg.mozilla.org/integration/autoland/rev/fd202991fa01
Part 4: Enable test browser_permission_delegate_geo.js in Fission. r=baku

Comment 24

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/44e715808901
https://hg.mozilla.org/mozilla-central/rev/a2bec965260d
https://hg.mozilla.org/mozilla-central/rev/4e6d8f786f87
https://hg.mozilla.org/mozilla-central/rev/fd202991fa01