1556082 - Storage::mStoragePrincipal/LSObject::mStoragePrincipalInfo should be changed

Status: RESOLVED FIXED

(Core :: Privacy: Anti-Tracking, defect)

Description

[(no longer active)]

Document::mStoragePrincipal is mutable, so we shouldn't store copies of it around.

Right?

Comment 1

[Andrea Marchesini [:baku]]

At the moment, mStoragePrincipal is not mutable.
Document::GetEffectiveStoragePrincipal() can return mStoragePrincipal or the NodePrincipal() based on the current storage access policy, but we never update mStoragePrincipal.

Comment 2

[(no longer active)]

Yes, you're right. What I meant was "semantically mutable".

Let's consider this sequence of events:

• Third party page accesses window.localStorage (L1). We examine the effective storage principal and create an LSObject instance using the resulting storage principal (P1).
• Page sets up a storage event listener on that object.
• Now storage access is granted in the third-party page. The result of computing the effective storage principal will be different from this point on (P2).
• At this point we call EnsureObserver() on the object from here: https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/dom/base/nsGlobalWindowInner.cpp#6981. This call uses LSObject::mStoragePrincipalInfo which points to P1 (because it originally came from here: https://searchfox.org/mozilla-central/rev/9eb2f739c165b4e294929f7b99fbb4d90f8a396b/dom/localstorage/LSObject.cpp#332). The wrong principal is being used here.
• A very similar problem exists in EnsureDatabase() which can be triggered if in the first step the script only instantiates localStorage and here it attempts to e.g. obtain its length: https://searchfox.org/mozilla-central/rev/9eb2f739c165b4e294929f7b99fbb4d90f8a396b/dom/localstorage/LSObject.cpp#846.
• Let's consider the case where the script sets a new item on L1. Storage::mStoragePrincipal for L1 points to P1. So the NotifyChange() function will be invoked with P1 and we'll attach it to the storage event here: https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/dom/storage/Storage.cpp#128 and then we get to here: https://searchfox.org/mozilla-central/rev/9eb2f739c165b4e294929f7b99fbb4d90f8a396b/dom/storage/StorageNotifierService.cpp#79. Now we're going to check P1 against the new effective storage principal (P2) and they won't match, so as far as I can tell no storage event will be dispatched anywhere. (Perhaps this part is actually expected.)

This is all that I could gather by code inspection. Perhaps the last bullet point is fine but it seems like there are some bugs in the other ones. Do you mind taking a careful look at how this setup all fits together and whether these copies of the principal on the storage objects ever will have wrong/unexpected values? (I'm not super familiar with the storage code so some of my conclusions above may be mistaken...)

Comment 3

[Andrea Marchesini [:baku]]

• At this point we call EnsureObserver() on the object from here: https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/dom/base/nsGlobalWindowInner.cpp#6981. This call uses LSObject::mStoragePrincipalInfo which points to P1 (because it originally came from here: https://searchfox.org/mozilla-central/rev/9eb2f739c165b4e294929f7b99fbb4d90f8a396b/dom/localstorage/LSObject.cpp#332). The wrong principal is being used here.

This is wrong. When the storage access is granted, the window should instantiate a new LSObject. The 2 objects (the previous localStorage and the new one), will be both active.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: Bug 1556082 - LocalStorage must be recreated when storage-access is granted, r?ehsan,asuth

This behavior is similar to indexedDB and other QuotaManager-related APIs.

Comment 5

[(no longer active)]

And what about the other problems? :-)

Comment 6

[Andrea Marchesini [:baku]]

My patch is specifically for this bug. Let me comment your STR step by step:

• Third party page accesses window.localStorage (L1). We examine the effective storage principal and create an LSObject instance using the resulting storage principal (P1).
• Page sets up a storage event listener on that object.
• Now storage access is granted in the third-party page. The result of computing the effective storage principal will be different from this point on (P2).

The page must set a new event listener on the new localStorage. The old one will be able to receive events only from the partitioned cookie jar.

• At this point we call EnsureObserver() on the object from here: https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/dom/base/nsGlobalWindowInner.cpp#6981. This call uses LSObject::mStoragePrincipalInfo which points to P1 (because it originally came from here: https://searchfox.org/mozilla-central/rev/9eb2f739c165b4e294929f7b99fbb4d90f8a396b/dom/localstorage/LSObject.cpp#332). The wrong principal is being used here.

This is fine with my patch. EnsureObserver() will be called on the new localStorage.

• A very similar problem exists in EnsureDatabase() which can be triggered if in the first step the script only instantiates localStorage and here it attempts to e.g. obtain its length: https://searchfox.org/mozilla-central/rev/9eb2f739c165b4e294929f7b99fbb4d90f8a396b/dom/localstorage/LSObject.cpp#846.

Same here.

• Let's consider the case where the script sets a new item on L1. Storage::mStoragePrincipal for L1 points to P1. So the NotifyChange() function will be invoked with P1 and we'll attach it to the storage event here: https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/dom/storage/Storage.cpp#128 and then we get to here: https://searchfox.org/mozilla-central/rev/9eb2f739c165b4e294929f7b99fbb4d90f8a396b/dom/storage/StorageNotifierService.cpp#79. Now we're going to check P1 against the new effective storage principal (P2) and they won't match, so as far as I can tell no storage event will be dispatched anywhere. (Perhaps this part is actually expected.)

The new L1 will be able to talk with the first-party localStorage objects only. This is expected.

Comment 7

[(no longer active)]

Ah very nice! Thanks a lot for explaining, this wasn't really clear to me at all to be honest. :-) Now it all makes perfect sense.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b99db031cc0
LocalStorage must be recreated when storage-access is granted, r=Ehsan,asuth

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/1b99db031cc0