From a20147439aa3f1ff0e42edd4cb2a870c80f25a1b Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Wed, 26 Jan 2022 20:44:33 +0100 Subject: [PATCH 01/14] make various changes for correctness --- src/crypto/algorithms/base.ts | 2 +- src/crypto/algorithms/megolm.ts | 167 ++++++++++++++++---------------- src/crypto/algorithms/olm.ts | 4 +- src/crypto/olmlib.ts | 2 +- 4 files changed, 89 insertions(+), 86 deletions(-) diff --git a/src/crypto/algorithms/base.ts b/src/crypto/algorithms/base.ts index add9111efe0..22bd4505d57 100644 --- a/src/crypto/algorithms/base.ts +++ b/src/crypto/algorithms/base.ts @@ -179,7 +179,7 @@ export abstract class DecryptionAlgorithm { * * @param {module:models/event.MatrixEvent} params event key event */ - public onRoomKeyEvent(params: MatrixEvent): void { + public async onRoomKeyEvent(params: MatrixEvent): Promise { // ignore by default } diff --git a/src/crypto/algorithms/megolm.ts b/src/crypto/algorithms/megolm.ts index 31beb64ef2c..4e23cd29ed4 100644 --- a/src/crypto/algorithms/megolm.ts +++ b/src/crypto/algorithms/megolm.ts @@ -213,6 +213,8 @@ class OutboundSessionInfo { } } } + + return false; } } @@ -231,7 +233,7 @@ class MegolmEncryption extends EncryptionAlgorithm { // are using, and which devices we have shared the keys with. It resolves // with an OutboundSessionInfo (or undefined, for the first message in the // room). - private setupPromise = Promise.resolve(undefined); + private setupPromise = Promise.resolve(null); // Map of outbound sessions by sessions ID. Used if we need a particular // session (the session we're currently using to send is always obtained @@ -240,8 +242,8 @@ class MegolmEncryption extends EncryptionAlgorithm { private readonly sessionRotationPeriodMsgs: number; private readonly sessionRotationPeriodMs: number; - private encryptionPreparation: Promise; - private encryptionPreparationMetadata: { + private encryptionPreparation?: { + promise: Promise; startTime: number; }; @@ -277,37 +279,37 @@ class MegolmEncryption extends EncryptionAlgorithm { // Updates `session` to hold the final OutboundSessionInfo. // // returns a promise which resolves once the keyshare is successful. - const prepareSession = async (oldSession: OutboundSessionInfo) => { - session = oldSession; - + const prepareSession = async (oldSession: OutboundSessionInfo | null) => { const sharedHistory = isRoomSharedHistory(room); // history visibility changed - if (session && sharedHistory !== session.sharedHistory) { - session = null; + if (oldSession != null && sharedHistory !== oldSession.sharedHistory) { + oldSession = null; } // need to make a brand new session? - if (session && session.needsRotation(this.sessionRotationPeriodMsgs, + if (oldSession != null && oldSession.needsRotation(this.sessionRotationPeriodMsgs, this.sessionRotationPeriodMs) ) { logger.log("Starting new megolm session because we need to rotate."); - session = null; + oldSession = null; } // determine if we have shared with anyone we shouldn't have - if (session && session.sharedWithTooManyDevices(devicesInRoom)) { - session = null; + if (oldSession != null && oldSession.sharedWithTooManyDevices(devicesInRoom)) { + oldSession = null; } - if (!session) { + if (oldSession == null) { logger.log(`Starting new megolm session for room ${this.roomId}`); - session = await this.prepareNewSession(sharedHistory); - logger.log(`Started new megolm session ${session.sessionId} ` + + oldSession = await this.prepareNewSession(sharedHistory); + logger.log(`Started new megolm session ${oldSession.sessionId} ` + `for room ${this.roomId}`); - this.outboundSessions[session.sessionId] = session; + this.outboundSessions[oldSession.sessionId] = oldSession; } + session = oldSession; + // now check if we need to share with any devices const shareMap: Record = {}; @@ -386,7 +388,7 @@ class MegolmEncryption extends EncryptionAlgorithm { for (const server of failedServers) { failedServerMap.add(server); } - const failedDevices = []; + const failedDevices: IOlmDevice[] = []; for (const { userId, deviceInfo } of errorDevices) { const userHS = userId.slice(userId.indexOf(":") + 1); if (failedServerMap.has(userHS)) { @@ -853,7 +855,7 @@ class MegolmEncryption extends EncryptionAlgorithm { logger.debug(`Ensuring Olm sessions for devices in ${this.roomId}`); const devicemap = await olmlib.ensureOlmSessionsForDevices( this.olmDevice, this.baseApis, devicesByUser, false, otkTimeout, failedServers, - logger.withPrefix(`[${this.roomId}]`), + logger.withPrefix?.(`[${this.roomId}]`), ); logger.debug(`Ensured Olm sessions for devices in ${this.roomId}`); @@ -993,11 +995,11 @@ class MegolmEncryption extends EncryptionAlgorithm { * @param {module:models/room} room the room the event is in */ public prepareToEncrypt(room: Room): void { - if (this.encryptionPreparation) { + if (this.encryptionPreparation != null) { // We're already preparing something, so don't do anything else. // FIXME: check if we need to restart // (https://github.com/matrix-org/matrix-js-sdk/issues/1255) - const elapsedTime = Date.now() - this.encryptionPreparationMetadata.startTime; + const elapsedTime = Date.now() - this.encryptionPreparation.startTime; logger.debug( `Already started preparing to encrypt for ${this.roomId} ` + `${elapsedTime} ms ago, skipping`, @@ -1007,32 +1009,31 @@ class MegolmEncryption extends EncryptionAlgorithm { logger.debug(`Preparing to encrypt events for ${this.roomId}`); - this.encryptionPreparationMetadata = { + this.encryptionPreparation = { startTime: Date.now(), - }; - this.encryptionPreparation = (async () => { - try { - logger.debug(`Getting devices in ${this.roomId}`); - const [devicesInRoom, blocked] = await this.getDevicesInRoom(room); - - if (this.crypto.getGlobalErrorOnUnknownDevices()) { - // Drop unknown devices for now. When the message gets sent, we'll - // throw an error, but we'll still be prepared to send to the known - // devices. - this.removeUnknownDevices(devicesInRoom); - } + promise: (async () => { + try { + logger.debug(`Getting devices in ${this.roomId}`); + const [devicesInRoom, blocked] = await this.getDevicesInRoom(room); + + if (this.crypto.getGlobalErrorOnUnknownDevices()) { + // Drop unknown devices for now. When the message gets sent, we'll + // throw an error, but we'll still be prepared to send to the known + // devices. + this.removeUnknownDevices(devicesInRoom); + } - logger.debug(`Ensuring outbound session in ${this.roomId}`); - await this.ensureOutboundSession(room, devicesInRoom, blocked, true); + logger.debug(`Ensuring outbound session in ${this.roomId}`); + await this.ensureOutboundSession(room, devicesInRoom, blocked, true); - logger.debug(`Ready to encrypt events for ${this.roomId}`); - } catch (e) { - logger.error(`Failed to prepare to encrypt events for ${this.roomId}`, e); - } finally { - delete this.encryptionPreparationMetadata; - delete this.encryptionPreparation; - } - })(); + logger.debug(`Ready to encrypt events for ${this.roomId}`); + } catch (e) { + logger.error(`Failed to prepare to encrypt events for ${this.roomId}`, e); + } finally { + delete this.encryptionPreparation; + } + })(), + }; } /** @@ -1047,12 +1048,12 @@ class MegolmEncryption extends EncryptionAlgorithm { public async encryptMessage(room: Room, eventType: string, content: object): Promise { logger.log(`Starting to encrypt event for ${this.roomId}`); - if (this.encryptionPreparation) { + if (this.encryptionPreparation != null) { // If we started sending keys, wait for it to be done. // FIXME: check if we need to cancel // (https://github.com/matrix-org/matrix-js-sdk/issues/1255) try { - await this.encryptionPreparation; + await this.encryptionPreparation.promise; } catch (e) { // ignore any errors -- if the preparation failed, we'll just // restart everything here @@ -1390,6 +1391,7 @@ class MegolmDecryption extends DecryptionAlgorithm { if (!senderPendingEvents.has(sessionId)) { senderPendingEvents.set(sessionId, new Set()); } + // @ts-ignore: TS isn't smart enough to figure out `has` + `set` above makes this non-null senderPendingEvents.get(sessionId).add(event); } @@ -1424,17 +1426,17 @@ class MegolmDecryption extends DecryptionAlgorithm { * * @param {module:models/event.MatrixEvent} event key event */ - public onRoomKeyEvent(event: MatrixEvent): Promise { - const content = event.getContent(); - const sessionId = content.session_id; + public async onRoomKeyEvent(event: MatrixEvent): Promise { + const content: Partial = event.getContent(); let senderKey = event.getSenderKey(); - let forwardingKeyChain = []; + let forwardingKeyChain: string[] = []; let exportFormat = false; let keysClaimed; if (!content.room_id || - !sessionId || - !content.session_key + !content.session_key || + !content.session_id || + !content.algorithm ) { logger.error("key event is missing fields"); return; @@ -1447,19 +1449,17 @@ class MegolmDecryption extends DecryptionAlgorithm { if (event.getType() == "m.forwarded_room_key") { exportFormat = true; - forwardingKeyChain = content.forwarding_curve25519_key_chain; - if (!Array.isArray(forwardingKeyChain)) { - forwardingKeyChain = []; - } + forwardingKeyChain = content.forwarding_curve25519_key_chain ?? []; // copy content before we modify it forwardingKeyChain = forwardingKeyChain.slice(); forwardingKeyChain.push(senderKey); - senderKey = content.sender_key; - if (!senderKey) { + if (!content.sender_key) { logger.error("forwarded_room_key event is missing sender_key field"); return; + } else { + senderKey = content.sender_key; } const ed25519Key = content.sender_claimed_ed25519_key; @@ -1481,34 +1481,34 @@ class MegolmDecryption extends DecryptionAlgorithm { if (content["org.matrix.msc3061.shared_history"]) { extraSessionData.sharedHistory = true; } - return this.olmDevice.addInboundGroupSession( - content.room_id, senderKey, forwardingKeyChain, sessionId, - content.session_key, keysClaimed, - exportFormat, extraSessionData, - ).then(() => { + + try { + await this.olmDevice.addInboundGroupSession( + content.room_id, senderKey, forwardingKeyChain, content.session_id, + content.session_key, keysClaimed, + exportFormat, extraSessionData, + ); + // have another go at decrypting events sent with this session. - this.retryDecryption(senderKey, sessionId) - .then((success) => { - // cancel any outstanding room key requests for this session. - // Only do this if we managed to decrypt every message in the - // session, because if we didn't, we leave the other key - // requests in the hopes that someone sends us a key that - // includes an earlier index. - if (success) { - this.crypto.cancelRoomKeyRequest({ - algorithm: content.algorithm, - room_id: content.room_id, - session_id: content.session_id, - sender_key: senderKey, - }); - } + if (await this.retryDecryption(senderKey, content.session_id)) { + // cancel any outstanding room key requests for this session. + // Only do this if we managed to decrypt every message in the + // session, because if we didn't, we leave the other key + // requests in the hopes that someone sends us a key that + // includes an earlier index. + this.crypto.cancelRoomKeyRequest({ + algorithm: content.algorithm, + room_id: content.room_id, + session_id: content.session_id, + sender_key: senderKey, }); - }).then(() => { + } + // don't wait for the keys to be backed up for the server - this.crypto.backupManager.backupGroupSession(senderKey, content.session_id); - }).catch((e) => { + await this.crypto.backupManager.backupGroupSession(senderKey, content.session_id); + } catch (e) { logger.error(`Error handling m.room_key_event: ${e}`); - }); + } } /** @@ -1701,7 +1701,10 @@ class MegolmDecryption extends DecryptionAlgorithm { * @param {boolean} [opts.untrusted] whether the key should be considered as untrusted * @param {string} [opts.source] where the key came from */ - public importRoomKey(session: IMegolmSessionData, opts: any = {}): Promise { + public importRoomKey( + session: IMegolmSessionData, + opts: { untrusted?: boolean, source?: string } = {}, + ): Promise { const extraSessionData: any = {}; if (opts.untrusted || session.untrusted) { extraSessionData.untrusted = true; diff --git a/src/crypto/algorithms/olm.ts b/src/crypto/algorithms/olm.ts index 36917873788..b157941509b 100644 --- a/src/crypto/algorithms/olm.ts +++ b/src/crypto/algorithms/olm.ts @@ -51,7 +51,7 @@ interface IMessage { */ class OlmEncryption extends EncryptionAlgorithm { private sessionPrepared = false; - private prepPromise: Promise = null; + private prepPromise: Promise | null = null; /** * @private @@ -116,7 +116,7 @@ class OlmEncryption extends EncryptionAlgorithm { ciphertext: {}, }; - const promises = []; + const promises: Promise[] = []; for (let i = 0; i < users.length; ++i) { const userId = users[i]; diff --git a/src/crypto/olmlib.ts b/src/crypto/olmlib.ts index 19c34fe2b55..a6fecbc7946 100644 --- a/src/crypto/olmlib.ts +++ b/src/crypto/olmlib.ts @@ -76,7 +76,7 @@ export interface IOlmSessionResult { export async function encryptMessageForDevice( resultsObject: Record, ourUserId: string, - ourDeviceId: string, + ourDeviceId: string | undefined, olmDevice: OlmDevice, recipientUserId: string, recipientDevice: DeviceInfo, From c801ea578ba61d7db81716b56e33b0d08fdf83a6 Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Thu, 27 Jan 2022 13:52:45 +0100 Subject: [PATCH 02/14] apply some review feedback --- src/crypto/algorithms/megolm.ts | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/crypto/algorithms/megolm.ts b/src/crypto/algorithms/megolm.ts index 4e23cd29ed4..b8b665a4ad5 100644 --- a/src/crypto/algorithms/megolm.ts +++ b/src/crypto/algorithms/megolm.ts @@ -283,24 +283,22 @@ class MegolmEncryption extends EncryptionAlgorithm { const sharedHistory = isRoomSharedHistory(room); // history visibility changed - if (oldSession != null && sharedHistory !== oldSession.sharedHistory) { + if (oldSession && sharedHistory !== oldSession.sharedHistory) { oldSession = null; } // need to make a brand new session? - if (oldSession != null && oldSession.needsRotation(this.sessionRotationPeriodMsgs, - this.sessionRotationPeriodMs) - ) { + if (oldSession?.needsRotation(this.sessionRotationPeriodMsgs, this.sessionRotationPeriodMs)) { logger.log("Starting new megolm session because we need to rotate."); oldSession = null; } // determine if we have shared with anyone we shouldn't have - if (oldSession != null && oldSession.sharedWithTooManyDevices(devicesInRoom)) { + if (oldSession?.sharedWithTooManyDevices(devicesInRoom)) { oldSession = null; } - if (oldSession == null) { + if (!oldSession) { logger.log(`Starting new megolm session for room ${this.roomId}`); oldSession = await this.prepareNewSession(sharedHistory); logger.log(`Started new megolm session ${oldSession.sessionId} ` + @@ -1427,7 +1425,7 @@ class MegolmDecryption extends DecryptionAlgorithm { * @param {module:models/event.MatrixEvent} event key event */ public async onRoomKeyEvent(event: MatrixEvent): Promise { - const content: Partial = event.getContent(); + const content = event.getContent>(); let senderKey = event.getSenderKey(); let forwardingKeyChain: string[] = []; let exportFormat = false; @@ -1458,9 +1456,8 @@ class MegolmDecryption extends DecryptionAlgorithm { if (!content.sender_key) { logger.error("forwarded_room_key event is missing sender_key field"); return; - } else { - senderKey = content.sender_key; } + senderKey = content.sender_key; const ed25519Key = content.sender_claimed_ed25519_key; if (!ed25519Key) { @@ -1484,9 +1481,14 @@ class MegolmDecryption extends DecryptionAlgorithm { try { await this.olmDevice.addInboundGroupSession( - content.room_id, senderKey, forwardingKeyChain, content.session_id, - content.session_key, keysClaimed, - exportFormat, extraSessionData, + content.room_id, + senderKey, + forwardingKeyChain, + content.session_id, + content.session_key, + keysClaimed, + exportFormat, + extraSessionData, ); // have another go at decrypting events sent with this session. From 99acb956d59b5ff5f88d0003bbac4b99d0793a52 Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Mon, 16 May 2022 16:47:56 +0200 Subject: [PATCH 03/14] Address some review feedback --- src/crypto/algorithms/megolm.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/crypto/algorithms/megolm.ts b/src/crypto/algorithms/megolm.ts index 7c317781dba..97e4a8f4afa 100644 --- a/src/crypto/algorithms/megolm.ts +++ b/src/crypto/algorithms/megolm.ts @@ -1390,8 +1390,7 @@ class MegolmDecryption extends DecryptionAlgorithm { if (!senderPendingEvents.has(sessionId)) { senderPendingEvents.set(sessionId, new Set()); } - // @ts-ignore: TS isn't smart enough to figure out `has` + `set` above makes this non-null - senderPendingEvents.get(sessionId).add(event); + senderPendingEvents.get(sessionId)?.add(event); } /** @@ -1448,7 +1447,8 @@ class MegolmDecryption extends DecryptionAlgorithm { if (event.getType() == "m.forwarded_room_key") { exportFormat = true; - forwardingKeyChain = content.forwarding_curve25519_key_chain ?? []; + forwardingKeyChain = Array.isArray(content.forwarding_curve25519_key_chain) ? + content.forwarding_curve25519_key_chain : []; // copy content before we modify it forwardingKeyChain = forwardingKeyChain.slice(); From d239fe0c1c4046b1ff74cc191f6bbaa0c00df8cb Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Mon, 16 May 2022 17:03:49 +0200 Subject: [PATCH 04/14] add some more correctness --- src/crypto/algorithms/olm.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/crypto/algorithms/olm.ts b/src/crypto/algorithms/olm.ts index d2017d0ed20..40b0cf69858 100644 --- a/src/crypto/algorithms/olm.ts +++ b/src/crypto/algorithms/olm.ts @@ -122,6 +122,10 @@ class OlmEncryption extends EncryptionAlgorithm { const userId = users[i]; const devices = this.crypto.getStoredDevicesForUser(userId); + if (devices == null) { + continue; + } + for (let j = 0; j < devices.length; ++j) { const deviceInfo = devices[j]; const key = deviceInfo.getIdentityKey(); @@ -239,7 +243,7 @@ class OlmDecryption extends DecryptionAlgorithm { throw new DecryptionError( "OLM_BAD_ROOM", "Message intended for room " + payload.room_id, { - reported_room: event.getRoomId(), + reported_room: event.getRoomId() || "ROOM_ID_UNDEFINED", }, ); } From 5466e8f41c213828325596c840954f0ffeaee7f5 Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Mon, 16 May 2022 17:34:15 +0200 Subject: [PATCH 05/14] refactor ensureOutboundSession to fit types better --- src/crypto/algorithms/megolm.ts | 338 +++++++++++++++++--------------- 1 file changed, 178 insertions(+), 160 deletions(-) diff --git a/src/crypto/algorithms/megolm.ts b/src/crypto/algorithms/megolm.ts index 97e4a8f4afa..99dc732575c 100644 --- a/src/crypto/algorithms/megolm.ts +++ b/src/crypto/algorithms/megolm.ts @@ -272,191 +272,209 @@ class MegolmEncryption extends EncryptionAlgorithm { blocked: IBlockedMap, singleOlmCreationPhase = false, ): Promise { - let session: OutboundSessionInfo; - // takes the previous OutboundSessionInfo, and considers whether to create // a new one. Also shares the key with any (new) devices in the room. - // Updates `session` to hold the final OutboundSessionInfo. + // + // Returns the successful session whether keyshare succeeds or not. // // returns a promise which resolves once the keyshare is successful. - const prepareSession = async (oldSession: OutboundSessionInfo | null) => { + const prepareSession = async (oldSession: OutboundSessionInfo | null): Promise => { const sharedHistory = isRoomSharedHistory(room); - // history visibility changed - if (oldSession && sharedHistory !== oldSession.sharedHistory) { - oldSession = null; - } + const session = await this.prepareSession(devicesInRoom, sharedHistory, oldSession); - // need to make a brand new session? - if (oldSession?.needsRotation(this.sessionRotationPeriodMsgs, this.sessionRotationPeriodMs)) { - logger.log("Starting new megolm session because we need to rotate."); - oldSession = null; + try { + await this.shareSession(devicesInRoom, sharedHistory, singleOlmCreationPhase, blocked, session); + } catch (e) { + logger.error(`Failed to ensure outbound session in ${this.roomId}`, e); } - // determine if we have shared with anyone we shouldn't have - if (oldSession?.sharedWithTooManyDevices(devicesInRoom)) { - oldSession = null; - } + return session; + }; - if (!oldSession) { - logger.log(`Starting new megolm session for room ${this.roomId}`); - oldSession = await this.prepareNewSession(sharedHistory); - logger.log(`Started new megolm session ${oldSession.sessionId} ` + - `for room ${this.roomId}`); - this.outboundSessions[oldSession.sessionId] = oldSession; - } + // first wait for the previous share to complete + const prom = this.setupPromise.then(prepareSession); - session = oldSession; + // Ensure any failures are logged for debugging + prom.catch(e => { + logger.error(`Failed to ensure outbound session in ${this.roomId}`, e); + }); - // now check if we need to share with any devices - const shareMap: Record = {}; + // setupPromise resolves to `session` whether or not the share succeeds + this.setupPromise = prom; - for (const [userId, userDevices] of Object.entries(devicesInRoom)) { - for (const [deviceId, deviceInfo] of Object.entries(userDevices)) { - const key = deviceInfo.getIdentityKey(); - if (key == this.olmDevice.deviceCurve25519Key) { - // don't bother sending to ourself - continue; - } + // but we return a promise which only resolves if the share was successful. + return prom; + } - if ( - !session.sharedWithDevices[userId] || - session.sharedWithDevices[userId][deviceId] === undefined - ) { - shareMap[userId] = shareMap[userId] || []; - shareMap[userId].push(deviceInfo); - } - } - } + private async prepareSession( + devicesInRoom: DeviceInfoMap, + sharedHistory: boolean, + session: OutboundSessionInfo | null, + ): Promise { + // history visibility changed + if (session && sharedHistory !== session.sharedHistory) { + session = null; + } - const key = this.olmDevice.getOutboundGroupSessionKey(session.sessionId); - const payload: IPayload = { - type: "m.room_key", - content: { - "algorithm": olmlib.MEGOLM_ALGORITHM, - "room_id": this.roomId, - "session_id": session.sessionId, - "session_key": key.key, - "chain_index": key.chain_index, - "org.matrix.msc3061.shared_history": sharedHistory, - }, - }; - const [devicesWithoutSession, olmSessions] = await olmlib.getExistingOlmSessions( - this.olmDevice, this.baseApis, shareMap, - ); + // need to make a brand new session? + if (session?.needsRotation(this.sessionRotationPeriodMsgs, this.sessionRotationPeriodMs)) { + logger.log("Starting new megolm session because we need to rotate."); + session = null; + } - await Promise.all([ - (async () => { - // share keys with devices that we already have a session for - logger.debug(`Sharing keys with existing Olm sessions in ${this.roomId}`, olmSessions); - await this.shareKeyWithOlmSessions(session, key, payload, olmSessions); - logger.debug(`Shared keys with existing Olm sessions in ${this.roomId}`); - })(), - (async () => { - logger.debug( - `Sharing keys (start phase 1) with new Olm sessions in ${this.roomId}`, - devicesWithoutSession, - ); - const errorDevices: IOlmDevice[] = []; - - // meanwhile, establish olm sessions for devices that we don't - // already have a session for, and share keys with them. If - // we're doing two phases of olm session creation, use a - // shorter timeout when fetching one-time keys for the first - // phase. - const start = Date.now(); - const failedServers: string[] = []; - await this.shareKeyWithDevices( - session, key, payload, devicesWithoutSession, errorDevices, - singleOlmCreationPhase ? 10000 : 2000, failedServers, - ); - logger.debug(`Shared keys (end phase 1) with new Olm sessions in ${this.roomId}`); - - if (!singleOlmCreationPhase && (Date.now() - start < 10000)) { - // perform the second phase of olm session creation if requested, - // and if the first phase didn't take too long - (async () => { - // Retry sending keys to devices that we were unable to establish - // an olm session for. This time, we use a longer timeout, but we - // do this in the background and don't block anything else while we - // do this. We only need to retry users from servers that didn't - // respond the first time. - const retryDevices: Record = {}; - const failedServerMap = new Set; - for (const server of failedServers) { - failedServerMap.add(server); - } - const failedDevices: IOlmDevice[] = []; - for (const { userId, deviceInfo } of errorDevices) { - const userHS = userId.slice(userId.indexOf(":") + 1); - if (failedServerMap.has(userHS)) { - retryDevices[userId] = retryDevices[userId] || []; - retryDevices[userId].push(deviceInfo); - } else { - // if we aren't going to retry, then handle it - // as a failed device - failedDevices.push({ userId, deviceInfo }); - } - } + // determine if we have shared with anyone we shouldn't have + if (session?.sharedWithTooManyDevices(devicesInRoom)) { + session = null; + } - logger.debug(`Sharing keys (start phase 2) with new Olm sessions in ${this.roomId}`); - await this.shareKeyWithDevices( - session, key, payload, retryDevices, failedDevices, 30000, - ); - logger.debug(`Shared keys (end phase 2) with new Olm sessions in ${this.roomId}`); + if (!session) { + logger.log(`Starting new megolm session for room ${this.roomId}`); + session = await this.prepareNewSession(sharedHistory); + logger.log(`Started new megolm session ${session.sessionId} ` + + `for room ${this.roomId}`); + this.outboundSessions[session.sessionId] = session; + } - await this.notifyFailedOlmDevices(session, key, failedDevices); - })(); - } else { - await this.notifyFailedOlmDevices(session, key, errorDevices); - } - logger.debug(`Shared keys (all phases done) with new Olm sessions in ${this.roomId}`); - })(), - (async () => { - logger.debug(`There are ${Object.entries(blocked).length} blocked devices in ${this.roomId}`, - Object.entries(blocked)); - - // also, notify newly blocked devices that they're blocked - logger.debug(`Notifying newly blocked devices in ${this.roomId}`); - const blockedMap: Record> = {}; - let blockedCount = 0; - for (const [userId, userBlockedDevices] of Object.entries(blocked)) { - for (const [deviceId, device] of Object.entries(userBlockedDevices)) { - if ( - !session.blockedDevicesNotified[userId] || - session.blockedDevicesNotified[userId][deviceId] === undefined - ) { - blockedMap[userId] = blockedMap[userId] || {}; - blockedMap[userId][deviceId] = { device }; - blockedCount++; - } - } - } + return session; + } - await this.notifyBlockedDevices(session, blockedMap); - logger.debug(`Notified ${blockedCount} newly blocked devices in ${this.roomId}`, blockedMap); - })(), - ]); - }; + private async shareSession( + devicesInRoom: DeviceInfoMap, + sharedHistory: boolean, + singleOlmCreationPhase: boolean, + blocked: IBlockedMap, + session: OutboundSessionInfo, + ) { + // now check if we need to share with any devices + const shareMap: Record = {}; - // helper which returns the session prepared by prepareSession - function returnSession() { - return session; + for (const [userId, userDevices] of Object.entries(devicesInRoom)) { + for (const [deviceId, deviceInfo] of Object.entries(userDevices)) { + const key = deviceInfo.getIdentityKey(); + if (key == this.olmDevice.deviceCurve25519Key) { + // don't bother sending to ourself + continue; + } + + if ( + !session.sharedWithDevices[userId] || + session.sharedWithDevices[userId][deviceId] === undefined + ) { + shareMap[userId] = shareMap[userId] || []; + shareMap[userId].push(deviceInfo); + } + } } - // first wait for the previous share to complete - const prom = this.setupPromise.then(prepareSession); + const key = this.olmDevice.getOutboundGroupSessionKey(session.sessionId); + const payload: IPayload = { + type: "m.room_key", + content: { + "algorithm": olmlib.MEGOLM_ALGORITHM, + "room_id": this.roomId, + "session_id": session.sessionId, + "session_key": key.key, + "chain_index": key.chain_index, + "org.matrix.msc3061.shared_history": sharedHistory, + }, + }; + const [devicesWithoutSession, olmSessions] = await olmlib.getExistingOlmSessions( + this.olmDevice, this.baseApis, shareMap, + ); - // Ensure any failures are logged for debugging - prom.catch(e => { - logger.error(`Failed to ensure outbound session in ${this.roomId}`, e); - }); + await Promise.all([ + (async () => { + // share keys with devices that we already have a session for + logger.debug(`Sharing keys with existing Olm sessions in ${this.roomId}`, olmSessions); + await this.shareKeyWithOlmSessions(session, key, payload, olmSessions); + logger.debug(`Shared keys with existing Olm sessions in ${this.roomId}`); + })(), + (async () => { + logger.debug( + `Sharing keys (start phase 1) with new Olm sessions in ${this.roomId}`, + devicesWithoutSession, + ); + const errorDevices: IOlmDevice[] = []; + + // meanwhile, establish olm sessions for devices that we don't + // already have a session for, and share keys with them. If + // we're doing two phases of olm session creation, use a + // shorter timeout when fetching one-time keys for the first + // phase. + const start = Date.now(); + const failedServers: string[] = []; + await this.shareKeyWithDevices( + session, key, payload, devicesWithoutSession, errorDevices, + singleOlmCreationPhase ? 10000 : 2000, failedServers, + ); + logger.debug(`Shared keys (end phase 1) with new Olm sessions in ${this.roomId}`); + + if (!singleOlmCreationPhase && (Date.now() - start < 10000)) { + // perform the second phase of olm session creation if requested, + // and if the first phase didn't take too long + (async () => { + // Retry sending keys to devices that we were unable to establish + // an olm session for. This time, we use a longer timeout, but we + // do this in the background and don't block anything else while we + // do this. We only need to retry users from servers that didn't + // respond the first time. + const retryDevices: Record = {}; + const failedServerMap = new Set; + for (const server of failedServers) { + failedServerMap.add(server); + } + const failedDevices: IOlmDevice[] = []; + for (const { userId, deviceInfo } of errorDevices) { + const userHS = userId.slice(userId.indexOf(":") + 1); + if (failedServerMap.has(userHS)) { + retryDevices[userId] = retryDevices[userId] || []; + retryDevices[userId].push(deviceInfo); + } else { + // if we aren't going to retry, then handle it + // as a failed device + failedDevices.push({ userId, deviceInfo }); + } + } - // setupPromise resolves to `session` whether or not the share succeeds - this.setupPromise = prom.then(returnSession, returnSession); + logger.debug(`Sharing keys (start phase 2) with new Olm sessions in ${this.roomId}`); + await this.shareKeyWithDevices( + session, key, payload, retryDevices, failedDevices, 30000, + ); + logger.debug(`Shared keys (end phase 2) with new Olm sessions in ${this.roomId}`); - // but we return a promise which only resolves if the share was successful. - return prom.then(returnSession); + await this.notifyFailedOlmDevices(session, key, failedDevices); + })(); + } else { + await this.notifyFailedOlmDevices(session, key, errorDevices); + } + logger.debug(`Shared keys (all phases done) with new Olm sessions in ${this.roomId}`); + })(), + (async () => { + logger.debug(`There are ${Object.entries(blocked).length} blocked devices in ${this.roomId}`, + Object.entries(blocked)); + + // also, notify newly blocked devices that they're blocked + logger.debug(`Notifying newly blocked devices in ${this.roomId}`); + const blockedMap: Record> = {}; + let blockedCount = 0; + for (const [userId, userBlockedDevices] of Object.entries(blocked)) { + for (const [deviceId, device] of Object.entries(userBlockedDevices)) { + if ( + !session.blockedDevicesNotified[userId] || + session.blockedDevicesNotified[userId][deviceId] === undefined + ) { + blockedMap[userId] = blockedMap[userId] || {}; + blockedMap[userId][deviceId] = { device }; + blockedCount++; + } + } + } + + await this.notifyBlockedDevices(session, blockedMap); + logger.debug(`Notified ${blockedCount} newly blocked devices in ${this.roomId}`, blockedMap); + })(), + ]); } /** From a44fa6c745a8bb1fd541458a409691e63b726949 Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Mon, 16 May 2022 17:37:17 +0200 Subject: [PATCH 06/14] change variable naming slightly to prevent confusion --- src/crypto/algorithms/megolm.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/crypto/algorithms/megolm.ts b/src/crypto/algorithms/megolm.ts index 99dc732575c..38441e37ed3 100644 --- a/src/crypto/algorithms/megolm.ts +++ b/src/crypto/algorithms/megolm.ts @@ -278,7 +278,7 @@ class MegolmEncryption extends EncryptionAlgorithm { // Returns the successful session whether keyshare succeeds or not. // // returns a promise which resolves once the keyshare is successful. - const prepareSession = async (oldSession: OutboundSessionInfo | null): Promise => { + const setup = async (oldSession: OutboundSessionInfo | null): Promise => { const sharedHistory = isRoomSharedHistory(room); const session = await this.prepareSession(devicesInRoom, sharedHistory, oldSession); @@ -293,7 +293,7 @@ class MegolmEncryption extends EncryptionAlgorithm { }; // first wait for the previous share to complete - const prom = this.setupPromise.then(prepareSession); + const prom = this.setupPromise.then(setup); // Ensure any failures are logged for debugging prom.catch(e => { From 5ad296934f8c3dc3b00a92e166dd43989b9d93c2 Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Mon, 16 May 2022 17:49:31 +0200 Subject: [PATCH 07/14] some wording around exception-catching --- src/crypto/algorithms/megolm.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crypto/algorithms/megolm.ts b/src/crypto/algorithms/megolm.ts index 38441e37ed3..44053b2739b 100644 --- a/src/crypto/algorithms/megolm.ts +++ b/src/crypto/algorithms/megolm.ts @@ -297,7 +297,7 @@ class MegolmEncryption extends EncryptionAlgorithm { // Ensure any failures are logged for debugging prom.catch(e => { - logger.error(`Failed to ensure outbound session in ${this.roomId}`, e); + logger.error(`Failed to setup outbound session in ${this.roomId}`, e); }); // setupPromise resolves to `session` whether or not the share succeeds From 0747c0c977a449a0e1c7e96a3767dd03a7ce3a85 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 10 Jun 2022 12:14:45 +0100 Subject: [PATCH 08/14] Tidy test --- spec/unit/crypto.spec.js | 249 +++++++++++++++++++-------------------- 1 file changed, 122 insertions(+), 127 deletions(-) diff --git a/spec/unit/crypto.spec.js b/spec/unit/crypto.spec.js index 3c3a738becd..a50ded5a426 100644 --- a/spec/unit/crypto.spec.js +++ b/spec/unit/crypto.spec.js @@ -17,6 +17,14 @@ import { logger } from '../../src/logger'; const Olm = global.Olm; +function awaitEvent(emitter, event) { + return new Promise((resolve, reject) => { + emitter.once(event, (result) => { + resolve(result); + }); + }); +} + describe("Crypto", function() { if (!CRYPTO_ENABLED) { return; @@ -203,136 +211,123 @@ describe("Crypto", function() { bobClient.stopClient(); }); - it( - "does not cancel keyshare requests if some messages are not decrypted", - async function() { - function awaitEvent(emitter, event) { - return new Promise((resolve, reject) => { - emitter.once(event, (result) => { - resolve(result); - }); - }); + it("does not cancel keyshare requests if some messages are not decrypted", async function() { + async function keyshareEventForEvent(event, index) { + const eventContent = event.getWireContent(); + const key = await aliceClient.crypto.olmDevice + .getInboundGroupSessionKey( + roomId, eventContent.sender_key, eventContent.session_id, + index, + ); + const ksEvent = new MatrixEvent({ + type: "m.forwarded_room_key", + sender: "@alice:example.com", + content: { + algorithm: olmlib.MEGOLM_ALGORITHM, + room_id: roomId, + sender_key: eventContent.sender_key, + sender_claimed_ed25519_key: key.sender_claimed_ed25519_key, + session_id: eventContent.session_id, + session_key: key.key, + chain_index: key.chain_index, + forwarding_curve25519_key_chain: + key.forwarding_curve_key_chain, + }, + }); + // make onRoomKeyEvent think this was an encrypted event + ksEvent.senderCurve25519Key = "akey"; + return ksEvent; + } + + const encryptionCfg = { + "algorithm": "m.megolm.v1.aes-sha2", + }; + const roomId = "!someroom"; + const aliceRoom = new Room(roomId, aliceClient, "@alice:example.com", {}); + const bobRoom = new Room(roomId, bobClient, "@bob:example.com", {}); + aliceClient.store.storeRoom(aliceRoom); + bobClient.store.storeRoom(bobRoom); + await aliceClient.setRoomEncryption(roomId, encryptionCfg); + await bobClient.setRoomEncryption(roomId, encryptionCfg); + const events = [ + new MatrixEvent({ + type: "m.room.message", + sender: "@alice:example.com", + room_id: roomId, + event_id: "$1", + content: { + msgtype: "m.text", + body: "1", + }, + }), + new MatrixEvent({ + type: "m.room.message", + sender: "@alice:example.com", + room_id: roomId, + event_id: "$2", + content: { + msgtype: "m.text", + body: "2", + }, + }), + ]; + await Promise.all(events.map(async (event) => { + // alice encrypts each event, and then bob tries to decrypt + // them without any keys, so that they'll be in pending + await aliceClient.crypto.encryptEvent(event, aliceRoom); + event.clearEvent = undefined; + event.senderCurve25519Key = null; + event.claimedEd25519Key = null; + try { + await bobClient.crypto.decryptEvent(event); + } catch (e) { + // we expect this to fail because we don't have the + // decryption keys yet } + })); - async function keyshareEventForEvent(event, index) { - const eventContent = event.getWireContent(); - const key = await aliceClient.crypto.olmDevice - .getInboundGroupSessionKey( - roomId, eventContent.sender_key, eventContent.session_id, - index, - ); - const ksEvent = new MatrixEvent({ - type: "m.forwarded_room_key", - sender: "@alice:example.com", - content: { - algorithm: olmlib.MEGOLM_ALGORITHM, - room_id: roomId, - sender_key: eventContent.sender_key, - sender_claimed_ed25519_key: key.sender_claimed_ed25519_key, - session_id: eventContent.session_id, - session_key: key.key, - chain_index: key.chain_index, - forwarding_curve25519_key_chain: - key.forwarding_curve_key_chain, - }, - }); - // make onRoomKeyEvent think this was an encrypted event - ksEvent.senderCurve25519Key = "akey"; - return ksEvent; - } + const bobDecryptor = bobClient.crypto.getRoomDecryptor( + roomId, olmlib.MEGOLM_ALGORITHM, + ); - const encryptionCfg = { - "algorithm": "m.megolm.v1.aes-sha2", - }; - const roomId = "!someroom"; - const aliceRoom = new Room(roomId, aliceClient, "@alice:example.com", {}); - const bobRoom = new Room(roomId, bobClient, "@bob:example.com", {}); - aliceClient.store.storeRoom(aliceRoom); - bobClient.store.storeRoom(bobRoom); - await aliceClient.setRoomEncryption(roomId, encryptionCfg); - await bobClient.setRoomEncryption(roomId, encryptionCfg); - const events = [ - new MatrixEvent({ - type: "m.room.message", - sender: "@alice:example.com", - room_id: roomId, - event_id: "$1", - content: { - msgtype: "m.text", - body: "1", - }, - }), - new MatrixEvent({ - type: "m.room.message", - sender: "@alice:example.com", - room_id: roomId, - event_id: "$2", - content: { - msgtype: "m.text", - body: "2", - }, - }), - ]; - await Promise.all(events.map(async (event) => { - // alice encrypts each event, and then bob tries to decrypt - // them without any keys, so that they'll be in pending - await aliceClient.crypto.encryptEvent(event, aliceRoom); - event.clearEvent = undefined; - event.senderCurve25519Key = null; - event.claimedEd25519Key = null; - try { - await bobClient.crypto.decryptEvent(event); - } catch (e) { - // we expect this to fail because we don't have the - // decryption keys yet - } - })); - - const bobDecryptor = bobClient.crypto.getRoomDecryptor( - roomId, olmlib.MEGOLM_ALGORITHM, - ); - - let eventPromise = Promise.all(events.map((ev) => { - return awaitEvent(ev, "Event.decrypted"); - })); - - // keyshare the session key starting at the second message, so - // the first message can't be decrypted yet, but the second one - // can - let ksEvent = await keyshareEventForEvent(events[1], 1); - await bobDecryptor.onRoomKeyEvent(ksEvent); - await eventPromise; - expect(events[0].getContent().msgtype).toBe("m.bad.encrypted"); - expect(events[1].getContent().msgtype).not.toBe("m.bad.encrypted"); - - const cryptoStore = bobClient.cryptoStore; - const eventContent = events[0].getWireContent(); - const senderKey = eventContent.sender_key; - const sessionId = eventContent.session_id; - const roomKeyRequestBody = { - algorithm: olmlib.MEGOLM_ALGORITHM, - room_id: roomId, - sender_key: senderKey, - session_id: sessionId, - }; - // the room key request should still be there, since we haven't - // decrypted everything - expect(await cryptoStore.getOutgoingRoomKeyRequest(roomKeyRequestBody)) - .toBeDefined(); - - // keyshare the session key starting at the first message, so - // that it can now be decrypted - eventPromise = awaitEvent(events[0], "Event.decrypted"); - ksEvent = await keyshareEventForEvent(events[0], 0); - await bobDecryptor.onRoomKeyEvent(ksEvent); - await eventPromise; - expect(events[0].getContent().msgtype).not.toBe("m.bad.encrypted"); - await sleep(1); - // the room key request should be gone since we've now decrypted everything - expect(await cryptoStore.getOutgoingRoomKeyRequest(roomKeyRequestBody)) - .toBeFalsy(); - }, - ); + let eventPromise = Promise.all(events.map((ev) => { + return awaitEvent(ev, "Event.decrypted"); + })); + + // keyshare the session key starting at the second message, so + // the first message can't be decrypted yet, but the second one + // can + let ksEvent = await keyshareEventForEvent(events[1], 1); + await bobDecryptor.onRoomKeyEvent(ksEvent); + await eventPromise; + expect(events[0].getContent().msgtype).toBe("m.bad.encrypted"); + expect(events[1].getContent().msgtype).not.toBe("m.bad.encrypted"); + + const cryptoStore = bobClient.cryptoStore; + const eventContent = events[0].getWireContent(); + const senderKey = eventContent.sender_key; + const sessionId = eventContent.session_id; + const roomKeyRequestBody = { + algorithm: olmlib.MEGOLM_ALGORITHM, + room_id: roomId, + sender_key: senderKey, + session_id: sessionId, + }; + // the room key request should still be there, since we haven't + // decrypted everything + expect(await cryptoStore.getOutgoingRoomKeyRequest(roomKeyRequestBody)).toBeDefined(); + + // keyshare the session key starting at the first message, so + // that it can now be decrypted + eventPromise = awaitEvent(events[0], "Event.decrypted"); + ksEvent = await keyshareEventForEvent(events[0], 0); + await bobDecryptor.onRoomKeyEvent(ksEvent); + await eventPromise; + expect(events[0].getContent().msgtype).not.toBe("m.bad.encrypted"); + await sleep(1); + // the room key request should be gone since we've now decrypted everything + expect(await cryptoStore.getOutgoingRoomKeyRequest(roomKeyRequestBody)).toBeFalsy(); + }); it("creates a new keyshare request if we request a keyshare", async function() { // make sure that cancelAndResend... creates a new keyshare request From 243d6d475d1cf81af798e519094e88b97736a238 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 10 Jun 2022 13:25:46 +0100 Subject: [PATCH 09/14] Simplify --- src/crypto/algorithms/olm.ts | 6 +----- src/crypto/index.ts | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/crypto/algorithms/olm.ts b/src/crypto/algorithms/olm.ts index 40b0cf69858..9dcd1be61d3 100644 --- a/src/crypto/algorithms/olm.ts +++ b/src/crypto/algorithms/olm.ts @@ -120,11 +120,7 @@ class OlmEncryption extends EncryptionAlgorithm { for (let i = 0; i < users.length; ++i) { const userId = users[i]; - const devices = this.crypto.getStoredDevicesForUser(userId); - - if (devices == null) { - continue; - } + const devices = this.crypto.getStoredDevicesForUser(userId) || []; for (let j = 0; j < devices.length; ++j) { const deviceInfo = devices[j]; diff --git a/src/crypto/index.ts b/src/crypto/index.ts index 474bfd3f636..153ab20eb0e 100644 --- a/src/crypto/index.ts +++ b/src/crypto/index.ts @@ -2590,7 +2590,7 @@ export class Crypto extends TypedEventEmitter = null; if (!existingConfig) { storeConfigPromise = this.roomList.setRoomEncryption(roomId, config); } From a3fe49a79e8a7012afb726b6a6d87a223af331b7 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 10 Jun 2022 14:18:34 +0100 Subject: [PATCH 10/14] Add tests --- spec/unit/crypto.spec.js | 104 +++++++++++++++------ spec/unit/crypto/algorithms/megolm.spec.js | 12 +++ src/crypto/algorithms/megolm.ts | 2 +- src/crypto/olmlib.ts | 2 +- 4 files changed, 89 insertions(+), 31 deletions(-) diff --git a/spec/unit/crypto.spec.js b/spec/unit/crypto.spec.js index 1781f7a59c3..9675db13ce1 100644 --- a/spec/unit/crypto.spec.js +++ b/spec/unit/crypto.spec.js @@ -25,6 +25,34 @@ function awaitEvent(emitter, event) { }); } +async function keyshareEventForEvent(client, event, index) { + const roomId = event.getRoomId(); + const eventContent = event.getWireContent(); + const key = await client.crypto.olmDevice + .getInboundGroupSessionKey( + roomId, eventContent.sender_key, eventContent.session_id, + index, + ); + const ksEvent = new MatrixEvent({ + type: "m.forwarded_room_key", + sender: "@alice:example.com", + content: { + algorithm: olmlib.MEGOLM_ALGORITHM, + room_id: roomId, + sender_key: eventContent.sender_key, + sender_claimed_ed25519_key: key.sender_claimed_ed25519_key, + session_id: eventContent.session_id, + session_key: key.key, + chain_index: key.chain_index, + forwarding_curve25519_key_chain: + key.forwarding_curve_key_chain, + }, + }); + // make onRoomKeyEvent think this was an encrypted event + ksEvent.senderCurve25519Key = "akey"; + return ksEvent; +} + describe("Crypto", function() { if (!CRYPTO_ENABLED) { return; @@ -212,33 +240,6 @@ describe("Crypto", function() { }); it("does not cancel keyshare requests if some messages are not decrypted", async function() { - async function keyshareEventForEvent(event, index) { - const eventContent = event.getWireContent(); - const key = await aliceClient.crypto.olmDevice - .getInboundGroupSessionKey( - roomId, eventContent.sender_key, eventContent.session_id, - index, - ); - const ksEvent = new MatrixEvent({ - type: "m.forwarded_room_key", - sender: "@alice:example.com", - content: { - algorithm: olmlib.MEGOLM_ALGORITHM, - room_id: roomId, - sender_key: eventContent.sender_key, - sender_claimed_ed25519_key: key.sender_claimed_ed25519_key, - session_id: eventContent.session_id, - session_key: key.key, - chain_index: key.chain_index, - forwarding_curve25519_key_chain: - key.forwarding_curve_key_chain, - }, - }); - // make onRoomKeyEvent think this was an encrypted event - ksEvent.senderCurve25519Key = "akey"; - return ksEvent; - } - const encryptionCfg = { "algorithm": "m.megolm.v1.aes-sha2", }; @@ -297,7 +298,7 @@ describe("Crypto", function() { // keyshare the session key starting at the second message, so // the first message can't be decrypted yet, but the second one // can - let ksEvent = await keyshareEventForEvent(events[1], 1); + let ksEvent = await keyshareEventForEvent(aliceClient, events[1], 1); await bobDecryptor.onRoomKeyEvent(ksEvent); await eventPromise; expect(events[0].getContent().msgtype).toBe("m.bad.encrypted"); @@ -320,7 +321,7 @@ describe("Crypto", function() { // keyshare the session key starting at the first message, so // that it can now be decrypted eventPromise = awaitEvent(events[0], "Event.decrypted"); - ksEvent = await keyshareEventForEvent(events[0], 0); + ksEvent = await keyshareEventForEvent(aliceClient, events[0], 0); await bobDecryptor.onRoomKeyEvent(ksEvent); await eventPromise; expect(events[0].getContent().msgtype).not.toBe("m.bad.encrypted"); @@ -329,6 +330,51 @@ describe("Crypto", function() { expect(await cryptoStore.getOutgoingRoomKeyRequest(roomKeyRequestBody)).toBeFalsy(); }); + it("should error if a forwarded room key lacks a content.sender_key", async function() { + const encryptionCfg = { + "algorithm": "m.megolm.v1.aes-sha2", + }; + const roomId = "!someroom"; + const aliceRoom = new Room(roomId, aliceClient, "@alice:example.com", {}); + const bobRoom = new Room(roomId, bobClient, "@bob:example.com", {}); + aliceClient.store.storeRoom(aliceRoom); + bobClient.store.storeRoom(bobRoom); + await aliceClient.setRoomEncryption(roomId, encryptionCfg); + await bobClient.setRoomEncryption(roomId, encryptionCfg); + const event = new MatrixEvent({ + type: "m.room.message", + sender: "@alice:example.com", + room_id: roomId, + event_id: "$1", + content: { + msgtype: "m.text", + body: "1", + }, + }); + // alice encrypts each event, and then bob tries to decrypt + // them without any keys, so that they'll be in pending + await aliceClient.crypto.encryptEvent(event, aliceRoom); + event.clearEvent = undefined; + event.senderCurve25519Key = null; + event.claimedEd25519Key = null; + try { + await bobClient.crypto.decryptEvent(event); + } catch (e) { + // we expect this to fail because we don't have the + // decryption keys yet + } + + const bobDecryptor = bobClient.crypto.getRoomDecryptor( + roomId, olmlib.MEGOLM_ALGORITHM, + ); + + const ksEvent = await keyshareEventForEvent(aliceClient, event, 1); + ksEvent.getContent().sender_key = undefined; // test + bobClient.crypto.addInboundGroupSession = jest.fn(); + await bobDecryptor.onRoomKeyEvent(ksEvent); + expect(bobClient.crypto.addInboundGroupSession).not.toHaveBeenCalled(); + }); + it("creates a new keyshare request if we request a keyshare", async function() { // make sure that cancelAndResend... creates a new keyshare request // if there wasn't an already-existing one diff --git a/spec/unit/crypto/algorithms/megolm.spec.js b/spec/unit/crypto/algorithms/megolm.spec.js index de8894210ad..5e4a4233cc7 100644 --- a/spec/unit/crypto/algorithms/megolm.spec.js +++ b/spec/unit/crypto/algorithms/megolm.spec.js @@ -329,6 +329,18 @@ describe("MegolmDecryption", function() { }; }); + it("should use larger otkTimeout when preparing to encrypt room", async () => { + megolmEncryption.prepareToEncrypt(mockRoom); + await megolmEncryption.encryptMessage(mockRoom, "a.fake.type", { + body: "Some text", + }); + expect(mockRoom.getEncryptionTargetMembers).toHaveBeenCalled(); + + expect(mockBaseApis.claimOneTimeKeys).toHaveBeenCalledWith( + [['@alice:home.server', 'aliceDevice']], 'signed_curve25519', 10000, + ); + }); + it("re-uses sessions for sequential messages", async function() { const ct1 = await megolmEncryption.encryptMessage(mockRoom, "a.fake.type", { body: "Some text", diff --git a/src/crypto/algorithms/megolm.ts b/src/crypto/algorithms/megolm.ts index 04fc4a1f281..95806f97086 100644 --- a/src/crypto/algorithms/megolm.ts +++ b/src/crypto/algorithms/megolm.ts @@ -1461,7 +1461,7 @@ class MegolmDecryption extends DecryptionAlgorithm { let senderKey = event.getSenderKey(); let forwardingKeyChain: string[] = []; let exportFormat = false; - let keysClaimed; + let keysClaimed: ReturnType; if (!content.room_id || !content.session_key || diff --git a/src/crypto/olmlib.ts b/src/crypto/olmlib.ts index a6fecbc7946..9d4478449b3 100644 --- a/src/crypto/olmlib.ts +++ b/src/crypto/olmlib.ts @@ -323,7 +323,7 @@ export async function ensureOlmSessionsForDevices( } const oneTimeKeyAlgorithm = "signed_curve25519"; - let res; + let res: IClaimOTKsResult; let taskDetail = `one-time keys for ${devicesWithoutSession.length} devices`; try { log.debug(`Claiming ${taskDetail}`); From 1b238d28615ca70aa900c3cc1dd3f00d24a2a9da Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 10 Jun 2022 14:33:12 +0100 Subject: [PATCH 11/14] Add more test coverage --- spec/unit/crypto/algorithms/megolm.spec.js | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/spec/unit/crypto/algorithms/megolm.spec.js b/spec/unit/crypto/algorithms/megolm.spec.js index 5e4a4233cc7..7d78528ebd4 100644 --- a/spec/unit/crypto/algorithms/megolm.spec.js +++ b/spec/unit/crypto/algorithms/megolm.spec.js @@ -257,6 +257,8 @@ describe("MegolmDecryption", function() { }); describe("session reuse and key reshares", () => { + const rotationPeriodMs = 999 * 24 * 60 * 60 * 1000; + let megolmEncryption; let aliceDeviceInfo; let mockRoom; @@ -318,7 +320,7 @@ describe("MegolmDecryption", function() { baseApis: mockBaseApis, roomId: ROOM_ID, config: { - rotation_period_ms: 9999999999999, + rotation_period_ms: rotationPeriodMs, }, }); mockRoom = { @@ -341,6 +343,19 @@ describe("MegolmDecryption", function() { ); }); + it("should generate a new session if this one needs rotation", async () => { + const session = await megolmEncryption.prepareNewSession(false); + session.creationTime -= rotationPeriodMs + 10000; // a smidge over the rotation time + // Inject expired session which needs rotation + megolmEncryption.setupPromise = Promise.resolve(session); + + const prepareNewSessionSpy = jest.spyOn(megolmEncryption, "prepareNewSession"); + await megolmEncryption.encryptMessage(mockRoom, "a.fake.type", { + body: "Some text", + }); + expect(prepareNewSessionSpy).toHaveBeenCalledTimes(1); + }); + it("re-uses sessions for sequential messages", async function() { const ct1 = await megolmEncryption.encryptMessage(mockRoom, "a.fake.type", { body: "Some text", From 323a72475b4d84847ef4b148fc811d823fbba927 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 10 Jun 2022 22:40:12 +0100 Subject: [PATCH 12/14] Apply suggestions from code review Co-authored-by: Travis Ralston --- spec/unit/crypto.spec.js | 11 ++++++----- spec/unit/crypto/algorithms/megolm.spec.js | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/spec/unit/crypto.spec.js b/spec/unit/crypto.spec.js index 9675db13ce1..6b3bce22b30 100644 --- a/spec/unit/crypto.spec.js +++ b/spec/unit/crypto.spec.js @@ -28,11 +28,12 @@ function awaitEvent(emitter, event) { async function keyshareEventForEvent(client, event, index) { const roomId = event.getRoomId(); const eventContent = event.getWireContent(); - const key = await client.crypto.olmDevice - .getInboundGroupSessionKey( - roomId, eventContent.sender_key, eventContent.session_id, - index, - ); + const key = await client.crypto.olmDevice.getInboundGroupSessionKey( + roomId, + eventContent.sender_key, + eventContent.session_id, + index, + ); const ksEvent = new MatrixEvent({ type: "m.forwarded_room_key", sender: "@alice:example.com", diff --git a/spec/unit/crypto/algorithms/megolm.spec.js b/spec/unit/crypto/algorithms/megolm.spec.js index 7d78528ebd4..aa603b04954 100644 --- a/spec/unit/crypto/algorithms/megolm.spec.js +++ b/spec/unit/crypto/algorithms/megolm.spec.js @@ -257,7 +257,7 @@ describe("MegolmDecryption", function() { }); describe("session reuse and key reshares", () => { - const rotationPeriodMs = 999 * 24 * 60 * 60 * 1000; + const rotationPeriodMs = 999 * 24 * 60 * 60 * 1000; // 999 days, so we don't have to deal with it let megolmEncryption; let aliceDeviceInfo; From 24264d8c8e358d8de2f2c458e76c040fe8ee8bfb Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 10 Jun 2022 22:42:12 +0100 Subject: [PATCH 13/14] Update crypto.spec.js --- spec/unit/crypto.spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/unit/crypto.spec.js b/spec/unit/crypto.spec.js index 6b3bce22b30..e16e220e6d2 100644 --- a/spec/unit/crypto.spec.js +++ b/spec/unit/crypto.spec.js @@ -29,8 +29,8 @@ async function keyshareEventForEvent(client, event, index) { const roomId = event.getRoomId(); const eventContent = event.getWireContent(); const key = await client.crypto.olmDevice.getInboundGroupSessionKey( - roomId, - eventContent.sender_key, + roomId, + eventContent.sender_key, eventContent.session_id, index, ); From 541307834cfbb74bbc058a920128122b848a654c Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Mon, 13 Jun 2022 19:58:52 +0100 Subject: [PATCH 14/14] Update spec/unit/crypto.spec.js Co-authored-by: Faye Duxovni --- spec/unit/crypto.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/crypto.spec.js b/spec/unit/crypto.spec.js index e16e220e6d2..bce8a9d3bc2 100644 --- a/spec/unit/crypto.spec.js +++ b/spec/unit/crypto.spec.js @@ -36,7 +36,7 @@ async function keyshareEventForEvent(client, event, index) { ); const ksEvent = new MatrixEvent({ type: "m.forwarded_room_key", - sender: "@alice:example.com", + sender: client.getUserId(), content: { algorithm: olmlib.MEGOLM_ALGORITHM, room_id: roomId,