From 99ecac479a4ea8b679c8b4fe5914b297b30f4579 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 5 Feb 2020 10:43:26 +0000 Subject: [PATCH 1/4] Check sender_key matches on inbound encrypted events. If they don't then the device lists are probably out of sync. --- synapse/handlers/federation.py | 60 ++++++++++++++++++++++++++-------- 1 file changed, 47 insertions(+), 13 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 488200a2d1e3..7049971fd576 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -754,27 +754,61 @@ async def _process_received_pdu( # if we don't then we mark the device cache for that user as stale. if event.type == EventTypes.Encrypted: device_id = event.content.get("device_id") + sender_key = event.content.get("sender_key") + + cached_devices = await self.store.get_cached_devices_for_user(event.sender) + + resync = False if device_id is not None: - cached_devices = await self.store.get_cached_devices_for_user( - event.sender - ) - if device_id not in cached_devices: + device = cached_devices.get(device_id) + if device is None: logger.info( "Received event from remote device not in our cache: %s %s", event.sender, device_id, ) - await self.store.mark_remote_user_device_cache_as_stale( - event.sender + resync = True + + # We also check if the `sender_key` matches what we expect. + if sender_key is not None: + # We pull out the expected key from the list of devices for + # logging purposes. Note that the key name depends on the + # algorithm. + if device_id: + if event.content.get("algorithm") == "m.megolm.v1.aes-sha2": + key_name = "curve25519:%s" % (device_id,) + keys = device.get("keys", {}).get("keys", {}) + current_key = keys.get(key_name) + else: + current_key = "" + else: + current_key = "" + + # We check that one of the keys matches. + if sender_key not in ( + key + for device in cached_devices + for key in device.get("keys", {}).get("keys", {}).values() + ): + logger.info( + "Received event from remote device with different sender key: %s %s %s (current: %s)", + event.sender, + device_id, + sender_key, + current_key, ) + resync = True - # Immediately attempt a resync in the background - if self.config.worker_app: - return run_in_background(self._user_device_resync, event.sender) - else: - return run_in_background( - self._device_list_updater.user_device_resync, event.sender - ) + if resync: + await self.store.mark_remote_user_device_cache_as_stale(event.sender) + + # Immediately attempt a resync in the background + if self.config.worker_app: + return run_in_background(self._user_device_resync, event.sender) + else: + return run_in_background( + self._device_list_updater.user_device_resync, event.sender + ) @log_function async def backfill(self, dest, room_id, limit, extremities): From b0d06cac36b45eee289ff653828d69ddba0bbd7a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 5 Feb 2020 10:46:27 +0000 Subject: [PATCH 2/4] Add info logging when we receive device update --- synapse/handlers/device.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/device.py b/synapse/handlers/device.py index 26ef5e150c92..a9bd431486a7 100644 --- a/synapse/handlers/device.py +++ b/synapse/handlers/device.py @@ -598,7 +598,13 @@ def _handle_device_updates(self, user_id): # happens if we've missed updates. resync = yield self._need_to_do_resync(user_id, pending_updates) - logger.debug("Need to re-sync devices for %r? %r", user_id, resync) + if logger.isEnabledFor(logging.INFO): + logger.info( + "Received device list update for %s, requiring resync: %s. Devices: %s", + user_id, + resync, + ", ".join(u[0] for u in pending_updates), + ) if resync: yield self.user_device_resync(user_id) From 904e3fc3fceda926fb229602bfc3c83e10111158 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 5 Feb 2020 10:51:26 +0000 Subject: [PATCH 3/4] Newsfile --- changelog.d/6850.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/6850.misc diff --git a/changelog.d/6850.misc b/changelog.d/6850.misc new file mode 100644 index 000000000000..418569113f35 --- /dev/null +++ b/changelog.d/6850.misc @@ -0,0 +1 @@ +Detect unexpected sender keys on inbound encrypted events and resync device lists. From dd3a6b7c9e292c3173416e8e45a1c2133446f55c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 5 Feb 2020 12:23:49 +0000 Subject: [PATCH 4/4] Fixup and expand comments --- synapse/handlers/federation.py | 50 +++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 7049971fd576..e9441bbeff45 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -758,7 +758,9 @@ async def _process_received_pdu( cached_devices = await self.store.get_cached_devices_for_user(event.sender) - resync = False + resync = False # Whether we should resync device lists. + + device = None if device_id is not None: device = cached_devices.get(device_id) if device is None: @@ -771,31 +773,41 @@ async def _process_received_pdu( # We also check if the `sender_key` matches what we expect. if sender_key is not None: - # We pull out the expected key from the list of devices for - # logging purposes. Note that the key name depends on the - # algorithm. - if device_id: + # Figure out what sender key we're expecting. If we know the + # device and recognize the algorithm then we can work out the + # exact key to expect. Otherwise check it matches any key we + # have for that device. + if device: + keys = device.get("keys", {}).get("keys", {}) + if event.content.get("algorithm") == "m.megolm.v1.aes-sha2": + # For this algorithm we expect a curve25519 key. key_name = "curve25519:%s" % (device_id,) - keys = device.get("keys", {}).get("keys", {}) - current_key = keys.get(key_name) + current_keys = [keys.get(key_name)] else: - current_key = "" + # We don't know understand the algorithm, so we just + # check it matches a key for the device. + current_keys = keys.values() + elif device_id: + # We don't have any keys for the device ID. + current_keys = [] else: - current_key = "" - - # We check that one of the keys matches. - if sender_key not in ( - key - for device in cached_devices - for key in device.get("keys", {}).get("keys", {}).values() - ): + # The event didn't include a device ID, so we just look for + # keys across all devices. + current_keys = ( + key + for device in cached_devices + for key in device.get("keys", {}).get("keys", {}).values() + ) + + # We now check that the sender key matches (one of) the expected + # keys. + if sender_key not in current_keys: logger.info( - "Received event from remote device with different sender key: %s %s %s (current: %s)", + "Received event from remote device with unexpected sender key: %s %s: %s", event.sender, - device_id, + device_id or "", sender_key, - current_key, ) resync = True