From c02b3ee984d65a3a1126cfc621ab59058ff548c8 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 18 Sep 2020 11:07:05 +0100 Subject: [PATCH 1/4] Use _check_sigs_and_hash_and_fetch to validate backfill requests This is a bit of a hack, as `_check_sigs_and_hash_and_fetch` is intended for attempting to pull an event from the database/(re)pull it from the server that originally sent the event if checking the signature of the event fails. During backfill we *know* that we won't have the event in our database, however it is still useful to be able to query the original sending server as the server we're backfilling from may be acting maliciously. The main benefit and reason for this change however is that `_check_sigs_and_hash_and_fetch` will drop an event during backfill if it cannot be successfully validated, whereas the current code will simply fail the backfill request - resulting in the client's /messages request silently being dropped. This is a quick patch to fix backfilling rooms that contain malformed events. A better implementation in planned in future. --- synapse/federation/federation_client.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index a2e8d96ea27c..d42930d1b94b 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -217,11 +217,9 @@ async def backfill( for p in transaction_data["pdus"] ] - # FIXME: We should handle signature failures more gracefully. - pdus[:] = await make_deferred_yieldable( - defer.gatherResults( - self._check_sigs_and_hashes(room_version, pdus), consumeErrors=True, - ).addErrback(unwrapFirstError) + # Check signatures and hash of pdus, removing any from the list that fail checks + pdus[:] = await self._check_sigs_and_hash_and_fetch( + dest, pdus, outlier=True, room_version=room_version ) return pdus From e5d52507506dc9461291b35b7f9acbdc5b04ad9d Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 18 Sep 2020 12:35:58 +0100 Subject: [PATCH 2/4] changelog --- changelog.d/8350.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/8350.bugfix diff --git a/changelog.d/8350.bugfix b/changelog.d/8350.bugfix new file mode 100644 index 000000000000..0e493c028214 --- /dev/null +++ b/changelog.d/8350.bugfix @@ -0,0 +1 @@ +Partially mitigate bug where newly joined servers couldn't get past events in a room when there is a malformed event. \ No newline at end of file From 8552287a792efcfdf44882731d55375a76f69765 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 18 Sep 2020 13:35:20 +0100 Subject: [PATCH 3/4] Don't check the local database when backfilling events --- synapse/federation/federation_client.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index d42930d1b94b..8d33436c0135 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -219,7 +219,7 @@ async def backfill( # Check signatures and hash of pdus, removing any from the list that fail checks pdus[:] = await self._check_sigs_and_hash_and_fetch( - dest, pdus, outlier=True, room_version=room_version + dest, pdus, outlier=True, room_version=room_version, check_db=False ) return pdus @@ -348,6 +348,7 @@ async def _check_sigs_and_hash_and_fetch( room_version: RoomVersion, outlier: bool = False, include_none: bool = False, + check_db: bool = True, ) -> List[EventBase]: """Takes a list of PDUs and checks the signatures and hashes of each one. If a PDU fails its signature check then we check if we have it in From 04de323e01bca46b840700cf49885b44d5e12c7c Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 18 Sep 2020 14:36:38 +0100 Subject: [PATCH 4/4] Revert "Don't check the local database when backfilling events" This reverts commit 8552287a792efcfdf44882731d55375a76f69765. --- synapse/federation/federation_client.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 8d33436c0135..d42930d1b94b 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -219,7 +219,7 @@ async def backfill( # Check signatures and hash of pdus, removing any from the list that fail checks pdus[:] = await self._check_sigs_and_hash_and_fetch( - dest, pdus, outlier=True, room_version=room_version, check_db=False + dest, pdus, outlier=True, room_version=room_version ) return pdus @@ -348,7 +348,6 @@ async def _check_sigs_and_hash_and_fetch( room_version: RoomVersion, outlier: bool = False, include_none: bool = False, - check_db: bool = True, ) -> List[EventBase]: """Takes a list of PDUs and checks the signatures and hashes of each one. If a PDU fails its signature check then we check if we have it in