Skip to content

Commit 19b09bf

Browse files
turt2liveturt2live
committed
Protect against double device reuse
Fixes #131
1 parent 8e2188d commit 19b09bf

8 files changed

+160
-83
lines changed

‎src/e2ee/CryptoClient.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,7 @@ export class CryptoClient {
332332
LogService.warn("CryptoClient", `Server injected unexpected user: ${userId} - not claiming keys`);
333333
continue;
334334
}
335-
const storedDevices = await this.client.cryptoStore.getUserDevices(userId);
335+
const storedDevices = await this.client.cryptoStore.getActiveUserDevices(userId);
336336
for (const deviceId of Object.keys(claimed.one_time_keys[userId])) {
337337
try {
338338
if (!otkClaimRequest[userId][deviceId]) {
@@ -579,7 +579,7 @@ export class CryptoClient {
579579
}
580580

581581
const encrypted = event.megolmProperties;
582-
const senderDevice = await this.client.cryptoStore.getUserDevice(event.sender, encrypted.device_id);
582+
const senderDevice = await this.client.cryptoStore.getActiveUserDevice(event.sender, encrypted.device_id);
583583
if (!senderDevice) {
584584
throw new Error("Unable to decrypt: Unknown device for sender");
585585
}
@@ -650,7 +650,7 @@ export class CryptoClient {
650650
return;
651651
}
652652

653-
const userDevices = await this.client.cryptoStore.getUserDevices(message.sender);
653+
const userDevices = await this.client.cryptoStore.getActiveUserDevices(message.sender);
654654
const senderDevice = userDevices.find(d => d.keys[`${DeviceKeyAlgorithm.Curve25519}:${d.device_id}`] === message.content.sender_key);
655655
if (!senderDevice) {
656656
LogService.warn("CryptoClient", "Received encrypted message from unknown identity key (ignoring message):", message.content.sender_key);

‎src/e2ee/DeviceTracker.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ export class DeviceTracker {
3030

3131
const userDeviceMap: Record<string, UserDevice[]> = {};
3232
for (const userId of userIds) {
33-
userDeviceMap[userId] = await this.client.cryptoStore.getUserDevices(userId);
33+
userDeviceMap[userId] = await this.client.cryptoStore.getActiveUserDevices(userId);
3434
}
3535
return userDeviceMap;
3636
}
@@ -88,7 +88,7 @@ export class DeviceTracker {
8888
continue;
8989
}
9090

91-
const currentDevices = await this.client.cryptoStore.getUserDevices(userId);
91+
const currentDevices = await this.client.cryptoStore.getAllUserDevices(userId);
9292
const existingDevice = currentDevices.find(d => d.device_id === deviceId);
9393

9494
if (existingDevice) {
@@ -114,7 +114,7 @@ export class DeviceTracker {
114114
validated.push(device);
115115
}
116116

117-
await this.client.cryptoStore.setUserDevices(userId, validated);
117+
await this.client.cryptoStore.setActiveUserDevices(userId, validated);
118118
}
119119
} catch (e) {
120120
LogService.error("DeviceTracker", "Error updating device lists:", e);

‎src/models/Crypto.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,18 @@ export interface UserDevice {
9999
};
100100
}
101101

102+
/**
103+
* Represents a user's stored device.
104+
* @category Models
105+
*/
106+
export interface StoredUserDevice extends UserDevice {
107+
unsigned: {
108+
[k: string]: any;
109+
device_display_name?: string;
110+
bsdkIsActive: boolean;
111+
};
112+
}
113+
102114
/**
103115
* Device list response for a multi-user query.
104116
* @category Models

‎src/storage/ICryptoStorageProvider.ts

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
import { EncryptionEventContent } from "../models/events/EncryptionEvent";
2-
import { IInboundGroupSession, IOlmSession, IOutboundGroupSession, UserDevice } from "../models/Crypto";
2+
import {
3+
IInboundGroupSession,
4+
IOlmSession,
5+
IOutboundGroupSession,
6+
StoredUserDevice,
7+
UserDevice,
8+
} from "../models/Crypto";
39

410
/**
511
* A storage provider capable of only providing crypto-related storage.
@@ -70,23 +76,32 @@ export interface ICryptoStorageProvider {
7076
* @param {UserDevice[]} devices The devices to set for the user.
7177
* @returns {Promise<void>} Resolves when complete.
7278
*/
73-
setUserDevices(userId: string, devices: UserDevice[]): Promise<void>;
79+
setActiveUserDevices(userId: string, devices: UserDevice[]): Promise<void>;
7480

7581
/**
76-
* Gets the user's stored devices. If no devices are stored, an empty array is returned.
82+
* Gets the user's active stored devices. If no devices are stored, an empty array is returned.
7783
* @param {string} userId The user ID to get devices for.
7884
* @returns {Promise<UserDevice[]>} Resolves to the array of devices for the user. If no
7985
* devices are known, the array will be empty.
8086
*/
81-
getUserDevices(userId: string): Promise<UserDevice[]>;
87+
getActiveUserDevices(userId: string): Promise<UserDevice[]>;
8288

8389
/**
84-
* Gets a user's stored device. If the device is not known or active, falsy is returned.
90+
* Gets a user's active stored device. If the device is not known or active, falsy is returned.
8591
* @param {string} userId The user ID.
8692
* @param {string} deviceId The device ID.
8793
* @returns {Promise<UserDevice>} Resolves to the user's device, or falsy if not known.
8894
*/
89-
getUserDevice(userId: string, deviceId: string): Promise<UserDevice>;
95+
getActiveUserDevice(userId: string, deviceId: string): Promise<UserDevice>;
96+
97+
/**
98+
* Gets all of the user's devices, regardless of whether or not they are active. The active flag
99+
* will be stored in the unsigned portion of the returned device.
100+
* @param {string} userId The user ID to get devices for.
101+
* @returns {Promise<StoredUserDevice[]>} Resolves to the array of devices for the user, or empty
102+
* if no devices are known.
103+
*/
104+
getAllUserDevices(userId: string): Promise<StoredUserDevice[]>;
90105

91106
/**
92107
* Flags multiple user's device lists as outdated.

‎src/storage/SqliteCryptoStorageProvider.ts

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,13 @@
11
import { ICryptoStorageProvider } from "./ICryptoStorageProvider";
22
import { EncryptionEventContent } from "../models/events/EncryptionEvent";
33
import * as Database from "better-sqlite3";
4-
import { IInboundGroupSession, IOlmSession, IOutboundGroupSession, UserDevice } from "../models/Crypto";
4+
import {
5+
IInboundGroupSession,
6+
IOlmSession,
7+
IOutboundGroupSession,
8+
StoredUserDevice,
9+
UserDevice,
10+
} from "../models/Crypto";
511

612
/**
713
* Sqlite crypto storage provider. Requires `better-sqlite3` package to be installed.
@@ -19,7 +25,8 @@ export class SqliteCryptoStorageProvider implements ICryptoStorageProvider {
1925
private userDeviceUpsert: Database.Statement;
2026
private userDevicesDelete: Database.Statement;
2127
private userDevicesSelect: Database.Statement;
22-
private userDeviceSelect: Database.Statement;
28+
private userActiveDevicesSelect: Database.Statement;
29+
private userActiveDeviceSelect: Database.Statement;
2330
private obGroupSessionUpsert: Database.Statement;
2431
private obGroupSessionSelect: Database.Statement;
2532
private obGroupCurrentSessionSelect: Database.Statement;
@@ -45,7 +52,7 @@ export class SqliteCryptoStorageProvider implements ICryptoStorageProvider {
4552
this.db.exec("CREATE TABLE IF NOT EXISTS kv (name TEXT PRIMARY KEY NOT NULL, value TEXT NOT NULL)");
4653
this.db.exec("CREATE TABLE IF NOT EXISTS rooms (room_id TEXT PRIMARY KEY NOT NULL, config TEXT NOT NULL)");
4754
this.db.exec("CREATE TABLE IF NOT EXISTS users (user_id TEXT PRIMARY KEY NOT NULL, outdated TINYINT NOT NULL)");
48-
this.db.exec("CREATE TABLE IF NOT EXISTS user_devices (user_id TEXT NOT NULL, device_id TEXT NOT NULL, device TEXT NOT NULL, PRIMARY KEY (user_id, device_id))");
55+
this.db.exec("CREATE TABLE IF NOT EXISTS user_devices (user_id TEXT NOT NULL, device_id TEXT NOT NULL, device TEXT NOT NULL, active TINYINT NOT NULL, PRIMARY KEY (user_id, device_id))");
4956
this.db.exec("CREATE TABLE IF NOT EXISTS outbound_group_sessions (session_id TEXT NOT NULL, room_id TEXT NOT NULL, current TINYINT NOT NULL, pickled TEXT NOT NULL, uses_left NUMBER NOT NULL, expires_ts NUMBER NOT NULL, PRIMARY KEY (session_id, room_id))");
5057
this.db.exec("CREATE TABLE IF NOT EXISTS sent_outbound_group_sessions (session_id TEXT NOT NULL, room_id TEXT NOT NULL, session_index INT NOT NULL, user_id TEXT NOT NULL, device_id TEXT NOT NULL, PRIMARY KEY (session_id, room_id, user_id, device_id, session_index))");
5158
this.db.exec("CREATE TABLE IF NOT EXISTS olm_sessions (user_id TEXT NOT NULL, device_id TEXT NOT NULL, session_id TEXT NOT NULL, last_decryption_ts NUMBER NOT NULL, pickled TEXT NOT NULL, PRIMARY KEY (user_id, device_id, session_id))");
@@ -62,10 +69,11 @@ export class SqliteCryptoStorageProvider implements ICryptoStorageProvider {
6269
this.userUpsert = this.db.prepare("INSERT INTO users (user_id, outdated) VALUES (@userId, @outdated) ON CONFLICT (user_id) DO UPDATE SET outdated = @outdated");
6370
this.userSelect = this.db.prepare("SELECT user_id, outdated FROM users WHERE user_id = @userId");
6471

65-
this.userDeviceUpsert = this.db.prepare("INSERT INTO user_devices (user_id, device_id, device) VALUES (@userId, @deviceId, @device) ON CONFLICT (user_id, device_id) DO UPDATE SET device = @device");
66-
this.userDevicesDelete = this.db.prepare("DELETE FROM user_devices WHERE user_id = @userId");
67-
this.userDevicesSelect = this.db.prepare("SELECT user_id, device_id, device FROM user_devices WHERE user_id = @userId");
68-
this.userDeviceSelect = this.db.prepare("SELECT user_id, device_id, device FROM user_devices WHERE user_id = @userId AND device_id = @deviceId");
72+
this.userDeviceUpsert = this.db.prepare("INSERT INTO user_devices (user_id, device_id, device, active) VALUES (@userId, @deviceId, @device, @active) ON CONFLICT (user_id, device_id) DO UPDATE SET device = @device, active = @active");
73+
this.userDevicesDelete = this.db.prepare("UPDATE user_devices SET active = 0 WHERE user_id = @userId");
74+
this.userDevicesSelect = this.db.prepare("SELECT user_id, device_id, device, active FROM user_devices WHERE user_id = @userId");
75+
this.userActiveDevicesSelect = this.db.prepare("SELECT user_id, device_id, device, active FROM user_devices WHERE user_id = @userId AND active = 1");
76+
this.userActiveDeviceSelect = this.db.prepare("SELECT user_id, device_id, device, active FROM user_devices WHERE user_id = @userId AND device_id = @deviceId AND active = 1");
6977

7078
this.obGroupSessionUpsert = this.db.prepare("INSERT INTO outbound_group_sessions (session_id, room_id, current, pickled, uses_left, expires_ts) VALUES (@sessionId, @roomId, @current, @pickled, @usesLeft, @expiresTs) ON CONFLICT (session_id, room_id) DO UPDATE SET pickled = @pickled, current = @current, uses_left = @usesLeft, expires_ts = @expiresTs");
7179
this.obGroupSessionSelect = this.db.prepare("SELECT session_id, room_id, current, pickled, uses_left, expires_ts FROM outbound_group_sessions WHERE session_id = @sessionId AND room_id = @roomId");
@@ -135,28 +143,34 @@ export class SqliteCryptoStorageProvider implements ICryptoStorageProvider {
135143
return val ? JSON.parse(val) : null;
136144
}
137145

138-
public async setUserDevices(userId: string, devices: UserDevice[]): Promise<void> {
146+
public async setActiveUserDevices(userId: string, devices: UserDevice[]): Promise<void> {
139147
this.db.transaction(() => {
140148
this.userUpsert.run({userId: userId, outdated: 0});
141149
this.userDevicesDelete.run({userId: userId});
142150
for (const device of devices) {
143-
this.userDeviceUpsert.run({userId: userId, deviceId: device.device_id, device: JSON.stringify(device)});
151+
this.userDeviceUpsert.run({userId: userId, deviceId: device.device_id, device: JSON.stringify(device), active: 1});
144152
}
145153
})();
146154
}
147155

148-
public async getUserDevices(userId: string): Promise<UserDevice[]> {
149-
const results = this.userDevicesSelect.all({userId: userId})
156+
public async getActiveUserDevices(userId: string): Promise<UserDevice[]> {
157+
const results = this.userActiveDevicesSelect.all({userId: userId})
150158
if (!results) return [];
151159
return results.map(d => JSON.parse(d.device));
152160
}
153161

154-
public async getUserDevice(userId: string, deviceId: string): Promise<UserDevice> {
155-
const result = this.userDeviceSelect.get({userId: userId, deviceId: deviceId});
162+
public async getActiveUserDevice(userId: string, deviceId: string): Promise<UserDevice> {
163+
const result = this.userActiveDeviceSelect.get({userId: userId, deviceId: deviceId});
156164
if (!result) return null;
157165
return JSON.parse(result.device);
158166
}
159167

168+
public async getAllUserDevices(userId: string): Promise<StoredUserDevice[]> {
169+
const results = this.userDevicesSelect.all({userId: userId})
170+
if (!results) return [];
171+
return results.map(d => Object.assign({}, JSON.parse(d.device), {unsigned: {bsdkIsActive: d.active === 1}}));
172+
}
173+
160174
public async flagUsersOutdated(userIds: string[]): Promise<void> {
161175
this.db.transaction(() => {
162176
for (const userId of userIds) {

‎test/encryption/CryptoClientTest.ts

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -712,7 +712,7 @@ describe('CryptoClient', () => {
712712
throw new Error("Not called appropriately");
713713
};
714714

715-
client.cryptoStore.getUserDevices = async (uid) => {
715+
client.cryptoStore.getActiveUserDevices = async (uid) => {
716716
expect(uid).toEqual(targetUserId);
717717
return [{
718718
user_id: targetUserId,
@@ -800,7 +800,7 @@ describe('CryptoClient', () => {
800800
return session;
801801
};
802802

803-
client.cryptoStore.getUserDevices = async (uid) => {
803+
client.cryptoStore.getActiveUserDevices = async (uid) => {
804804
expect(uid).toEqual(claimUserId);
805805
return [{
806806
user_id: claimUserId,
@@ -892,7 +892,7 @@ describe('CryptoClient', () => {
892892
return null;
893893
};
894894

895-
client.cryptoStore.getUserDevices = async (uid) => {
895+
client.cryptoStore.getActiveUserDevices = async (uid) => {
896896
expect(uid).toEqual(claimUserId);
897897
return [{
898898
user_id: claimUserId,
@@ -978,7 +978,7 @@ describe('CryptoClient', () => {
978978
return null;
979979
};
980980

981-
client.cryptoStore.getUserDevices = async (uid) => {
981+
client.cryptoStore.getActiveUserDevices = async (uid) => {
982982
expect(uid).toEqual(claimUserId);
983983
return [{
984984
user_id: claimUserId,
@@ -1063,7 +1063,7 @@ describe('CryptoClient', () => {
10631063
return null;
10641064
};
10651065

1066-
client.cryptoStore.getUserDevices = async (uid) => {
1066+
client.cryptoStore.getActiveUserDevices = async (uid) => {
10671067
expect(uid).toEqual(claimUserId);
10681068
return [{
10691069
user_id: claimUserId,
@@ -1135,7 +1135,7 @@ describe('CryptoClient', () => {
11351135
return null;
11361136
};
11371137

1138-
client.cryptoStore.getUserDevices = async (uid) => {
1138+
client.cryptoStore.getActiveUserDevices = async (uid) => {
11391139
expect(uid).toEqual(claimUserId);
11401140
return [{
11411141
user_id: claimUserId,
@@ -1199,7 +1199,7 @@ describe('CryptoClient', () => {
11991199
return null;
12001200
};
12011201

1202-
client.cryptoStore.getUserDevices = async (uid) => {
1202+
client.cryptoStore.getActiveUserDevices = async (uid) => {
12031203
expect(uid).toEqual(claimUserId);
12041204
return [{
12051205
user_id: claimUserId,
@@ -1279,7 +1279,7 @@ describe('CryptoClient', () => {
12791279
});
12801280
client.claimOneTimeKeys = claimSpy;
12811281

1282-
client.cryptoStore.getUserDevices = async (uid) => {
1282+
client.cryptoStore.getActiveUserDevices = async (uid) => {
12831283
expect(uid).toEqual(claimUserId);
12841284
return [{
12851285
user_id: claimUserId,
@@ -2147,7 +2147,7 @@ describe('CryptoClient', () => {
21472147
LogService.setLogger({ warn: logSpy } as any as ILogger);
21482148

21492149
const sender = "@bob:example.org";
2150-
client.cryptoStore.getUserDevices = async (uid) => {
2150+
client.cryptoStore.getActiveUserDevices = async (uid) => {
21512151
expect(uid).toEqual(sender);
21522152
return [STATIC_TEST_DEVICES["NTTFKSVBSI"]];
21532153
};
@@ -2256,7 +2256,7 @@ describe('CryptoClient', () => {
22562256

22572257
beforeEach(async () => {
22582258
await client.crypto.prepare([]);
2259-
client.cryptoStore.getUserDevices = async (uid) => {
2259+
client.cryptoStore.getActiveUserDevices = async (uid) => {
22602260
expect(uid).toEqual(senderDevice.user_id);
22612261
return [senderDevice];
22622262
};
@@ -2873,7 +2873,7 @@ describe('CryptoClient', () => {
28732873
expect(did).toEqual(event.content.device_id);
28742874
return null;
28752875
});
2876-
client.cryptoStore.getUserDevice = getSpy;
2876+
client.cryptoStore.getActiveUserDevice = getSpy;
28772877

28782878
try {
28792879
await client.crypto.decryptRoomEvent(event, roomId);
@@ -2907,7 +2907,7 @@ describe('CryptoClient', () => {
29072907
expect(did).toEqual(event.content.device_id);
29082908
return RECEIVER_DEVICE;
29092909
});
2910-
client.cryptoStore.getUserDevice = getSpy;
2910+
client.cryptoStore.getActiveUserDevice = getSpy;
29112911

29122912
try {
29132913
await client.crypto.decryptRoomEvent(event, roomId);
@@ -2941,7 +2941,7 @@ describe('CryptoClient', () => {
29412941
expect(did).toEqual(event.content.device_id);
29422942
return RECEIVER_DEVICE;
29432943
});
2944-
client.cryptoStore.getUserDevice = getDeviceSpy;
2944+
client.cryptoStore.getActiveUserDevice = getDeviceSpy;
29452945

29462946
const getSessionSpy = simple.stub().callFn(async (uid, did, rid, sid) => {
29472947
expect(uid).toEqual(userId);
@@ -2968,7 +2968,7 @@ describe('CryptoClient', () => {
29682968
it('should fail the decryption looks like a replay attack', async () => {
29692969
await client.crypto.prepare([]);
29702970

2971-
await client.cryptoStore.setUserDevices(RECEIVER_DEVICE.user_id, [RECEIVER_DEVICE]);
2971+
await client.cryptoStore.setActiveUserDevices(RECEIVER_DEVICE.user_id, [RECEIVER_DEVICE]);
29722972

29732973
// Make an encrypted event, and store the outbound keys as inbound
29742974
const plainType = "org.example.plain";
@@ -3030,7 +3030,7 @@ describe('CryptoClient', () => {
30303030
it('should succeed at re-decryption (valid replay)', async () => {
30313031
await client.crypto.prepare([]);
30323032

3033-
await client.cryptoStore.setUserDevices(RECEIVER_DEVICE.user_id, [RECEIVER_DEVICE]);
3033+
await client.cryptoStore.setActiveUserDevices(RECEIVER_DEVICE.user_id, [RECEIVER_DEVICE]);
30343034

30353035
// Make an encrypted event, and store the outbound keys as inbound
30363036
const plainType = "org.example.plain";
@@ -3097,7 +3097,7 @@ describe('CryptoClient', () => {
30973097
it('should succeed at decryption', async () => {
30983098
await client.crypto.prepare([]);
30993099

3100-
await client.cryptoStore.setUserDevices(RECEIVER_DEVICE.user_id, [RECEIVER_DEVICE]);
3100+
await client.cryptoStore.setActiveUserDevices(RECEIVER_DEVICE.user_id, [RECEIVER_DEVICE]);
31013101

31023102
// Make an encrypted event, and store the outbound keys as inbound
31033103
const plainType = "org.example.plain";

0 commit comments

Comments
 (0)