From 00771fc209da3eed222f59f1b1c85754bb6f9cce Mon Sep 17 00:00:00 2001 From: Christian Pauly Date: Thu, 9 Sep 2021 11:12:00 +0200 Subject: [PATCH] refactor: _updateUserDeviceKeys method --- lib/encryption/key_manager.dart | 13 ++-- lib/src/client.dart | 78 ++++++++++++++------- lib/src/database/database_api.dart | 10 +++ lib/src/database/hive_database.dart | 71 ++++++++++++++++++- test/client_test.dart | 12 +++- test/encryption/key_manager_test.dart | 75 ++++++++++++++++++-- test/encryption/key_verification_test.dart | 9 ++- test/encryption/online_key_backup_test.dart | 2 +- 8 files changed, 228 insertions(+), 42 deletions(-) diff --git a/lib/encryption/key_manager.dart b/lib/encryption/key_manager.dart index bcb62de3..f2b3f302 100644 --- a/lib/encryption/key_manager.dart +++ b/lib/encryption/key_manager.dart @@ -370,10 +370,12 @@ class KeyManager { for (final device in devicesToReceive) { inboundSess.allowedAtIndex[device.userId] ??= {}; if (!inboundSess.allowedAtIndex[device.userId] - .containsKey(device.deviceId) || - inboundSess.allowedAtIndex[device.userId][device.deviceId] > + .containsKey(device.curve25519Key) || + inboundSess.allowedAtIndex[device.userId] + [device.curve25519Key] > sess.outboundGroupSession.message_index()) { - inboundSess.allowedAtIndex[device.userId][device.deviceId] = + inboundSess.allowedAtIndex[device.userId] + [device.curve25519Key] = sess.outboundGroupSession.message_index(); } } @@ -471,7 +473,7 @@ class KeyManager { final allowedAtIndex = >{}; for (final device in deviceKeys) { allowedAtIndex[device.userId] ??= {}; - allowedAtIndex[device.userId][device.deviceId] = + allowedAtIndex[device.userId][device.curve25519Key] = outboundGroupSession.message_index(); } setInboundGroupSession( @@ -832,7 +834,8 @@ class KeyManager { // if we know the user may see the message, then we can just forward the key. // we do not need to check if the device is verified, just if it is not blocked, // as that is the logic we already initially try to send out the room keys. - final index = session.allowedAtIndex[device.userId][device.deviceId]; + final index = + session.allowedAtIndex[device.userId][device.curve25519Key]; Logs().i( '[KeyManager] Valid foreign request, forwarding key at index $index...'); await roomKeyRequest.forwardKey(index); diff --git a/lib/src/client.dart b/lib/src/client.dart index 58adf8e8..cea6e880 100644 --- a/lib/src/client.dart +++ b/lib/src/client.dart @@ -1138,7 +1138,7 @@ class Client extends MatrixApi { // ignore: unawaited_futures database?.deleteOldFiles( DateTime.now().subtract(Duration(days: 30)).millisecondsSinceEpoch); - await _updateUserDeviceKeys(); + await updateUserDeviceKeys(); if (encryptionEnabled) { encryption.onSync(); } @@ -1642,9 +1642,9 @@ class Client extends MatrixApi { final Map _keyQueryFailures = {}; - Future _updateUserDeviceKeys() async { + Future updateUserDeviceKeys() async { try { - if (!isLogged()) return; + if (!isLogged() || database == null) return; final dbActions = Function()>[]; final trackedUserIds = await _getUserIdsInEncryptedRooms(); if (!isLogged()) return; @@ -1691,11 +1691,45 @@ class Client extends MatrixApi { // Set the new device key for this device final entry = DeviceKeys.fromMatrixDeviceKeys( rawDeviceKeyEntry.value, this, oldKeys[deviceId]?.lastActive); - if (entry.isValid) { + if (entry.isValid && deviceId == entry.deviceId) { + // Check if deviceId or deviceKeys are known + if (!oldKeys.containsKey(deviceId)) { + final oldPublicKeys = + await database.deviceIdSeen(id, userId, deviceId); + if (oldPublicKeys != null && + oldPublicKeys != entry.curve25519Key + entry.ed25519Key) { + Logs().w( + 'Already seen Device ID has been added again. This might be an attack!'); + continue; + } + final oldDeviceId = + await database.publicKeySeen(id, entry.ed25519Key); + if (oldDeviceId != null && oldDeviceId != deviceId) { + Logs().w( + 'Already seen ED25519 has been added again. This might be an attack!'); + continue; + } + final oldDeviceId2 = + await database.publicKeySeen(id, entry.curve25519Key); + if (oldDeviceId2 != null && oldDeviceId2 != deviceId) { + Logs().w( + 'Already seen Curve25519 has been added again. This might be an attack!'); + continue; + } + await database.addSeenDeviceId(id, userId, deviceId, + entry.curve25519Key + entry.ed25519Key); + await database.addSeenPublicKey( + id, entry.ed25519Key, deviceId); + await database.addSeenPublicKey( + id, entry.curve25519Key, deviceId); + } + // is this a new key or the same one as an old one? // better store an update - the signatures might have changed! if (!oldKeys.containsKey(deviceId) || - oldKeys[deviceId].ed25519Key == entry.ed25519Key) { + (oldKeys[deviceId].ed25519Key == entry.ed25519Key && + oldKeys[deviceId].curve25519Key == + entry.curve25519Key)) { if (oldKeys.containsKey(deviceId)) { // be sure to save the verified status entry.setDirectVerified(oldKeys[deviceId].directVerified); @@ -1708,17 +1742,15 @@ class Client extends MatrixApi { // Always trust the own device entry.setDirectVerified(true); } - if (database != null) { - dbActions.add(() => database.storeUserDeviceKey( - id, - userId, - deviceId, - json.encode(entry.toJson()), - entry.directVerified, - entry.blocked, - entry.lastActive.millisecondsSinceEpoch, - )); - } + dbActions.add(() => database.storeUserDeviceKey( + id, + userId, + deviceId, + json.encode(entry.toJson()), + entry.directVerified, + entry.blocked, + entry.lastActive.millisecondsSinceEpoch, + )); } else if (oldKeys.containsKey(deviceId)) { // This shouldn't ever happen. The same device ID has gotten // a new public key. So we ignore the update. TODO: ask krille @@ -1731,14 +1763,12 @@ class Client extends MatrixApi { } } // delete old/unused entries - if (database != null) { - for (final oldDeviceKeyEntry in oldKeys.entries) { - final deviceId = oldDeviceKeyEntry.key; - if (!_userDeviceKeys[userId].deviceKeys.containsKey(deviceId)) { - // we need to remove an old key - dbActions.add( - () => database.removeUserDeviceKey(id, userId, deviceId)); - } + for (final oldDeviceKeyEntry in oldKeys.entries) { + final deviceId = oldDeviceKeyEntry.key; + if (!_userDeviceKeys[userId].deviceKeys.containsKey(deviceId)) { + // we need to remove an old key + dbActions.add( + () => database.removeUserDeviceKey(id, userId, deviceId)); } } _userDeviceKeys[userId].outdated = false; diff --git a/lib/src/database/database_api.dart b/lib/src/database/database_api.dart index 554ede22..11a86416 100644 --- a/lib/src/database/database_api.dart +++ b/lib/src/database/database_api.dart @@ -318,6 +318,16 @@ abstract class DatabaseApi { Future> getInboundGroupSessionsToUpload(); + Future addSeenDeviceId( + int clientId, String userId, String deviceId, String publicKeys); + + Future addSeenPublicKey( + int clientId, String publicKey, String deviceId); + + Future deviceIdSeen(int clientId, userId, deviceId); + + Future publicKeySeen(int clientId, String publicKey); + Future close(); Future transaction(Future Function() action); diff --git a/lib/src/database/hive_database.dart b/lib/src/database/hive_database.dart index b7b255fd..5f05635c 100644 --- a/lib/src/database/hive_database.dart +++ b/lib/src/database/hive_database.dart @@ -36,7 +36,7 @@ import 'package:hive/hive.dart'; /// /// This database does not support file caching! class FamedlySdkHiveDatabase extends DatabaseApi { - static const int version = 4; + static const int version = 5; final String name; late Box _clientBox; late Box _accountDataBox; @@ -74,6 +74,11 @@ class FamedlySdkHiveDatabase extends DatabaseApi { /// Key is a tuple as MultiKey(roomId, eventId) late LazyBox _eventsBox; + /// Key is a tuple as MultiKey(userId, deviceId) + late LazyBox _seenDeviceIdsBox; + + late LazyBox _seenDeviceKeysBox; + String get _clientBoxName => '$name.box.client'; String get _accountDataBoxName => '$name.box.account_data'; String get _roomsBoxName => '$name.box.rooms'; @@ -93,6 +98,8 @@ class FamedlySdkHiveDatabase extends DatabaseApi { String get _presencesBoxName => '$name.box.presences'; String get _timelineFragmentsBoxName => '$name.box.timeline_fragments'; String get _eventsBoxName => '$name.box.events'; + String get _seenDeviceIdsBoxName => '$name.box.seen_device_ids'; + String get _seenDeviceKeysBoxName => '$name.box.seen_device_keys'; final HiveCipher? encryptionCipher; @@ -120,6 +127,8 @@ class FamedlySdkHiveDatabase extends DatabaseApi { action(_presencesBox), action(_timelineFragmentsBox), action(_eventsBox), + action(_seenDeviceIdsBox), + action(_seenDeviceKeysBox), ]); Future open() async { @@ -191,6 +200,14 @@ class FamedlySdkHiveDatabase extends DatabaseApi { _eventsBoxName, encryptionCipher: encryptionCipher, ); + _seenDeviceIdsBox = await Hive.openLazyBox( + _seenDeviceIdsBoxName, + encryptionCipher: encryptionCipher, + ); + _seenDeviceKeysBox = await Hive.openLazyBox( + _seenDeviceKeysBoxName, + encryptionCipher: encryptionCipher, + ); // Check version and check if we need a migration final currentVersion = (await _clientBox.get('version') as int?); @@ -205,6 +222,25 @@ class FamedlySdkHiveDatabase extends DatabaseApi { Future _migrateFromVersion(int currentVersion) async { Logs().i('Migrate Hive database from version $currentVersion to $version'); + if (version == 5) { + for (final key in _userDeviceKeysBox.keys) { + try { + final raw = await _userDeviceKeysBox.get(key) as Map; + if (!raw.containsKey('keys')) continue; + final deviceKeys = DeviceKeys.fromJson( + convertToJson(raw), + Client(''), + ); + await addSeenDeviceId(0, deviceKeys.userId, deviceKeys.deviceId, + deviceKeys.curve25519Key + deviceKeys.ed25519Key); + await addSeenPublicKey(0, deviceKeys.ed25519Key, deviceKeys.deviceId); + await addSeenPublicKey( + 0, deviceKeys.curve25519Key, deviceKeys.deviceId); + } catch (e) { + Logs().w('Can not migrate device $key', e); + } + } + } await clearCache(0); await _clientBox.put('version', version); } @@ -1232,6 +1268,39 @@ class FamedlySdkHiveDatabase extends DatabaseApi { .map((raw) => StoredInboundGroupSession.fromJson(convertToJson(raw))) .toList(); } + + @override + Future addSeenDeviceId( + int clientId, + String userId, + String deviceId, + String publicKeysHash, + ) => + _seenDeviceIdsBox.put( + MultiKey(userId, deviceId).toString(), publicKeysHash); + + @override + Future addSeenPublicKey( + int clientId, + String publicKey, + String deviceId, + ) => + _seenDeviceKeysBox.put(publicKey.toHiveKey, deviceId); + + @override + Future deviceIdSeen(int clientId, userId, deviceId) async { + final raw = + await _seenDeviceIdsBox.get(MultiKey(userId, deviceId).toString()); + if (raw == null) return null; + return raw as String; + } + + @override + Future publicKeySeen(int clientId, String publicKey) async { + final raw = await _seenDeviceKeysBox.get(publicKey.toHiveKey); + if (raw == null) return null; + return raw as String; + } } dynamic _castValue(dynamic value) { diff --git a/test/client_test.dart b/test/client_test.dart index 858f5d21..2d5572d0 100644 --- a/test/client_test.dart +++ b/test/client_test.dart @@ -52,7 +52,11 @@ void main() { /// Check if all Elements get created - matrix = Client('testclient', httpClient: FakeMatrixApi()); + matrix = Client( + 'testclient', + httpClient: FakeMatrixApi(), + databaseBuilder: getDatabase, + ); eventUpdateListFuture = matrix.onEvent.stream.toList(); toDeviceUpdateListFuture = matrix.onToDeviceEvent.stream.toList(); @@ -291,7 +295,11 @@ void main() { }); test('Login', () async { - matrix = Client('testclient', httpClient: FakeMatrixApi()); + matrix = Client( + 'testclient', + httpClient: FakeMatrixApi(), + databaseBuilder: getDatabase, + ); eventUpdateListFuture = matrix.onEvent.stream.toList(); diff --git a/test/encryption/key_manager_test.dart b/test/encryption/key_manager_test.dart index d8e78197..7a896a4c 100644 --- a/test/encryption/key_manager_test.dart +++ b/test/encryption/key_manager_test.dart @@ -25,6 +25,7 @@ import 'package:test/test.dart'; import 'package:olm/olm.dart' as olm; import '../fake_client.dart'; +import '../fake_matrix_api.dart'; void main() { group('Key Manager', () { @@ -113,8 +114,14 @@ void main() { var inbound = client.encryption.keyManager.getInboundGroupSession( roomId, sess.outboundGroupSession.session_id(), client.identityKey); expect(inbound != null, true); - expect(inbound.allowedAtIndex['@alice:example.com']['JLAFKJWSCS'], 0); - expect(inbound.allowedAtIndex['@alice:example.com']['OTHERDEVICE'], 0); + expect( + inbound.allowedAtIndex['@alice:example.com'] + ['L+4+JCl8MD63dgo8z5Ta+9QAHXiANyOVSfgbHA5d3H8'], + 0); + expect( + inbound.allowedAtIndex['@alice:example.com'] + ['wMIDhiQl5jEXQrTB03ePOSQfR8sA/KMrW0CIfFfXKEE'], + 0); // rotate after too many messages sess.sentMessages = 300; @@ -208,9 +215,18 @@ void main() { true); inbound = client.encryption.keyManager.getInboundGroupSession( roomId, sess.outboundGroupSession.session_id(), client.identityKey); - expect(inbound.allowedAtIndex['@alice:example.com']['JLAFKJWSCS'], 0); - expect(inbound.allowedAtIndex['@alice:example.com']['OTHERDEVICE'], 0); - expect(inbound.allowedAtIndex['@alice:example.com']['NEWDEVICE'], 1); + expect( + inbound.allowedAtIndex['@alice:example.com'] + ['L+4+JCl8MD63dgo8z5Ta+9QAHXiANyOVSfgbHA5d3H8'], + 0); + expect( + inbound.allowedAtIndex['@alice:example.com'] + ['wMIDhiQl5jEXQrTB03ePOSQfR8sA/KMrW0CIfFfXKEE'], + 0); + expect( + inbound.allowedAtIndex['@alice:example.com'] + ['bnKQp6pPW0l9cGoIgHpBoK5OUi4h0gylJ7upc4asFV8'], + 1); // do not rotate if new user is added member.content['membership'] = 'leave'; @@ -500,9 +516,56 @@ void main() { session.free(); }); + test('Reused deviceID attack', () async { + if (!olmEnabled) return; + Logs().level = Level.warning; + client ??= await getClient(); + + // Ensure the device came from sync + expect( + client.userDeviceKeys['@alice:example.com'] + .deviceKeys['JLAFKJWSCS'] != + null, + true); + + // Alice removes her device + client.userDeviceKeys['@alice:example.com'].deviceKeys + .remove('JLAFKJWSCS'); + + // Alice adds her device with same device ID but different keys + final oldResp = FakeMatrixApi.api['POST']['/client/r0/keys/query'](null); + FakeMatrixApi.api['POST']['/client/r0/keys/query'] = (_) { + oldResp['device_keys']['@alice:example.com']['JLAFKJWSCS'] = { + 'user_id': '@alice:example.com', + 'device_id': 'JLAFKJWSCS', + 'algorithms': [ + 'm.olm.v1.curve25519-aes-sha2', + 'm.megolm.v1.aes-sha2' + ], + 'keys': { + 'curve25519:JLAFKJWSCS': + 'WbwrNyD7nvtmcLQ0TTuVPFGJq6JznfjrVsjIpmBqvDw', + 'ed25519:JLAFKJWSCS': 'vl0d54pTVRcvBgUzoQFa8e6TldHWG9O8bh0iuIvgd/I' + }, + 'signatures': { + '@alice:example.com': { + 'ed25519:JLAFKJWSCS': + 's/L86jLa8BTroL8GsBeqO0gRLC3ZrSA7Gch6UoLI2SefC1+1ycmnP9UGbLPh3qBJOmlhczMpBLZwelg87qNNDA' + } + } + }; + return oldResp; + }; + client.userDeviceKeys['@alice:example.com'].outdated = true; + await client.updateUserDeviceKeys(); + expect( + client.userDeviceKeys['@alice:example.com'].deviceKeys['JLAFKJWSCS'], + null); + }); + test('dispose client', () async { if (!olmEnabled) return; - await client.dispose(closeDatabase: true); + await client.dispose(closeDatabase: false); }); }); } diff --git a/test/encryption/key_verification_test.dart b/test/encryption/key_verification_test.dart index b4d7e242..1b7aa433 100644 --- a/test/encryption/key_verification_test.dart +++ b/test/encryption/key_verification_test.dart @@ -26,6 +26,7 @@ import 'package:test/test.dart'; import 'package:olm/olm.dart' as olm; import '../fake_client.dart'; +import '../fake_database.dart'; import '../fake_matrix_api.dart'; class MockSSSS extends SSSS { @@ -84,9 +85,11 @@ void main() { if (!olmEnabled) return; client1 = await getClient(); - client2 = Client('othertestclient', - httpClient: FakeMatrixApi(), - databaseBuilder: (_) => client1.database); + client2 = Client( + 'othertestclient', + httpClient: FakeMatrixApi(), + databaseBuilder: getDatabase, + ); await client2.checkHomeserver('https://fakeserver.notexisting', checkWellKnown: false); await client2.init( diff --git a/test/encryption/online_key_backup_test.dart b/test/encryption/online_key_backup_test.dart index 12f33995..4cd8feac 100644 --- a/test/encryption/online_key_backup_test.dart +++ b/test/encryption/online_key_backup_test.dart @@ -119,7 +119,7 @@ void main() { test('dispose client', () async { if (!olmEnabled) return; - await client.dispose(closeDatabase: true); + await client.dispose(closeDatabase: false); }); }); }