From 5be4548b3d8abf16631bb09545505890c90d93e7 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 4 Jul 2023 12:31:03 +0100 Subject: [PATCH] Fix an instance of failed to decrypt error when an in flight `/keys/query` fails. (#3486) * Fix an instance of failed to decrypt error Specifically, when checking the event sender matches who sent us the session keys we skip waiting for pending device list updates if we already know who owns the session key. * Apply suggestions from code review Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update src/crypto/algorithms/olm.ts Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Fix line wrapping * Update src/crypto/algorithms/olm.ts * Fix null check --------- Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- src/crypto/algorithms/olm.ts | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/src/crypto/algorithms/olm.ts b/src/crypto/algorithms/olm.ts index 39afe1d9e..d5cec6f70 100644 --- a/src/crypto/algorithms/olm.ts +++ b/src/crypto/algorithms/olm.ts @@ -194,9 +194,10 @@ class OlmDecryption extends DecryptionAlgorithm { // check that the device that encrypted the event belongs to the user that the event claims it's from. // - // To do this, we need to make sure that our device list is up-to-date. If the device is unknown, we can only - // assume that the device logged out and accept it anyway. Some event handlers, such as secret sharing, may be - // more strict and reject events that come from unknown devices. + // If the device is unknown then we check that we don't have any pending key-query requests for the sender. If + // after that the device is still unknown, then we can only assume that the device logged out and accept it + // anyway. Some event handlers, such as secret sharing, may be more strict and reject events that come from + // unknown devices. // // This is a defence against the following scenario: // @@ -204,14 +205,26 @@ class OlmDecryption extends DecryptionAlgorithm { // * Mallory gets control of Alice's server, and sends a megolm session to Alice using her (Mallory's) // senderkey, but claiming to be from Bob. // * Mallory sends more events using that session, claiming to be from Bob. - // * Alice sees that the senderkey is verified (since she verified Mallory) so marks events those - // events as verified even though the sender is forged. + // * Alice sees that the senderkey is verified (since she verified Mallory) so marks events those events as + // verified even though the sender is forged. // // In practice, it's not clear that the js-sdk would behave that way, so this may be only a defence in depth. - await this.crypto.deviceList.downloadKeys([event.getSender()!], false); - const senderKeyUser = this.crypto.deviceList.getUserByIdentityKey(olmlib.OLM_ALGORITHM, deviceKey); - if (senderKeyUser !== event.getSender() && senderKeyUser != undefined) { + let senderKeyUser = this.crypto.deviceList.getUserByIdentityKey(olmlib.OLM_ALGORITHM, deviceKey); + if (senderKeyUser === undefined || senderKeyUser === null) { + // Wait for any pending key query fetches for the user to complete before trying the lookup again. + try { + await this.crypto.deviceList.downloadKeys([event.getSender()!], false); + } catch (e) { + throw new DecryptionError("OLM_BAD_SENDER_CHECK_FAILED", "Could not verify sender identity", { + sender: deviceKey, + err: e as Error, + }); + } + + senderKeyUser = this.crypto.deviceList.getUserByIdentityKey(olmlib.OLM_ALGORITHM, deviceKey); + } + if (senderKeyUser !== event.getSender() && senderKeyUser !== undefined && senderKeyUser !== null) { throw new DecryptionError("OLM_BAD_SENDER", "Message claimed to be from " + event.getSender(), { real_sender: senderKeyUser, });