From 47d557b38dab64b310c5531fc23d49c277da72c3 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Thu, 10 Nov 2022 17:20:59 +0100 Subject: [PATCH 1/6] expand comment how to handle race here --- src/matrix/e2ee/DeviceTracker.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/matrix/e2ee/DeviceTracker.js b/src/matrix/e2ee/DeviceTracker.js index 484a6d0b..55f666e4 100644 --- a/src/matrix/e2ee/DeviceTracker.js +++ b/src/matrix/e2ee/DeviceTracker.js @@ -202,6 +202,9 @@ export class DeviceTracker { async _queryKeys(userIds, hsApi, log) { // TODO: we need to handle the race here between /sync and /keys/query just like we need to do for the member list ... // there are multiple requests going out for /keys/query though and only one for /members + // So, while doing /keys/query, writeDeviceChanges should add userIds marked as outdated to a list + // when /keys/query returns, we should check that list and requery if we queried for a given user. + // and then remove the list. const deviceKeyResponse = await hsApi.queryKeys({ "timeout": 10000, From 860f435855611c0191ed55dcd382bdbcf59efd8d Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Thu, 10 Nov 2022 17:26:21 +0100 Subject: [PATCH 2/6] log session afterSyncCompleted with normal log level --- src/matrix/Sync.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/matrix/Sync.js b/src/matrix/Sync.js index c995e3e2..4c414149 100644 --- a/src/matrix/Sync.js +++ b/src/matrix/Sync.js @@ -158,7 +158,7 @@ export class Sync { const isCatchupSync = this._status.get() === SyncStatus.CatchupSync; const sessionPromise = (async () => { try { - await log.wrap("session", log => this._session.afterSyncCompleted(sessionChanges, isCatchupSync, log), log.level.Detail); + await log.wrap("session", log => this._session.afterSyncCompleted(sessionChanges, isCatchupSync, log)); } catch (err) {} // error is logged, but don't fail sessionPromise })(); const roomsPromises = roomStates.map(async rs => { From 31579b4945352c3622d16e09e037629eff916e5b Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Thu, 10 Nov 2022 17:26:50 +0100 Subject: [PATCH 3/6] when tracking room, check roomId isn't on user we shouldn't share with --- src/matrix/e2ee/DeviceTracker.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/matrix/e2ee/DeviceTracker.js b/src/matrix/e2ee/DeviceTracker.js index 55f666e4..edfdbd31 100644 --- a/src/matrix/e2ee/DeviceTracker.js +++ b/src/matrix/e2ee/DeviceTracker.js @@ -120,6 +120,7 @@ export class DeviceTracker { const txn = await this._storage.readWriteTxn([ this._storage.storeNames.roomSummary, this._storage.storeNames.userIdentities, + this._storage.storeNames.deviceIdentities, // to remove all devices in _removeRoomFromUserIdentity ]); try { let isTrackingChanges; @@ -127,9 +128,13 @@ export class DeviceTracker { isTrackingChanges = room.writeIsTrackingMembers(true, txn); const members = Array.from(memberList.members.values()); log.set("members", members.length); + // TODO: should we remove any userIdentities we should not share the key with?? + // e.g. as an extra security measure if we had a mistake in other code? await Promise.all(members.map(async member => { if (shouldShareKey(member.membership, historyVisibility)) { await this._addRoomToUserIdentity(member.roomId, member.userId, txn); + } else { + await this._removeRoomFromUserIdentity(member.roomId, member.userId, txn); } })); } catch (err) { From c78bed846ec1719c93c1966d588175bc7b8c4932 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Thu, 10 Nov 2022 17:28:18 +0100 Subject: [PATCH 4/6] create unknown userIdentity when processing /keys/query response this can happen when the room isn't tracked yet, which is a use case we add support for in the next commit to verify senders that we don't know about yet (e.g. when the room isn't tracked). --- src/matrix/e2ee/DeviceTracker.js | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/matrix/e2ee/DeviceTracker.js b/src/matrix/e2ee/DeviceTracker.js index edfdbd31..410dfe85 100644 --- a/src/matrix/e2ee/DeviceTracker.js +++ b/src/matrix/e2ee/DeviceTracker.js @@ -21,13 +21,17 @@ import {RoomMember} from "../room/members/RoomMember.js"; const TRACKING_STATUS_OUTDATED = 0; const TRACKING_STATUS_UPTODATE = 1; +function createUserIdentity(userId, initialRoomId = undefined) { + return { + userId: userId, + roomIds: initialRoomId ? [initialRoomId] : [], + deviceTrackingStatus: TRACKING_STATUS_OUTDATED, + }; +} + function addRoomToIdentity(identity, userId, roomId) { if (!identity) { - identity = { - userId: userId, - roomIds: [roomId], - deviceTrackingStatus: TRACKING_STATUS_OUTDATED, - }; + identity = createUserIdentity(userId, roomId); return identity; } else { if (!identity.roomIds.includes(roomId)) { @@ -272,7 +276,15 @@ export class DeviceTracker { txn.deviceIdentities.set(deviceIdentity); } // mark user identities as up to date - const identity = await txn.userIdentities.get(userId); + let identity = await txn.userIdentities.get(userId); + if (!identity) { + // create the identity if it doesn't exist, which can happen if + // we request devices before tracking the room. + // IMPORTANT here that the identity gets created without any roomId! + // if we claim that we share and e2ee room with the user without having + // checked, we could share keys with that user without them being in the room + identity = createUserIdentity(userId); + } identity.deviceTrackingStatus = TRACKING_STATUS_UPTODATE; txn.userIdentities.set(identity); From 3d5a733267742a1a730f72ff160e058909d0bd08 Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Thu, 10 Nov 2022 17:51:40 +0100 Subject: [PATCH 5/6] split up _devicesForUserIds to reuse with different outdated criteria --- src/matrix/e2ee/DeviceTracker.js | 60 ++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 19 deletions(-) diff --git a/src/matrix/e2ee/DeviceTracker.js b/src/matrix/e2ee/DeviceTracker.js index 410dfe85..d200850b 100644 --- a/src/matrix/e2ee/DeviceTracker.js +++ b/src/matrix/e2ee/DeviceTracker.js @@ -343,6 +343,7 @@ export class DeviceTracker { /** * Gives all the device identities for a room that is already tracked. + * Can be used to decide which users to share keys with. * Assumes room is already tracked. Call `trackRoom` first if unsure. * @param {String} roomId [description] * @return {[type]} [description] @@ -359,41 +360,67 @@ export class DeviceTracker { // So, this will also contain non-joined memberships const userIds = await txn.roomMembers.getAllUserIds(roomId); - - return await this._devicesForUserIds(roomId, userIds, txn, hsApi, log); + // TODO: check here if userIds is safe? yes it is + return await this._devicesForUserIdsInTrackedRoom(roomId, userIds, txn, hsApi, log); } + /** + * Can be used to decide which users to share keys with. + * Assumes room is already tracked. Call `trackRoom` first if unsure. + */ async devicesForRoomMembers(roomId, userIds, hsApi, log) { const txn = await this._storage.readTxn([ this._storage.storeNames.userIdentities, ]); - return await this._devicesForUserIds(roomId, userIds, txn, hsApi, log); + return await this._devicesForUserIdsInTrackedRoom(roomId, userIds, txn, hsApi, log); + } } /** - * @param {string} roomId [description] - * @param {Array} userIds a set of user ids to try and find the identity for. Will be check to belong to roomId. + * Gets all the device identities with which keys should be shared for a set of users in a tracked room. + * If any userIdentities are outdated, it will fetch them from the homeserver. + * @param {string} roomId the id of the tracked room to filter users by. + * @param {Array} userIds a set of user ids to try and find the identity for. * @param {Transaction} userIdentityTxn to read the user identities * @param {HomeServerApi} hsApi - * @return {Array} + * @return {Array} all devices identities for the given users we should share keys with. */ - async _devicesForUserIds(roomId, userIds, userIdentityTxn, hsApi, log) { + async _devicesForUserIdsInTrackedRoom(roomId, userIds, userIdentityTxn, hsApi, log) { const allMemberIdentities = await Promise.all(userIds.map(userId => userIdentityTxn.userIdentities.get(userId))); const identities = allMemberIdentities.filter(identity => { - // identity will be missing for any userIds that don't have - // membership join in any of your encrypted rooms + // we use roomIds to decide with whom we should share keys for a given room, + // taking into account the membership and room history visibility. + // so filter out anyone who we shouldn't share keys with. + // Given we assume the room is tracked, + // also exclude any userId which doesn't have a userIdentity yet. return identity && identity.roomIds.includes(roomId); }); const upToDateIdentities = identities.filter(i => i.deviceTrackingStatus === TRACKING_STATUS_UPTODATE); - const outdatedIdentities = identities.filter(i => i.deviceTrackingStatus === TRACKING_STATUS_OUTDATED); + const outdatedUserIds = identities + .filter(i => i.deviceTrackingStatus === TRACKING_STATUS_OUTDATED) + .map(i => i.userId); + let devices = await this._devicesForUserIdentities(upToDateIdentities, outdatedUserIds, hsApi, log); + // filter out our own device as we should never share keys with it. + devices = devices.filter(device => { + const isOwnDevice = device.userId === this._ownUserId && device.deviceId === this._ownDeviceId; + return !isOwnDevice; + }); + return devices; + } + + /** Gets the device identites for a set of user identities that + * are known to be up to date, and a set of userIds that are known + * to be absent from our store our outdated. The outdated user ids + * will have their keys fetched from the homeserver. */ + async _devicesForUserIdentities(upToDateIdentities, outdatedUserIds, hsApi, log) { log.set("uptodate", upToDateIdentities.length); - log.set("outdated", outdatedIdentities.length); + log.set("outdated", outdatedUserIds.length); let queriedDevices; - if (outdatedIdentities.length) { + if (outdatedUserIds.length) { // TODO: ignore the race between /sync and /keys/query for now, // where users could get marked as outdated or added/removed from the room while // querying keys - queriedDevices = await this._queryKeys(outdatedIdentities.map(i => i.userId), hsApi, log); + queriedDevices = await this._queryKeys(outdatedUserIds, hsApi, log); } const deviceTxn = await this._storage.readTxn([ @@ -406,12 +433,7 @@ export class DeviceTracker { if (queriedDevices && queriedDevices.length) { flattenedDevices = flattenedDevices.concat(queriedDevices); } - // filter out our own device - const devices = flattenedDevices.filter(device => { - const isOwnDevice = device.userId === this._ownUserId && device.deviceId === this._ownDeviceId; - return !isOwnDevice; - }); - return devices; + return flattenedDevices; } async getDeviceByCurve25519Key(curve25519Key, txn) { From 155f4beba87b46d24452705999b55076ae58eb8c Mon Sep 17 00:00:00 2001 From: Bruno Windels <274386+bwindels@users.noreply.github.com> Date: Thu, 10 Nov 2022 17:53:11 +0100 Subject: [PATCH 6/6] add devicesForUser to fetch devices for untracked room and use it when fetching senders to verify. --- src/matrix/e2ee/DeviceTracker.js | 53 +++++++++++++++++++++++++++++++ src/matrix/e2ee/RoomEncryption.js | 7 ++-- 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/src/matrix/e2ee/DeviceTracker.js b/src/matrix/e2ee/DeviceTracker.js index d200850b..592ad24e 100644 --- a/src/matrix/e2ee/DeviceTracker.js +++ b/src/matrix/e2ee/DeviceTracker.js @@ -374,6 +374,29 @@ export class DeviceTracker { ]); return await this._devicesForUserIdsInTrackedRoom(roomId, userIds, txn, hsApi, log); } + + /** + * Cannot be used to decide which users to share keys with. + * Does not assume membership to any room or whether any room is tracked. + */ + async devicesForUsers(userIds, hsApi, log) { + const txn = await this._storage.readTxn([ + this._storage.storeNames.userIdentities, + ]); + + const upToDateIdentities = []; + const outdatedUserIds = []; + await Promise.all(userIds.map(async userId => { + const i = await txn.userIdentities.get(userId); + if (i && i.deviceTrackingStatus === TRACKING_STATUS_UPTODATE) { + upToDateIdentities.push(i); + } else if (!i || i.deviceTrackingStatus === TRACKING_STATUS_OUTDATED) { + // allow fetching for userIdentities we don't know about yet, + // as we don't assume the room is tracked here. + outdatedUserIds.push(userId); + } + })); + return this._devicesForUserIdentities(upToDateIdentities, outdatedUserIds, hsApi, log); } /** @@ -773,5 +796,35 @@ export function tests() { const txn2 = await storage.readTxn([storage.storeNames.userIdentities]); assert.deepEqual((await txn2.userIdentities.get("@bob:hs.tld")).roomIds, ["!abc:hs.tld", "!def:hs.tld"]); }, + "devicesForUsers fetches users even though they aren't in any tracked room": async assert => { + const storage = await createMockStorage(); + const tracker = new DeviceTracker({ + storage, + getSyncToken: () => "token", + olmUtil: {ed25519_verify: () => {}}, // valid if it does not throw + ownUserId: "@alice:hs.tld", + ownDeviceId: "ABCD", + }); + const hsApi = createQueryKeysHSApiMock(); + const devices = await tracker.devicesForUsers(["@bob:hs.tld"], hsApi, NullLoggerInstance.item); + assert.equal(devices.length, 1); + assert.equal(devices[0].curve25519Key, "curve25519:@bob:hs.tld:device1:key"); + const txn1 = await storage.readTxn([storage.storeNames.userIdentities]); + assert.deepEqual((await txn1.userIdentities.get("@bob:hs.tld")).roomIds, []); + }, + "devicesForUsers doesn't add any roomId when creating userIdentity": async assert => { + const storage = await createMockStorage(); + const tracker = new DeviceTracker({ + storage, + getSyncToken: () => "token", + olmUtil: {ed25519_verify: () => {}}, // valid if it does not throw + ownUserId: "@alice:hs.tld", + ownDeviceId: "ABCD", + }); + const hsApi = createQueryKeysHSApiMock(); + await tracker.devicesForUsers(["@bob:hs.tld"], hsApi, NullLoggerInstance.item); + const txn1 = await storage.readTxn([storage.storeNames.userIdentities]); + assert.deepEqual((await txn1.userIdentities.get("@bob:hs.tld")).roomIds, []); + } } } diff --git a/src/matrix/e2ee/RoomEncryption.js b/src/matrix/e2ee/RoomEncryption.js index 4dd6cd0b..27388e1c 100644 --- a/src/matrix/e2ee/RoomEncryption.js +++ b/src/matrix/e2ee/RoomEncryption.js @@ -230,9 +230,10 @@ export class RoomEncryption { return senders.add(r.encryptedEvent.sender); }, new Set())); log.set("senders", sendersWithoutDevice); - // fetch the devices, ignore return value, - // and just reuse _verifyDecryptionResults method so we only have one impl how to verify - await this._deviceTracker.devicesForRoomMembers(this._room.id, sendersWithoutDevice, hsApi, log); + // Fetch the devices, ignore return value, and just reuse + // _verifyDecryptionResults method so we only have one impl how to verify. + // Use devicesForUsers rather than devicesForRoomMembers as the room might not be tracked yet + await this._deviceTracker.devicesForUsers(sendersWithoutDevice, hsApi, log); // now that we've fetched the missing devices, try verifying the results again const txn = await this._storage.readTxn([this._storage.storeNames.deviceIdentities]); await this._verifyDecryptionResults(resultsWithoutDevice, txn);