1599438 - Sandbox flags should be snapshotted at start of navigation

Status: RESOLVED FIXED

(Core :: DOM: Navigation, enhancement, P3)

Description

[Ian Clelland]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/78.0.3904.108 Safari/537.36

Steps to reproduce:

Create an iframe element, insert into a document to kick off a navigation. Set the iframe's sandbox flags after navigation starts, but before the response is returned and the document created.

See WPT at https://github.com/web-platform-tests/wpt/pull/20411

Actual results:

Document gets sandbox flags from the state of the iframe container when the navigation completes.

Expected results:

Document sandboxing flags should come from the state of the iframe container as the navigation starts. See recent HTML spec change at https://github.com/whatwg/html/pull/5098.

Comment 1

[Matt Woodrow (:mattwoodrow)]

I chatted with kmag about this.

We're currently grabbing the sandbox flags from the browsing context when we construct the loaded Document.

We should instead make the sandbox flags a property of the LoadInfo (still read from the BrowsingContext), which we construct when initiating a load. We can then read the value from the LoadInfo into the Document when it is created.

Comment 2

[Kris Maglione [:kmag]]

I wonder if we may need to add another step of copying it to the DocShellLoadState in whatever process initiates the load, and then copying that value to the LoadInfo. In practice, I think it shouldn't make a difference, but theoretically, under Fission, a frame with a cross-process parent could initiate a load in a cross-process sibling frame at the same time as the sandbox flags are changed in the parent. But in principal, that frame should also know nothing about the sandbox flags of its cross-origin sibling of a cross-origin parent, so... the more I think about it, it shouldn't have any say in the matter, and it shouldn't make a difference from its perspective, anyway.

Comment 3

[Boris Zbarsky [:bzbarsky]]

We're currently grabbing the sandbox flags from the browsing context when we construct the loaded Document.

It's a lot more messed up than that. We're currently sort-of grabbing the SANDBOXED_ORIGIN flag when the loadinfo is created (see the SEC_SANDBOXED flag in the loadinfo's mSecurityFlags). We grab all the other flags when we create the document. And yes, I think the right fix is to change those places where we set that up right now to store the whole sandbox flag word in the loadinfo....

I'd certainly label this a "defect", not an "enhancement", in that we can actually end up with things that have a sandboxed principal but not the corresponding sandbox flag, or the other way around!

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1599438 - Store sandbox flags on the LoadInfo when creating a channel for a docshell, so that we don't read a stale value from the BrowsingContext later. r?kmag

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3538b7016aa
Store sandbox flags on the LoadInfo when creating a channel for a docshell, so that we don't read a stale value from the BrowsingContext later. r=bzbarsky

Comment 6

[Narcis Beleuzu [:NarcisB]]

Backed out for bustages on TestHttpFuzzing.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/f883bf64e3da22f75df5b92b9f5ef9d60ecfb68e
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=284970525&repo=autoland&lineNumber=17154

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d55ddfe4e0fa
Store sandbox flags on the LoadInfo when creating a channel for a docshell, so that we don't read a stale value from the BrowsingContext later. r=bzbarsky

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d55ddfe4e0fa