Open Bug 1587743 Opened 5 months ago Updated 1 month ago

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

Categories

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

task

Tracking

()

Fission Milestone M4.1

People

(Reporter: tnguyen, Unassigned)

References

(Blocks 5 open bugs)

Details

(Whiteboard: [domsecurity-backlog1])

Attachments

(1 file)

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
You need to log in before you can comment on or make changes to this bug.