Storage::mStoragePrincipal/LSObject::mStoragePrincipalInfo should be changed
Categories
(Core :: Privacy: Anti-Tracking, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox69 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: baku)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
Document::mStoragePrincipal is mutable, so we shouldn't store copies of it around.
Right?
Assignee | ||
Comment 1•5 years ago
|
||
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.
Reporter | ||
Comment 2•5 years ago
|
||
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 anLSObject
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 usesLSObject::mStoragePrincipalInfo
which points toP1
(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
forL1
points toP1
. So theNotifyChange()
function will be invoked withP1
and we'll attach it to thestorage
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 checkP1
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...)
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Comment 3•5 years ago
|
||
- 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 usesLSObject::mStoragePrincipalInfo
which points toP1
(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.
Assignee | ||
Comment 4•5 years ago
|
||
This behavior is similar to indexedDB and other QuotaManager-related APIs.
Reporter | ||
Comment 5•5 years ago
|
||
And what about the other problems? :-)
Assignee | ||
Comment 6•5 years ago
|
||
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 anLSObject
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 usesLSObject::mStoragePrincipalInfo
which points toP1
(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
forL1
points toP1
. So theNotifyChange()
function will be invoked withP1
and we'll attach it to thestorage
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 checkP1
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.
Reporter | ||
Comment 7•5 years ago
|
||
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 9•5 years ago
|
||
bugherder |
Description
•