refactor: _updateUserDeviceKeys method

lib/encryption/key_manager.dart

@@ -370,10 +370,12 @@ class KeyManager {
             for (final device in devicesToReceive) {
               inboundSess.allowedAtIndex[device.userId] ??= <String, int>{};
               if (!inboundSess.allowedAtIndex[device.userId]
-                      .containsKey(device.deviceId) ||
+                      .containsKey(device.curve25519Key) ||
-                  inboundSess.allowedAtIndex[device.userId][device.deviceId] >
+                  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 = <String, Map<String, int>>{};
     for (final device in deviceKeys) {
       allowedAtIndex[device.userId] ??= <String, int>{};
-      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);

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<String, DateTime> _keyQueryFailures = {};
 
-  Future<void> _updateUserDeviceKeys() async {
+  Future<void> updateUserDeviceKeys() async {
     try {
-      if (!isLogged()) return;
+      if (!isLogged() || database == null) return;
       final dbActions = <Future<dynamic> 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,7 +1742,6 @@ class Client extends MatrixApi {
                     // Always trust the own device
                     entry.setDirectVerified(true);
                   }
-                  if (database != null) {
                   dbActions.add(() => database.storeUserDeviceKey(
                         id,
                         userId,
@@ -1718,7 +1751,6 @@ class Client extends MatrixApi {
                         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,7 +1763,6 @@ 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)) {
@@ -1740,7 +1771,6 @@ class Client extends MatrixApi {
                     () => database.removeUserDeviceKey(id, userId, deviceId));
               }
             }
-            }
             _userDeviceKeys[userId].outdated = false;
             if (database != null) {
               dbActions.add(

lib/src/database/database_api.dart

@@ -318,6 +318,16 @@ abstract class DatabaseApi {
 
   Future<List<StoredInboundGroupSession>> getInboundGroupSessionsToUpload();
 
+  Future<void> addSeenDeviceId(
+      int clientId, String userId, String deviceId, String publicKeys);
+
+  Future<void> addSeenPublicKey(
+      int clientId, String publicKey, String deviceId);
+
+  Future<String?> deviceIdSeen(int clientId, userId, deviceId);
+
+  Future<String?> publicKeySeen(int clientId, String publicKey);
+
   Future<dynamic> close();
 
   Future<T> transaction<T>(Future<T> Function() action);

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<void> 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<void> _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<void> addSeenDeviceId(
+    int clientId,
+    String userId,
+    String deviceId,
+    String publicKeysHash,
+  ) =>
+      _seenDeviceIdsBox.put(
+          MultiKey(userId, deviceId).toString(), publicKeysHash);
+
+  @override
+  Future<void> addSeenPublicKey(
+    int clientId,
+    String publicKey,
+    String deviceId,
+  ) =>
+      _seenDeviceKeysBox.put(publicKey.toHiveKey, deviceId);
+
+  @override
+  Future<String?> 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<String?> 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) {

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();
 

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(
-      expect(inbound.allowedAtIndex['@alice:example.com']['OTHERDEVICE'], 0);
+          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(
-      expect(inbound.allowedAtIndex['@alice:example.com']['OTHERDEVICE'], 0);
+          inbound.allowedAtIndex['@alice:example.com']
-      expect(inbound.allowedAtIndex['@alice:example.com']['NEWDEVICE'], 1);
+              ['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);
     });
   });
 }

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',
+      client2 = Client(
+        'othertestclient',
         httpClient: FakeMatrixApi(),
-          databaseBuilder: (_) => client1.database);
+        databaseBuilder: getDatabase,
+      );
       await client2.checkHomeserver('https://fakeserver.notexisting',
           checkWellKnown: false);
       await client2.init(

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);
     });
   });
 }