1878577 - (CVE-2024-4767) Private repository not removed on browser shutdown when PBM autostart is enabled

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, defect, P3)

Description

[Giorgio Maone [:ma1]]

Originally reported by a 김도훈_5791 to the Tor Project (tracked by this issue).

If a website uses IndexDB in a private browsing window, the storage files are encrypted with a key kept in RAM only, and put in a directory named private.
This directory gets removed as soon as the PB window is closed.
Unfortunately, if the browser.privatebrowsing.autostart preference is true (Tor Browser's default) the private directory and its content are kept in place after browser's shutdown.

Steps to reproduce:

1. On a clean Firefox profile, set browser.privatebrowsing.autostart to true and restart the browser
2. Navigate to https://people.torproject.org/~ma1/test/idb/
3. Verify that a $PROFILE_DIR/storage/private has been created with encrypted db files inside
4. Quit Firefox

Expected result: the $PROFILE_DIR/storage/private directory doesn't exist anymore
Actual result: the $PROFILE_DIR/storage/private is still there with its content

Actually retrieving data from the encrypted database or even knowing which website it belongs requires searching a frozen snapshot of the RAM for the decryption keys, kept there during the browsing session. Furthermore you could always end in this situation if the browser just crashes, or if a determined and resourceful attacker manages to recover the encrypted files from the persistent storage after deletion.
Nonetheless I think we want this deletion policy to work as reliably as possible in global PBM as well, for consistency sake if nothing else.

Comment 1

[Giorgio Maone [:ma1]]

Adding the original reporter [e-mail] aka 김도훈_5791.

Comment 3

[Jan Varga [:janv]]

This is probably caused by not broadcasting the "last-pb-context-exiting" topic when the last private browsing window is closed.
The other option is that it's broadcasted too late during shutdown.

Comment 4

[Harveer Singh]

I think it's probably not broadcasting the message at all in case of autostart=true. I will further take a look into it.

Comment 5

[Harveer Singh]

Seems like we use 'last-pb-context-exited' topic to purge any PBM data and I see the notification is not sent in case we are in permanent private browsing mode https://searchfox.org/mozilla-central/rev/26dd94a024f21971e539a8650c2f177323c98fe2/docshell/base/CanonicalBrowsingContext.cpp#107

Comment 6

[Paul Zühlcke [:pbz]]

(In reply to Harveer Singh from comment #5)

Seems like we use 'last-pb-context-exited' topic to purge any PBM data and I see the notification is not sent in case we are in permanent private browsing mode https://searchfox.org/mozilla-central/rev/26dd94a024f21971e539a8650c2f177323c98fe2/docshell/base/CanonicalBrowsingContext.cpp#107

I think the message is currently only used for clearing on PBM session end during runtime. For PBM autostart that doesn't apply. Dispatching this message on shutdown could lead to issues where clearing races with shutdown or shutdown gets slower because clearing needs to happen on consumer side first. QM is special here because it stores PBM data on disk - where other implementations use memory only and application shutdown automatically cleans up state.

Comment 7

[Jan Varga [:janv]]

Ok, so we need a more flexible solution for clearing PBM data on disk. I suggest to inspect all the places where the message is currently used/broadcasted and add explicit nsIQuotaManagerService::ClearStoragesForPrivateBrowsing call. We can then remove the observer from QM.

Comment 8

[Paul Zühlcke [:pbz]]

Alternatively, could you on shutdown (I assume you have a shutdown blocker for QM), check if PBM autostart is enabled and then trigger the clearing yourself, just like the observer message would? I'm not familiar with your implementation so unsure how easy / hard that is.

Comment 9

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

(mid-aired)

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

I think the message is currently only used for clearing on PBM session end during runtime. For PBM autostart that doesn't apply. Dispatching this message on shutdown could lead to issues where clearing races with shutdown or shutdown gets slower because clearing needs to happen on consumer side first. QM is special here because it stores PBM data on disk - where other implementations use memory only and application shutdown automatically cleans up state.

That's a good point about races. The observer notification mechanism is somewhat a legacy approach at this point that should be a shutdown blocker. Arguably it would be better if nsIClearDataService and/or Sanitizer.sys.mjs were involved. For example sanitizeOnShutdown is doing basically the same thing but for a different configuration. Alternately, QuotaManager can clear PrivateBrowsing data automatically at its shutdown if it has not been explicitly cleared. This might be a good failsafe no matter what, but it seems preferable for QM to not be encoding the policy logic for permanent PBM.

In terms of delayed shutdown, there's no getting around that we need to clear the QuotaManager data here, although I believe our background task will help with this on non-OSX platforms. Although we will automatically clear the "private" directory at next startup as part of our behavior for handling crashes, our on-disk representation's resistance to meta-data fingerprinting (timestamps, file sizes, file sequence for IDB) isn't sufficient to leave the data around at browser shutdown even though the encryption keys will no longer be available to decrypt the actual data.

Comment 10

[Jan Varga [:janv]]

Sure, it will be deleted after next startup, but I assumed that the reporter expects the data to be cleared during shutdown.

Comment 11

[Jan Varga [:janv]]

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

Alternatively, could you on shutdown (I assume you have a shutdown blocker for QM), check if PBM autostart is enabled and then trigger the clearing yourself, just like the observer message would? I'm not familiar with your implementation so unsure how easy / hard that is.

That might work as well.

Comment 12

[Andrew McCreight [:mccr8]]

I'll mark this sec-moderate because it is sort of a private browsing leak, but it is on the low end of things, so maybe it is more of a sec-low?

Comment 13

[Harveer Singh]

Attachment: Bug 1878577: Ensure private storage repo is always cleaned up, even with PBM autostart.r=janv

Comment 14

[Harveer Singh]

Attachment: Bug 1878577: Removing redundant dummy.py and references.r=janv

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a0299d02e89
Ensure private storage repo is always cleaned up, even with PBM autostart.r=dom-storage-reviewers,janv

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2ab0989403c
Backed out changeset 3a0299d02e89 for causing marionette failures in test_registered_service_worker_after_restart CLOSED TREE

Comment 17

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

Backed out for failing Marionettes's dom/workers/test/marionette/test_service_workers_at_startup.py:

https://hg.mozilla.org/integration/autoland/rev/e2ab0989403cfb5dbb9d91a2b82c8d9ef787baca

Push with failures
Failure log

[task 2024-04-04T23:16:54.794Z] 23:16:54     INFO -  1712272614794	Marionette	DEBUG	2 -> [0,117,"Marionette:SetContext",{"value":"content"}]
[task 2024-04-04T23:16:54.794Z] 23:16:54     INFO -  1712272614794	Marionette	DEBUG	2 <- [1,117,null,{"value":null}]
[task 2024-04-04T23:16:54.795Z] 23:16:54     INFO -  1712272614794	Marionette	DEBUG	2 -> [0,118,"Marionette:GetContext",{}]
[task 2024-04-04T23:16:54.795Z] 23:16:54     INFO -  1712272614794	Marionette	DEBUG	2 <- [1,118,null,{"value":"content"}]
[task 2024-04-04T23:16:54.795Z] 23:16:54     INFO -  1712272614795	Marionette	DEBUG	2 -> [0,119,"Marionette:SetContext",{"value":"content"}]
[task 2024-04-04T23:16:54.795Z] 23:16:54     INFO -  1712272614795	Marionette	DEBUG	2 <- [1,119,null,{"value":null}]
[task 2024-04-04T23:16:54.795Z] 23:16:54     INFO -  1712272614795	Marionette	DEBUG	2 -> [0,120,"WebDriver:GetPageSource",{}]
[task 2024-04-04T23:16:54.798Z] 23:16:54     INFO -  1712272614798	RemoteAgent	TRACE	WebDriverProcessData actor created for PID 2246
[task 2024-04-04T23:16:54.799Z] 23:16:54     INFO -  1712272614798	Marionette	TRACE	[11] MarionetteCommands actor created for window id 6442450945
[task 2024-04-04T23:16:54.800Z] 23:16:54     INFO -  1712272614800	Marionette	DEBUG	2 <- [1,120,null,{"value":"<html><head>\n        <title>Install Service Worker</title>\n    </head>\n    <body>\n        <script>\n            navigator.serviceWorker.register(\"serviceworker.js\");\n        </script>\n    \n\n</body></html>"}]
[task 2024-04-04T23:16:54.801Z] 23:16:54     INFO -  1712272614800	Marionette	DEBUG	2 -> [0,121,"Marionette:SetContext",{"value":"content"}]
[task 2024-04-04T23:16:54.801Z] 23:16:54     INFO -  1712272614800	Marionette	DEBUG	2 <- [1,121,null,{"value":null}]
[task 2024-04-04T23:16:54.849Z] 23:16:54     INFO - TEST-UNEXPECTED-ERROR | dom/workers/test/marionette/test_service_workers_at_startup.py ServiceWorkerAtStartupTestCase.test_registered_service_worker_after_restart | marionette_driver.errors.TimeoutException: Timed out after 5.2 seconds with message: Service worker not successfully installed
[task 2024-04-04T23:16:54.849Z] 23:16:54     INFO - Traceback (most recent call last):
[task 2024-04-04T23:16:54.849Z] 23:16:54     INFO -   File "/opt/worker/tasks/task_171227180560693/build/venv/lib/python3.11/site-packages/marionette_harness/marionette_test/testcases.py", line 179, in run
[task 2024-04-04T23:16:54.849Z] 23:16:54     INFO -     self.setUp()
[task 2024-04-04T23:16:54.849Z] 23:16:54     INFO -   File "/opt/worker/tasks/task_171227180560693/build/tests/marionette/tests/dom/workers/test/marionette/test_service_workers_at_startup.py", line 18, in setUp
[task 2024-04-04T23:16:54.849Z] 23:16:54     INFO -     self.install_service_worker("serviceworker/install_serviceworker.html")
[task 2024-04-04T23:16:54.849Z] 23:16:54     INFO -   File "/opt/worker/tasks/task_171227180560693/build/tests/marionette/tests/dom/workers/test/marionette/service_worker_utils.py", line 15, in install_service_worker
[task 2024-04-04T23:16:54.849Z] 23:16:54     INFO -     Wait(self.marionette).until(
[task 2024-04-04T23:16:54.849Z] 23:16:54     INFO -   File "/opt/worker/tasks/task_171227180560693/build/venv/lib/python3.11/site-packages/marionette_driver/wait.py", line 153, in until
[task 2024-04-04T23:16:54.849Z] 23:16:54     INFO -     raise errors.TimeoutException(

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30cced675b7a
Ensure private storage repo is always cleaned up, even with PBM autostart.r=dom-storage-reviewers,janv

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/30cced675b7a

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6a2c5d63a70
Removing redundant dummy.py and references.r=janv

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/f6a2c5d63a70

Comment 22

[Donal Meehan [:dmeehan]]

Harveer, could you please add an esr115 uplift request on this? It will need a patch rebased onto esr115

Comment 23

[Harveer Singh]

Comment on attachment 9379250 [details]
Bug 1878577: Ensure private storage repo is always cleaned up, even with PBM autostart.r=janv

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Since we shipped IndexedDB private browsing support in ESR 115, so this patch fix becomes important for 115 as it ensures that private repository always gets cleaned up on shutdown.
• User impact if declined: There would be no visible user impact.
• Fix Landed on Version: 126
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): there are not any complicated changes in this patch so there's less chance of causing regressions.

Comment 24

[Harveer Singh]

Original patch: https://phabricator.services.mozilla.com/D201246 was updated to make it suitable for ESR 115 branch.

Comment 25

[Donal Meehan [:dmeehan]]

Harveer, it's not standard practice to update the original Phabricator revision.

Could you attach a new revision rebased onto esr115? This can either be in Phabricator or in a patch attached to the bug

Comment 26

[Harveer Singh]

I was told to do it like that. okay, I will attach it here then.

Comment 27

[Harveer Singh]

Attachment: esr115.patch

Comment 28

[Harveer Singh]

Attachment: Bug 1878577: Ensure private storage repo is always cleaned up, even with PBM autostart.r=janv

Comment 29

[Harveer Singh]

I have it both ways now. patch file and a standalone phab revision.

Comment 30

[Donal Meehan [:dmeehan]]

Comment on attachment 9379250 [details]
Bug 1878577: Ensure private storage repo is always cleaned up, even with PBM autostart.r=janv

Approved for 115.11esr.

Thanks for adding a rebased patch.
Not sure where/how it's documented to edit the original patch. Please correct it if it's part of some internal doc to your team.

Comment 31

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/4965e5dd8803

Comment 32

[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 33

[Jesse Schwartzentruber (:truber)]

Attachment: advisory.txt

Comment 34

[Daniel Veditz [:dveditz]]

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons