diff --git a/spec/integ/crypto/crypto.spec.ts b/spec/integ/crypto/crypto.spec.ts index e819cb7cb18..ed3af3b8e8e 100644 --- a/spec/integ/crypto/crypto.spec.ts +++ b/spec/integ/crypto/crypto.spec.ts @@ -29,6 +29,7 @@ import { getSyncResponse, InitCrypto, mkEventCustom, + mkMembershipCustom, syncPromise, } from "../../test-utils/test-utils"; import * as testData from "../../test-utils/test-data"; @@ -38,6 +39,7 @@ import { BOB_TEST_USER_ID, SIGNED_CROSS_SIGNING_KEYS_DATA, SIGNED_TEST_DEVICE_DATA, + TEST_ROOM_ID, TEST_ROOM_ID as ROOM_ID, TEST_USER_ID, } from "../../test-utils/test-data"; @@ -230,9 +232,6 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, /** an object which intercepts `/keys/upload` requests from {@link #aliceClient} to catch the uploaded keys */ let keyReceiver: E2EKeyReceiver; - /** an object which intercepts `/keys/query` requests on the test homeserver */ - let keyResponder: E2EKeyResponder; - /** an object which intercepts `/sync` requests from {@link #aliceClient} */ let syncResponder: ISyncResponder; @@ -368,6 +367,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, accessToken: "akjgkrgjs", deviceId: "xzcvb", cryptoCallbacks: createCryptoCallbacks(), + logger: logger.getChild("aliceClient"), }); /* set up listeners for /keys/upload and /sync */ @@ -701,7 +701,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, it("prepareToEncrypt", async () => { const homeserverUrl = aliceClient.getHomeserverUrl(); - keyResponder = new E2EKeyResponder(homeserverUrl); + const keyResponder = new E2EKeyResponder(homeserverUrl); keyResponder.addKeyReceiver("@alice:localhost", keyReceiver); const testDeviceKeys = getTestOlmAccountKeys(testOlmAccount, "@bob:xyz", "DEVICE_ID"); @@ -732,7 +732,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, it("Alice sends a megolm message with GlobalErrorOnUnknownDevices=false", async () => { aliceClient.setGlobalErrorOnUnknownDevices(false); const homeserverUrl = aliceClient.getHomeserverUrl(); - keyResponder = new E2EKeyResponder(homeserverUrl); + const keyResponder = new E2EKeyResponder(homeserverUrl); keyResponder.addKeyReceiver("@alice:localhost", keyReceiver); const testDeviceKeys = getTestOlmAccountKeys(testOlmAccount, "@bob:xyz", "DEVICE_ID"); @@ -760,7 +760,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, it("We should start a new megolm session after forceDiscardSession", async () => { aliceClient.setGlobalErrorOnUnknownDevices(false); const homeserverUrl = aliceClient.getHomeserverUrl(); - keyResponder = new E2EKeyResponder(homeserverUrl); + const keyResponder = new E2EKeyResponder(homeserverUrl); keyResponder.addKeyReceiver("@alice:localhost", keyReceiver); const testDeviceKeys = getTestOlmAccountKeys(testOlmAccount, "@bob:xyz", "DEVICE_ID"); @@ -2070,7 +2070,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, it("Sending an event initiates a member list sync", async () => { const homeserverUrl = aliceClient.getHomeserverUrl(); - keyResponder = new E2EKeyResponder(homeserverUrl); + const keyResponder = new E2EKeyResponder(homeserverUrl); keyResponder.addKeyReceiver("@alice:localhost", keyReceiver); const testDeviceKeys = getTestOlmAccountKeys(testOlmAccount, "@bob:xyz", "DEVICE_ID"); @@ -2093,7 +2093,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, it("loading the membership list inhibits a later load", async () => { const homeserverUrl = aliceClient.getHomeserverUrl(); - keyResponder = new E2EKeyResponder(homeserverUrl); + const keyResponder = new E2EKeyResponder(homeserverUrl); keyResponder.addKeyReceiver("@alice:localhost", keyReceiver); const testDeviceKeys = getTestOlmAccountKeys(testOlmAccount, "@bob:xyz", "DEVICE_ID"); @@ -2903,7 +2903,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, // anything that we don't have a specific matcher for silently returns a 404 fetchMock.catch(404); - keyResponder = new E2EKeyResponder(aliceClient.getHomeserverUrl()); + const keyResponder = new E2EKeyResponder(aliceClient.getHomeserverUrl()); keyResponder.addCrossSigningData(SIGNED_CROSS_SIGNING_KEYS_DATA); keyResponder.addDeviceKeys(SIGNED_TEST_DEVICE_DATA); keyResponder.addKeyReceiver(BOB_TEST_USER_ID, keyReceiver); @@ -2939,4 +2939,180 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, expect(hasCrossSigningKeysForUser).toBe(false); }); }); + + /** Guards against downgrade attacks from servers hiding or manipulating the crypto settings. */ + describe("Persistent encryption settings", () => { + let persistentStoreClient: MatrixClient; + let client2: MatrixClient; + + beforeEach(async () => { + const homeserverurl = "https://alice-server.com"; + const userId = "@alice:localhost"; + + const keyResponder = new E2EKeyResponder(homeserverurl); + keyResponder.addKeyReceiver(userId, keyReceiver); + + // For legacy crypto, these tests only work properly with a proper (indexeddb-based) CryptoStore, so + // rather than using the existing `aliceClient`, create a new client. Once we drop legacy crypto, we can + // just use `aliceClient` here. + persistentStoreClient = await makeNewClient(homeserverurl, userId, "persistentStoreClient"); + await persistentStoreClient.startClient({}); + }); + + afterEach(async () => { + persistentStoreClient.stopClient(); + client2?.stopClient(); + }); + + test("Sending a message in a room where the server is hiding the state event does not send a plaintext event", async () => { + // Alice is in an encrypted room + const encryptionState = mkEncryptionEvent({ algorithm: "m.megolm.v1.aes-sha2" }); + syncResponder.sendOrQueueSyncResponse(getSyncResponseWithState([encryptionState])); + await syncPromise(persistentStoreClient); + + // Send a message, and expect to get an `m.room.encrypted` event. + await Promise.all([persistentStoreClient.sendTextMessage(ROOM_ID, "test"), expectEncryptedSendMessage()]); + + // We now replace the client, and allow the new one to resync, *without* the encryption event. + client2 = await replaceClient(persistentStoreClient); + syncResponder.sendOrQueueSyncResponse(getSyncResponseWithState([])); + await client2.startClient({}); + await syncPromise(client2); + logger.log(client2.getUserId() + ": restarted"); + + await expectSendMessageToFail(client2); + }); + + test("Changes to the rotation period should be ignored", async () => { + // Alice is in an encrypted room, where the rotation period is set to 2 messages + const encryptionState = mkEncryptionEvent({ algorithm: "m.megolm.v1.aes-sha2", rotation_period_msgs: 2 }); + syncResponder.sendOrQueueSyncResponse(getSyncResponseWithState([encryptionState])); + await syncPromise(persistentStoreClient); + + // Send a message, and expect to get an `m.room.encrypted` event. + const [, msg1Content] = await Promise.all([ + persistentStoreClient.sendTextMessage(ROOM_ID, "test1"), + expectEncryptedSendMessage(), + ]); + + // Replace the state with one which bumps the rotation period. This should be ignored, though it's not + // clear that is correct behaviour (see https://github.com/element-hq/element-meta/issues/69) + const encryptionState2 = mkEncryptionEvent({ + algorithm: "m.megolm.v1.aes-sha2", + rotation_period_msgs: 100, + }); + syncResponder.sendOrQueueSyncResponse({ + next_batch: "1", + rooms: { join: { [TEST_ROOM_ID]: { timeline: { events: [encryptionState2], prev_batch: "" } } } }, + }); + await syncPromise(persistentStoreClient); + + // Send two more messages. The first should use the same megolm session as the first; the second should + // use a different one. + const [, msg2Content] = await Promise.all([ + persistentStoreClient.sendTextMessage(ROOM_ID, "test2"), + expectEncryptedSendMessage(), + ]); + expect(msg2Content.session_id).toEqual(msg1Content.session_id); + const [, msg3Content] = await Promise.all([ + persistentStoreClient.sendTextMessage(ROOM_ID, "test3"), + expectEncryptedSendMessage(), + ]); + expect(msg3Content.session_id).not.toEqual(msg1Content.session_id); + }); + + test("Changes to the rotation period should be ignored after a client restart", async () => { + // Alice is in an encrypted room, where the rotation period is set to 2 messages + const encryptionState = mkEncryptionEvent({ algorithm: "m.megolm.v1.aes-sha2", rotation_period_msgs: 2 }); + syncResponder.sendOrQueueSyncResponse(getSyncResponseWithState([encryptionState])); + await syncPromise(persistentStoreClient); + + // Send a message, and expect to get an `m.room.encrypted` event. + await Promise.all([persistentStoreClient.sendTextMessage(ROOM_ID, "test1"), expectEncryptedSendMessage()]); + + // We now replace the client, and allow the new one to resync with a *different* encryption event. + client2 = await replaceClient(persistentStoreClient); + const encryptionState2 = mkEncryptionEvent({ + algorithm: "m.megolm.v1.aes-sha2", + rotation_period_msgs: 100, + }); + syncResponder.sendOrQueueSyncResponse(getSyncResponseWithState([encryptionState2])); + await client2.startClient({}); + await syncPromise(client2); + logger.log(client2.getUserId() + ": restarted"); + + // Now send another message, which should (for now) be rejected. + await expectSendMessageToFail(client2); + }); + + /** Shut down `oldClient`, and build a new MatrixClient for the same user. */ + async function replaceClient(oldClient: MatrixClient) { + oldClient.stopClient(); + syncResponder.sendOrQueueSyncResponse({}); // flush pending request from old client + return makeNewClient(oldClient.getHomeserverUrl(), oldClient.getSafeUserId(), "client2"); + } + + async function makeNewClient( + homeserverUrl: string, + userId: string, + loggerPrefix: string, + ): Promise { + const client = createClient({ + baseUrl: homeserverUrl, + userId: userId, + accessToken: "akjgkrgjs", + deviceId: "xzcvb", + cryptoCallbacks: createCryptoCallbacks(), + logger: logger.getChild(loggerPrefix), + + // For legacy crypto, these tests only work with a proper persistent cryptoStore. + cryptoStore: new IndexedDBCryptoStore(indexedDB, "test"), + }); + await initCrypto(client); + mockInitialApiRequests(client.getHomeserverUrl()); + return client; + } + + function mkEncryptionEvent(content: Object) { + return mkEventCustom({ + sender: persistentStoreClient.getSafeUserId(), + type: "m.room.encryption", + state_key: "", + content: content, + }); + } + + /** Sync response which includes `TEST_ROOM_ID`, where alice is a member + * + * @param stateEvents - Additional state events for the test room + */ + function getSyncResponseWithState(stateEvents: Array) { + const roomResponse = { + state: { + events: [ + mkMembershipCustom({ membership: "join", sender: persistentStoreClient.getSafeUserId() }), + ...stateEvents, + ], + }, + timeline: { + events: [], + prev_batch: "", + }, + }; + + return { + next_batch: "1", + rooms: { join: { [TEST_ROOM_ID]: roomResponse } }, + }; + } + + /** Send a message with the given client, and check that it is not sent in plaintext */ + async function expectSendMessageToFail(aliceClient2: MatrixClient) { + // The precise failure mode here is somewhat up for debate (https://github.com/element-hq/element-meta/issues/69). + // For now, the attempt to send is rejected with an exception. The text is different between old and new stacks. + await expect(aliceClient2.sendTextMessage(ROOM_ID, "test")).rejects.toThrow( + /unconfigured room !room:id|Room !room:id was previously configured to use encryption/, + ); + } + }); }); diff --git a/spec/test-utils/mockEndpoints.ts b/spec/test-utils/mockEndpoints.ts index 22dda0b88b2..988d6f13b6c 100644 --- a/spec/test-utils/mockEndpoints.ts +++ b/spec/test-utils/mockEndpoints.ts @@ -24,11 +24,21 @@ import { KeyBackupInfo } from "../../src/crypto-api"; * @param homeserverUrl - the homeserver url for the client under test */ export function mockInitialApiRequests(homeserverUrl: string) { - fetchMock.getOnce(new URL("/_matrix/client/versions", homeserverUrl).toString(), { versions: ["v1.1"] }); - fetchMock.getOnce(new URL("/_matrix/client/v3/pushrules/", homeserverUrl).toString(), {}); - fetchMock.postOnce(new URL("/_matrix/client/v3/user/%40alice%3Alocalhost/filter", homeserverUrl).toString(), { - filter_id: "fid", - }); + fetchMock.getOnce( + new URL("/_matrix/client/versions", homeserverUrl).toString(), + { versions: ["v1.1"] }, + { overwriteRoutes: true }, + ); + fetchMock.getOnce( + new URL("/_matrix/client/v3/pushrules/", homeserverUrl).toString(), + {}, + { overwriteRoutes: true }, + ); + fetchMock.postOnce( + new URL("/_matrix/client/v3/user/%40alice%3Alocalhost/filter", homeserverUrl).toString(), + { filter_id: "fid" }, + { overwriteRoutes: true }, + ); } /** diff --git a/spec/unit/matrix-client.spec.ts b/spec/unit/matrix-client.spec.ts index f1382bca563..f9fab0864ba 100644 --- a/spec/unit/matrix-client.spec.ts +++ b/spec/unit/matrix-client.spec.ts @@ -1469,8 +1469,8 @@ describe("MatrixClient", function () { expect(getRoomId).toEqual(roomId); return mockRoom; }; - mockCrypto = { + isEncryptionEnabledInRoom: jest.fn().mockResolvedValue(true), encryptEvent: jest.fn(), stop: jest.fn(), } as unknown as Mocked; diff --git a/src/client.ts b/src/client.ts index 78011c1b83c..23bf2af75fd 100644 --- a/src/client.ts +++ b/src/client.ts @@ -3251,6 +3251,9 @@ export class MatrixClient extends TypedEventEmitter { if (!this.crypto) { @@ -3263,6 +3266,9 @@ export class MatrixClient extends TypedEventEmitter { if (event.isEncrypted()) { // this event has already been encrypted; this happens if the // encryption step succeeded, but the send step failed on the first @@ -4878,7 +4884,7 @@ export class MatrixClient extends TypedEventEmitter; + /** + * Check if we believe the given room to be encrypted. + * + * This method returns true if the room has been configured with encryption. The setting is persistent, so that + * even if the encryption event is removed from the room state, it still returns true. This helps to guard against + * a downgrade attack wherein a server admin attempts to remove encryption. + * + * @returns `true` if the room with the supplied ID is encrypted. `false` if the room is not encrypted, or is unknown to + * us. + */ + isEncryptionEnabledInRoom(roomId: string): Promise; + /** * Perform any background tasks that can be done before a message is ready to * send, in order to speed up sending of the message. @@ -189,7 +201,7 @@ export interface CryptoApi { * Cross-signing a device indicates, to our other devices and to other users, that we have verified that it really * belongs to us. * - * Requires that cross-signing has been set up on this device (normally by calling {@link bootstrapCrossSigning}. + * Requires that cross-signing has been set up on this device (normally by calling {@link bootstrapCrossSigning}). * * *Note*: Do not call this unless you have verified, somehow, that the device is genuine! * diff --git a/src/crypto/index.ts b/src/crypto/index.ts index 16e37605b94..1a0e4f43dae 100644 --- a/src/crypto/index.ts +++ b/src/crypto/index.ts @@ -4273,6 +4273,13 @@ export class Crypto extends TypedEventEmitter { + return this.isRoomEncrypted(roomId); + } + /** * @returns information about the encryption on the room with the supplied * ID, or null if the room is not encrypted or unknown to us. diff --git a/src/rust-crypto/RoomEncryptor.ts b/src/rust-crypto/RoomEncryptor.ts index 199db95ca35..2de00f19b34 100644 --- a/src/rust-crypto/RoomEncryptor.ts +++ b/src/rust-crypto/RoomEncryptor.ts @@ -88,7 +88,8 @@ export class RoomEncryptor { */ public onCryptoEvent(config: IContent): void { if (JSON.stringify(this.encryptionSettings) != JSON.stringify(config)) { - this.prefixedLogger.error(`Ignoring m.room.encryption event which requests a change of config`); + // This should currently be unreachable, since the Rust SDK will reject any attempts to change config. + throw new Error("Cannot reconfigure an active RoomEncryptor"); } } diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 239fae8a833..27277f57c9e 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -311,6 +311,16 @@ export class RustCrypto extends TypedEventEmitter { + const roomSettings: RustSdkCryptoJs.RoomSettings | undefined = await this.olmMachine.getRoomSettings( + new RustSdkCryptoJs.RoomId(roomId), + ); + return Boolean(roomSettings?.algorithm); + } + /** * Implementation of {@link CryptoApi#getOwnDeviceKeys}. */ @@ -1285,7 +1295,27 @@ export class RustCrypto extends TypedEventEmitter { const config = event.getContent(); + const settings = new RustSdkCryptoJs.RoomSettings(); + + if (config.algorithm === "m.megolm.v1.aes-sha2") { + settings.algorithm = RustSdkCryptoJs.EncryptionAlgorithm.MegolmV1AesSha2; + } else { + // Among other situations, this happens if the crypto state event is redacted. + this.logger.warn(`Room ${room.roomId}: ignoring crypto event with invalid algorithm ${config.algorithm}`); + return; + } + + try { + settings.sessionRotationPeriodMs = config.rotation_period_ms; + settings.sessionRotationPeriodMessages = config.rotation_period_msgs; + await this.olmMachine.setRoomSettings(new RustSdkCryptoJs.RoomId(room.roomId), settings); + } catch (e) { + this.logger.warn(`Room ${room.roomId}: ignoring crypto event which caused error: ${e}`); + return; + } + // If we got this far, the SDK found the event acceptable. + // We need to either create or update the active RoomEncryptor. const existingEncryptor = this.roomEncryptors[room.roomId]; if (existingEncryptor) { existingEncryptor.onCryptoEvent(config);