From e86ce755ff780d2cc90889ce6e4b423be8e66b7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Thu, 1 Dec 2022 16:03:16 +0100 Subject: [PATCH 1/8] Refactor the room key handling method --- src/crypto/algorithms/megolm.ts | 419 +++++++++++++++++++++++--------- 1 file changed, 301 insertions(+), 118 deletions(-) diff --git a/src/crypto/algorithms/megolm.ts b/src/crypto/algorithms/megolm.ts index ef04e8f4e0b..55407b08cc0 100644 --- a/src/crypto/algorithms/megolm.ts +++ b/src/crypto/algorithms/megolm.ts @@ -73,6 +73,18 @@ export interface IOlmDevice { deviceInfo: T; } +interface RoomKey { + senderKey: string; + sessionId: string; + sessionKey: string; + exportFormat: boolean; + roomId: string; + algorithm: string; + forwardingKeyChain: string[]; + keysClaimed: Partial>; + extraSessionData: OlmGroupSessionExtraData; +} + /* eslint-disable camelcase */ export interface IOutboundGroupSessionKey { chain_index: number; @@ -1396,13 +1408,20 @@ export class MegolmDecryption extends DecryptionAlgorithm { } } - public async onRoomKeyEvent(event: MatrixEvent): Promise { + /** + * Parse a RoomKey out of an `m.room_key` event. + * + * @param event - the event containing the room key. + * + * @returns The `RoomKey` if it could be successfully parsed out of the + * event. + * + * @internal + * + */ + private roomKeyFromEvent(event: MatrixEvent): RoomKey | undefined { + const senderKey = event.getSenderKey()!; const content = event.getContent>(); - let senderKey = event.getSenderKey()!; - let forwardingKeyChain: string[] = []; - let exportFormat = false; - let keysClaimed: ReturnType; - const extraSessionData: OlmGroupSessionExtraData = {}; if (!content.room_id || !content.session_key || !content.session_id || !content.algorithm) { @@ -1419,154 +1438,318 @@ export class MegolmDecryption extends DecryptionAlgorithm { extraSessionData.sharedHistory = true; } - if (event.getType() == "m.forwarded_room_key") { - const deviceInfo = this.crypto.deviceList.getDeviceByIdentityKey(olmlib.OLM_ALGORITHM, senderKey); - const senderKeyUser = this.baseApis.crypto!.deviceList.getUserByIdentityKey( - olmlib.OLM_ALGORITHM, - senderKey, - ); - if (senderKeyUser !== event.getSender()) { - logger.error("sending device does not belong to the user it claims to be from"); - return; - } - const outgoingRequests = deviceInfo - ? await this.crypto.cryptoStore.getOutgoingRoomKeyRequestsByTarget( - event.getSender()!, - deviceInfo.deviceId, - [RoomKeyRequestState.Sent], - ) - : []; - const weRequested = outgoingRequests.some( - (req) => - req.requestBody.room_id === content.room_id && req.requestBody.session_id === content.session_id, - ); - const room = this.baseApis.getRoom(content.room_id); - const memberEvent = room?.getMember(this.userId)?.events.member; - const fromInviter = - memberEvent?.getSender() === event.getSender() || - (memberEvent?.getUnsigned()?.prev_sender === event.getSender() && - memberEvent?.getPrevContent()?.membership === "invite"); - const fromUs = event.getSender() === this.baseApis.getUserId(); - - if (!weRequested && !fromUs) { - // If someone sends us an unsolicited key and they're - // not one of our other devices and it's not shared - // history, ignore it - if (!extraSessionData.sharedHistory) { - logger.log("forwarded key not shared history - ignoring"); - return; - } + const roomKey: RoomKey = { + senderKey: senderKey, + sessionId: content.session_id, + sessionKey: content.session_key, + extraSessionData, + exportFormat: false, + roomId: content.room_id, + algorithm: content.algorithm, + forwardingKeyChain: [], + keysClaimed: event.getKeysClaimed(), + }; - // If someone sends us an unsolicited key for a room - // we're already in, and they're not one of our other - // devices or the one who invited us, ignore it - if (room && !fromInviter) { - logger.log("forwarded key not from inviter or from us - ignoring"); - return; - } - } + return roomKey; + } - exportFormat = true; - forwardingKeyChain = Array.isArray(content.forwarding_curve25519_key_chain) - ? content.forwarding_curve25519_key_chain - : []; + /** + * Parse a RoomKey out of an `m.forwarded_room_key` event. + * + * @param event - the event containing the forwarded room key. + * + * @returns The `RoomKey` if it could be successfully parsed out of the + * event. + * + * @internal + * + */ + private forwardedRoomKeyFromEvent(event: MatrixEvent): RoomKey | undefined { + let roomKey = this.roomKeyFromEvent(event); - // copy content before we modify it - forwardingKeyChain = forwardingKeyChain.slice(); - forwardingKeyChain.push(senderKey); + if (!roomKey) { + return; + } - if (!content.sender_key) { - logger.error("forwarded_room_key event is missing sender_key field"); - return; - } + const senderKey = event.getSenderKey()!; + const content = event.getContent>(); - const ed25519Key = content.sender_claimed_ed25519_key; - if (!ed25519Key) { - logger.error(`forwarded_room_key_event is missing sender_claimed_ed25519_key field`); - return; - } + const senderKeyUser = this.baseApis.crypto!.deviceList.getUserByIdentityKey(olmlib.OLM_ALGORITHM, senderKey); - keysClaimed = { - ed25519: ed25519Key, - }; + // We received this room key from event.getSenderKey(), but the original + // creator of the room key is claimed in the content. Let's try to fetch + // those keys now. + const claimed_curve25519Key = content.sender_key; + const claimed_ed25519Key = content.sender_claimed_ed25519_key; - // If this is a key for a room we're not in, don't load it - // yet, just park it in case *this sender* invites us to - // that room later - if (!room) { - const parkedData = { - senderId: event.getSender()!, - senderKey: content.sender_key, - sessionId: content.session_id, - sessionKey: content.session_key, - keysClaimed, - forwardingCurve25519KeyChain: forwardingKeyChain, - }; - await this.crypto.cryptoStore.doTxn( - "readwrite", - ["parked_shared_history"], - (txn) => this.crypto.cryptoStore.addParkedSharedHistory(content.room_id!, parkedData, txn), - logger.withPrefix("[addParkedSharedHistory]"), - ); - return; - } + let forwardingKeyChain = Array.isArray(content.forwarding_curve25519_key_chain) + ? content.forwarding_curve25519_key_chain + : []; - const sendingDevice = - this.crypto.deviceList.getDeviceByIdentityKey(olmlib.OLM_ALGORITHM, senderKey) ?? undefined; - const deviceTrust = this.crypto.checkDeviceInfoTrust(event.getSender()!, sendingDevice); + // copy content before we modify it + forwardingKeyChain = forwardingKeyChain.slice(); + forwardingKeyChain.push(senderKey); - if (fromUs && !deviceTrust.isVerified()) { - return; - } + // Check if we have all the fields we need. + if (senderKeyUser !== event.getSender()) { + logger.error("sending device does not belong to the user it claims to be from"); + return; + } - // forwarded keys are always untrusted - extraSessionData.untrusted = true; + if (!claimed_curve25519Key) { + logger.error("forwarded_room_key event is missing sender_key field"); + return; + } + + if (!claimed_ed25519Key) { + logger.error(`forwarded_room_key_event is missing sender_claimed_ed25519_key field`); + return; + } + + const keysClaimed = { + ed25519: claimed_ed25519Key, + }; - // replace the sender key with the sender key of the session - // creator for storage - senderKey = content.sender_key; + // FIXME: We're reusing the same field to track both: + // + // 1. The Olm identity we've received this room key from. + // 2. The Olm identity deduced (in the trusted case) or claiming (in the + // untrusted case) to be the original creator of this room key. + // + // We now overwrite the value tracking usage 1 with the value tracking usage 2. + roomKey.senderKey = claimed_curve25519Key; + // Replace our keysClaimed as well. + roomKey.keysClaimed = keysClaimed; + roomKey.exportFormat = true; + roomKey.forwardingKeyChain = forwardingKeyChain; + // forwarded keys are always untrusted + roomKey.extraSessionData.untrusted = true; + + return roomKey; + } + + /** + * Should we accept the forwarded room key that was found in the given + * event. + * + * @param event - An `m.forwarded_room_key` event. + * @param roomKey - The room key that was found in the event. + * + * @returns promise that will resolve to a boolean telling us if it's ok to + * accept the given forwarded room key. + * + * @internal + * + */ + private async shouldAcceptForwardedKey(event: MatrixEvent, roomKey: RoomKey): Promise { + const senderKey = event.getSenderKey()!; + + const sendingDevice = + this.crypto.deviceList.getDeviceByIdentityKey(olmlib.OLM_ALGORITHM, senderKey) ?? undefined; + const deviceTrust = this.crypto.checkDeviceInfoTrust(event.getSender()!, sendingDevice); + + // Using the plaintext sender here is fine since we checked that the + // sender matches to the user id in the device keys when this event was + // originally decrypted. This can obviously only happen if the device + // keys have been downloaded, but if they haven't the + // `deviceTrust.isVerified()` flag would be false as well. + // + // It would still be far nicer if the `sendingDevice` had a user ID + // attached to it that went through signature checks. + const fromUs = event.getSender() === this.baseApis.getUserId(); + const keyFromOurVerifiedDevice = deviceTrust.isVerified() && fromUs; + const weRequested = await this.wasRoomKeyRequested(event, roomKey); + const fromInviter = this.wasRoomKeyForwardedByInviter(event, roomKey); + const sharedAsHistory = this.wasRoomKeyForwardedAsHistory(roomKey); + + return (weRequested && keyFromOurVerifiedDevice) || (fromInviter && sharedAsHistory); + } + + /** + * Did we ever request the given room key from the event sender and its + * accompanying device. + * + * @param event - An `m.forwarded_room_key` event. + * @param roomKey - The room key that was found in the event. + * + * @internal + * + */ + private async wasRoomKeyRequested(event: MatrixEvent, roomKey: RoomKey): Promise { + // We send the `m.room_key_request` out as a wildcard to-device request, + // otherwise we would have to duplicate the same content for each + // device. This is why we need to pass in "*" as the device id here. + const outgoingRequests = await this.crypto.cryptoStore.getOutgoingRoomKeyRequestsByTarget( + event.getSender()!, + "*", + [RoomKeyRequestState.Sent], + ); + + return outgoingRequests.some( + (req) => req.requestBody.room_id === roomKey.roomId && req.requestBody.session_id === roomKey.sessionId, + ); + } + + private wasRoomKeyForwardedByInviter(event: MatrixEvent, roomKey: RoomKey): boolean { + // TODO: This is supposed to have a time limit. We should only accept + // such keys if we happen to receive them for a recently joined room. + const room = this.baseApis.getRoom(roomKey.roomId); + const senderKey = event.getSenderKey(); + + if (!senderKey) { + return false; + } + + const senderKeyUser = this.crypto.deviceList.getUserByIdentityKey(olmlib.OLM_ALGORITHM, senderKey); + + if (!senderKeyUser) { + return false; + } + + const memberEvent = room?.getMember(this.userId)?.events.member; + const fromInviter = + memberEvent?.getSender() === senderKeyUser || + (memberEvent?.getUnsigned()?.prev_sender === senderKeyUser && + memberEvent?.getPrevContent()?.membership === "invite"); + + if (room && fromInviter) { + return true; } else { - keysClaimed = event.getKeysClaimed(); + return false; } + } - if (content["org.matrix.msc3061.shared_history"]) { - extraSessionData.sharedHistory = true; + private wasRoomKeyForwardedAsHistory(roomKey: RoomKey): boolean { + const room = this.baseApis.getRoom(roomKey.roomId); + + if (room && roomKey.extraSessionData.sharedHistory) { + return true; + } else { + return false; + } + } + + private shouldParkForwardedKey(roomKey: RoomKey): boolean { + const room = this.baseApis.getRoom(roomKey.roomId); + + if (!room && roomKey.extraSessionData.sharedHistory) { + return true; + } else { + return false; } + } + + /** + * Park the given room key to our store. + * + * @param event - An `m.forwarded_room_key` event. + * @param roomKey - The room key that was found in the event. + * + * @internal + * + */ + private async parkForwardedKey(event: MatrixEvent, roomKey: RoomKey): Promise { + const parkedData = { + senderId: event.getSender()!, + senderKey: roomKey.senderKey, + sessionId: roomKey.sessionId, + sessionKey: roomKey.sessionKey, + keysClaimed: roomKey.keysClaimed, + forwardingCurve25519KeyChain: roomKey.forwardingKeyChain, + }; + await this.crypto.cryptoStore.doTxn( + "readwrite", + ["parked_shared_history"], + (txn) => this.crypto.cryptoStore.addParkedSharedHistory(roomKey.roomId, parkedData, txn), + logger.withPrefix("[addParkedSharedHistory]"), + ); + } + /** + * Add the given room key to our store. + * + * @param roomKey - The room key that should be added to the store. + * + * @internal + * + */ + private async addRoomKey(roomKey: RoomKey): Promise { try { await this.olmDevice.addInboundGroupSession( - content.room_id, - senderKey, - forwardingKeyChain, - content.session_id, - content.session_key, - keysClaimed, - exportFormat, - extraSessionData, + roomKey.roomId, + roomKey.senderKey, + roomKey.forwardingKeyChain, + roomKey.sessionId, + roomKey.sessionKey, + roomKey.keysClaimed, + roomKey.exportFormat, + roomKey.extraSessionData, ); // have another go at decrypting events sent with this session. - if (await this.retryDecryption(senderKey, content.session_id, !extraSessionData.untrusted)) { + if (await this.retryDecryption(roomKey.senderKey, roomKey.sessionId, !roomKey.extraSessionData.untrusted)) { // 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, + algorithm: roomKey.algorithm, + room_id: roomKey.roomId, + session_id: roomKey.sessionId, + sender_key: roomKey.senderKey, }); } // don't wait for the keys to be backed up for the server - await this.crypto.backupManager.backupGroupSession(senderKey, content.session_id); + await this.crypto.backupManager.backupGroupSession(roomKey.senderKey, roomKey.sessionId); } catch (e) { logger.error(`Error handling m.room_key_event: ${e}`); } } + /** + * Handle room keys that have been forwarded to us as an + * `m.forwarded_room_key` event. + * + * Forwarded room keys need special handling since we have no way of knowing + * who the original creator of the room key was. This naturally means that + * forwarded room keys are always untrusted and should only be accepted in + * some cases. + * + * @param event - An `m.forwarded_room_key` event. + * + * @internal + * + */ + private async onForwardedRoomKey(event: MatrixEvent): Promise { + let roomKey = this.forwardedRoomKeyFromEvent(event); + + if (!roomKey) { + return; + } + + if (await this.shouldAcceptForwardedKey(event, roomKey)) { + await this.addRoomKey(roomKey); + } else if (this.shouldParkForwardedKey(roomKey)) { + await this.parkForwardedKey(event, roomKey); + } + } + + public async onRoomKeyEvent(event: MatrixEvent): Promise { + if (event.getType() == "m.forwarded_room_key") { + await this.onForwardedRoomKey(event); + } else { + const roomKey = this.roomKeyFromEvent(event); + + if (!roomKey) { + return; + } + + await this.addRoomKey(roomKey); + } + } + /** * @param event - key event */ From 94b053ab53aed66c8fb646971f3e1f369279ecb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Wed, 7 Dec 2022 13:34:39 +0100 Subject: [PATCH 2/8] Fix the forwarded room key test to use the same user ids. We have some tests that check if receiving a forwarded room key works. These claim to use the same user id, but in fact they change the user id in the last moment before the event is passed into the client. Let's change this so we're always operating with the same user id. --- spec/unit/crypto.spec.ts | 61 ++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/spec/unit/crypto.spec.ts b/spec/unit/crypto.spec.ts index 3b7689ca0ce..27fdc43bc5d 100644 --- a/spec/unit/crypto.spec.ts +++ b/spec/unit/crypto.spec.ts @@ -271,20 +271,24 @@ describe("Crypto", function () { describe("Key requests", function () { let aliceClient: MatrixClient; + let secondAliceClient: MatrixClient; let bobClient: MatrixClient; let claraClient: MatrixClient; beforeEach(async function () { aliceClient = new TestClient("@alice:example.com", "alicedevice").client; + secondAliceClient = new TestClient("@alice:example.com", "secondAliceDevice").client; bobClient = new TestClient("@bob:example.com", "bobdevice").client; claraClient = new TestClient("@clara:example.com", "claradevice").client; await aliceClient.initCrypto(); + await secondAliceClient.initCrypto(); await bobClient.initCrypto(); await claraClient.initCrypto(); }); afterEach(async function () { aliceClient.stopClient(); + secondAliceClient.stopClient(); bobClient.stopClient(); claraClient.stopClient(); }); @@ -535,17 +539,17 @@ describe("Crypto", function () { expect(aliceSendToDevice.mock.calls[2][2]).not.toBe(txnId); }); - it("should accept forwarded keys which it requested", async function () { + it("should accept forwarded keys it requested from one of its own user's other devices", 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", {}); + const bobRoom = new Room(roomId, secondAliceClient, "@alice:example.com", {}); aliceClient.store.storeRoom(aliceRoom); - bobClient.store.storeRoom(bobRoom); + secondAliceClient.store.storeRoom(bobRoom); await aliceClient.setRoomEncryption(roomId, encryptionCfg); - await bobClient.setRoomEncryption(roomId, encryptionCfg); + await secondAliceClient.setRoomEncryption(roomId, encryptionCfg); const events = [ new MatrixEvent({ type: "m.room.message", @@ -581,7 +585,7 @@ describe("Crypto", function () { // @ts-ignore private properties event.claimedEd25519Key = null; try { - await bobClient.crypto!.decryptEvent(event); + await secondAliceClient.crypto!.decryptEvent(event); } catch (e) { // we expect this to fail because we don't have the // decryption keys yet @@ -590,10 +594,11 @@ describe("Crypto", function () { ); const device = new DeviceInfo(aliceClient.deviceId!); - bobClient.crypto!.deviceList.getDeviceByIdentityKey = () => device; - bobClient.crypto!.deviceList.getUserByIdentityKey = () => "@alice:example.com"; + device.verified = DeviceInfo.DeviceVerification.VERIFIED; + secondAliceClient.crypto!.deviceList.getDeviceByIdentityKey = () => device; + secondAliceClient.crypto!.deviceList.getUserByIdentityKey = () => "@alice:example.com"; - const cryptoStore = bobClient.crypto!.cryptoStore; + const cryptoStore = secondAliceClient.crypto!.cryptoStore; const eventContent = events[0].getWireContent(); const senderKey = eventContent.sender_key; const sessionId = eventContent.session_id; @@ -609,7 +614,7 @@ describe("Crypto", function () { state: RoomKeyRequestState.Sent, }); - const bobDecryptor = bobClient.crypto!.getRoomDecryptor(roomId, olmlib.MEGOLM_ALGORITHM); + const bobDecryptor = secondAliceClient.crypto!.getRoomDecryptor(roomId, olmlib.MEGOLM_ALGORITHM); const decryptEventsPromise = Promise.all( events.map((ev) => { @@ -618,7 +623,7 @@ describe("Crypto", function () { ); const ksEvent = await keyshareEventForEvent(aliceClient, events[0], 0); await bobDecryptor.onRoomKeyEvent(ksEvent); - const key = await bobClient.crypto!.olmDevice.getInboundGroupSessionKey( + const key = await secondAliceClient.crypto!.olmDevice.getInboundGroupSessionKey( roomId, events[0].getWireContent().sender_key, events[0].getWireContent().session_id, @@ -722,7 +727,7 @@ describe("Crypto", function () { expect(events[1].getContent().msgtype).not.toBe("m.bad.encrypted"); }); - it("should accept forwarded keys from one of its own user's other devices", async function () { + it("should not accept requested forwarded keys from other users", async function () { const encryptionCfg = { algorithm: "m.megolm.v1.aes-sha2", }; @@ -776,31 +781,39 @@ describe("Crypto", function () { }), ); - const device = new DeviceInfo(claraClient.deviceId!); + const cryptoStore = bobClient.crypto!.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, + }; + const outgoingReq = await cryptoStore.getOutgoingRoomKeyRequest(roomKeyRequestBody); + expect(outgoingReq).toBeDefined(); + await cryptoStore.updateOutgoingRoomKeyRequest(outgoingReq!.requestId, RoomKeyRequestState.Unsent, { + state: RoomKeyRequestState.Sent, + }); + + const device = new DeviceInfo(aliceClient.deviceId!); device.verified = DeviceInfo.DeviceVerification.VERIFIED; bobClient.crypto!.deviceList.getDeviceByIdentityKey = () => device; - bobClient.crypto!.deviceList.getUserByIdentityKey = () => "@bob:example.com"; + bobClient.crypto!.deviceList.getUserByIdentityKey = () => "@alice:example.com"; const bobDecryptor = bobClient.crypto!.getRoomDecryptor(roomId, olmlib.MEGOLM_ALGORITHM); - const decryptEventsPromise = Promise.all( - events.map((ev) => { - return awaitEvent(ev, "Event.decrypted"); - }), - ); const ksEvent = await keyshareEventForEvent(aliceClient, events[0], 0); - ksEvent.event.sender = bobClient.getUserId()!; - ksEvent.sender = new RoomMember(roomId, bobClient.getUserId()!); + ksEvent.event.sender = aliceClient.getUserId()!; + ksEvent.sender = new RoomMember(roomId, aliceClient.getUserId()!); await bobDecryptor.onRoomKeyEvent(ksEvent); const key = await bobClient.crypto!.olmDevice.getInboundGroupSessionKey( roomId, events[0].getWireContent().sender_key, events[0].getWireContent().session_id, ); - expect(key).not.toBeNull(); - await decryptEventsPromise; - expect(events[0].getContent().msgtype).not.toBe("m.bad.encrypted"); - expect(events[1].getContent().msgtype).not.toBe("m.bad.encrypted"); + expect(key).toBeNull(); }); it("should not accept unexpected forwarded keys for a room it's in", async function () { From 6dd15e13011a8838de584480b0c5861b70a77c22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Wed, 7 Dec 2022 14:26:42 +0100 Subject: [PATCH 3/8] Stop requesting room keys from other users We never accept such room keys, so there isn't a point in requesting them. --- src/models/event.ts | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/models/event.ts b/src/models/event.ts index 1f1c3cfa366..87120a5671b 100644 --- a/src/models/event.ts +++ b/src/models/event.ts @@ -786,22 +786,14 @@ export class MatrixEvent extends TypedEventEmitter Date: Wed, 14 Dec 2022 12:53:39 +0100 Subject: [PATCH 4/8] fixup! Refactor the room key handling method --- src/crypto/algorithms/megolm.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/crypto/algorithms/megolm.ts b/src/crypto/algorithms/megolm.ts index 55407b08cc0..c54eaa26404 100644 --- a/src/crypto/algorithms/megolm.ts +++ b/src/crypto/algorithms/megolm.ts @@ -1465,7 +1465,7 @@ export class MegolmDecryption extends DecryptionAlgorithm { * */ private forwardedRoomKeyFromEvent(event: MatrixEvent): RoomKey | undefined { - let roomKey = this.roomKeyFromEvent(event); + const roomKey = this.roomKeyFromEvent(event); if (!roomKey) { return; @@ -1479,8 +1479,8 @@ export class MegolmDecryption extends DecryptionAlgorithm { // We received this room key from event.getSenderKey(), but the original // creator of the room key is claimed in the content. Let's try to fetch // those keys now. - const claimed_curve25519Key = content.sender_key; - const claimed_ed25519Key = content.sender_claimed_ed25519_key; + const claimedCurve25519Key = content.sender_key; + const claimedEd25519Key = content.sender_claimed_ed25519_key; let forwardingKeyChain = Array.isArray(content.forwarding_curve25519_key_chain) ? content.forwarding_curve25519_key_chain @@ -1496,18 +1496,18 @@ export class MegolmDecryption extends DecryptionAlgorithm { return; } - if (!claimed_curve25519Key) { + if (!claimedCurve25519Key) { logger.error("forwarded_room_key event is missing sender_key field"); return; } - if (!claimed_ed25519Key) { + if (!claimedEd25519Key) { logger.error(`forwarded_room_key_event is missing sender_claimed_ed25519_key field`); return; } const keysClaimed = { - ed25519: claimed_ed25519Key, + ed25519: claimedEd25519Key, }; // FIXME: We're reusing the same field to track both: @@ -1517,7 +1517,7 @@ export class MegolmDecryption extends DecryptionAlgorithm { // untrusted case) to be the original creator of this room key. // // We now overwrite the value tracking usage 1 with the value tracking usage 2. - roomKey.senderKey = claimed_curve25519Key; + roomKey.senderKey = claimedCurve25519Key; // Replace our keysClaimed as well. roomKey.keysClaimed = keysClaimed; roomKey.exportFormat = true; @@ -1723,7 +1723,7 @@ export class MegolmDecryption extends DecryptionAlgorithm { * */ private async onForwardedRoomKey(event: MatrixEvent): Promise { - let roomKey = this.forwardedRoomKeyFromEvent(event); + const roomKey = this.forwardedRoomKeyFromEvent(event); if (!roomKey) { return; From 9fd66a2c94d2c7e044a6280d171c39769b8679dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Mon, 26 Dec 2022 12:02:01 +0100 Subject: [PATCH 5/8] Apply suggestions from code review Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- src/crypto/algorithms/megolm.ts | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/crypto/algorithms/megolm.ts b/src/crypto/algorithms/megolm.ts index c54eaa26404..50a2cce0e1c 100644 --- a/src/crypto/algorithms/megolm.ts +++ b/src/crypto/algorithms/megolm.ts @@ -73,13 +73,25 @@ export interface IOlmDevice { deviceInfo: T; } +/** The result of parsing the an `m.room_key` or `m.forwarded_room_key` to-device event */ interface RoomKey { + /** + * The Curve25519 key of the megolm session creator. + * + * For `m.room_key`, this is also the sender of the `m.room_key` to-device event. + * For `m.forwarded_room_key`, the two are different (and the key of the sender of the + * `m.forwarded_room_key` event is included in `forwardingKeyChain`) + */ senderKey: string; sessionId: string; sessionKey: string; exportFormat: boolean; roomId: string; algorithm: string; + /** + * A list of the curve25519 keys of the users involved in forwarding this key, most recent last. + * For `m.room_key` events, this is empty. + */ forwardingKeyChain: string[]; keysClaimed: Partial>; extraSessionData: OlmGroupSessionExtraData; @@ -1465,6 +1477,8 @@ export class MegolmDecryption extends DecryptionAlgorithm { * */ private forwardedRoomKeyFromEvent(event: MatrixEvent): RoomKey | undefined { + // the properties in m.forwarded_room_key are a superset of those in m.room_key, so + // start by parsing the m.room_key fields. const roomKey = this.roomKeyFromEvent(event); if (!roomKey) { @@ -1476,9 +1490,8 @@ export class MegolmDecryption extends DecryptionAlgorithm { const senderKeyUser = this.baseApis.crypto!.deviceList.getUserByIdentityKey(olmlib.OLM_ALGORITHM, senderKey); - // We received this room key from event.getSenderKey(), but the original - // creator of the room key is claimed in the content. Let's try to fetch - // those keys now. + // We received this to-device event from event.getSenderKey(), but the original + // creator of the room key is claimed in the content. const claimedCurve25519Key = content.sender_key; const claimedEd25519Key = content.sender_claimed_ed25519_key; @@ -1529,7 +1542,7 @@ export class MegolmDecryption extends DecryptionAlgorithm { } /** - * Should we accept the forwarded room key that was found in the given + * Determine if we should accept the forwarded room key that was found in the given * event. * * @param event - An `m.forwarded_room_key` event. From fb4d9b662158911dbb5eb5cb8af685a94b55d0fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Mon, 26 Dec 2022 14:22:15 +0100 Subject: [PATCH 6/8] fixup! Refactor the room key handling method --- src/crypto/algorithms/megolm.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/crypto/algorithms/megolm.ts b/src/crypto/algorithms/megolm.ts index 50a2cce0e1c..2e6928f6c5b 100644 --- a/src/crypto/algorithms/megolm.ts +++ b/src/crypto/algorithms/megolm.ts @@ -1642,6 +1642,13 @@ export class MegolmDecryption extends DecryptionAlgorithm { } } + /** + * Check if a forwarded room key should be parked. + * + * A forwarded room key should be parked if it's a key for a room we're not + * in. We park the forwarded room key in case *this sender* invites us to + * that room later. + */ private shouldParkForwardedKey(roomKey: RoomKey): boolean { const room = this.baseApis.getRoom(roomKey.roomId); From b7759a0f3dea60e2082fa2088b533d797d38544a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Mon, 26 Dec 2022 14:24:34 +0100 Subject: [PATCH 7/8] fixup! Apply suggestions from code review --- src/crypto/algorithms/megolm.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/crypto/algorithms/megolm.ts b/src/crypto/algorithms/megolm.ts index 2e6928f6c5b..e2795cf4b89 100644 --- a/src/crypto/algorithms/megolm.ts +++ b/src/crypto/algorithms/megolm.ts @@ -75,13 +75,13 @@ export interface IOlmDevice { /** The result of parsing the an `m.room_key` or `m.forwarded_room_key` to-device event */ interface RoomKey { - /** - * The Curve25519 key of the megolm session creator. - * - * For `m.room_key`, this is also the sender of the `m.room_key` to-device event. - * For `m.forwarded_room_key`, the two are different (and the key of the sender of the - * `m.forwarded_room_key` event is included in `forwardingKeyChain`) - */ + /** + * The Curve25519 key of the megolm session creator. + * + * For `m.room_key`, this is also the sender of the `m.room_key` to-device event. + * For `m.forwarded_room_key`, the two are different (and the key of the sender of the + * `m.forwarded_room_key` event is included in `forwardingKeyChain`) + */ senderKey: string; sessionId: string; sessionKey: string; From dd4973fd4c0ede7b195f3a7a584f376d925d52a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Mon, 27 Feb 2023 15:23:09 +0100 Subject: [PATCH 8/8] fixup! Refactor the room key handling method --- src/crypto/algorithms/megolm.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/crypto/algorithms/megolm.ts b/src/crypto/algorithms/megolm.ts index e2795cf4b89..d3a98f80452 100644 --- a/src/crypto/algorithms/megolm.ts +++ b/src/crypto/algorithms/megolm.ts @@ -1635,6 +1635,11 @@ export class MegolmDecryption extends DecryptionAlgorithm { private wasRoomKeyForwardedAsHistory(roomKey: RoomKey): boolean { const room = this.baseApis.getRoom(roomKey.roomId); + // If the key is not for a known room, then something fishy is going on, + // so we reject the key out of caution. In practice, this is a bit moot + // because we'll only accept shared_history forwarded by the inviter, and + // we won't know who was the inviter for an unknown room, so we'll reject + // it anyway. if (room && roomKey.extraSessionData.sharedHistory) { return true; } else {