Closed Bug 1580462 Opened 5 months ago Closed 4 months ago

Make Feature Policy cross origin iframe works when fission enable.

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla72
Fission Milestone M4
Tracking Status
firefox72 --- fixed

People

(Reporter: tnguyen, Assigned: tnguyen)

References

(Blocks 4 open bugs)

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 files)

Checking bug Bug 1580074 I could see default allowlist does not work in fission cross origin iframe.
This need to be fixed to enable fission

Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
Blocks: 1572461
Type: defect → enhancement
Priority: -- → P2
Whiteboard: [domsecurity-active]
Depends on: 1582999
Depends on: 1580074
Summary: Make FP default allowlist works when fission enable. → Make Feature Policy cross origin works when fission enable.
Summary: Make Feature Policy cross origin works when fission enable. → Make Feature Policy cross origin iframe works when fission enable.
Blocks: 1582999
No longer depends on: 1582999

The featurepolicy is inherited from document (document's mFeaturePolicy) -> iframe (HTMLIFrameELement's mFeaturePolicy) -> iframe's document (document's mFeaturePolicy).

https://searchfox.org/mozilla-central/rev/e3fc8f8970491aef14d3212b2d052942f4d29818/dom/base/Document.cpp#3365
In fission,
nsCOMPtr<nsINode> node = containerWindow->GetFrameElementInternal();

The node I get from containerWIndow is null due to the new cross process iframe system. That makes FeaturePolicy was not inherited correctly.
Hi Nika, I am lost of how fission code works, could you please give me some clues of how I could get the container iframe propertly?
Thanks

Flags: needinfo?(nika)

The frame element in the parent document is probably going to need to sync its feature policy information to its FrameLoader's BrowsingContext, so that the child can check its own feature policy directly without trying to walk up to a cross-process parent.

Flags: needinfo?(nika)

Thanks Kris. The featurepolicy is bound with a node and contains various things (particularly and possibly several arrays of principals). I think it's not good to store it directly in BrowsingContext and ipc sync it.
Perhaps we just need to send a request to trigger "compute inheritance", or in the worst case, expose a new class FeaturePolicyIPCInfo which only contains important things in string.

The feature policy inheritance goes just in 1 direction (document -> iframes -> iframes' documents) and it should be easy to be serialized as an IPC data struct. You just care about a list of features and the default origin. Probably, we can simply migrate from 'Feature.h' to and IPC version of it.
You can store it in the client-info, or in the browsingContext. Let me know if you need any help here.

Thanks Baku, that should work. I only concern that we are trying to serialize/deserialize many data struct (array of features and each features contains an array of principals). Doing that seems overkill to me and I am trying to simplify that data.
Anyway, fission is disabled by default, I will disable the fail test first and come back to this next week.

Trying to simplify the data is probably the best option, in any case. We try to limit the data that we make available to cross-origin processes, so if you can synchronize the minimal set of data that's required to make your decision, that is generally best.

That said, synchronizing complex data over IPC is generally not that difficult. IPDL handles things like arrays and principals pretty trivially, so if that's what you need to do, you should be able to.

Type: enhancement → task
Attachment #9100141 - Attachment description: Bug 1580462 - Storing iframe's FeaturePolicy tp inherit cross origin document. → Bug 1580462 - Storing iframe's FeaturePolicy to inherit cross origin document.
Attachment #9100141 - Attachment description: Bug 1580462 - Storing iframe's FeaturePolicy to inherit cross origin document. → Bug 1580462 - Store iframe's FeaturePolicy in browsingContext to inherit cross origin document.
Pushed by tnguyen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae33b9c001e5
Store iframe's FeaturePolicy in browsingContext to inherit cross origin document. r=baku,farre

Thank you, now there're 2 Features with different namespaces, then cause build failure in various platform. Will fix it

Flags: needinfo?(tnguyen)
Pushed by tnguyen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1708688569b4
Store iframe's FeaturePolicy in browsingContext to inherit cross origin document. r=baku,farre
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

This patch is missing an #include "mozilla/dom/BrowsingContext.h" in DOMIntersectionObserver.cpp which is causing the Thunderbird build to fail. I'm about to post a patch which I hope can be reviewed and landed ASAP.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/integration/autoland/rev/b05f555b78d8
Follow-up: include a missing header file. r=baku
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/ba30626ccb8c
Follow-up: include a missing header file. r=baku a=tb-fix
Blocks: 1580074
No longer depends on: 1580074

Retroactively moving fixed bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to an appropriate Fission 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: --- → M4
You need to log in before you can comment on or make changes to this bug.