1790207 - Deserializing of a FileSystemHandle should check origin

Status: RESOLVED FIXED

(Core :: DOM: File, defect, P1)

Description

[Peter Van der Beken [:peterv]]

Attachment: Patch to add a mochitest

See https://fs.spec.whatwg.org/#filesystemhandle, we don't seem to be doing step 1:

Their deserialization steps, given serialized and value are:

1. If serialized.[[Origin]] is not same origin with value’s relevant settings object's origin, then throw a DataCloneError.

2. Set value’s entry to serialized.[[Entry]]

Attaching a mochitest, but there do seem to be WPTs that test this already (though right now all testing/web-platform/tests/fs WPTs fail for me, even with dom.fs.enabled set to true).

I often try to annotate code with the steps from the spec algorithm (within reason), to make sure I don't forget any.

Comment 1

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

Set release status flags based on info from the regressing bug 1751681

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

My understanding is:

• The core logic behind the checks for the same-origin de-serialization checks exists at https://phabricator.services.mozilla.com/D149264. We spent a lot of time discussing the best way to do this. That said, I don't see where the actual hookups happen in the stack at the current moment.
  • I am not sure why the WebIDL serialization annotations landed without the security checks, though? It seems like they should have landed at the same time and I'll be paying more attention to relative positioning of WebIDL patches in whatever stack they're in going forward since the Phabricator review between the author's comments and my (WebIDL) review comments should have contextualized the changes in terms of their dependencies if everything was not going to be in place at the time of landing and explain the rationale for enabling the WebIDL prior to the rest of the logic being there. That said...
• There's a major meta-issue with our infrastructure for development efforts that involve large patch-stacks, especially when there are multiple people collaborating. Right now I understand the core OPFS team to be very awkwardly juggling patches through phabricator, and so there's a general desire to get patches out of the human-effort-intensive phabricator stack and into the tree since this ends significant overhead cost. The other potential approach available was a branch but that would tend to have much worse results given the number of significant changes that touch areas of the tree outside of dom/fs and would greatly increase the potential to regress m-c code and for landings to be crash-landings of massive patches.
• The security check here is actually a policy check rather than a SOP check; without the check (and assuming the actor hookups were in place in the stack right now, which I believe they aren't) we have largely the same characteristics as Blobs/Files albeit with mutability. (That said, we very much want the policy check.)

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1790207: Provide cross-origin security checks for OPFS r=nika,peterv,asuth

Comment 4

[Pascal Chevrel:pascalc]

Jan, is there potential for an uplift to 106 once a patch is on nightly given that you set it to P1/S2 or can the fix ride the 107 train when it is ready? Thanks

Comment 5

[Pascal Chevrel:pascalc]

(In reply to Pascal Chevrel:pascalc from comment #4)

Jan, is there potential for an uplift to 106 once a patch is on nightly given that you set it to P1/S2 or can the fix ride the 107 train when it is ready? Thanks

Randell, could you give me a status update wrt to our 106Y release? Thanks

Comment 6

[Jan Varga [:janv]]

Well, OPFS is still disabled by default on all branches, so the possible security issue could be only observed if someone flips the pref manually. I gave it P1/S2 just to get this higher priority over other OPFS bugs which are mostly P2/S3.
Uplifting the fix would be nice to have, but it's not a must IMO.

Comment 7

[Donal Meehan [:dmeehan]]

Looks like it's too late for 107.
The attached patch doesn't look like something we would take in an uplift.

Comment 8

[Jim Mathies [:jimm]]

feature restricted to nightly, depends on main tracker.

Comment 9

[Jan Varga [:janv]]

Attachment: Bug 1790207 - Change nsIGlobalObject::GetStorageKey to return NS_ERROR_NOT_AVAILABLE by default; r=#dom-storage

nsGlobalWindowInner, WorkerGlobalScopeBase, BackstagePas and SandboxPrivate now
explicitly provide GetStorageKey implementation which explicitly block null and
expanded principals. All other globals (nsGlobalWindowOuter, SimpleGlobalObject,
ShadowRealmGlobalScope and WorkletGlobalScope) inherit the default
implementation which returns NS_ERROR_NOT_AVAILABLE.

Depends on D157892

Comment 10

[Jan Varga [:janv]]

Attachment: Bug 1790207 - Forward declaration of mozilla::Result in nsIGlobalObject.h and other cleanup; r=#dom-storage

Depends on D162087

Comment 11

[Jan Varga [:janv]]

Attachment: Bug 1790207 - Fix the function for comparing storage keys; r=#dom-storage

Depends on D162088

Comment 12

[Jan Varga [:janv]]

https://treeherder.mozilla.org/jobs?repo=try&revision=679847fdaaf297e58dcc6816c0f88138e47e450d

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d1bce61c9c4
Provide cross-origin security checks for OPFS r=nika,asuth
https://hg.mozilla.org/integration/autoland/rev/1bb90b57e047
Change nsIGlobalObject::GetStorageKey to return NS_ERROR_NOT_AVAILABLE by default; r=dom-storage-reviewers,jesup,asuth
https://hg.mozilla.org/integration/autoland/rev/fe07529cfe78
Forward declaration of mozilla::Result in nsIGlobalObject.h and other cleanup; r=dom-storage-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/54bf7e90e341
Fix the function for comparing storage keys; r=dom-storage-reviewers,jesup,asuth

Comment 14

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/7d1bce61c9c4
https://hg.mozilla.org/mozilla-central/rev/1bb90b57e047
https://hg.mozilla.org/mozilla-central/rev/fe07529cfe78
https://hg.mozilla.org/mozilla-central/rev/54bf7e90e341

Comment 15

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

The patch landed in nightly and beta is affected.
:jesup, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox108 to wontfix.

For more information, please visit auto_nag documentation.

Comment 16

[Jan Varga [:janv]]

This feature (OPFS) is still not enabled by default, so no need for an uplift.