From 4c145af7a3bc0a67f4201a57d16a1a787796539d Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 14 Jul 2022 13:07:30 +0100 Subject: [PATCH 1/2] Don't restore session unless crypto data is found Add a check to ensure that we find crypto data in the crypto store when we're restoring a session and otherwise abort the session restore. This will prevent us from restoring a session and generating new keys when there was a previous session with different keys. ***This will force a logout for all users*** See the linked issue (and the comment in code) for more detail. Fixes https://github.com/vector-im/element-call/issues/464 --- src/ClientContext.tsx | 15 +++++--- src/auth/useInteractiveLogin.ts | 15 +++++--- src/auth/useInteractiveRegistration.ts | 15 +++++--- src/matrix-utils.ts | 51 ++++++++++++++++++++++++-- 4 files changed, 75 insertions(+), 21 deletions(-) diff --git a/src/ClientContext.tsx b/src/ClientContext.tsx index 68676ad5..503c3f8f 100644 --- a/src/ClientContext.tsx +++ b/src/ClientContext.tsx @@ -101,12 +101,15 @@ export const ClientProvider: FC = ({ children }) => { const { user_id, device_id, access_token, passwordlessUser } = session; - const client = await initClient({ - baseUrl: defaultHomeserver, - accessToken: access_token, - userId: user_id, - deviceId: device_id, - }); + const client = await initClient( + { + baseUrl: defaultHomeserver, + accessToken: access_token, + userId: user_id, + deviceId: device_id, + }, + true + ); /* eslint-enable camelcase */ return { client, isPasswordlessUser: passwordlessUser }; diff --git a/src/auth/useInteractiveLogin.ts b/src/auth/useInteractiveLogin.ts index 6379c67c..9ffa7ba6 100644 --- a/src/auth/useInteractiveLogin.ts +++ b/src/auth/useInteractiveLogin.ts @@ -57,12 +57,15 @@ export const useInteractiveLogin = () => passwordlessUser: false, }; - const client = await initClient({ - baseUrl: defaultHomeserver, - accessToken: access_token, - userId: user_id, - deviceId: device_id, - }); + const client = await initClient( + { + baseUrl: defaultHomeserver, + accessToken: access_token, + userId: user_id, + deviceId: device_id, + }, + false + ); /* eslint-enable camelcase */ return [client, session]; diff --git a/src/auth/useInteractiveRegistration.ts b/src/auth/useInteractiveRegistration.ts index af611f9d..8b8647a0 100644 --- a/src/auth/useInteractiveRegistration.ts +++ b/src/auth/useInteractiveRegistration.ts @@ -90,12 +90,15 @@ export const useInteractiveRegistration = (): [ const { user_id, access_token, device_id } = (await interactiveAuth.attemptAuth()) as any; - const client = await initClient({ - baseUrl: defaultHomeserver, - accessToken: access_token, - userId: user_id, - deviceId: device_id, - }); + const client = await initClient( + { + baseUrl: defaultHomeserver, + accessToken: access_token, + userId: user_id, + deviceId: device_id, + }, + false + ); await client.setDisplayName(displayName); diff --git a/src/matrix-utils.ts b/src/matrix-utils.ts index 1a1a2ea6..ea2920b0 100644 --- a/src/matrix-utils.ts +++ b/src/matrix-utils.ts @@ -24,6 +24,19 @@ export const defaultHomeserver = export const defaultHomeserverHost = new URL(defaultHomeserver).host; +export class CryptoStoreIntegrityError extends Error { + constructor() { + super("Crypto store data was expected, but none was found"); + } +} + +const SYNC_STORE_NAME = "element-call-sync"; +// Note that the crypto store name has changed from previous versions +// deliberately in order to force a logout for all users due to +// https://github.com/vector-im/element-call/issues/464 +// (It's a good opportunity to make the database names consistent.) +const CRYPTO_STORE_NAME = "element-call-crypto"; + function waitForSync(client: MatrixClient) { return new Promise((resolve, reject) => { const onSync = ( @@ -43,8 +56,18 @@ function waitForSync(client: MatrixClient) { }); } +/** + * Initialises and returns a new Matrix Client + * If true is passed for the 'restore' parameter, a check will be made + * to ensure that corresponding crypto data is stored and recovered. + * If the check fails, CryptoStoreIntegrityError will be thrown. + * @param clientOptions Object of options passed through to the client + * @param restore Whether the session is being restored from storage + * @returns The MatrixClient instance + */ export async function initClient( - clientOptions: ICreateClientOpts + clientOptions: ICreateClientOpts, + restore: boolean ): Promise { // TODO: https://gitlab.matrix.org/matrix-org/olm/-/issues/10 window.OLM_OPTIONS = {}; @@ -62,17 +85,39 @@ export async function initClient( storeOpts.store = new IndexedDBStore({ indexedDB: window.indexedDB, localStorage, - dbName: "element-call-sync", + dbName: SYNC_STORE_NAME, workerFactory: () => new IndexedDBWorker(), }); } else if (localStorage) { storeOpts.store = new MemoryStore({ localStorage }); } + // Check whether we have crypto data store. If we are restoring a session + // from storage then we will have started the crypto store and therefore + // have generated keys for that device, so if we can't recover those keys, + // we must not continue or we'll generate new keys and anyone who saw our + // previous keys will not accept our new key. + if (restore) { + if (indexedDB) { + const cryptoStoreExists = await IndexedDBCryptoStore.exists( + indexedDB, + CRYPTO_STORE_NAME + ); + if (!cryptoStoreExists) throw new CryptoStoreIntegrityError(); + } else if (localStorage) { + if (!LocalStorageCryptoStore.exists(localStorage)) + throw new CryptoStoreIntegrityError(); + } else { + // if we get here then we're using the memory store, which cannot + // possibly have remembered a session, so it's an error. + throw new CryptoStoreIntegrityError(); + } + } + if (indexedDB) { storeOpts.cryptoStore = new IndexedDBCryptoStore( indexedDB, - "matrix-js-sdk:crypto" + CRYPTO_STORE_NAME ); } else if (localStorage) { storeOpts.cryptoStore = new LocalStorageCryptoStore(localStorage); From 873e68e1e17db6f3bcb19fbb26976bf45eff9bb8 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 14 Jul 2022 13:24:22 +0100 Subject: [PATCH 2/2] Add notes from thinking through the need for storing what crypto db we use --- src/matrix-utils.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/matrix-utils.ts b/src/matrix-utils.ts index ea2920b0..3e576e78 100644 --- a/src/matrix-utils.ts +++ b/src/matrix-utils.ts @@ -97,6 +97,12 @@ export async function initClient( // have generated keys for that device, so if we can't recover those keys, // we must not continue or we'll generate new keys and anyone who saw our // previous keys will not accept our new key. + // It's worth mentioning here that if support for indexeddb or localstorage + // appears or disappears between sessions (it happens) then the failure mode + // here will be that we'll try a different store, not find crypto data and + // fail to restore the session. An alternative would be to continue using + // whatever we were using before, but that could be confusing since you could + // enable indexeddb and but the app would still not be using it. if (restore) { if (indexedDB) { const cryptoStoreExists = await IndexedDBCryptoStore.exists(