ElementR: Fix missing key check values in 4S key storage (#3950)

* fix missing key check in key storage

* code review

* fix tests

* add recovery keys test for both backends

* fix api break on GeneratedSecretStorageKey

* fix test

* fix test

* Update src/crypto-api.ts

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

* Update spec/unit/rust-crypto/rust-crypto.spec.ts

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

* Update src/crypto-api.ts

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

---------

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

spec/integ/crypto/crypto.spec.ts

@@ -74,7 +74,7 @@ import {
     mockSetupCrossSigningRequests,
     mockSetupMegolmBackupRequests,
 } from "../../test-utils/mockEndpoints";
-import { AddSecretStorageKeyOpts } from "../../../src/secret-storage";
+import { SecretStorageKeyDescription } from "../../../src/secret-storage";
 import {
     CrossSigningKey,
     CryptoCallbacks,
@@ -340,7 +340,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
     function createCryptoCallbacks(): CryptoCallbacks {
         // Store the cached secret storage key and return it when `getSecretStorageKey` is called
         let cachedKey: { keyId: string; key: Uint8Array };
-        const cacheSecretStorageKey = (keyId: string, keyInfo: AddSecretStorageKeyOpts, key: Uint8Array) => {
+        const cacheSecretStorageKey = (keyId: string, keyInfo: SecretStorageKeyDescription, key: Uint8Array) => {
             cachedKey = {
                 keyId,
                 key,
@@ -2567,6 +2567,30 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
             await backupStatusUpdate;
         }
 
+        describe("Generate 4S recovery keys", () => {
+            it("should create a random recovery key", async () => {
+                const generatedKey = await aliceClient.getCrypto()!.createRecoveryKeyFromPassphrase();
+                expect(generatedKey.privateKey).toBeDefined();
+                expect(generatedKey.privateKey).toBeInstanceOf(Uint8Array);
+                expect(generatedKey.privateKey.length).toBe(32);
+                expect(generatedKey.keyInfo?.passphrase).toBeUndefined();
+                expect(generatedKey.encodedPrivateKey).toBeDefined();
+                expect(generatedKey.encodedPrivateKey!.indexOf("Es")).toBe(0);
+            });
+
+            it("should create a recovery key from passphrase", async () => {
+                const generatedKey = await aliceClient.getCrypto()!.createRecoveryKeyFromPassphrase("mypassphrase");
+                expect(generatedKey.privateKey).toBeDefined();
+                expect(generatedKey.privateKey).toBeInstanceOf(Uint8Array);
+                expect(generatedKey.privateKey.length).toBe(32);
+                expect(generatedKey.keyInfo?.passphrase?.algorithm).toBe("m.pbkdf2");
+                expect(generatedKey.keyInfo?.passphrase?.iterations).toBe(500000);
+
+                expect(generatedKey.encodedPrivateKey).toBeDefined();
+                expect(generatedKey.encodedPrivateKey!.indexOf("Es")).toBe(0);
+            });
+        });
+
         describe("bootstrapSecretStorage", () => {
             // Doesn't work with legacy crypto, which will try to bootstrap even without private key, which is buggy.
             newBackendOnly(
@@ -2592,6 +2616,14 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
                 // Wait for the key to be uploaded in the account data
                 const secretStorageKey = await awaitSecretStorageKeyStoredInAccountData();
 
+                // check that the key content contains the key check info
+                const keyContent = accountDataAccumulator.accountDataEvents.get(
+                    `m.secret_storage.key.${secretStorageKey}`,
+                )!;
+                // In order to verify if the key is valid, a zero secret is encrypted with the key
+                expect(keyContent.iv).toBeDefined();
+                expect(keyContent.mac).toBeDefined();
+
                 // Return the newly created key in the sync response
                 accountDataAccumulator.sendSyncResponseWithUpdatedAccountData(syncResponder);
 

spec/unit/crypto/crypto-utils.ts

@@ -33,12 +33,10 @@ export async function resetCrossSigningKeys(
 
 export async function createSecretStorageKey(): Promise<IRecoveryKey> {
     const decryption = new global.Olm.PkDecryption();
-    const storagePublicKey = decryption.generate_key();
+    decryption.generate_key();
     const storagePrivateKey = decryption.get_private_key();
     decryption.free();
     return {
-        // `pubkey` not used anymore with symmetric 4S
-        keyInfo: { pubkey: storagePublicKey, key: undefined! },
         privateKey: storagePrivateKey,
     };
 }

spec/unit/crypto/secrets.spec.ts

@@ -190,10 +190,7 @@ describe("Secrets", function () {
         };
         resetCrossSigningKeys(alice);
 
-        const { keyId: newKeyId } = await alice.addSecretStorageKey(SECRET_STORAGE_ALGORITHM_V1_AES, {
+        const { keyId: newKeyId } = await alice.addSecretStorageKey(SECRET_STORAGE_ALGORITHM_V1_AES, { key });
-            pubkey: undefined,
-            key: undefined,
-        });
         // we don't await on this because it waits for the event to come down the sync
         // which won't happen in the test setup
         alice.setDefaultSecretStorageKeyId(newKeyId);
@@ -335,7 +332,6 @@ describe("Secrets", function () {
 
         it("bootstraps when cross-signing keys in secret storage", async function () {
             const decryption = new global.Olm.PkDecryption();
-            const storagePublicKey = decryption.generate_key();
             const storagePrivateKey = decryption.get_private_key();
 
             const bob: MatrixClient = await makeTestClient(
@@ -378,8 +374,6 @@ describe("Secrets", function () {
             });
             await bob.bootstrapSecretStorage({
                 createSecretStorageKey: async () => ({
-                    // `pubkey` not used anymore with symmetric 4S
-                    keyInfo: { pubkey: storagePublicKey },
                     privateKey: storagePrivateKey,
                 }),
             });

spec/unit/rust-crypto/rust-crypto.spec.ts

@@ -739,8 +739,8 @@ describe("RustCrypto", () => {
             // Expect the private key to be an Uint8Array with a length of 32
             expect(recoveryKey.privateKey).toBeInstanceOf(Uint8Array);
             expect(recoveryKey.privateKey.length).toBe(32);
-            // Expect keyInfo to be empty
+            // Expect passphrase info to be absent
-            expect(Object.keys(recoveryKey.keyInfo!).length).toBe(0);
+            expect(recoveryKey.keyInfo?.passphrase).toBeUndefined();
         });
 
         it("should create a recovery key with password", async () => {

spec/unit/secret-storage.spec.ts

@@ -33,7 +33,9 @@ describe("ServerSideSecretStorageImpl", function () {
         it("should allow storing a default key", async function () {
             const accountDataAdapter = mockAccountDataClient();
             const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, {});
-            const result = await secretStorage.addKey("m.secret_storage.v1.aes-hmac-sha2");
+            const result = await secretStorage.addKey("m.secret_storage.v1.aes-hmac-sha2", {
+                key: new Uint8Array(32),
+            });
 
             // it should have made up a 32-character key id
             expect(result.keyId.length).toEqual(32);
@@ -46,7 +48,13 @@ describe("ServerSideSecretStorageImpl", function () {
         it("should allow storing a key with an explicit id", async function () {
             const accountDataAdapter = mockAccountDataClient();
             const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, {});
-            const result = await secretStorage.addKey("m.secret_storage.v1.aes-hmac-sha2", {}, "myKeyId");
+            const result = await secretStorage.addKey(
+                "m.secret_storage.v1.aes-hmac-sha2",
+                {
+                    key: new Uint8Array(32),
+                },
+                "myKeyId",
+            );
 
             // it should have made up a 32-character key id
             expect(result.keyId).toEqual("myKeyId");
@@ -59,7 +67,10 @@ describe("ServerSideSecretStorageImpl", function () {
         it("should allow storing a key with a name", async function () {
             const accountDataAdapter = mockAccountDataClient();
             const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, {});
-            const result = await secretStorage.addKey("m.secret_storage.v1.aes-hmac-sha2", { name: "mykey" });
+            const result = await secretStorage.addKey("m.secret_storage.v1.aes-hmac-sha2", {
+                name: "mykey",
+                key: new Uint8Array(32),
+            });
 
             expect(result.keyInfo.name).toEqual("mykey");
 
@@ -80,6 +91,7 @@ describe("ServerSideSecretStorageImpl", function () {
             };
             const result = await secretStorage.addKey("m.secret_storage.v1.aes-hmac-sha2", {
                 passphrase,
+                key: new Uint8Array(32),
             });
 
             expect(result.keyInfo.passphrase).toEqual(passphrase);
@@ -93,7 +105,9 @@ describe("ServerSideSecretStorageImpl", function () {
         it("should complain about invalid algorithm", async function () {
             const accountDataAdapter = mockAccountDataClient();
             const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, {});
-            await expect(() => secretStorage.addKey("bad_alg")).rejects.toThrow("Unknown key algorithm");
+            await expect(() => secretStorage.addKey("bad_alg", { key: new Uint8Array(32) })).rejects.toThrow(
+                "Unknown key algorithm",
+            );
         });
     });
 

src/crypto-api.ts

@@ -18,7 +18,7 @@ import type { IMegolmSessionData } from "./@types/crypto";
 import { Room } from "./models/room";
 import { DeviceMap } from "./models/device";
 import { UIAuthCallback } from "./interactive-auth";
-import { AddSecretStorageKeyOpts, SecretStorageCallbacks, SecretStorageKeyDescription } from "./secret-storage";
+import { PassphraseInfo, SecretStorageCallbacks, SecretStorageKeyDescription } from "./secret-storage";
 import { VerificationRequest } from "./crypto-api/verification";
 import { BackupTrustInfo, KeyBackupCheck, KeyBackupInfo } from "./crypto-api/keybackup";
 import { ISignatures } from "./@types/signed";
@@ -710,10 +710,15 @@ export interface CrossSigningKeyInfo {
 }
 
 /**
- * Recovery key created by {@link CryptoApi#createRecoveryKeyFromPassphrase}
+ * Recovery key created by {@link CryptoApi#createRecoveryKeyFromPassphrase} or {@link CreateSecretStorageOpts#createSecretStorageKey}.
  */
 export interface GeneratedSecretStorageKey {
-    keyInfo?: AddSecretStorageKeyOpts;
+    keyInfo?: {
+        /** If the key was derived from a passphrase, information (algorithm, salt, etc) on that derivation. */
+        passphrase?: PassphraseInfo;
+        /** Optional human-readable name for the key, to be stored in account_data. */
+        name?: string;
+    };
     /** The raw generated private key. */
     privateKey: Uint8Array;
     /** The generated key, encoded for display to the user per https://spec.matrix.org/v1.7/client-server-api/#key-representation. */

src/crypto/SecretStorage.ts

@@ -73,11 +73,7 @@ export class SecretStorage<B extends MatrixClient | undefined = MatrixClient> im
     /**
      * Add a key for encrypting secrets.
      */
-    public addKey(
+    public addKey(algorithm: string, opts: AddSecretStorageKeyOpts, keyId?: string): Promise<SecretStorageKeyObject> {
-        algorithm: string,
-        opts: AddSecretStorageKeyOpts = {},
-        keyId?: string,
-    ): Promise<SecretStorageKeyObject> {
         return this.storageImpl.addKey(algorithm, opts, keyId);
     }
 

src/crypto/index.ts

@@ -708,25 +708,30 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
     public async createRecoveryKeyFromPassphrase(password?: string): Promise<IRecoveryKey> {
         const decryption = new global.Olm.PkDecryption();
         try {
-            const keyInfo: Partial<IRecoveryKey["keyInfo"]> = {};
             if (password) {
                 const derivation = await keyFromPassphrase(password);
-                keyInfo.passphrase = {
+
+                decryption.init_with_private_key(derivation.key);
+                const privateKey = decryption.get_private_key();
+                return {
+                    keyInfo: {
+                        passphrase: {
                             algorithm: "m.pbkdf2",
                             iterations: derivation.iterations,
                             salt: derivation.salt,
+                        },
+                    },
+                    privateKey: privateKey,
+                    encodedPrivateKey: encodeRecoveryKey(privateKey),
                 };
-                keyInfo.pubkey = decryption.init_with_private_key(derivation.key);
             } else {
-                keyInfo.pubkey = decryption.generate_key();
+                decryption.generate_key();
-            }
                 const privateKey = decryption.get_private_key();
-            const encodedPrivateKey = encodeRecoveryKey(privateKey);
                 return {
-                keyInfo: keyInfo as IRecoveryKey["keyInfo"],
+                    privateKey: privateKey,
-                encodedPrivateKey,
+                    encodedPrivateKey: encodeRecoveryKey(privateKey),
-                privateKey,
                 };
+            }
         } finally {
             decryption?.free();
         }
@@ -986,17 +991,11 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
         let newKeyId: string | null = null;
 
         // create a new SSSS key and set it as default
-        const createSSSS = async (opts: AddSecretStorageKeyOpts, privateKey?: Uint8Array): Promise<string> => {
+        const createSSSS = async (opts: AddSecretStorageKeyOpts): Promise<string> => {
-            if (privateKey) {
-                opts.key = privateKey;
-            }
-
             const { keyId, keyInfo } = await secretStorage.addKey(SECRET_STORAGE_ALGORITHM_V1_AES, opts);
 
-            if (privateKey) {
             // make the private key available to encrypt 4S secrets
-                builder.ssssCryptoCallbacks.addPrivateKey(keyId, keyInfo, privateKey);
+            builder.ssssCryptoCallbacks.addPrivateKey(keyId, keyInfo, opts.key);
-            }
 
             await secretStorage.setDefaultKeyId(keyId);
             return keyId;
@@ -1060,8 +1059,8 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
             // secrets using it, in theory. We could move them to the new key but a)
             // that would mean we'd need to prompt for the old passphrase, and b)
             // it's not clear that would be the right thing to do anyway.
-            const { keyInfo = {} as AddSecretStorageKeyOpts, privateKey } = await createSecretStorageKey();
+            const { keyInfo, privateKey } = await createSecretStorageKey();
-            newKeyId = await createSSSS(keyInfo, privateKey);
+            newKeyId = await createSSSS({ passphrase: keyInfo?.passphrase, key: privateKey, name: keyInfo?.name });
         } else if (!storageExists && keyBackupInfo) {
             // we have an existing backup, but no SSSS
             logger.log("Secret storage does not exist, using key backup key");
@@ -1071,7 +1070,7 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
             const backupKey = (await this.getSessionBackupPrivateKey()) || (await getKeyBackupPassphrase?.());
 
             // create a new SSSS key and use the backup key as the new SSSS key
-            const opts = {} as AddSecretStorageKeyOpts;
+            const opts = { key: backupKey } as AddSecretStorageKeyOpts;
 
             if (keyBackupInfo.auth_data.private_key_salt && keyBackupInfo.auth_data.private_key_iterations) {
                 // FIXME: ???
@@ -1083,7 +1082,7 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
                 };
             }
 
-            newKeyId = await createSSSS(opts, backupKey);
+            newKeyId = await createSSSS(opts);
 
             // store the backup key in secret storage
             await secretStorage.store("m.megolm_backup.v1", encodeBase64(backupKey!), [newKeyId]);

src/rust-crypto/rust-crypto.ts

@@ -54,7 +54,7 @@ import {
 import { deviceKeysToDeviceMap, rustDeviceToJsDevice } from "./device-converter";
 import { IDownloadKeyResult, IQueryKeysRequest } from "../client";
 import { Device, DeviceMap } from "../models/device";
-import { AddSecretStorageKeyOpts, SECRET_STORAGE_ALGORITHM_V1_AES, ServerSideSecretStorage } from "../secret-storage";
+import { SECRET_STORAGE_ALGORITHM_V1_AES, ServerSideSecretStorage } from "../secret-storage";
 import { CrossSigningIdentity } from "./CrossSigningIdentity";
 import { secretStorageCanAccessSecrets, secretStorageContainsCrossSigningKeys } from "./secret-storage";
 import { keyFromPassphrase } from "../crypto/key_passphrase";
@@ -748,15 +748,11 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
      * @param secretStorageKey - The secret storage key to add in the secret storage.
      */
     private async addSecretStorageKeyToSecretStorage(secretStorageKey: GeneratedSecretStorageKey): Promise<void> {
-        // keyInfo is required to continue
+        const secretStorageKeyObject = await this.secretStorage.addKey(SECRET_STORAGE_ALGORITHM_V1_AES, {
-        if (!secretStorageKey.keyInfo) {
+            passphrase: secretStorageKey.keyInfo?.passphrase,
-            throw new Error("missing keyInfo field in the secret storage key");
+            name: secretStorageKey.keyInfo?.name,
-        }
+            key: secretStorageKey.privateKey,
-
+        });
-        const secretStorageKeyObject = await this.secretStorage.addKey(
-            SECRET_STORAGE_ALGORITHM_V1_AES,
-            secretStorageKey.keyInfo,
-        );
 
         await this.secretStorage.setDefaultKeyId(secretStorageKeyObject.keyId);
 
@@ -817,31 +813,30 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
      * Implementation of {@link CryptoApi#createRecoveryKeyFromPassphrase}
      */
     public async createRecoveryKeyFromPassphrase(password?: string): Promise<GeneratedSecretStorageKey> {
-        let key: Uint8Array;
-
-        const keyInfo: AddSecretStorageKeyOpts = {};
         if (password) {
             // Generate the key from the passphrase
             const derivation = await keyFromPassphrase(password);
-            keyInfo.passphrase = {
+            return {
+                keyInfo: {
+                    passphrase: {
                         algorithm: "m.pbkdf2",
                         iterations: derivation.iterations,
                         salt: derivation.salt,
+                    },
+                },
+                privateKey: derivation.key,
+                encodedPrivateKey: encodeRecoveryKey(derivation.key),
             };
-            key = derivation.key;
         } else {
             // Using the navigator crypto API to generate the private key
-            key = new Uint8Array(32);
+            const key = new Uint8Array(32);
             crypto.getRandomValues(key);
-        }
-
-        const encodedPrivateKey = encodeRecoveryKey(key);
             return {
-            keyInfo,
-            encodedPrivateKey,
                 privateKey: key,
+                encodedPrivateKey: encodeRecoveryKey(key),
             };
         }
+    }
 
     /**
      * Implementation of {@link CryptoApi.getEncryptionInfoForEvent}.

src/secret-storage.ts

@@ -100,10 +100,12 @@ export interface PassphraseInfo {
  * Options for {@link ServerSideSecretStorageImpl#addKey}.
  */
 export interface AddSecretStorageKeyOpts {
-    pubkey?: string;
+    /** Information for deriving the key from a passphrase if any. */
     passphrase?: PassphraseInfo;
+    /** Optional name of the key. */
     name?: string;
-    key?: Uint8Array;
+    /** The private key. Will be used to generate the key check values in the key info; it will not be stored on the server */
+    key: Uint8Array;
 }
 
 /**
@@ -380,7 +382,7 @@ export class ServerSideSecretStorageImpl implements ServerSideSecretStorage {
      */
     public async addKey(
         algorithm: string,
-        opts: AddSecretStorageKeyOpts = {},
+        opts: AddSecretStorageKeyOpts,
         keyId?: string,
     ): Promise<SecretStorageKeyObject> {
         if (algorithm !== SECRET_STORAGE_ALGORITHM_V1_AES) {
@@ -396,11 +398,10 @@ export class ServerSideSecretStorageImpl implements ServerSideSecretStorage {
         if (opts.passphrase) {
             keyInfo.passphrase = opts.passphrase;
         }
-        if (opts.key) {
+
         const { iv, mac } = await calculateKeyCheck(opts.key);
         keyInfo.iv = iv;
         keyInfo.mac = mac;
-        }
 
         // Create a unique key id. XXX: this is racey.
         if (!keyId) {