fix: Verify device signatures before storing devices and block those with invalid signatures

2020-12-21 17:54:25 +01:00 · 2020-12-21 17:54:25 +01:00 · 6f9deb5ae2
parent 00299f292d
commit 6f9deb5ae2
8 changed files with 162 additions and 45 deletions
--- a/lib/matrix_api.dart
+++ b/lib/matrix_api.dart
@ -20,6 +20,7 @@ library matrix_api;

 export 'matrix_api/matrix_api.dart';
 export 'matrix_api/utils/logs.dart';
+export 'matrix_api/utils/try_get_map_extension.dart';
 export 'matrix_api/model/algorithm_types.dart';
 export 'matrix_api/model/basic_event.dart';
 export 'matrix_api/model/basic_event_with_sender.dart';
--- a/lib/src/client.dart
+++ b/lib/src/client.dart
@ -1419,6 +1419,8 @@ sort order of ${prevState.sortOrder}. This should never happen...''');
                _userDeviceKeys[userId].deviceKeys[deviceId] =
                    oldKeys[deviceId];
              }
+            } else {
+              Logs().w('Invalid device ${entry.userId}:${entry.deviceId}');
            }
            if (database != null) {
              dbActions.add(() => database.storeUserDeviceKey(
--- a/lib/src/utils/device_keys_list.dart
+++ b/lib/src/utils/device_keys_list.dart
@ -164,8 +164,17 @@ abstract class SignableKey extends MatrixSignableKey {
    return String.fromCharCodes(canonicalJson.encode(data));
  }

-  bool _verifySignature(String pubKey, String signature) {
-    final olmutil = olm.Utility();
+  bool _verifySignature(String pubKey, String signature,
+      {bool isSignatureWithoutLibolmValid = false}) {
+    olm.Utility olmutil;
+    try {
+      olmutil = olm.Utility();
+    } on NoSuchMethodError {
+      // if no libolm is present we land in this catch block, and return the default
+      // set if no libolm is there. Some signatures should be assumed-valid while others
+      // should be assumed-invalid
+      return isSignatureWithoutLibolmValid;
+    }
    var valid = false;
    try {
      olmutil.ed25519_verify(pubKey, signingContent, signature);
@ -209,6 +218,10 @@ abstract class SignableKey extends MatrixSignableKey {
          continue;
        }
        final keyId = fullKeyId.substring('ed25519:'.length);
+        // we ignore self-signatures here
+        if (otherUserId == userId && keyId == identifier) {
+          continue;
+        }
        SignableKey key;
        if (client.userDeviceKeys[otherUserId].deviceKeys.containsKey(keyId)) {
          key = client.userDeviceKeys[otherUserId].deviceKeys[keyId];
@ -356,12 +369,26 @@ class DeviceKeys extends SignableKey {
  String get deviceDisplayName =>
      unsigned != null ? unsigned['device_display_name'] : null;

+  bool get selfSigned => signatures
+              ?.tryGet<Map<String, dynamic>>(userId)
+              ?.tryGet<String>('ed25519:$deviceId') ==
+          null
+      ? false
+      // without libolm we still want to be able to add devices. In that case we ofc just can't
+      // verify the signature
+      : _verifySignature(ed25519Key, signatures[userId]['ed25519:$deviceId'],
+          isSignatureWithoutLibolmValid: true);
+
+  @override
+  bool get blocked => super.blocked || !selfSigned;
+
  bool get isValid =>
      userId != null &&
      deviceId != null &&
      keys != null &&
      curve25519Key != null &&
-      ed25519Key != null;
+      ed25519Key != null &&
+      selfSigned;

  @override
  Future<void> setVerified(bool newVerified, [bool sign = true]) {
--- a/test/device_keys_list_test.dart
+++ b/test/device_keys_list_test.dart
@ -92,6 +92,42 @@ void main() {
      client = await getClient();
    });

+    test('reject devices without self-signature', () async {
+      var key = DeviceKeys.fromJson({
+        'user_id': '@test:fakeServer.notExisting',
+        'device_id': 'BADDEVICE',
+        'algorithms': [
+          AlgorithmTypes.olmV1Curve25519AesSha2,
+          AlgorithmTypes.megolmV1AesSha2
+        ],
+        'keys': {
+          'curve25519:BADDEVICE': 'ds6+bItpDiWyRaT/b0ofoz1R+GCy7YTbORLJI4dmYho',
+          'ed25519:BADDEVICE': 'CdDKVf44LO2QlfWopP6VWmqedSrRaf9rhHKvdVyH38w'
+        },
+      }, client);
+      expect(key.isValid, false);
+      expect(key.selfSigned, false);
+      key = DeviceKeys.fromJson({
+        'user_id': '@test:fakeServer.notExisting',
+        'device_id': 'BADDEVICE',
+        'algorithms': [
+          AlgorithmTypes.olmV1Curve25519AesSha2,
+          AlgorithmTypes.megolmV1AesSha2
+        ],
+        'keys': {
+          'curve25519:BADDEVICE': 'ds6+bItpDiWyRaT/b0ofoz1R+GCy7YTbORLJI4dmYho',
+          'ed25519:BADDEVICE': 'CdDKVf44LO2QlfWopP6VWmqedSrRaf9rhHKvdVyH38w'
+        },
+        'signatures': {
+          '@test:fakeServer.notExisting': {
+            'ed25519:BADDEVICE': 'invalid',
+          },
+        },
+      }, client);
+      expect(key.isValid, false);
+      expect(key.selfSigned, false);
+    });
+
    test('set blocked / verified', () async {
      final key =
          client.userDeviceKeys[client.userID].deviceKeys['OTHERDEVICE'];
@ -104,10 +140,17 @@ void main() {
          AlgorithmTypes.megolmV1AesSha2
        ],
        'keys': {
-          'curve25519:UNSIGNEDDEVICE': 'blah',
-          'ed25519:UNSIGNEDDEVICE': 'blah'
+          'curve25519:UNSIGNEDDEVICE':
+              'ds6+bItpDiWyRaT/b0ofoz1R+GCy7YTbORLJI4dmYho',
+          'ed25519:UNSIGNEDDEVICE':
+              'CdDKVf44LO2QlfWopP6VWmqedSrRaf9rhHKvdVyH38w'
+        },
+        'signatures': {
+          '@test:fakeServer.notExisting': {
+            'ed25519:UNSIGNEDDEVICE':
+                'f2p1kv6PIz+hnoFYnHEurhUKIyRsdxwR2RTKT1EnQ3aF2zlZOjmnndOCtIT24Q8vs2PovRw+/jkHKj4ge2yDDw',
+          },
        },
-        'signatures': <String, dynamic>{},
      }, client);
      final masterKey = client.userDeviceKeys[client.userID].masterKey;
      masterKey.setDirectVerified(true);
@ -185,7 +228,8 @@ void main() {
      user.deviceKeys['OTHERDEVICE'].setDirectVerified(false);

      user.masterKey.setDirectVerified(true);
-      user.deviceKeys['GHTYAJCE'].signatures.clear();
+      user.deviceKeys['GHTYAJCE'].signatures[client.userID]
+          .removeWhere((k, v) => k != 'ed25519:GHTYAJCE');
      expect(user.deviceKeys['GHTYAJCE'].verified,
          true); // it's our own device, should be direct verified
      expect(
--- a/test/encryption/key_manager_test.dart
+++ b/test/encryption/key_manager_test.dart
@ -170,10 +170,15 @@ void main() {
          AlgorithmTypes.megolmV1AesSha2
        ],
        'keys': {
-          'curve25519:JLAFKJWSCS':
-              '3C5BFWi2Y8MaVvjM8M22DBmh24PmgR0nPvJOIArzgyI',
-          'ed25519:JLAFKJWSCS': 'lEuiRJBit0IG6nUf5pUzWTUEsRVVe/HJkoKuEww9ULI'
+          'curve25519:NEWDEVICE': 'bnKQp6pPW0l9cGoIgHpBoK5OUi4h0gylJ7upc4asFV8',
+          'ed25519:NEWDEVICE': 'ZZhPdvWYg3MRpGy2MwtI+4MHXe74wPkBli5hiEOUi8Y'
        },
+        'signatures': {
+          '@alice:example.com': {
+            'ed25519:NEWDEVICE':
+                '94GSg8N9vNB8wyWHJtKaaX3MGNWPVOjBatJM+TijY6B1RlDFJT5Cl1h/tjr17AoQz0CDdOf6uFhrYsBkH1/ABg'
+          }
+        }
      }, client);
      await client.encryption.keyManager.clearOrUseOutboundGroupSession(roomId);
      expect(
--- a/test/encryption/key_request_test.dart
+++ b/test/encryption/key_request_test.dart
@ -302,7 +302,7 @@ void main() {
            'forwarding_curve25519_key_chain': [],
          },
          encryptedContent: {
-            'sender_key': '3C5BFWi2Y8MaVvjM8M22DBmh24PmgR0nPvJOIArzgyI',
+            'sender_key': 'L+4+JCl8MD63dgo8z5Ta+9QAHXiANyOVSfgbHA5d3H8',
          });
      await matrix.encryption.keyManager.handleToDeviceEvent(event);
      expect(
@ -327,7 +327,7 @@ void main() {
            'forwarding_curve25519_key_chain': [],
          },
          encryptedContent: {
-            'sender_key': '3C5BFWi2Y8MaVvjM8M22DBmh24PmgR0nPvJOIArzgyI',
+            'sender_key': 'L+4+JCl8MD63dgo8z5Ta+9QAHXiANyOVSfgbHA5d3H8',
          });
      await matrix.encryption.keyManager.handleToDeviceEvent(event);
      expect(
--- a/test/fake_matrix_api.dart
+++ b/test/fake_matrix_api.dart
@ -26,6 +26,16 @@ import 'package:famedlysdk/matrix_api.dart';
 import 'package:http/http.dart';
 import 'package:http/testing.dart';

+Map<String, dynamic> decodeJson(dynamic data) {
+  if (data is String) {
+    return json.decode(data);
+  }
+  if (data.isEmpty) {
+    return <String, dynamic>{};
+  }
+  return data;
+}
+
 class FakeMatrixApi extends MockClient {
  static final calledEndpoints = <String, List<dynamic>>{};
  static int eventCounter = 0;
@ -98,7 +108,7 @@ class FakeMatrixApi extends MockClient {
            final syncUpdate = sdk.SyncUpdate()
              ..accountData = [
                sdk.BasicEvent()
-                  ..content = json.decode(data)
+                  ..content = decodeJson(data)
                  ..type = type
              ];
            if (client.database != null) {
@ -1743,32 +1753,37 @@ class FakeMatrixApi extends MockClient {
      '/client/r0/keys/claim': (var req) => {
            'failures': {},
            'one_time_keys': {
-              '@alice:example.com': {
-                'JLAFKJWSCS': {
-                  'signed_curve25519:AAAAHg': {
-                    'key': 'zKbLg+NrIjpnagy+pIY6uPL4ZwEG2v+8F9lmgsnlZzs',
-                    'signatures': {
-                      '@alice:example.com': {
-                        'ed25519:JLAFKJWSCS':
-                            'FLWxXqGbwrb8SM3Y795eB6OA8bwBcoMZFXBqnTn58AYWZSqiD45tlBVcDa2L7RwdKXebW/VzDlnfVJ+9jok1Bw'
+              if (decodeJson(req)['one_time_keys']['@alice:example.com'] !=
+                  null)
+                '@alice:example.com': {
+                  'JLAFKJWSCS': {
+                    'signed_curve25519:AAAAAQ': {
+                      'key': 'ikMXajRlkS7Xi9CROrAh3jXnbygk8mLBdSaY9/al0X0',
+                      'signatures': {
+                        '@alice:example.com': {
+                          'ed25519:JLAFKJWSCS':
+                              'XdboCa0Ljoh0Y0i/IVnmMqy/+T1hJyu8BA/nRYniJMQ7QWh/pGS5AsWswdARD+MAX+r4u98Qzk0y27HUddZXDA'
+                        }
                      }
                    }
                  }
-                }
-              },
-              '@test:fakeServer.notExisting': {
-                'GHTYAJCE': {
-                  'signed_curve25519:AAAAAQ': {
-                    'key': 'qc72ve94cA28iuE0fXa98QO3uls39DHWdQlYyvvhGh0',
-                    'signatures': {
-                      '@test:fakeServer.notExisting': {
-                        'ed25519:GHTYAJCE':
-                            'dFwffr5kTKefO7sjnWLMhTzw7oV31nkPIDRxFy5OQT2OP5++Ao0KRbaBZ6qfuT7lW1owKK0Xk3s7QTBvc/eNDA',
+                },
+              if (decodeJson(req)['one_time_keys']
+                      ['@test:fakeServer.notExisting'] !=
+                  null)
+                '@test:fakeServer.notExisting': {
+                  'GHTYAJCE': {
+                    'signed_curve25519:AAAAAQ': {
+                      'key': 'qc72ve94cA28iuE0fXa98QO3uls39DHWdQlYyvvhGh0',
+                      'signatures': {
+                        '@test:fakeServer.notExisting': {
+                          'ed25519:GHTYAJCE':
+                              'dFwffr5kTKefO7sjnWLMhTzw7oV31nkPIDRxFy5OQT2OP5++Ao0KRbaBZ6qfuT7lW1owKK0Xk3s7QTBvc/eNDA',
+                        },
                      },
                    },
                  },
                },
-              },
            }
          },
      '/client/r0/rooms/!localpart%3Aexample.com/invite': (var req) => {},
@ -1786,7 +1801,7 @@ class FakeMatrixApi extends MockClient {
            'one_time_key_counts': {
              'curve25519': 10,
              'signed_curve25519':
-                  json.decode(req)['one_time_keys']?.keys?.length ?? 0,
+                  decodeJson(req)['one_time_keys']?.keys?.length ?? 0,
            }
          },
      '/client/r0/keys/query': (var req) => {
@ -1802,14 +1817,14 @@ class FakeMatrixApi extends MockClient {
                  ],
                  'keys': {
                    'curve25519:JLAFKJWSCS':
-                        '3C5BFWi2Y8MaVvjM8M22DBmh24PmgR0nPvJOIArzgyI',
+                        'L+4+JCl8MD63dgo8z5Ta+9QAHXiANyOVSfgbHA5d3H8',
                    'ed25519:JLAFKJWSCS':
-                        'lEuiRJBit0IG6nUf5pUzWTUEsRVVe/HJkoKuEww9ULI'
+                        'rUFJftIWpFF/jqqz3bexGGYiG8UobKhzkeabqw1v0zM'
                  },
                  'signatures': {
                    '@alice:example.com': {
                      'ed25519:JLAFKJWSCS':
-                          'dSO80A01XiigH3uBiDVx/EjzaoycHcjq9lfQX0uWsqxl2giMIiSPR8a4d291W1ihKJL/a+myXS367WT6NAIcBA'
+                          'go3mi5o3Ile+Ik+lCEpHmBmyJmKWfnRDCBBvfaVlKsMyha5IORuYcxwEUrAeLyAeeeHvkWDFX+No5eY1jYeKBw'
                    }
                  },
                  'unsigned': {'device_display_name': 'Alices mobile phone'}
@ -1822,10 +1837,17 @@ class FakeMatrixApi extends MockClient {
                    AlgorithmTypes.megolmV1AesSha2
                  ],
                  'keys': {
-                    'curve25519:OTHERDEVICE': 'blah',
-                    'ed25519:OTHERDEVICE': 'blah'
+                    'curve25519:OTHERDEVICE':
+                        'wMIDhiQl5jEXQrTB03ePOSQfR8sA/KMrW0CIfFfXKEE',
+                    'ed25519:OTHERDEVICE':
+                        '2Lyaj5NB7HPqKZMjZpA/pECXuQ+9wi8AGFdw33y3DuQ'
+                  },
+                  'signatures': {
+                    '@alice:example.com': {
+                      'ed25519:OTHERDEVICE':
+                          'bwHd6ylISP13AICdDPd0HQd4V6dvvd4vno8/OwUNdm9UAprr3YjkDqVw425I74u2UQAarq9bytBqVqFyD6trAw',
+                    }
                  },
-                  'signatures': {},
                },
              },
              '@test:fakeServer.notExisting': {
@ -1844,6 +1866,8 @@ class FakeMatrixApi extends MockClient {
                  },
                  'signatures': {
                    '@test:fakeServer.notExisting': {
+                      'ed25519:GHTYAJCE':
+                          'NEQeTgv7ew1IZSLQphWd0y60EdHdcNfHgvoaMQco5XKeIYyiUZIWd7F4x/mkPDjUizv6yWMbTDCWdSg5XcgNBA',
                      'ed25519:F9ypFzgbISXCzxQhhSnXMkc1vq12Luna3Nw5rqViOJY':
                          'Q4/55vZjEJD7M2EC40bgZqd9Zuy/4C75UPVopJdXeioQVaKtFf6EF0nUUuql0yD+r3hinsZcock0wO6Q2xcoAQ',
                    },
@ -1857,13 +1881,17 @@ class FakeMatrixApi extends MockClient {
                    AlgorithmTypes.megolmV1AesSha2
                  ],
                  'keys': {
-                    'curve25519:OTHERDEVICE': 'blah',
-                    'ed25519:OTHERDEVICE': 'blah'
+                    'curve25519:OTHERDEVICE':
+                        'R96BA0qE1+QAWLp7E1jyWSTJ1VXMLpEdiM2SZHlKMXM',
+                    'ed25519:OTHERDEVICE':
+                        'EQo9eYbSygIbOR+tVJziqAY1NI6Gga+JQOVIqJe4mr4'
                  },
                  'signatures': {
                    '@test:fakeServer.notExisting': {
+                      'ed25519:OTHERDEVICE':
+                          '/rT6pVRypJWxGos1QcI7jHL9HwcA83nkHLHqMcRPeLSxXHh4oHWvC0/tl0Xg06ogyiGw4NuB7TpOISvJBdt7BA',
                      'ed25519:F9ypFzgbISXCzxQhhSnXMkc1vq12Luna3Nw5rqViOJY':
-                          'o7ucKPWrF2VKx7wYqP1f+aw4QohLMz7kX+SIw6aWCYsLC3XyIlg8rX/7QQ9B8figCVnRK7IjtjWvQodBCfWCAA',
+                          'qnjiLl36h/1jlLvcAgt46Igaod2T9lOSnoSVkV0KC+c7vYIjG4QBzXpH+hycfufOT/y+a/kl52dUTLQWctMKCA',
                    },
                  },
                },
@ -1882,7 +1910,12 @@ class FakeMatrixApi extends MockClient {
                    'ed25519:FOXDEVICE':
                        'R5/p04tticvdlNIxiiBIP0j9OQWv8ep6eEU6/lWKDxw',
                  },
-                  'signatures': {},
+                  'signatures': {
+                    '@othertest:fakeServer.notExisting': {
+                      'ed25519:FOXDEVICE':
+                          '2lJ3atmRIWgkyQNC9gvWEpxwuozsBQsg33M2IMDJqLhx/+g3Ds1vQ683dJsYIu04ORa4U0L9TqieHVpV/7qqDA',
+                    },
+                  },
                },
              },
            },
@ -2002,7 +2035,7 @@ class FakeMatrixApi extends MockClient {
      '/client/r0/rooms/!localpart%3Aserver.abc/invite': (var reqI) => {},
      '/client/unstable/keys/device_signing/upload': (var reqI) {
        if (client != null) {
-          final jsonBody = json.decode(reqI);
+          final jsonBody = decodeJson(reqI);
          for (final keyType in {
            'master_key',
            'self_signing_key',
--- a/test/matrix_api_test.dart
+++ b/test/matrix_api_test.dart
@ -1113,7 +1113,12 @@ void main() {
        },
        timeout: 10,
      );
-      expect(FakeMatrixApi.api['POST']['/client/r0/keys/claim']({}),
+      expect(
+          FakeMatrixApi.api['POST']['/client/r0/keys/claim']({
+            'one_time_keys': {
+              '@alice:example.com': {'JLAFKJWSCS': 'signed_curve25519'}
+            }
+          }),
          response.toJson());

      matrixApi.homeserver = matrixApi.accessToken = null;