From 53de9f529d1a22f1acdab3a64910aa37c22a6454 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 12 Jan 2023 20:08:01 +0000 Subject: [PATCH 1/2] Retry decryption after room_key_withheld events ... to update the error message --- src/crypto/algorithms/megolm.ts | 132 +++++++++++++++++--------------- 1 file changed, 72 insertions(+), 60 deletions(-) diff --git a/src/crypto/algorithms/megolm.ts b/src/crypto/algorithms/megolm.ts index 76ad7eab0be..c49fd5b2da2 100644 --- a/src/crypto/algorithms/megolm.ts +++ b/src/crypto/algorithms/megolm.ts @@ -1613,76 +1613,88 @@ export class MegolmDecryption extends DecryptionAlgorithm { const senderKey = content.sender_key; if (content.code === "m.no_olm") { - const sender = event.getSender()!; - this.prefixedLogger.warn(`${sender}:${senderKey} was unable to establish an olm session with us`); - // if the sender says that they haven't been able to establish an olm - // session, let's proactively establish one + await this.onNoOlmWithheldEvent(event); + } else { + await this.olmDevice.addInboundGroupSessionWithheld( + content.room_id, + senderKey, + content.session_id, + content.code, + content.reason, + ); + } - // Note: after we record that the olm session has had a problem, we - // trigger retrying decryption for all the messages from the sender's + // Having recorded the problem, retry decryption on any affected messages. + // It's unlikely we'll be able to decrypt sucessfully now, but this will + // update the error message. + // + if (content.session_id) { + await this.retryDecryption(senderKey, content.session_id); + } else { + // no_olm messages aren't specific to a given megolm session, so + // we trigger retrying decryption for all the messages from the sender's // key, so that we can update the error message to indicate the olm // session problem. + await this.retryDecryptionFromSender(senderKey); + } + } - if (await this.olmDevice.getSessionIdForDevice(senderKey)) { - // a session has already been established, so we don't need to - // create a new one. - this.prefixedLogger.debug("New session already created. Not creating a new one."); - await this.olmDevice.recordSessionProblem(senderKey, "no_olm", true); - this.retryDecryptionFromSender(senderKey); - return; - } - let device = this.crypto.deviceList.getDeviceByIdentityKey(content.algorithm, senderKey); + private async onNoOlmWithheldEvent(event: MatrixEvent): Promise { + const content = event.getContent(); + const senderKey = content.sender_key; + const sender = event.getSender()!; + this.prefixedLogger.warn(`${sender}:${senderKey} was unable to establish an olm session with us`); + // if the sender says that they haven't been able to establish an olm + // session, let's proactively establish one + + if (await this.olmDevice.getSessionIdForDevice(senderKey)) { + // a session has already been established, so we don't need to + // create a new one. + this.prefixedLogger.debug("New session already created. Not creating a new one."); + await this.olmDevice.recordSessionProblem(senderKey, "no_olm", true); + return; + } + let device = this.crypto.deviceList.getDeviceByIdentityKey(content.algorithm, senderKey); + if (!device) { + // if we don't know about the device, fetch the user's devices again + // and retry before giving up + await this.crypto.downloadKeys([sender], false); + device = this.crypto.deviceList.getDeviceByIdentityKey(content.algorithm, senderKey); if (!device) { - // if we don't know about the device, fetch the user's devices again - // and retry before giving up - await this.crypto.downloadKeys([sender], false); - device = this.crypto.deviceList.getDeviceByIdentityKey(content.algorithm, senderKey); - if (!device) { - this.prefixedLogger.info( - "Couldn't find device for identity key " + senderKey + ": not establishing session", - ); - await this.olmDevice.recordSessionProblem(senderKey, "no_olm", false); - this.retryDecryptionFromSender(senderKey); - return; - } + this.prefixedLogger.info( + "Couldn't find device for identity key " + senderKey + ": not establishing session", + ); + await this.olmDevice.recordSessionProblem(senderKey, "no_olm", false); + return; } + } - // XXX: switch this to use encryptAndSendToDevices() rather than duplicating it? + // XXX: switch this to use encryptAndSendToDevices() rather than duplicating it? - await olmlib.ensureOlmSessionsForDevices(this.olmDevice, this.baseApis, { [sender]: [device] }, false); - const encryptedContent: IEncryptedContent = { - algorithm: olmlib.OLM_ALGORITHM, - sender_key: this.olmDevice.deviceCurve25519Key!, - ciphertext: {}, - [ToDeviceMessageId]: uuidv4(), - }; - await olmlib.encryptMessageForDevice( - encryptedContent.ciphertext, - this.userId, - undefined, - this.olmDevice, - sender, - device, - { type: "m.dummy" }, - ); + await olmlib.ensureOlmSessionsForDevices(this.olmDevice, this.baseApis, { [sender]: [device] }, false); + const encryptedContent: IEncryptedContent = { + algorithm: olmlib.OLM_ALGORITHM, + sender_key: this.olmDevice.deviceCurve25519Key!, + ciphertext: {}, + [ToDeviceMessageId]: uuidv4(), + }; + await olmlib.encryptMessageForDevice( + encryptedContent.ciphertext, + this.userId, + undefined, + this.olmDevice, + sender, + device, + { type: "m.dummy" }, + ); - await this.olmDevice.recordSessionProblem(senderKey, "no_olm", true); - this.retryDecryptionFromSender(senderKey); + await this.olmDevice.recordSessionProblem(senderKey, "no_olm", true); - await this.baseApis.sendToDevice("m.room.encrypted", { - [sender]: { - [device.deviceId]: encryptedContent, - }, - }); - } else { - await this.olmDevice.addInboundGroupSessionWithheld( - content.room_id, - senderKey, - content.session_id, - content.code, - content.reason, - ); - } + await this.baseApis.sendToDevice("m.room.encrypted", { + [sender]: { + [device.deviceId]: encryptedContent, + }, + }); } public hasKeysForKeyRequest(keyRequest: IncomingRoomKeyRequest): Promise { From 22a0997dcabba6daeca03142b8125a047c6793e8 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 12 Jan 2023 20:14:54 +0000 Subject: [PATCH 2/2] Fix spurious "Decryption key withheld" messages When we receive an `m.unavailable` notification, do not show it as "Decryption key withheld". --- spec/integ/megolm-integ.spec.ts | 75 +++++++++++++++++++++++++++++++++ src/crypto/algorithms/megolm.ts | 3 ++ 2 files changed, 78 insertions(+) diff --git a/spec/integ/megolm-integ.spec.ts b/spec/integ/megolm-integ.spec.ts index 1bdf3a09f47..88e438e7510 100644 --- a/spec/integ/megolm-integ.spec.ts +++ b/spec/integ/megolm-integ.spec.ts @@ -1678,4 +1678,79 @@ describe("megolm", () => { await Promise.all([sendPromise, megolmMessagePromise, aliceTestClient.httpBackend.flush("/keys/query", 1)]); }); }); + + describe("m.room_key.withheld handling", () => { + // TODO: there are a bunch more tests for this sort of thing in spec/unit/crypto/algorithms/megolm.spec.ts. + // They should be converted to integ tests and moved. + + it("does not block decryption on an 'm.unavailable' report", async function () { + await aliceTestClient.start(); + + // there may be a key downloads for alice + aliceTestClient.httpBackend.when("POST", "/keys/query").respond(200, {}); + aliceTestClient.httpBackend.flush("/keys/query", 1, 5000); + + // encrypt a message with a group session. + const groupSession = new Olm.OutboundGroupSession(); + groupSession.create(); + const messageEncryptedEvent = encryptMegolmEvent({ + senderKey: testSenderKey, + groupSession: groupSession, + room_id: ROOM_ID, + }); + + // Alice gets the room message, but not the key + aliceTestClient.httpBackend.when("GET", "/sync").respond(200, { + next_batch: 1, + rooms: { + join: { [ROOM_ID]: { timeline: { events: [messageEncryptedEvent] } } }, + }, + }); + await aliceTestClient.flushSync(); + + // alice will (eventually) send a room-key request + aliceTestClient.httpBackend.when("PUT", "/sendToDevice/m.room_key_request/").respond(200, {}); + await aliceTestClient.httpBackend.flush("/sendToDevice/m.room_key_request/", 1, 1000); + + // at this point, the message should be a decryption failure + const room = aliceTestClient.client.getRoom(ROOM_ID)!; + const event = room.getLiveTimeline().getEvents()[0]; + expect(event.isDecryptionFailure()).toBeTruthy(); + + // we want to wait for the message to be updated, so create a promise for it + const retryPromise = new Promise((resolve) => { + event.once(MatrixEventEvent.Decrypted, (ev) => { + resolve(ev); + }); + }); + + // alice gets back a room-key-withheld notification + aliceTestClient.httpBackend.when("GET", "/sync").respond(200, { + next_batch: 2, + to_device: { + events: [ + { + type: "m.room_key.withheld", + sender: "@bob:example.com", + content: { + algorithm: "m.megolm.v1.aes-sha2", + room_id: ROOM_ID, + session_id: groupSession.session_id(), + sender_key: testSenderKey, + code: "m.unavailable", + reason: "", + }, + }, + ], + }, + }); + await aliceTestClient.flushSync(); + + // the withheld notification should trigger a retry; wait for it + await retryPromise; + + // finally: the message should still be a regular decryption failure, not a withheld notification. + expect(event.getContent().body).not.toContain("withheld"); + }); + }); }); diff --git a/src/crypto/algorithms/megolm.ts b/src/crypto/algorithms/megolm.ts index c49fd5b2da2..163d3953d45 100644 --- a/src/crypto/algorithms/megolm.ts +++ b/src/crypto/algorithms/megolm.ts @@ -1614,6 +1614,9 @@ export class MegolmDecryption extends DecryptionAlgorithm { if (content.code === "m.no_olm") { await this.onNoOlmWithheldEvent(event); + } else if (content.code === "m.unavailable") { + // this simply means that the other device didn't have the key, which isn't very useful information. Don't + // record it in the storage } else { await this.olmDevice.addInboundGroupSessionWithheld( content.room_id,