From 6e72b3554e4edbe7da10484a2220b6ad7b8f1119 Mon Sep 17 00:00:00 2001
From: Florian Duros <florianduros@element.io>
Date: Wed, 5 Feb 2025 11:45:56 +0100
Subject: [PATCH] Fix `resetEncryption` to remove secrets in 4S (#4683)

* fix(crypto): `resetEncryption` remove secrets in 4S

Remove the cross signing keys and the backup decryption key of the 4S when calling `resetEncryption`

* test(crypto): expect secrets to be deleted in 4S when `resetEncryption` is called

* test(secret storage): add test case when the secret is set at null

* fix(crypto): remove default key in 4S

* test(crypto): default key should be removed from 4S
---
 spec/unit/rust-crypto/rust-crypto.spec.ts |  8 +++++++
 spec/unit/secret-storage.spec.ts          | 18 ++++++++++++----
 src/rust-crypto/rust-crypto.ts            |  9 ++++++++
 src/secret-storage.ts                     | 26 ++++++++++++-----------
 4 files changed, 45 insertions(+), 16 deletions(-)

diff --git a/spec/unit/rust-crypto/rust-crypto.spec.ts b/spec/unit/rust-crypto/rust-crypto.spec.ts
index de87b52d6..52eb8bfe9 100644
--- a/spec/unit/rust-crypto/rust-crypto.spec.ts
+++ b/spec/unit/rust-crypto/rust-crypto.spec.ts
@@ -2247,6 +2247,8 @@ describe("RustCrypto", () => {
                 setDefaultKeyId: jest.fn(),
                 hasKey: jest.fn().mockResolvedValue(false),
                 getKey: jest.fn().mockResolvedValue(null),
+                store: jest.fn(),
+                getDefaultKeyId: jest.fn().mockResolvedValue("defaultKeyId"),
             } as unknown as ServerSideSecretStorage;
 
             fetchMock.post("path:/_matrix/client/v3/keys/upload", { one_time_key_counts: {} });
@@ -2285,6 +2287,12 @@ describe("RustCrypto", () => {
             const authUploadDeviceSigningKeys = jest.fn();
             await rustCrypto.resetEncryption(authUploadDeviceSigningKeys);
 
+            // The secrets in 4S should be deleted
+            expect(secretStorage.store).toHaveBeenCalledWith("m.cross_signing.master", null);
+            expect(secretStorage.store).toHaveBeenCalledWith("m.cross_signing.self_signing", null);
+            expect(secretStorage.store).toHaveBeenCalledWith("m.cross_signing.user_signing", null);
+            expect(secretStorage.store).toHaveBeenCalledWith("m.megolm_backup.v1", null);
+            expect(secretStorage.store).toHaveBeenCalledWith("m.secret_storage.key.defaultKeyId", null);
             // A new key backup should be created
             expect(newKeyBackupInfo.auth_data).toBeTruthy();
             // The new cross signing keys should be uploaded
diff --git a/spec/unit/secret-storage.spec.ts b/spec/unit/secret-storage.spec.ts
index e3c736429..2e262c290 100644
--- a/spec/unit/secret-storage.spec.ts
+++ b/spec/unit/secret-storage.spec.ts
@@ -243,11 +243,16 @@ describe("ServerSideSecretStorageImpl", function () {
     });
 
     describe("store", () => {
-        it("should ignore keys with unknown algorithm", async function () {
-            const accountDataAdapter = mockAccountDataClient();
-            const mockCallbacks = { getSecretStorageKey: jest.fn() } as Mocked<SecretStorageCallbacks>;
-            const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, mockCallbacks);
+        let secretStorage: ServerSideSecretStorage;
+        let accountDataAdapter: Mocked<AccountDataClient>;
 
+        beforeEach(() => {
+            accountDataAdapter = mockAccountDataClient();
+            const mockCallbacks = { getSecretStorageKey: jest.fn() } as Mocked<SecretStorageCallbacks>;
+            secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, mockCallbacks);
+        });
+
+        it("should ignore keys with unknown algorithm", async function () {
             // stub out getAccountData to return a key with an unknown algorithm
             const storedKey = { algorithm: "badalg" } as SecretStorageKeyDescriptionCommon;
             async function mockGetAccountData<K extends keyof AccountDataEvents>(
@@ -274,6 +279,11 @@ describe("ServerSideSecretStorageImpl", function () {
             // eslint-disable-next-line no-console
             expect(console.warn).toHaveBeenCalledWith(expect.stringContaining("unknown algorithm"));
         });
+
+        it("should set the secret with an empty object when the value is null", async function () {
+            await secretStorage.store("mySecret", null);
+            expect(accountDataAdapter.setAccountData).toHaveBeenCalledWith("mySecret", {});
+        });
     });
 
     describe("setDefaultKeyId", function () {
diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts
index c50dd95fc..4eb8f5be0 100644
--- a/src/rust-crypto/rust-crypto.ts
+++ b/src/rust-crypto/rust-crypto.ts
@@ -1508,6 +1508,15 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, CryptoEventH
         // Disable backup, and delete all the backups from the server
         await this.backupManager.deleteAllKeyBackupVersions();
 
+        // Remove the stored secrets in the secret storage
+        await this.secretStorage.store("m.cross_signing.master", null);
+        await this.secretStorage.store("m.cross_signing.self_signing", null);
+        await this.secretStorage.store("m.cross_signing.user_signing", null);
+        await this.secretStorage.store("m.megolm_backup.v1", null);
+
+        // Remove the recovery key
+        const defaultKeyId = await this.secretStorage.getDefaultKeyId();
+        if (defaultKeyId) await this.secretStorage.store(`m.secret_storage.key.${defaultKeyId}`, null);
         // Disable the recovery key and the secret storage
         await this.secretStorage.setDefaultKeyId(null);
 
diff --git a/src/secret-storage.ts b/src/secret-storage.ts
index 2e587b9a8..a9c1454d3 100644
--- a/src/secret-storage.ts
+++ b/src/secret-storage.ts
@@ -280,14 +280,18 @@ export interface ServerSideSecretStorage {
      * Store an encrypted secret on the server.
      *
      * Details of the encryption keys to be used must previously have been stored in account data
-     * (for example, via {@link ServerSideSecretStorage#addKey}.
+     * (for example, via {@link ServerSideSecretStorageImpl#addKey}. {@link SecretStorageCallbacks#getSecretStorageKey} will be called to obtain a secret storage
+     * key to decrypt the secret.
+     *
+     * If the secret is `null`, the secret value in the account data will be set to an empty object.
+     * This is considered as "removing" the secret.
      *
      * @param name - The name of the secret - i.e., the "event type" to be stored in the account data
      * @param secret - The secret contents.
      * @param keys - The IDs of the keys to use to encrypt the secret, or null/undefined to use the default key
      *     (will throw if no default key is set).
      */
-    store(name: string, secret: string, keys?: string[] | null): Promise<void>;
+    store(name: string, secret: string | null, keys?: string[] | null): Promise<void>;
 
     /**
      * Get a secret from storage, and decrypt it.
@@ -504,17 +508,15 @@ export class ServerSideSecretStorageImpl implements ServerSideSecretStorage {
     }
 
     /**
-     * Store an encrypted secret on the server.
-     *
-     * Details of the encryption keys to be used must previously have been stored in account data
-     * (for example, via {@link ServerSideSecretStorageImpl#addKey}. {@link SecretStorageCallbacks#getSecretStorageKey} will be called to obtain a secret storage
-     * key to decrypt the secret.
-     *
-     * @param name - The name of the secret - i.e., the "event type" to be stored in the account data
-     * @param secret - The secret contents.
-     * @param keys - The IDs of the keys to use to encrypt the secret, or null/undefined to use the default key.
+     * Implementation of {@link ServerSideSecretStorage#store}.
      */
-    public async store(name: SecretStorageKey, secret: string, keys?: string[] | null): Promise<void> {
+    public async store(name: SecretStorageKey, secret: string | null, keys?: string[] | null): Promise<void> {
+        if (secret === null) {
+            // remove secret
+            await this.accountDataAdapter.setAccountData(name, {});
+            return;
+        }
+
         const encrypted: Record<string, AESEncryptedSecretStoragePayload> = {};
 
         if (!keys) {