From 2f79e6c056c2f00952e4f61f8d7ae29e5d9d98b6 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Tue, 10 Oct 2023 10:38:30 +0100 Subject: [PATCH] Element-R: Don't mark QR code verification as done until it's done (#3791) * Element-R: Don't mark QR code verification as done too soon The rust crypto sdk doesn't actually finish QR code verification until the `m.key.verification.done` is received, so make sure we don't tell the application it is done before that happens. Fixes https://github.com/vector-im/element-web/issues/26293 * ignore fallback line * Revert unnecessary changes Can't get the coverage high enough on this and it's not needed. --- spec/integ/crypto/verification.spec.ts | 27 +++++++++++++++++++++++++- src/crypto-api/verification.ts | 18 ++++++++++++++--- src/rust-crypto/verification.ts | 6 +++++- 3 files changed, 46 insertions(+), 5 deletions(-) diff --git a/spec/integ/crypto/verification.spec.ts b/spec/integ/crypto/verification.spec.ts index 918ed7cab..ebe7e4600 100644 --- a/spec/integ/crypto/verification.spec.ts +++ b/spec/integ/crypto/verification.spec.ts @@ -419,6 +419,15 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st }); it("can verify another via QR code with an untrusted cross-signing key", async () => { + // This is a slightly weird thing to test; if we don't trust the cross-signing key, normally we would + // spam out a verification request to all devices rather than targeting a single device. Still, it's + // a thing both the Matrix protocol and the js-sdk API support, so we may as well test it. + // + // Since we don't yet trust the master key, this is a type 0x02 QR code: + // "self-verifying in which the current device does not yet trust the master key" + // + // By the end of it, we should trust the master key. + aliceClient = await startTestClient(); // QRCode fails if we don't yet have the cross-signing keys, so make sure we have them now. e2eKeyResponder.addCrossSigningData(SIGNED_CROSS_SIGNING_KEYS_DATA); @@ -490,12 +499,28 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st reciprocateQRCodeCallbacks.confirm(); await sendToDevicePromise; + // at this point, on legacy crypto, the master key is already marked as trusted, and the request is "Done". + // Rust crypto, on the other hand, waits for the 'done' to arrive from the other side. + if (request.phase === VerificationPhase.Done) { + // legacy crypto: we're all done + const userVerificationStatus = await aliceClient.getCrypto()!.getUserVerificationStatus(TEST_USER_ID); + // eslint-disable-next-line jest/no-conditional-expect + expect(userVerificationStatus.isCrossSigningVerified()).toBeTruthy(); + await verificationPromise; + } else { + // rust crypto: still in flight + // eslint-disable-next-line jest/no-conditional-expect + expect(request.phase).toEqual(VerificationPhase.Started); + } + // the dummy device replies with its own 'done' returnToDeviceMessageFromSync(buildDoneMessage(transactionId)); - // ... and the whole thing should be done! + // ... and now we're really done. await verificationPromise; expect(request.phase).toEqual(VerificationPhase.Done); + const userVerificationStatus = await aliceClient.getCrypto()!.getUserVerificationStatus(TEST_USER_ID); + expect(userVerificationStatus.isCrossSigningVerified()).toBeTruthy(); }); it("can try to generate a QR code when QR code is not supported", async () => { diff --git a/src/crypto-api/verification.ts b/src/crypto-api/verification.ts index acbade99f..dd42dc35c 100644 --- a/src/crypto-api/verification.ts +++ b/src/crypto-api/verification.ts @@ -224,13 +224,25 @@ export enum VerificationPhase { /** An `m.key.verification.ready` event has been sent or received, indicating the verification request is accepted. */ Ready, - /** An `m.key.verification.start` event has been sent or received, choosing a verification method */ + /** + * The verification is in flight. + * + * This means that an `m.key.verification.start` event has been sent or received, choosing a verification method; + * however the verification has not yet completed or been cancelled. + */ Started, - /** An `m.key.verification.cancel` event has been sent or received at any time before the `done` event, cancelling the verification request */ + /** + * An `m.key.verification.cancel` event has been sent or received at any time before the `done` event, cancelling + * the verification request + */ Cancelled, - /** An `m.key.verification.done` event has been **sent**, completing the verification request. */ + /** + * The verification request is complete. + * + * Normally this means that `m.key.verification.done` events have been sent and received. + */ Done, } diff --git a/src/rust-crypto/verification.ts b/src/rust-crypto/verification.ts index 308e7169c..4ce5a6c38 100644 --- a/src/rust-crypto/verification.ts +++ b/src/rust-crypto/verification.ts @@ -569,7 +569,11 @@ export class RustQrCodeVerifier extends BaseRustVerifer impl return VerificationPhase.Started; case QrState.Confirmed: // we have confirmed the other side's scan and sent an `m.key.verification.done`. - return VerificationPhase.Done; + // + // However, the verification is not yet "Done", because we have to wait until we have received the + // `m.key.verification.done` from the other side (in particular, we don't mark the device/identity as + // verified until that happens). If we return "Done" too soon, we risk the user cancelling the flow. + return VerificationPhase.Started; case QrState.Reciprocated: // although the rust SDK doesn't immediately send the `m.key.verification.start` on transition into this // state, `RustVerificationRequest.scanQrCode` immediately calls `reciprocate()` and does so, so in practice