1814733 - (CVE-2023-25750) `caches.open("foo")` doesn't throw on partitioned third-party iframe on PBM

Status: VERIFIED FIXED

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

Description

[Kagami Rosylight [:saschanaz] (they/them)]

Steps:

0. Turn off dom.caches.hide_in_pbmode.enabled (when bisecting)
1. Open Private Browsing Mode
2. Access https://faz.net
3. Allow cookie access if prompted
4. Scroll down and play an embedded YT video

Expected: No entry should appear with ^privateBrowsing=1 in /storage/default in the profile directory.
Actual: It does.

Mozregression shows https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a7402247dc7e4b5c0a4a173aa08545075a03f481&tochange=cd3007f4be5d49ea21cadf77ef79f8d0aa798b3d, and from there bug 1758745 looks most relevant.

Comment 1

[Kagami Rosylight [:saschanaz] (they/them)]

Hi :twisniewski, can you take a look? Thanks!

Comment 2

[Kagami Rosylight [:saschanaz] (they/them)]

  StorageAccess access = mGlobal->GetStorageAccess();
  return access > StorageAccess::ePrivateBrowsing ||
         (StaticPrefs::
              privacy_partition_always_partition_third_party_non_cookie_storage() &&
          ShouldPartitionStorage(access));

This still allows storage access in PBM, and I don't think we should do it - yet.

Comment 3

[Thomas Wisniewski [:twisniewski]]

:pbz, what do you think?

Comment 4

[Paul Zühlcke [:pbz]]

If I understand this correctly the change from bug 1758745 was meant to allow partitioned cache storage only if always partioning is enabled, but had the unintended side effect of also allowing it for PBM?
I'm curious why we didn't allow partitioned storage here in the first place, even before bug 1758745. Was this because of concerns with unpartitioning the cache (e.g. via storage access api, etc)?

Looks like mGlobal->GetStorageAccess() should return ePrivateBrowsing in the PBM case and not ePartitionForeignOrDeny? In that case the bug isn't actually this check but somewhere else where we compute the storage access flag. What's the source of truth for determining whether this storage should be enabled in PBM?

Comment 5

[Benjamin VanderSloot [:bvandersloot]]

If I understand this correctly the change from bug 1758745 was meant to allow partitioned cache storage only if always partioning is enabled, but had the unintended side effect of also allowing it for PBM?
More like it was meant to prevent access to unpartitioned cache storage if always partitioning is enabled, IIRC.

Looks like mGlobal->GetStorageAccess() should return ePrivateBrowsing in the PBM case and not ePartitionForeignOrDeny?
I think it is actually in the second half of the OR in what Kagami pointed out. That gives partitioning values a pass despite being < ePrivateBrowsing
We need to reconsider the total ordering of that enum [1] and the implications. Like should partition*OrDeny be < eDeny?

I think we can revert this diff to fix this regression: https://searchfox.org/mozilla-central/diff/d1c82fe05fb6d461afea6d7d46754065cbd31723/dom/cache/CacheStorage.cpp#565. This is what Kagami pointed to in comment 2.

[1] https://searchfox.org/mozilla-central/source/toolkit/components/antitracking/StorageAccess.h#28-30

Comment 6

[Paul Zühlcke [:pbz]]

(In reply to Benjamin VanderSloot from comment #5)

If I understand this correctly the change from bug 1758745 was meant to allow partitioned cache storage only if always partioning is enabled, but had the unintended side effect of also allowing it for PBM?
More like it was meant to prevent access to unpartitioned cache storage if always partitioning is enabled, IIRC.

Looks like mGlobal->GetStorageAccess() should return ePrivateBrowsing in the PBM case and not ePartitionForeignOrDeny?
I think it is actually in the second half of the OR in what Kagami pointed out. That gives partitioning values a pass despite being < ePrivateBrowsing
We need to reconsider the total ordering of that enum [1] and the implications. Like should partition*OrDeny be < eDeny?

I think we can revert this diff to fix this regression: https://searchfox.org/mozilla-central/diff/d1c82fe05fb6d461afea6d7d46754065cbd31723/dom/cache/CacheStorage.cpp#565. This is what Kagami pointed to in comment 2.

[1] https://searchfox.org/mozilla-central/source/toolkit/components/antitracking/StorageAccess.h#28-30

Yes that should fix the immediate regression, but wouldn't that also mean we don't support partitioned cache storage?

Comment 7

[Benjamin VanderSloot [:bvandersloot]]

Ugh, right. I think part of the issue is that partitioning should really be orthogonal to the other values of the enum.

Looks like mGlobal->GetStorageAccess() should return ePrivateBrowsing in the PBM case and not ePartitionForeignOrDeny?

I don't think so... Because then I think it will effectively disable storage partitioning in PBM because the result of mGlobal->GetStorageAccess() is passed into StoragePartitioningEnabled() (which looks for StorageAccess::ePartitionForeignOrDeny)to perform partitioning checks pretty often. Maybe I'm misremembering the semantic here though. Thoughts @pbz?

Comment 8

[Paul Zühlcke [:pbz]]

Ugh, right. I think part of the issue is that partitioning should really be orthogonal to the other values of the enum.

Yes - the current state makes it really confusing. When debugging this with the STR above for some reason I get -1 for storage access, which also seems unexpected.

It's really hard to trace where this is coming from. The storage access value is retrieved form the global which in this case should be WorkerGlobalScope I believe: https://searchfox.org/mozilla-central/rev/a7156afbfa575f12f60b1c8bf099d547c29bcadf/dom/workers/WorkerScope.cpp#287
This value comes from WorkerPrivate StorageAccess which gets it from loadInfo here: https://searchfox.org/mozilla-central/rev/a7156afbfa575f12f60b1c8bf099d547c29bcadf/dom/workers/WorkerPrivate.h#867
loadInfo has the storage access field populated here: https://searchfox.org/mozilla-central/rev/a7156afbfa575f12f60b1c8bf099d547c29bcadf/dom/workers/WorkerPrivate.cpp#2925
Looking into that method I can see a fallback here that returns ePrivateBrowsing: https://searchfox.org/mozilla-central/rev/a7156afbfa575f12f60b1c8bf099d547c29bcadf/toolkit/components/antitracking/StorageAccess.cpp#242
However that fallback doesn't seem to be used here, so we go into the regular storage access check. Time to debug that....

Comment 9

[Benjamin VanderSloot [:bvandersloot]]

Looking into that method I can see a fallback here that returns ePrivateBrowsing: https://searchfox.org/mozilla-central/rev/a7156afbfa575f12f60b1c8bf099d547c29bcadf/toolkit/components/antitracking/StorageAccess.cpp#242

That is just in the event that the window doesn't have a document, which it should in our case.

Instead we get all the way to the internal check on the window where we note that it is private browsing (https://searchfox.org/mozilla-central/source/toolkit/components/antitracking/StorageAccess.cpp#88) but then only return that if !StorageDisabledByAntiTracking(...): https://searchfox.org/mozilla-central/source/toolkit/components/antitracking/StorageAccess.cpp#129... I think

Comment 10

[Paul Zühlcke [:pbz]]

The storage access value returned is ePartitionTrackersOrDeny for the YouTube embed.

I've tested checking for PBM in CacheStorage::HasStorageAccess explicitly and denying storage access like this:

  if(nsIPrincipal* principal = mGlobal->PrincipalOrNull()) {
    if(!principal->IsSystemPrincipal() && principal->GetPrivateBrowsingId() != nsIScriptSecurityManager::DEFAULT_PRIVATE_BROWSING_ID) {
       printf("Denying dom cache storage access in PBM\n");
    return false;
    }
  }

That seems to work. However I'm not sure if this is the proper way to do it. The storage access checks don't have a way to return a combination of "partition, but in PBM deny".

What seems odd to me is that I don't see any partition key in the dom cache files. Even in normal browsing dom cache from YouTube looks unpartitioned, even though it's a third-party. Perhaps I'm just missing where the partition key is stored? Do we just not support it yet?

Here are my open questions:

• Should DOM Cache be partitioned? If so I don't think that's working properly at the moment?
• Why does the storage access check currently return ePartitionTrackersOrDeny and not ePartitionForeignOrDeny or ePrivateBrowsing?
• Which component decides whether a storage is enabled in PBM? Should that be determined in StorageAccess.cpp or in the storage code itself (since it happens on a case-by-case basis)?

Tim, could you please help answer some of these questions when you're back? Much appreciated!

Comment 11

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

(In reply to Paul Zühlcke [:pbz] from comment #10)

• Which component decides whether a storage is enabled in PBM? Should that be determined in StorageAccess.cpp or in the storage code itself (since it happens on a case-by-case basis)?

I've proposed introducing an explicit StorageKey abstraction in bug 1776271 which I think would be a reasonable place to implement all policy-related decision-making. I'll write a little more on this in that bug now, as I think I've talked about this before but haven't captured that in the bug.

Comment 12

[Tim Huang[:timhuang]]

(In reply to Paul Zühlcke [:pbz] from comment #10)

What seems odd to me is that I don't see any partition key in the dom cache files. Even in normal browsing dom cache from YouTube looks unpartitioned, even though it's a third-party. Perhaps I'm just missing where the partition key is stored? Do we just not support it yet?

Here are my open questions:

• Should DOM Cache be partitioned? If so I don't think that's working properly at the moment?

The DOM Cache should be partitioned in third-party contexts, see [1]. And we only do this if Always Partitioning Storage is enabled; DOM Cache is disabled in a third-party context if APS is disabled. Also, we have a test to ensure this when APS is enabled.

• Why does the storage access check currently return ePartitionTrackersOrDeny and not ePartitionForeignOrDeny or ePrivateBrowsing?

It's because youtube.com is considered a content tracker if the level 2 list is enabled.

• Which component decides whether a storage is enabled in PBM? Should that be determined in StorageAccess.cpp or in the storage code itself (since it happens on a case-by-case basis)?

It should be decided by the storage module itself by the current design. The StorageAccess.cpp provides a hint for the storage module to know which access type should be used, and the storage modules can decide their behavior according to the storage access.

[1] https://privacycg.github.io/storage-partitioning/

Comment 13

[Paul Zühlcke [:pbz]]

Attachment: Bug 1814733 r=timhuang!,#dom-storage-reviewers!

Comment 15

[Paul Zühlcke [:pbz]]

Looks like we can land this in Fx112 and uplift to Fx111. Moving faster than that seems risky.

Here is what we need before we can land the patch:

• sec-rating pending (I don't think that's still happening in this cycle?)
• Clarify the remaining review questions, Andrew could you please help with that? See https://phabricator.services.mozilla.com/D168976#inline-932882
• Consider if we need to do any cleanup. Does the cache storage persist or does it expire at some point automatically? If not we probably need some migration code that clears ^privateBrowsingId=1 entries from disks.

NI to Andrew for the latter 2 questions. Thank you!

Comment 16

[Daniel Veditz [:dveditz]]

What are the web-detectable impacts of this? Clearly it's writing things to disk in private mode which is a big failure for that, but more as a privacy matter than a "security" one: might call that sec-moderate. But if this leads to past private visits being web-detectable in non-private sessions or in future private sessions we should call this sec-high

Comment 17

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: poc.html

I fear it's sec-high in that case.

1. Download the attachment as index.html.
2. Run it locally, e.g. by python -m http.server
3. Open http://localhost:8000/index.html in PBM and see the console log. You'll see one error and one array, which should be empty initially.
4. Refresh and see the array became ["visited"].
5. Close the window and open the page again in PBM
6. See what the log says, it shows ["visited"].

Edit: Oh, in PBM I mean. Modified the steps.

Comment 18

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

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

Comment 19

[Paul Zühlcke [:pbz]]

(In reply to Kagami [:saschanaz] from comment #17)

Created attachment 9316714 [details]
poc.html

I fear it's sec-high in that case.

1. Download the attachment
2. Run it locally, e.g. by python -m http.server
3. Open http://localhost:8000/ in PBM and see the console log. You'll see one error and one array, which should be empty initially.
4. Refresh and see the array became ["visited"].
5. Close the window and open the page again in PBM
6. See what the log says, it shows ["visited"].

Edit: Oh, in PBM I mean. Modified the steps.

I can't reproduce your poc, I get Uncaught DOMException: The operation is insecure. index.html:11 in PBM. Both with localhost:8080 and another test origin origin1.com:8080. Am I doing something wrong?

Comment 20

[Paul Zühlcke [:pbz]]

Clearing NI for :asuth, we discussed this on Slack. The attached patch is r+. However we need to update the QuotaManager init to clean up anything that has a privateBrowsingId set. I'll add another patch for that.

Comment 21

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

(In reply to Paul Zühlcke [:pbz] from comment #19)

I can't reproduce your poc, I get Uncaught DOMException: The operation is insecure. index.html:11 in PBM. Both with localhost:8080 and another test origin origin1.com:8080. Am I doing something wrong?

It's possible this could be related to bug 1817893 about the Caches API continuing to have its own redundant SecureContext logic which I filed while digging into the existing control flow paths as part of this bug.

Comment 22

[Kagami Rosylight [:saschanaz] (they/them)]

(In reply to Paul Zühlcke [:pbz] from comment #19)

I can't reproduce your poc, I get Uncaught DOMException: The operation is insecure. index.html:11 in PBM. Both with localhost:8080 and another test origin origin1.com:8080. Am I doing something wrong?

You mean you see two errors?

BTW, I edited step 1 as it assumes index.html in localhost and 127.0.0.1.

Comment 23

[Paul Zühlcke [:pbz]]

(In reply to Kagami [:saschanaz] from comment #22)

(In reply to Paul Zühlcke [:pbz] from comment #19)

I can't reproduce your poc, I get Uncaught DOMException: The operation is insecure. index.html:11 in PBM. Both with localhost:8080 and another test origin origin1.com:8080. Am I doing something wrong?

You mean you see two errors?

BTW, I edited step 1 as it assumes index.html in localhost and 127.0.0.1.

Yes, two errors. I tested all kinds of domain combinations, some trusted like localhost and some untrusted like origin1.com
Do I need to set a pref for this to work, or do I need to serve the test page via HTTPS?

Comment 24

[Paul Zühlcke [:pbz]]

Update on the patch: I'm currently discussing with :asuth via DM whether we want to land the fix and the cleanup separately. And how we need to do the cleanup.

Comment 25

[Jan Varga [:janv]]

I discussed this with :asuth as well. I think it's ok to land the existing fix first (without the cleanup for now). There's also patch https://phabricator.services.mozilla.com/D86562 which introduces clearing of all origin directories on disk which contain the privateBrowsindId attribute. That should be enough for now. I would wait with the cleanup patch because there's also https://phabricator.services.mozilla.com/D161618 in the works which will map private browsing origins to UUIDs. It would be better to finish the cleanup patch after that.

Comment 26

[Paul Zühlcke [:pbz]]

Comment on attachment 9316161 [details]
Bug 1814733 r=timhuang!,#dom-storage-reviewers!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: From looking a the patch it's relatively easy to see that we allowed storage access for CacheStorage in PBM before. The fact that this storage persists across private browsing sessions however is not obvious.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: release, beta
• If not all supported branches, which bug introduced the flaw?: Bug 1758745
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Should be a simple uplift, I don't expect merge conflicts with Fx111 and Fx110.x (given we could still uplift this late)
• How likely is this patch to cause regressions; how much testing does it need?: Rather unlikely since the change is quite small, has automated test coverage and can be manually verified.
• Is Android affected?: Yes

Comment 27

[Paul Zühlcke [:pbz]]

Thanks Jan! I agree so I created a sec-approval request for the attached patch (without the cleanup/migration code).

Comment 28

[Kagami Rosylight [:saschanaz] (they/them)]

(In reply to Paul Zühlcke [:pbz] from comment #23)

Yes, two errors. I tested all kinds of domain combinations, some trusted like localhost and some untrusted like origin1.com
Do I need to set a pref for this to work, or do I need to serve the test page via HTTPS?

No, I just run python -m http.server and open localhost:3000 on a clean profile and then I can reproduce it. How are you running it?

Comment 29

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9316161 [details]
Bug 1814733 r=timhuang!,#dom-storage-reviewers!

Approved to request uplift and land.

Comment 30

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

To be clear, I think it's okay to land the test alongside the patch this time.

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

r=timhuang,dom-storage-reviewers,asuth
https://hg.mozilla.org/integration/autoland/rev/b8a8b74dbdd01cade5a6aa258ce75f922969b639
https://hg.mozilla.org/mozilla-central/rev/b8a8b74dbdd0

Comment 32

[Paul Zühlcke [:pbz]]

(In reply to Jan Varga [:janv] from comment #25)

I discussed this with :asuth as well. I think it's ok to land the existing fix first (without the cleanup for now). There's also patch https://phabricator.services.mozilla.com/D86562 which introduces clearing of all origin directories on disk which contain the privateBrowsindId attribute. That should be enough for now. I would wait with the cleanup patch because there's also https://phabricator.services.mozilla.com/D161618 in the works which will map private browsing origins to UUIDs. It would be better to finish the cleanup patch after that.

To clarify, are the patches you linked sufficient to clean up this leak or will we need another patch to do that?

Comment 33

[Jan Varga [:janv]]

(In reply to Paul Zühlcke [:pbz] from comment #32)

(In reply to Jan Varga [:janv] from comment #25)

I discussed this with :asuth as well. I think it's ok to land the existing fix first (without the cleanup for now). There's also patch https://phabricator.services.mozilla.com/D86562 which introduces clearing of all origin directories on disk which contain the privateBrowsindId attribute. That should be enough for now. I would wait with the cleanup patch because there's also https://phabricator.services.mozilla.com/D161618 in the works which will map private browsing origins to UUIDs. It would be better to finish the cleanup patch after that.

To clarify, are the patches you linked sufficient to clean up this leak or will we need another patch to do that?

https://phabricator.services.mozilla.com/D86562 is about to land. It introduces clearing of all origin directories with privateBrowsingId=1 after a private browsing session is finished. So there's only one theoretical case when the cleanup wouldn't be done, when the user never starts a private browsing session again. I think this should be ok for now, but we will find a way how to cleanup stuff which doesn't depend on finishing a private browsing session once we complete https://phabricator.services.mozilla.com/D161618 because we don't know yet how exactly origin directories will look like for private browsing after those changes.

Comment 34

[Paul Zühlcke [:pbz]]

Agreed, that seems sufficient for now. Thanks!

Comment 35

[:Gijs (he/him)]

I'm a bit confused about the state of this bug - it's RESOLVED FIXED in 112 but both 112 and 111 are still marked as affected. Should 112 be marked fixed, and should there be an uplift request for 111 here?

Comment 36

[Paul Zühlcke [:pbz]]

112 should be fixed. I'll create an uplift request now, just wanted this to land on central first.

Comment 37

[Paul Zühlcke [:pbz]]

Comment on attachment 9316161 [details]
Bug 1814733 r=timhuang!,#dom-storage-reviewers!

Beta/Release Uplift Approval Request

• User impact if declined: Private Browsing Mode leakage (also across PBM sessions); sec-high
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Smaller code change which effectively restores original behavior of blocking storage access in PBM. Has automated test coverage.
• String changes made/needed:
• Is Android affected?: Yes

Comment 38

[Donal Meehan [:dmeehan]]

Comment on attachment 9316161 [details]
Bug 1814733 r=timhuang!,#dom-storage-reviewers!

Approved for 111.0b7

Comment 39

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4143af401e11

Comment 40

[Hani Yacoub, Desktop QA]

Verified as fixed on macOS 11.6, Windows 10 x64 and on Ubuntu 20.04 x64 on both Firefox Nightly112.0a1 (2023-03-01) and Firefox 111.0b7.

Comment 43

[Frederik Braun [:freddy]]

Attachment: advisory.txt