1795710 - CRLite remote-settings downloads aren't cleaned up

Status: VERIFIED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Daniel Veditz [:dveditz]]

It appears that the remote-settings CRLite downloads are not cleaned up after they're processed: I've got nearly 1Gb of -filter and -filter.stash files in my ~/Library/Caches/Firefox/Profiles/xxxxx/settings/security-state/cert-revocations/ directory going back to September 2020.

I don't see a lot of other remote-settings files left in this "cache" spot, and in particular I don't see addon blocklist files which I believe are now using crlite technology.

This came to light from a fenix issue about app data growing even after you clear all the stored user browsing data (history, cache, cookies, etc). A couple of participants identified the cert-revocations directories as huge (480Mb). On Android these files are in the regular profile, not in the separate caches directory. CRLite is not enabled on Fenix, and about:config only exists on Nightly, but the participants who identified this were running Mull, a Firefox-derived browser using Arkenfox user.js settings. [FWIW I don't think this is the original Fenix reporter's issue]

Comment 1

[Daniel Veditz [:dveditz]]

On slack, @leplatrem says "

The remote-settings client code seems to delete stuff when records are deleted on the server: https://searchfox.org/mozilla-central/rev/b4150d1c6fae0c51c522df2d2c939cf5ad331d4c/security/manager/ssl/RemoteSecuritySettings.jsm#411

Maybe we need to clean up on the remote-settings server side?

Comment 2

[Daniel Veditz [:dveditz]]

Looks like CRLite is using a now-deprecated remote-settings feature and running into the problems the addons blocklist did (see bug 1634127).

Comment 3

[John Schanck [:jschanck]]

Attachment: Bug 1795710 - part 1. avoid deprecated downloadToDisk function. r=keeler

Comment 4

[John Schanck [:jschanck]]

Attachment: Bug 1795710 - part 2. clean cert-revocations attachment cache. r=keeler

Depends on D159535

Comment 5

[Rob Wu [:robwu]]

The RemoteSecuritySettings.jsm file doesn't have any logic to delete cached attachments. Replacing downloadToDisk with download doesn't fix that underlying problem. It will just move the junk from files to IndexedDB.

Use deleteDownloaded at the right time to delete attachments for stale records, i.e. https://searchfox.org/mozilla-central/rev/0a2eba79c24300ce0539f91c1bebac2e75264e58/services/settings/Attachments.jsm#273

(In reply to Daniel Veditz [:dveditz] from comment #2)

Looks like CRLite is using a now-deprecated remote-settings feature and running into the problems the addons blocklist did (see bug 1634127).

In the Blocklist case, a fixed attachmentId is used so there is only one stored attachment. In this crlite case the attachmentId is not explicitly set thus derived from the record id. An arbitrary number of attachments exists, and will continue to pile up without explicit cleanup logic.

Please fix this and add unit tests to verify that there are no lingering attachments.

:leplatrem: Could it make sense to add helpers / options to the RemoteSettingsClient or downloader to support the intended behavior of keeping attachments in sync with the stored records? I.e. delete attachments when no record references it any more. That would support the use case from this bug.

Comment 6

[Rob Wu [:robwu]]

See question above.

Comment 7

[Mathieu Leplatre [:leplatrem]]

This is correct, before, with the downloadToDisk() method, the attachments were stored as files using the filename field. Now that the code uses the IndexedDB's based implementation, the attachments are stored "per-record".

If you would want updates to overwrite previous attachments, you have to specify the attachmentId option for download() like addons blocklist does (default is record.id).

:leplatrem: Could it make sense to add helpers / options to the RemoteSettingsClient or downloader to support the intended behavior of keeping attachments in sync with the stored records? I.e. delete attachments when no record references it any more. That would support the use case from this bug.

In Bug 1763626, we want to remove downloaded attachments that don't match any record (probably comparing their hash or something similar...). Seeing this bug, I realize its priority has to be raised and its implementation planned.

Comment 8

[Rob Wu [:robwu]]

(In reply to Mathieu Leplatre [:leplatrem] from comment #7)

This is correct, before, with the downloadToDisk() method, the attachments were stored as files using the filename field. Now that the code uses the IndexedDB's based implementation, the attachments are stored "per-record".

If you would want updates to overwrite previous attachments, you have to specify the attachmentId option for download() like addons blocklist does (default is record.id).

This only works if the implementation has a known limited number of attachments. In the CRLite use case, I think that the number is arbitrary and it's not feasible to choose the attachmentId. So the minimum that the implementation must do is to call deleteDownloaded when a removed record is observed during the sync event, e.g. as documented at https://searchfox.org/mozilla-central/rev/0a2eba79c24300ce0539f91c1bebac2e75264e58/services/settings/docs/index.rst#116-124

Comment 9

[John Schanck [:jschanck]]

(In reply to Rob Wu [:robwu] from comment #8)

This only works if the implementation has a known limited number of attachments. In the CRLite use case, I think that the number is arbitrary and it's not feasible to choose the attachmentId. So the minimum that the implementation must do is to call deleteDownloaded when a removed record is observed during the sync event, e.g. as documented at https://searchfox.org/mozilla-central/rev/0a2eba79c24300ce0539f91c1bebac2e75264e58/services/settings/docs/index.rst#116-124

At any given time CRLite's cert-revocations collection has about 10 records with attachments, and each record persists for no more than 10 days. My understanding is that the default caching mechanism for the download function will automatically delete attachments for records that have been deleted. But I'm happy to add some unit tests to confirm this.

Comment 10

[Rob Wu [:robwu]]

(In reply to John Schanck [:jschanck] from comment #9)

(In reply to Rob Wu [:robwu] from comment #8)

This only works if the implementation has a known limited number of attachments. In the CRLite use case, I think that the number is arbitrary and it's not feasible to choose the attachmentId. So the minimum that the implementation must do is to call deleteDownloaded when a removed record is observed during the sync event, e.g. as documented at https://searchfox.org/mozilla-central/rev/0a2eba79c24300ce0539f91c1bebac2e75264e58/services/settings/docs/index.rst#116-124

At any given time CRLite's cert-revocations collection has about 10 records with attachments, and each record persists for no more than 10 days.

I see that the record ids at https://firefox.settings.services.mozilla.com/v1/buckets/security-state/collections/cert-revocations/records are random UUIDs. They are not fixed, so the storage used for the attachments is unbounded.

My understanding is that the default caching mechanism for the download function will automatically delete attachments for records that have been deleted.

This understanding is incorrect. The download method caches the attachment which is used if download is called again for the same record. The downloaded data is currently not actively managed by the RemoteSettings backend, it is the responsibility of the client user to overwrite or delete the data if needed, with the deleteDownloaded method (as mentioned before). Bug 1763626 is a feature request about offering functionality to delete stale attachments, but that is still open.

Does CRLite actually need to cache the attachment? It looks like the data is immediately imported in a separate cert_storage component. If the data is unlikely to be used after that, then you could just use downloadAsBytes instead of download.

But I'm happy to add some unit tests to confirm this.

I expect these unit tests to fail with the current implementation.

Comment 11

[John Schanck [:jschanck]]

Thanks for the clarification. I've updated the patch to manually delete the downloads.

(In reply to Rob Wu [:robwu] from comment #10)

Does CRLite actually need to cache the attachment? It looks like the data is immediately imported in a separate cert_storage component. If the data is unlikely to be used after that, then you could just use downloadAsBytes instead of download.

I was inclined to use downloadAsBytes, but I think that the original author was concerned about a second round of downloads beginning before the loaded_into_cert_storage flags were set. This seems plausible to me so I kept it as is.

Comment 12

[Rob Wu [:robwu]]

(In reply to John Schanck [:jschanck] from comment #11)

Thanks for the clarification. I've updated the patch to manually delete the downloads.

(In reply to Rob Wu [:robwu] from comment #10)

Does CRLite actually need to cache the attachment? It looks like the data is immediately imported in a separate cert_storage component. If the data is unlikely to be used after that, then you could just use downloadAsBytes instead of download.

I was inclined to use downloadAsBytes, but I think that the original author was concerned about a second round of downloads beginning before the loaded_into_cert_storage flags were set. This seems plausible to me so I kept it as is.

The logic was originally introduced in this patch to bug 1563056 at https://searchfox.org/mozilla-central/diff/ba0c7e0e3aa435144b98f216e9eae20cdc8883ed/security/manager/ssl/RemoteSecuritySettings.jsm#786

In that patch, there was no cert_storage backend, so the reliance on RemoteSettings' cache was very important.

Now there is a storage backend, so the question is whether it's truly necessary to persist the data to disk (whether as a file or in IndexedDB), or whether it suffices to just get the bytes instead. Maybe even with a temporary in-memory data structure if you are concerned about downloading twice in a short time span.

Dana, since you authored the original patch: what do you recommend here?

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Bug 1677399 implemented a way to keep track of what we've downloaded and added to cert_storage. Keeping a copy in the cache seems redundant to me (the only way it would be useful is if cert_storage's data got corrupted/deleted while the cache was still ok).

Comment 14

[John Schanck [:jschanck]]

Great, let's get rid of the caching. I updated the patch.

Comment 15

[Mathieu Leplatre [:leplatrem]]

When there is a signature verification error, which sometimes happens, the local data are deleted. And the markers that are put on records (loaded_into_cert_storage: true) are gone.
The attachments are thus all re-downloaded when the synchronization is retried.
I suspect that this is one of the reasons why our CDN gets massively hammered by attachments downloads for security-settings. I started to take notes about that https://docs.google.com/document/d/100Yv1kPRcyXlQ29SqAWrTU3c2R_P5q44IR2Mlui9FwE/edit#

I propose that we keep the cache. And more, that we add it to other collections like intermediates.

Comment 16

[John Schanck [:jschanck]]

I don't think there is a robust enough caching option for our use case at the moment. I've opened Bug 1796190 to re-investigate caching once there are better options, e.g. the mechanism from Bug 1763626.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/747e24d0339e
part 1. avoid deprecated downloadToDisk function. r=keeler,leplatrem,robwu
https://hg.mozilla.org/integration/autoland/rev/ac705dd27e0a
part 2. clean cert-revocations attachment cache. r=keeler,leplatrem,robwu

Comment 18

[John Schanck [:jschanck]]

[Tracking Requested - why for this release]: We're planning a staged rollout of CRLite in the 107 release, which will involve flipping the security.remote_settings.crlite_filters.enabled pref for some users after 107 goes to release. We need to uplift part 1 of the patch series to 107 beta to ensure we don't later pollute 107 release profiles. Part 2 is optional as release users who have not had the pref set in the past will not have stale filters in their cache.

Comment 19

[Natalia Csoregi [:nataliaCs]]

Backed out for causing failures on test_crlite_filters.js

• backout: https://hg.mozilla.org/integration/autoland/rev/c537f98572d625fe8ed52a9ed912d06412f0df45
• push: https://treeherder.mozilla.org/jobs?repo=autoland&revision=ac705dd27e0a49bb8d22a1f7d0b76a144926d8d5&group_state=expanded
• failure log: https://treeherder.mozilla.org/logviewer?job_id=393851322&repo=autoland&lineNumber=2429

[task 2022-10-20T17:22:35.899Z] 17:22:35     INFO -  TEST-START | security/manager/ssl/tests/unit/test_crlite_filters.js
[task 2022-10-20T17:22:35.940Z] 17:22:35     INFO -  adb launch_application: am startservice -W -n 'org.mozilla.geckoview.test_runner/org.mozilla.geckoview.test_runner.XpcshellTestRunnerService$i0' -a android.intent.action.MAIN --es env0 XPCOM_DEBUG_BREAK=stack-and-abort --es env1 MOZ_CRASHREPORTER=1 --es env2 MOZ_CRASHREPORTER_NO_REPORT=1 --es env3 MOZ_DISABLE_NONLOCAL_CONNECTIONS=1 --es env4 MOZ_DEVELOPER_REPO_DIR=/builds/worker/checkouts/gecko --es env5 MOZ_DISABLE_CONTENT_SANDBOX=1 --es env6 MOZ_FETCHES_DIR=/builds/worker/fetches --es env7 MOZ_DISABLE_SOCKET_PROCESS=1 --es env8 LD_LIBRARY_PATH=/data/local/tmp/test_root/xpcb --es env9 MOZ_LINKER_CACHE=/data/local/tmp/test_root/xpcb --es env10 GRE_HOME=/data/local/tmp/test_root/xpcb --es env11 XPCSHELL_TEST_PROFILE_DIR=/data/local/tmp/test_root/xpc/p/e0a5cbe6-80f0-41d8-9bd1-621c2736bc38 --es env12 HOME=/data/local/tmp/test_root/xpc/p --es env13 XPCSHELL_TEST_TEMP_DIR=/data/local/tmp/test_root/xpc/tmp/abde7069-81b1-46f5-9edc-c4e9b27d8b3a --es env14 MOZ_ANDROID_DATA_DIR=/data/local/tmp/test_root/xpcb --es env15 MOZ_IN_AUTOMATION=1 --es env16 MOZ_ANDROID_CPU_ABI=x86_64 --es env17 MOZHTTP2_PORT=45707 --es env18 MOZNODE_EXEC_PORT=44879 --es env19 TMPDIR=/data/local/tmp/test_root/xpc/p/e0a5cbe6-80f0-41d8-9bd1-621c2736bc38 --es env20 XPCSHELL_MINIDUMP_DIR=/data/local/tmp/test_root/xpc/minidumps/e0a5cbe6-80f0-41d8-9bd1-621c2736bc38 --es arg0 -g --es arg1 /data/local/tmp/test_root/xpcb --es arg2 --greomni --es arg3 /data/local/tmp/test_root/xpcb/geckoview-test_runner.apk --es arg4 -m --es arg5 -e --es arg6 'const _HEAD_JS_PATH = "/data/local/tmp/test_root/xpc/head.js";' --es arg7 -e --es arg8 'const _MOZINFO_JS_PATH = "/data/local/tmp/test_root/xpc/p/e0a5cbe6-80f0-41d8-9bd1-621c2736bc38/mozinfo.json";' --es arg9 -e --es arg10 'const _PREFS_FILE = "/data/local/tmp/test_root/xpc/user.js";' --es arg11 -e --es arg12 'const _TESTING_MODULES_DIR = "/data/local/tmp/test_root/xpc/m";' --es arg13 -f --es arg14 /data/local/tmp/test_root/xpc/head.js --es arg15 -e --es arg16 'const _HEAD_FILES = ["/data/local/tmp/test_root/xpc/security/manager/ssl/tests/unit/head_psm.js"];' --es arg17 -e --es arg18 'const _JSDEBUGGER_PORT = 0;' --es arg19 -e --es arg20 'const _TEST_CWD = "/data/local/tmp/test_root/xpc/security/manager/ssl/tests/unit";' --es arg21 -e --es arg22 'const _TEST_FILE = ["test_crlite_filters.js"];' --es arg23 -e --es arg24 'const _TEST_NAME = "security/manager/ssl/tests/unit/test_crlite_filters.js";' --es arg25 -e --es arg26 '_execute_test(); quit(0);' --ez use_multiprocess True --es out_file /data/local/tmp/test_root/xpc/logs/xpcshell-cd40dc18-463b-4cd3-b1f9-0bb34a132051.log
[task 2022-10-20T17:22:36.175Z] 17:22:36     INFO -  remotexpcshelltests.py | security/manager/ssl/tests/unit/test_crlite_filters.js | 28757 | Launched Test App
[task 2022-10-20T17:22:38.533Z] 17:22:38     INFO -  remotexpcshelltests.py | security/manager/ssl/tests/unit/test_crlite_filters.js | 28757 | Application ran for: 0:00:02.634081
[task 2022-10-20T17:22:38.596Z] 17:22:38  WARNING -  TEST-UNEXPECTED-FAIL | security/manager/ssl/tests/unit/test_crlite_filters.js | xpcshell return code: 0
[task 2022-10-20T17:22:38.596Z] 17:22:38     INFO -  TEST-INFO took 2697ms
[task 2022-10-20T17:22:38.596Z] 17:22:38     INFO -  >>>>>>>

Comment 20

[John Schanck [:jschanck]]

Revised patch fixes the bug: https://treeherder.mozilla.org/jobs?repo=try&revision=d76ea78d18fa5b8f11f196efff51cbb7cd5c9ebc

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e302dce9339
part 1. avoid deprecated downloadToDisk function. r=keeler,leplatrem,robwu
https://hg.mozilla.org/integration/autoland/rev/1909084518a4
part 2. clean cert-revocations attachment cache. r=keeler,leplatrem,robwu

Comment 22

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/6e302dce9339
https://hg.mozilla.org/mozilla-central/rev/1909084518a4

Comment 23

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

The patch landed in nightly and beta is affected.
:jschanck, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox107 to wontfix.

For more information, please visit auto_nag documentation.

Comment 24

[John Schanck [:jschanck]]

Comment on attachment 9298906 [details]
Bug 1795710 - part 1. avoid deprecated downloadToDisk function. r=keeler

Beta/Release Uplift Approval Request

• User impact if declined: Attachments from the cert-revocations Remote Settings collection will accumulate on disk.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch changes a downloadToDisk call to a downloadAsBytes call. The former writes a copy of the download to disk. Users were not using the on-disk cache.
• String changes made/needed:
• Is Android affected?: No

Comment 25

[Donal Meehan [:dmeehan]]

Comment on attachment 9298906 [details]
Bug 1795710 - part 1. avoid deprecated downloadToDisk function. r=keeler

Approved for 107.0b5.

Comment 26

[Donal Meehan [:dmeehan]]

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

Comment 27

[John Schanck [:jschanck]]

Comment on attachment 9298907 [details]
Bug 1795710 - part 2. clean cert-revocations attachment cache. r=keeler

Beta/Release Uplift Approval Request

• User impact if declined: The main purpose of this patch is to free disk space taken up by (unused) cached downloads. If the patch is not taken, the space will be freed when the client updates to 108+. However, I'm requesting uplift because this patch also includes a workaround for bug 1730026. On Oct 25, Remote Settings saw a spike in requests for attachments in the security_state/intermediates collection. The number of requests was in excess of the average number of users online at any given time, which suggests that individual clients were making multiple requests for the same file. This is likely due to bug 1730026, which causes multiple copies of IntermediatePreloadsClient to be initialized.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch has been tested on Nightly and manually verified by users on several platforms, including Android.
• String changes made/needed:
• Is Android affected?: Yes

Comment 28

[Donal Meehan [:dmeehan]]

Comment on attachment 9298907 [details]
Bug 1795710 - part 2. clean cert-revocations attachment cache. r=keeler

Approved for 107.0b6.

Comment 29

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6bfdbf5dbf7b

Comment 30

[Daniel Veditz [:dveditz]]

Forgot to update the bug, but I let John know: new files aren't being saved in my nightly profile, and all the old files were successfully cleaned up.