From aec8237975a822c77a5609f6b628536df7cd8c1c Mon Sep 17 00:00:00 2001 From: Erwan Leboucher Date: Tue, 1 Sep 2026 12:04:57 +0200 Subject: [PATCH] fix(crypto): require a trusted cross-signature before showing a device as verified --- .changeset/fix-device-verified-badge.md | 5 +++ src-tauri/src/matrix_crypto/devices.rs | 6 ++- src-tauri/src/matrix_crypto/dispatch.rs | 8 +++- src-tauri/src/matrix_crypto/mod.rs | 17 ++++++--- src-tauri/src/matrix_crypto/requests.rs | 16 ++++---- src-tauri/src/matrix_crypto/rooms.rs | 25 +++++++----- src-tauri/src/matrix_crypto/verification.rs | 25 +++--------- src/app/components/BackupRestore.test.tsx | 1 - src/app/components/BackupRestore.tsx | 6 --- .../components/ManualVerification.test.tsx | 8 ++-- src/app/components/ManualVerification.tsx | 6 --- src/app/crypto/engineCrypto/EngineCrypto.ts | 15 +++++++- .../bootstrapCrossSigning.test.ts | 38 +++++++++++++++++++ src/app/crypto/verification/verifier.ts | 2 +- .../useDeviceVerificationStatus.test.tsx | 8 ++-- src/app/utils/matrix-crypto.ts | 2 +- 16 files changed, 117 insertions(+), 71 deletions(-) create mode 100644 .changeset/fix-device-verified-badge.md diff --git a/.changeset/fix-device-verified-badge.md b/.changeset/fix-device-verified-badge.md new file mode 100644 index 0000000000..01e5724fa7 --- /dev/null +++ b/.changeset/fix-device-verified-badge.md @@ -0,0 +1,5 @@ +--- +default: patch +--- + +Fix devices showing as verified in Sable while other clients still see them as unverified diff --git a/src-tauri/src/matrix_crypto/devices.rs b/src-tauri/src/matrix_crypto/devices.rs index 4d39f9ec2d..ae06947e85 100644 --- a/src-tauri/src/matrix_crypto/devices.rs +++ b/src-tauri/src/matrix_crypto/devices.rs @@ -19,6 +19,8 @@ fn device_id(args: &Value, method: &str) -> Result { Ok(str_arg(args, method, "deviceId")?.into()) } +pub(super) const IDENTITY_QUERY_WAIT: Duration = Duration::from_secs(1); + fn timeout(args: &Value) -> Option { args.get("timeoutSecs") .and_then(Value::as_f64) @@ -139,7 +141,7 @@ async fn identity_for( ) -> Result, String> { let user = user_id(args, method, "userId")?; machine - .get_identity(&user, timeout(args)) + .get_identity(&user, timeout(args).or(Some(IDENTITY_QUERY_WAIT))) .await .map_err(|e| format!("{method} failed: {e}")) } @@ -189,7 +191,7 @@ fn query_keys_for_users(machine: &OlmMachine, args: &Value, method: &str) -> Res "body": super::requests::keys_query_body( &request.timeout, &json!(request.device_keys), - ), + )?, })) } diff --git a/src-tauri/src/matrix_crypto/dispatch.rs b/src-tauri/src/matrix_crypto/dispatch.rs index 6d6caf48e3..924af74fcb 100644 --- a/src-tauri/src/matrix_crypto/dispatch.rs +++ b/src-tauri/src/matrix_crypto/dispatch.rs @@ -132,7 +132,13 @@ fn processed_to_device_event_json( } }; - let mut value = serde_json::to_value(snapshot).ok()?; + let mut value = match serde_json::to_value(snapshot) { + Ok(value) => value, + Err(e) => { + log::warn!("Dropping an incoming to-device event we could not serialize: {e}"); + return None; + } + }; if let Some(request) = verification_request { value["verificationRequest"] = request; } diff --git a/src-tauri/src/matrix_crypto/mod.rs b/src-tauri/src/matrix_crypto/mod.rs index a0bd8b3a70..9f7c1b52df 100644 --- a/src-tauri/src/matrix_crypto/mod.rs +++ b/src-tauri/src/matrix_crypto/mod.rs @@ -173,7 +173,8 @@ fn store_dir( pub(super) static OPEN_GUARD: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); /// Opens a store and registers its machine, replacing any machine already open for the -/// account. Tauri-free so a cold push can open the same store without an `AppHandle`. +/// account. Callers holding `OPEN_GUARD` themselves use [`open_machine_locked`]. +#[cfg(test)] pub async fn open_machine( dir: &Path, passphrase: Option<&str>, @@ -244,16 +245,20 @@ pub async fn engine_open( None => store_dir(&app, &user_id, &device_id)?, }; - let (machine, info) = open_machine(&dir, passphrase.as_deref(), &user_id, &device_id).await?; - let account = account_key(&user_id, &device_id); + + let guard = OPEN_GUARD.lock().await; + let (machine, info) = + open_machine_locked(&dir, passphrase.as_deref(), &user_id, &device_id).await?; let listeners = events::spawn(&app, &machine, account.clone()); - if let Some(displaced) = engines() + let displaced = engines() .listeners .lock() .map_err(|e| e.to_string())? - .insert(account, listeners) - { + .insert(account, listeners); + drop(guard); + + if let Some(displaced) = displaced { for handle in displaced { handle.abort(); } diff --git a/src-tauri/src/matrix_crypto/requests.rs b/src-tauri/src/matrix_crypto/requests.rs index d80b8a905b..70c4fd9db3 100644 --- a/src-tauri/src/matrix_crypto/requests.rs +++ b/src-tauri/src/matrix_crypto/requests.rs @@ -32,23 +32,23 @@ pub(super) struct KeysClaimBody<'a> { pub(super) fn keys_claim_body( timeout: &Option, one_time_keys: &Value, -) -> String { +) -> Result { serde_json::to_string(&KeysClaimBody { timeout, one_time_keys, }) - .unwrap_or_default() + .map_err(|e| format!("serializing a /keys/claim body failed: {e}")) } pub(super) fn keys_query_body( timeout: &Option, device_keys: &Value, -) -> String { +) -> Result { serde_json::to_string(&KeysQueryBody { timeout, device_keys, }) - .unwrap_or_default() + .map_err(|e| format!("serializing a /keys/query body failed: {e}")) } /// `device_keys` is omitted when absent; an explicit `null` is not the same thing. @@ -93,12 +93,12 @@ pub async fn outgoing_requests(machine: &OlmMachine) -> Result { AnyOutgoingRequest::KeysQuery(req) => json!({ "type": request_type::KEYS_QUERY, "className": "KeysQueryRequest", - "body": keys_query_body(&req.timeout, &json!(req.device_keys)), + "body": keys_query_body(&req.timeout, &json!(req.device_keys))?, }), AnyOutgoingRequest::KeysClaim(req) => json!({ "type": request_type::KEYS_CLAIM, "className": "KeysClaimRequest", - "body": keys_claim_body(&req.timeout, &json!(req.one_time_keys)), + "body": keys_claim_body(&req.timeout, &json!(req.one_time_keys))?, }), AnyOutgoingRequest::ToDeviceRequest(req) => json!({ "type": request_type::TO_DEVICE, @@ -122,9 +122,9 @@ pub async fn outgoing_requests(machine: &OlmMachine) -> Result { }), }; entry["id"] = Value::String(id); - entry + Ok(entry) }) - .collect(), + .collect::, String>>()?, )) } diff --git a/src-tauri/src/matrix_crypto/rooms.rs b/src-tauri/src/matrix_crypto/rooms.rs index 5155eb751d..a485de12fb 100644 --- a/src-tauri/src/matrix_crypto/rooms.rs +++ b/src-tauri/src/matrix_crypto/rooms.rs @@ -97,8 +97,10 @@ fn collect_strategy(value: Option<&Value>, method: &str) -> Result Err(format!( @@ -268,15 +270,18 @@ pub async fn invoke( .get_missing_sessions(users.iter().map(AsRef::as_ref)) .await { - Ok(Some((txn_id, request))) => Ok(json!({ - "type": request_type::KEYS_CLAIM, - "className": "KeysClaimRequest", - "id": txn_id.to_string(), - "body": super::requests::keys_claim_body( - &request.timeout, - &json!(request.one_time_keys), - ), - })), + Ok(Some((txn_id, request))) => super::requests::keys_claim_body( + &request.timeout, + &json!(request.one_time_keys), + ) + .map(|body| { + json!({ + "type": request_type::KEYS_CLAIM, + "className": "KeysClaimRequest", + "id": txn_id.to_string(), + "body": body, + }) + }), Ok(None) => Ok(Value::Null), Err(e) => Err(format!("getMissingSessions failed: {e}")), } diff --git a/src-tauri/src/matrix_crypto/verification.rs b/src-tauri/src/matrix_crypto/verification.rs index ae58fb586e..8179c931ec 100644 --- a/src-tauri/src/matrix_crypto/verification.rs +++ b/src-tauri/src/matrix_crypto/verification.rs @@ -77,7 +77,7 @@ async fn identity( ) -> Result { let user = user_arg(args, method)?; machine - .get_identity(&user, None) + .get_identity(&user, Some(super::devices::IDENTITY_QUERY_WAIT)) .await .map_err(|e| format!("{method}: cannot load the identity of {user}: {e}"))? .ok_or_else(|| format!("{method}: no cross-signing identity for {user}")) @@ -466,25 +466,12 @@ pub async fn invoke( .map(verification_state) .unwrap_or(Value::Null)) } - "verificationRequest.accept" => { - request(machine, args, method).and_then(|request| { - match args.get("methods").and_then(Value::as_array) { - Some(codes) => { - let methods = codes - .iter() - .filter_map(Value::as_u64) - .map(|code| { - method_from_code(code).ok_or_else(|| { - format!("{method}: unknown verification method {code}") - }) - }) - .collect::, _>>()?; - Ok(optional_outgoing(request.accept_with_methods(methods))) - } - None => Ok(optional_outgoing(request.accept())), - } + "verificationRequest.accept" => request(machine, args, method).and_then(|request| { + Ok(match methods_arg(args, method)? { + Some(methods) => optional_outgoing(request.accept_with_methods(methods)), + None => optional_outgoing(request.accept()), }) - } + }), "verificationRequest.cancel" => { request(machine, args, method).map(|request| optional_outgoing(request.cancel())) } diff --git a/src/app/components/BackupRestore.test.tsx b/src/app/components/BackupRestore.test.tsx index 412ef81163..d2ccb2e628 100644 --- a/src/app/components/BackupRestore.test.tsx +++ b/src/app/components/BackupRestore.test.tsx @@ -107,7 +107,6 @@ describe('BackupRestoreTile', () => { ); expect(crypto.processDeviceLists).toHaveBeenCalledWith({ changed: ['@me:example.org'] }); expect(crypto.bootstrapCrossSigning).toHaveBeenCalled(); - expect(crypto.setDeviceVerified).toHaveBeenCalledWith('@me:example.org', 'DEVICE'); expect(crypto.loadSessionBackupPrivateKeyFromSecretStorage).toHaveBeenCalled(); }); diff --git a/src/app/components/BackupRestore.tsx b/src/app/components/BackupRestore.tsx index 549e2bc4eb..97643286da 100644 --- a/src/app/components/BackupRestore.tsx +++ b/src/app/components/BackupRestore.tsx @@ -73,12 +73,6 @@ function BackupKeyRecovery({ await cryptoBackend.bootstrapCrossSigning({}); await cryptoBackend.bootstrapSecretStorage({}); - const deviceId = mx.getDeviceId(); - if (!deviceId) { - throw new Error('Unexpected Error! Current device ID not found.'); - } - await cryptoBackend.setDeviceVerified(mx.getSafeUserId(), deviceId); - // Emits KeyBackupDecryptionKeyCached, which drives the restore. await crypto.loadSessionBackupPrivateKeyFromSecretStorage(); }, diff --git a/src/app/components/ManualVerification.test.tsx b/src/app/components/ManualVerification.test.tsx index 272ad7dfd5..8b89785ce2 100644 --- a/src/app/components/ManualVerification.test.tsx +++ b/src/app/components/ManualVerification.test.tsx @@ -11,7 +11,6 @@ const storePrivateKey = vi.hoisted(() => vi.fn<() => void>()); const processDeviceLists = vi.hoisted(() => vi.fn<() => Promise>()); const bootstrapCrossSigning = vi.hoisted(() => vi.fn<() => Promise>()); const bootstrapSecretStorage = vi.hoisted(() => vi.fn<() => Promise>()); -const setDeviceVerified = vi.hoisted(() => vi.fn<() => Promise>()); const loadSessionBackupPrivateKeyFromSecretStorage = vi.hoisted(() => vi.fn<() => Promise>()); vi.mock('$types/matrix-sdk', () => ({ decodeRecoveryKey })); @@ -26,7 +25,6 @@ vi.mock('$hooks/useMatrixClient', () => ({ processDeviceLists, bootstrapCrossSigning, bootstrapSecretStorage, - setDeviceVerified, loadSessionBackupPrivateKeyFromSecretStorage, }) as unknown as CryptoApi, }), @@ -59,7 +57,6 @@ describe('ManualVerificationTile', () => { processDeviceLists.mockResolvedValue(undefined); bootstrapCrossSigning.mockResolvedValue(undefined); bootstrapSecretStorage.mockResolvedValue(undefined); - setDeviceVerified.mockResolvedValue(undefined); loadSessionBackupPrivateKeyFromSecretStorage.mockResolvedValue(undefined); }); @@ -72,8 +69,9 @@ describe('ManualVerificationTile', () => { expect(storePrivateKey).toHaveBeenCalledWith(KEY_ID, recoveryKey); expect(processDeviceLists).toHaveBeenCalledWith({ changed: ['@me:example.org'] }); expect(bootstrapCrossSigning).toHaveBeenCalledAfter(processDeviceLists); - expect(setDeviceVerified).toHaveBeenCalledWith('@me:example.org', 'DEVICE'); - expect(setDeviceVerified).toHaveBeenCalledAfter(bootstrapCrossSigning); + expect(loadSessionBackupPrivateKeyFromSecretStorage).toHaveBeenCalledAfter( + bootstrapCrossSigning + ); }); it('invalidates the cached verification status after bootstrapping', async () => { diff --git a/src/app/components/ManualVerification.tsx b/src/app/components/ManualVerification.tsx index e9fc5fb6dc..3765e4fe1d 100644 --- a/src/app/components/ManualVerification.tsx +++ b/src/app/components/ManualVerification.tsx @@ -133,12 +133,6 @@ export function ManualVerificationTile({ await crypto.bootstrapCrossSigning({}); await crypto.bootstrapSecretStorage({}); - const deviceId = mx.getDeviceId(); - if (!deviceId) { - throw new Error('Unexpected Error! Current device ID not found.'); - } - await crypto.setDeviceVerified(mx.getSafeUserId(), deviceId); - await crypto.loadSessionBackupPrivateKeyFromSecretStorage(); refreshVerificationStatus(); diff --git a/src/app/crypto/engineCrypto/EngineCrypto.ts b/src/app/crypto/engineCrypto/EngineCrypto.ts index 4f256d4e66..f5e4c86aea 100644 --- a/src/app/crypto/engineCrypto/EngineCrypto.ts +++ b/src/app/crypto/engineCrypto/EngineCrypto.ts @@ -1555,11 +1555,19 @@ export class EngineCrypto }); } + async #queryOwnKeys(): Promise { + await this.#sendTracked( + await this.#call('queryKeysForUsers', { users: [this.#identity.userId] }) + ); + await this.#flushOutgoingRequests(); + } + async crossSignDevice(deviceId: string): Promise { await this.#sendTracked( await this.#call('device.verify', { userId: this.#identity.userId, deviceId }) ); await this.#flushOutgoingRequests(); + await this.#queryOwnKeys(); } async isCrossSigningReady(): Promise { @@ -1621,6 +1629,8 @@ export class EngineCrypto } async #importCrossSigningKeys(keys: Record): Promise { + await this.#queryOwnKeys(); + const status = (await this.#call('importCrossSigningKeys', keys)) as { hasMaster: boolean; hasSelfSigning: boolean; @@ -1634,7 +1644,10 @@ export class EngineCrypto userId: this.#identity.userId, deviceId: this.#identity.deviceId, })) as OutgoingRequest | null; - if (request) await sendOutgoingRequest(this.#mx, request); + if (request) { + await sendOutgoingRequest(this.#mx, request); + await this.#queryOwnKeys(); + } } async #exportCrossSigningKeysToStorage(): Promise { diff --git a/src/app/crypto/engineCrypto/bootstrapCrossSigning.test.ts b/src/app/crypto/engineCrypto/bootstrapCrossSigning.test.ts index a91b1c34a9..10ca86dab1 100644 --- a/src/app/crypto/engineCrypto/bootstrapCrossSigning.test.ts +++ b/src/app/crypto/engineCrypto/bootstrapCrossSigning.test.ts @@ -88,6 +88,44 @@ describe('bootstrapCrossSigning', () => { expect(requestsTo(authedRequest, '/_matrix/client/v3/keys/signatures/upload')).toHaveLength(1); }); + it('refreshes the published keys on both sides of a secret storage import', async () => { + engine((method) => { + if (method === 'crossSigningStatus') return noKeys; + if (method === 'queryKeysForUsers') + return { id: 'q1', type: 1, className: 'KeysQueryRequest', body: '{}' }; + if (method === 'importCrossSigningKeys') return allKeys; + if (method === 'device.verify') return { id: null, type: 4, body: '{}' }; + return undefined; + }); + const { mx, authedRequest } = clientStub({ storage: STORED_KEYS }); + + await new EngineCrypto(mx, { userId: '@me:e.org', deviceId: 'D' }).bootstrapCrossSigning({}); + + const methods = mockInvoke.mock.calls.map(([, name]) => name); + expect(methods.indexOf('queryKeysForUsers')).toBeLessThan( + methods.indexOf('importCrossSigningKeys') + ); + expect(methods.lastIndexOf('queryKeysForUsers')).toBeGreaterThan( + methods.indexOf('device.verify') + ); + expect(requestsTo(authedRequest, '/_matrix/client/v3/keys/query')).toHaveLength(2); + }); + + it('refreshes the published keys after blindly cross-signing a device', async () => { + engine((method) => { + if (method === 'queryKeysForUsers') + return { id: 'q1', type: 1, className: 'KeysQueryRequest', body: '{}' }; + if (method === 'device.verify') return { id: null, type: 4, body: '{}' }; + return undefined; + }); + const { mx, authedRequest } = clientStub(); + + await new EngineCrypto(mx, { userId: '@me:e.org', deviceId: 'D' }).crossSignDevice('OTHER'); + + expect(requestsTo(authedRequest, '/_matrix/client/v3/keys/signatures/upload')).toHaveLength(1); + expect(requestsTo(authedRequest, '/_matrix/client/v3/keys/query')).toHaveLength(1); + }); + it('refuses a secret storage import the engine did not actually apply', async () => { engine((method) => { if (method === 'crossSigningStatus') return noKeys; diff --git a/src/app/crypto/verification/verifier.ts b/src/app/crypto/verification/verifier.ts index 176f2599ad..6b4d6664d2 100644 --- a/src/app/crypto/verification/verifier.ts +++ b/src/app/crypto/verification/verifier.ts @@ -122,7 +122,7 @@ abstract class EngineVerifier private finishCancelled(flow: Record, error: Error): void { if (this.hasBeenCancelled) return; this.markCancelled(); - void this.call(this.cancelMethod, flow); + void this.call(this.cancelMethod, flow).catch(() => undefined); this.completion.reject(error); this.emit(VerifierEvent.Cancel, error); } diff --git a/src/app/hooks/useDeviceVerificationStatus.test.tsx b/src/app/hooks/useDeviceVerificationStatus.test.tsx index cf02c388c4..76a4710900 100644 --- a/src/app/hooks/useDeviceVerificationStatus.test.tsx +++ b/src/app/hooks/useDeviceVerificationStatus.test.tsx @@ -96,7 +96,7 @@ describe('useDeviceVerificationStatus', () => { await waitFor(() => expect(result.current).toBe(VerificationStatus.Verified)); }); - it('reports a locally verified device as verified', async () => { + it('reports a merely locally verified device as unverified', async () => { getDeviceVerificationStatus.mockResolvedValue( deviceVerificationStatus({ crossSigningVerified: false, localVerified: true }) ); @@ -105,10 +105,10 @@ describe('useDeviceVerificationStatus', () => { wrapper: createWrapper(), }); - await waitFor(() => expect(result.current).toBe(VerificationStatus.Verified)); + await waitFor(() => expect(result.current).toBe(VerificationStatus.Unverified)); }); - it('reports an owner-signed device as verified', async () => { + it('reports a device signed by an untrusted owner identity as unverified', async () => { getDeviceVerificationStatus.mockResolvedValue( deviceVerificationStatus({ crossSigningVerified: false, @@ -121,7 +121,7 @@ describe('useDeviceVerificationStatus', () => { wrapper: createWrapper(), }); - await waitFor(() => expect(result.current).toBe(VerificationStatus.Verified)); + await waitFor(() => expect(result.current).toBe(VerificationStatus.Unverified)); }); it('reports Unsupported when the device has no verification status', async () => { diff --git a/src/app/utils/matrix-crypto.ts b/src/app/utils/matrix-crypto.ts index 233ad0bd58..23ba678eb2 100644 --- a/src/app/utils/matrix-crypto.ts +++ b/src/app/utils/matrix-crypto.ts @@ -9,5 +9,5 @@ export const verifiedDevice = async ( if (!status) return null; - return !!(status.crossSigningVerified || status.localVerified || status.signedByOwner); + return status.crossSigningVerified; };