Closed Bug 1587743 Opened 5 years ago Closed 4 years ago

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

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla79
Fission Milestone M4.1
Tracking Status
firefox79 --- fixed

People

(Reporter: tnguyen, Assigned: timhuang)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [domsecurity-active])

Attachments

(5 files)

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

Summary: Fix get top level principal in fission enabled → Fix getTopLevelPrincipal when fission enabled
Type: defect → task
Blocks: 1582999

(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...

Blocks: etp-fission

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.

Priority: -- → P3
Whiteboard: [domsecurity-backlog1]

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?

See Also: → 1589754

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

Flags: needinfo?(nika)

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.
Flags: needinfo?(nika)

(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.

See Also: → 1595722

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

Fission Milestone: --- → ?

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

Assignee: nobody → tnguyen
Summary: Fix getTopLevelPrincipal when fission enabled → Moving GetPermission to parent to get correct top level principal when fission enabled
Status: NEW → ASSIGNED
Priority: P3 → P2
Attachment #9109686 - Attachment description: Bug 1587743 - Move get permission from top level principal to parent when fission enabled → Bug 1587743 - Move getPermission from top level principal to parent process when fission enabled
Fission Milestone: ? → M4.1

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.

Flags: needinfo?(tnguyen)
Flags: needinfo?(tnguyen)

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

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

Assignee: tnguyen → ckerschb
Status: ASSIGNED → NEW
Priority: P2 → P3
Assignee: ckerschb → nobody

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.

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

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

Assignee: nobody → tihuang
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
No longer blocks: etp-fission

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.

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.

Pushed by tihuang@mozilla.com: 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
Regressions: 1645997
Regressions: 1650655
Regressions: 1654803
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: