1794508 - (CVE-2022-45417) `GetStorageAccess() == ePrivateBrowsing` in ServiceWorkersEnabled() does not always detect private browsing mode

Status: VERIFIED FIXED

(Core :: DOM: Service Workers, defect, P1)

Description

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

ePartitionForeignOrDeny can appear in some third party iframes and thus cause detection failure.

We'll have to update dom/serviceworkers/test/test_privateBrowsing.html to cover that.

Comment 1

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

Attachment: Bug 1794508 - Replace GetStorageAccess with IsInPrivateBrowsing r=asuth

Comment 2

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

Comment on attachment 9297913 [details]
Bug 1794508 - Replace GetStorageAccess with IsInPrivateBrowsing r=asuth

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch changes how we detect private browsing. The previous way was faulty but it's not clear from the code how those two are different.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: 104
• If not all supported branches, which bug introduced the flaw?: Bug 1776109
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: This just reverts the change from bug 1776109 part 2 patch, which was a refactoring without new features. And its child patch is already reverted, so any regression is not likely.
• Is Android affected?: Yes

Comment 3

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

I'm going to attempt to summarize this situation which is discussed and investigated in much greater depth at https://mozilla.slack.com/archives/C4S6616E9/p1665422205062729 (although there's a process of discovery there).

:pascalc I see you are listed as the release owner for Firefox 106 at https://fx-trains.herokuapp.com/release/?version=106 so I wanted to bring this to your attention as it seems like if we don't land https://phabricator.services.mozilla.com/D159002 into the beta we may want it to be able to get into a dot release.

General Problem Statement

On release, beta, and nightly, it's possible for ServiceWorkers to be persisted to disk in private browsing mode for origins where ePartitionForeignOrDeny or ePartitionTrackersOrDeny are returned from the StorageAllowedFor* methods in StorageAccess.h. I don't fully understand what situations these are, but ideally :timhuang can help contextualize when this would be, I've set a needinfo.

The concern is that we have never intended to allow ServiceWorkers to be persisted to disk in private browsing mode because our threat model has generally been "data does not get written to disk in private browsing mode"[1]. When this happens, an origin the user has indirectly visited will have its name written to disk in the clear as well as the first-party top-level origin name and any data related to the script will be written in the clear, albeit compressed. It seems likely that the ServiceWorker would fail to be able to write any data to the Cache API storage though, so it should be just the ServiceWorker that gets installed.

This bug should stop new ServiceWorkers from being installed.

What happens to profiles where the bug happened? / Follow-ups

It's quite possible that ServiceWorkers installed via this mechanism will not be cleaned up and will stick around until the user cleans them up.

Specifically, as I understand it, Private Browsing cleanup happens via the "last-pb-context-exited" observer notification. I do not currently see any connective linkage between this observer notification and nsIClearDataService or sanitizer.jsm which could serve as a back-stop, but I hopefully am missing something[2]. Such glue would potentially look like a call to nsIClearDataService.deleteDataFromOriginAttributesPattern specifying a pattern specifying privateBrowsingId: 1. This would potentially be a reasonable thing for us to add if it's not already there. In particular, this should clean up the origin directory as well as the registration.

In terms of the simplest way to clean this up, we can add logic similar to the web-extension ServiceWorker purging logic we already implement which ensures that we ignore registrations we don't want and purge their related cache. I think there is actually an oversight in that logic, however, in that I think we're failing to call MaybeSendUnregister which will actually cause the registration to be dropped from the ServiceWorkerRegistrar's copy of the registrations. Without that, although we won't show the registration in about:serviceworkers, it would still be persistent on disk. This will remove the serviceworker but I do not believe it will be sufficient to clear the QuotaManager-managed origin directory.

An option to ensure that the origin directory gets cleaned up (separate from the registration) is to ensure that QuotaManager's initialization notices any directories with a private browsing id and purges them. This would not be sufficient to remove the registration from the serviceworker.txt file on its own.

Arguably implementing either of these clean-ups does make it more clear what's going on, so it might make sense for clean-up logic to land in a second set of fixes in the future if we're concerned about bad actors intentionally trying to install ServiceWorkers in private browsing mode. It's not clear if this is actually a concern though, compared to cleaning up the private browsing byproducts on disk. That said, the reversion in https://phabricator.services.mozilla.com/D159002 is extremely straightforward and safe to land and uplift all over the place, whereas most of these potential cleanup fixes want testing, etc. that we don't have ready to go yet.

1: Note that we are moving towards allowing encrypted data to be written to disk as long as all file paths also obscure the origins at play.
2: Like how does our IndexedDB private-browsing support clean up?

Comment 4

[Tim Huang[:timhuang]]

The ePrivateBrowsing storage access represents that the storage is allowed to access in the private browsing context. So, if the context has storage access, we will return ePrivateBrowsing.

However, we don't have a concept that storage is partitioned in private browsing context. So, the StorageAllowedFor* methods will return ePartition* when the third-party iframe doesn't have storage access regardless of whether it's in the private window. So, we might need to introduce this concept so that storage can handle partitioning in the private window correctly, then we can use storage access as the single source of truth. But, before we get to that point, we should still rely on the existing private window checks to decide the storage behavior in private windows.

FWIW, I think we should add tests to make sure this won't happen again. :)

Comment 5

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

I've been following up with the user who brought this to my attention and they indicated that there are indeed byproducts left on disk after Firefox shuts down because there is not an explicit sweep for permanent private browsing mode. Specific examples they gave from their testing:

PROFILE/storage/default/https+++learn-pwa-sw-registration.glitch.me^privateBrowsingId=1&partitionKey=%28https%2Cweb.dev%29
PROFILE/storage/default/https+++faststrip.apester.com^privateBrowsingId=1&partitionKey=%28https%2Cfilm.tv%29
PROFILE/storage/default/https+++vuiws.csb.app^privateBrowsingId=1&partitionKey=%28https%2Ccodesandbox.io%29

The 3rd (and I think 1st too? the 2nd is from their initial report) is from the reproduction they provided (and I posted in the slack thread) of https://codesandbox.io/s/vuiws?file=/src/index.js which when I attempted to reproduce on Firefox nightly I found that:

1. We 100% attempted to install the ServiceWorker
2. It failed to install because the SW through an exception which may have been due to an attempt to use the DOM Cache API which would fail due to a back-stop. However, the directory still gets created on disk.
3. I ended up with both the 1st and 3rd directories on my disk but no entries in serviceworker.txt or about:serviceworkers

We will definitely need cleanup logic in ServiceWorkerManager (SWM) and QuotaManager (QM) to ensure cleanups happen. In particular, the QM logic is necessary because serviceworker.txt did not end up with with registrations and also because even with the SWM deleting the cache for the script, it would not be sufficient to guarantee the QM directory gets wiped. These will be slightly larger fixes, but not huge; I could probably do them not on a sec-bug and avoid painting a super-obvious target as I think there is existing logic for both SWM and QM to remove things in this case that could be generalized and made to seem like a speculative backstop, but the patch would fundamentally need to reference privateBrowsingId to work.

Comment 6

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

Privacy Implications

Elaborating on the implications of comment 5 (and comment 3):

• Users using private browsing mode with tracking protection enabled to perform partitioning could have directories in their profile that contain both the origin name of the first-party / top-level site (as the partitionKey) they were visiting as well as the third-party site that triggered the attempted ServiceWorker installation.
• These directories will persist until the user takes explicit action to either clear the data for the sites on a per-site basis or clears all data. OR until we land cleanup fixes.
  • To reiterate, private browsing mode currently does not perform any failsafe cleanup pass via Sanitizer.jsm/nsIClearDataService as it assumes no data was written in the first place, so it is not trying to clear this data.

The intent behind private browsing mode is that we do not leave anything on disk that indicates the user's browsing history, whereas this does indicate parts of the user's browsing history, so this does need to be fixed with high priority.

Comment 7

[Tom Ritter [:tjr] (OOTO until mid-August)]

Comment on attachment 9297913 [details]
Bug 1794508 - Replace GetStorageAccess with IsInPrivateBrowsing r=asuth

Approved to land and uplift after beta branches

Comment 8

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

Comment on attachment 9297913 [details]
Bug 1794508 - Replace GetStorageAccess with IsInPrivateBrowsing r=asuth

Beta/Release Uplift Approval Request

• User impact if declined: Certain partitioned iframes can allow writing the name of the websites a user visited in Private Browsing Mode to the disk permanently. Comment #3 for more details.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Access https://codesandbox.io/s/vuiws?file=/src/index.js in Private Browsing mode

2. Check the page is correctly loaded and the console has some messages. (It should show an error at navigator.serviceWorker)
3. Check whether (profile directory)/storage/default got a directory with privateBrowsingId in its name. This currently must not happen.
4. about:serviceworkers should not show anything about codesandbox.io and *.csb.app

(Automatic tests will follow as a separate patch)

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This just reverts the change from bug 1776109 part 2 patch, which was a refactoring without new features. And its child patch is already reverted, so any regression is not likely.
• String changes made/needed:
• Is Android affected?: Yes

Comment 9

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

We might want to add a temporary cleaner as I already see directories with ^privateBrowsingId=1 from MDN and TripAdvisor in my profile. Thoughts?

Comment 10

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

Replace GetStorageAccess with IsInPrivateBrowsing r=asuth
https://hg.mozilla.org/integration/autoland/rev/a41265da4a2a475ed7cde77c9aa285ecdf413f3b
https://hg.mozilla.org/mozilla-central/rev/a41265da4a2a

Comment 11

[Donal Meehan [:dmeehan]]

Comment on attachment 9297913 [details]
Bug 1794508 - Replace GetStorageAccess with IsInPrivateBrowsing r=asuth

Approved for 107.0b2.

Comment 12

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cdce62059a85

Comment 13

[Ciprian Georgiu, Desktop QA]

Reproduced the bug using STR from comment 8, on an affected Nightly build (2022-10-10).

This is verified as fixed on latest Beta 107.0b2 and Nightly 108.0a1 (2022-10-19), across platforms: Win 10 x64, macOS 11 and Ubuntu 18.04 x64.

Comment 14

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

Attachment: Bug 1794508 - Add tests r=asuth

Comment 15

[Pascal Chevrel:pascalc]

We build our RC build on Monday, so that will be for 107.

Comment 16

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

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 17

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

I think it's okay for security issues to have patches separately landed, or am I wrong?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

I think it's okay for security issues to have patches separately landed, or am I wrong?

Yes, this is a situation where it's fine so long as you remember to land the test eventually :)

Comment 19

[Tom Ritter [:tjr] (OOTO until mid-August)]

Attachment: advisory.txt

It would be great if you could double-check this description.

Comment 20

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

I (and :asuth) haven't succeeded reproducing a living service worker being written to disk. "they would run again" in the second sentence may imply that they ever could run, and this is unconfirmed. I suggest omitting "again" there. (See also comment #5.)

"Service Workers being written" also sounds like that to me. Perhaps just "files being written", or "directories with domain names" if it can be more specific.

Comment 21

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

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

We might want to add a temporary cleaner as I already see directories with ^privateBrowsingId=1 from MDN and TripAdvisor in my profile. Thoughts?

I believe we ended up letting the falling edge of the normal PB exited observer notification clean these up with https://phabricator.services.mozilla.com/D175123 providing test coverage.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65a3b0af04d2
Add tests r=asuth,anti-tracking-reviewers,pbz

Comment 23

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/65a3b0af04d2