From d37c28374d9cdc2ed110342ad549c4b9ab2743e7 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 19 Oct 2021 11:39:17 +0100 Subject: [PATCH 1/6] Hoist `_check_for_soft_fail` and `_maybe_kick_guest_users` checks these two tests re only tenously related to event auth, and aren't required everywhere we do event auth, so let's hoist them out of `_check_event_auth`. --- synapse/handlers/federation_event.py | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 5a2f2e5ebb77..637cbd2d5e03 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -362,6 +362,7 @@ async def on_send_membership_event( # need to. await self._event_creation_handler.cache_joined_hosts_for_event(event, context) + await self._check_for_soft_fail(event, None, origin=origin) await self._run_push_actions_and_persist_event(event, context) return event, context @@ -1013,11 +1014,19 @@ async def _process_received_pdu( logger.exception("Unexpected AuthError from _check_event_auth") raise FederationError("ERROR", e.code, e.msg, affected=event.event_id) + if not backfilled and not context.rejected: + # For new (non-backfilled and non-outlier) events we check if the event + # passes auth based on the current state. If it doesn't then we + # "soft-fail" the event. + await self._check_for_soft_fail(event, state, origin=origin) + await self._run_push_actions_and_persist_event(event, context, backfilled) - if backfilled: + if backfilled or context.rejected: return + await self._maybe_kick_guest_users(event) + # For encrypted messages we check that we know about the sending device, # if we don't then we mark the device cache for that user as stale. if event.type == EventTypes.Encrypted: @@ -1445,10 +1454,6 @@ async def _check_event_auth( except AuthError as e: logger.warning("Failed auth resolution for %r because %s", event, e) context.rejected = RejectedReason.AUTH_ERROR - return context - - await self._check_for_soft_fail(event, state, backfilled, origin=origin) - await self._maybe_kick_guest_users(event) return context @@ -1468,7 +1473,6 @@ async def _check_for_soft_fail( self, event: EventBase, state: Optional[Iterable[EventBase]], - backfilled: bool, origin: str, ) -> None: """Checks if we should soft fail the event; if so, marks the event as @@ -1477,15 +1481,8 @@ async def _check_for_soft_fail( Args: event state: The state at the event if we don't have all the event's prev events - backfilled: Whether the event is from backfill origin: The host the event originates from. """ - # For new (non-backfilled and non-outlier) events we check if the event - # passes auth based on the current state. If it doesn't then we - # "soft-fail" the event. - if backfilled or event.internal_metadata.is_outlier(): - return - extrem_ids_list = await self._store.get_latest_event_ids_in_room(event.room_id) extrem_ids = set(extrem_ids_list) prev_event_ids = set(event.prev_event_ids()) From 44a076299667e4f7027fe9358c7031c8b201a501 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 6 Oct 2021 23:00:09 +0100 Subject: [PATCH 2/6] Stop papering over broken send_join responses --- synapse/handlers/federation_event.py | 30 +++++++++++----------------- 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 637cbd2d5e03..fbbcabca7010 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -408,16 +408,20 @@ async def process_remote_join( Persists the event separately. Notifies about the persisted events where appropriate. - Will attempt to fetch missing auth events. - Args: origin: Where the events came from - room_id, + room_id: auth_events state event room_version: The room version we expect this room to have, and will raise if it doesn't match the version in the create event. + + Returns: + The stream ID after which all events have been persisted. + + Raises: + SynapseError if the response is in some way invalid. """ events_to_context = {} for e in itertools.chain(auth_events, state): @@ -446,24 +450,14 @@ async def process_remote_join( if room_version.identifier != room_version_id: raise SynapseError(400, "Room version mismatch") - missing_auth_events = set() + # we should have all of the auth events in the chain. for e in itertools.chain(auth_events, state, [event]): for e_id in e.auth_event_ids(): if e_id not in event_map: - missing_auth_events.add(e_id) - - for e_id in missing_auth_events: - m_ev = await self._federation_client.get_pdu( - [origin], - e_id, - room_version=room_version, - outlier=True, - timeout=10000, - ) - if m_ev and m_ev.event_id == e_id: - event_map[e_id] = m_ev - else: - logger.info("Failed to find auth event %r", e_id) + logger.info( + "Auth chain incomplete: %s refers to missing event %s", e, e_id + ) + raise SynapseError(400, "Auth chain incomplete") for e in itertools.chain(auth_events, state, [event]): auth_for_e = [ From 3701fee7e9b30cd6732fb3920408248835a2b6a2 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 7 Oct 2021 01:09:56 +0100 Subject: [PATCH 3/6] process_remote_join: use auth_and_persist_outliers Fixes a bug where we would accept events whose auth_events were rejected. --- synapse/handlers/federation_event.py | 50 +++++++--------------------- 1 file changed, 12 insertions(+), 38 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index fbbcabca7010..29ea0f5055f9 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -404,9 +404,8 @@ async def process_remote_join( """Persists the events returned by a send_join Checks the auth chain is valid (and passes auth checks) for the - state and event. Then persists the auth chain and state atomically. - Persists the event separately. Notifies about the persisted events - where appropriate. + state and event. Then persists all of the events. + Notifies about the persisted events where appropriate. Args: origin: Where the events came from @@ -423,14 +422,10 @@ async def process_remote_join( Raises: SynapseError if the response is in some way invalid. """ - events_to_context = {} for e in itertools.chain(auth_events, state): e.internal_metadata.outlier = True - events_to_context[e.event_id] = EventContext.for_outlier() - event_map = { - e.event_id: e for e in itertools.chain(auth_events, state, [event]) - } + event_map = {e.event_id: e for e in itertools.chain(auth_events, state)} create_event = None for e in auth_events: @@ -459,38 +454,17 @@ async def process_remote_join( ) raise SynapseError(400, "Auth chain incomplete") - for e in itertools.chain(auth_events, state, [event]): - auth_for_e = [ - event_map[e_id] for e_id in e.auth_event_ids() if e_id in event_map - ] - if create_event: - auth_for_e.append(create_event) + # filter out any events we have already seen + seen_remotes = await self._store.have_seen_events(room_id, event_map.keys()) + for s in seen_remotes: + event_map.pop(s, None) - try: - validate_event_for_room_version(room_version, e) - check_auth_rules_for_event(room_version, e, auth_for_e) - except SynapseError as err: - # we may get SynapseErrors here as well as AuthErrors. For - # instance, there are a couple of (ancient) events in some - # rooms whose senders do not have the correct sigil; these - # cause SynapseErrors in auth.check. We don't want to give up - # the attempt to federate altogether in such cases. - - logger.warning("Rejecting %s because %s", e.event_id, err.msg) - - if e == event: - raise - events_to_context[e.event_id].rejected = RejectedReason.AUTH_ERROR - - if auth_events or state: - await self.persist_events_and_notify( - room_id, - [ - (e, events_to_context[e.event_id]) - for e in itertools.chain(auth_events, state) - ], - ) + # persist the auth chain and state events. + # any invalid events here will be marked as rejected, and we'll carry on. + await self._auth_and_persist_outliers(room_id, event_map.values()) + # and now persist the join event itself. + logger.info("Peristing join-via-remote %s", event) new_event_context = await self._state_handler.compute_event_context( event, old_state=state ) From 50f58a028920e6c6a5d18623ba6ed6824950a560 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 7 Oct 2021 01:18:25 +0100 Subject: [PATCH 4/6] Also auth the join event itself --- synapse/handlers/federation_event.py | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 29ea0f5055f9..f4af512268fe 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -465,13 +465,16 @@ async def process_remote_join( # and now persist the join event itself. logger.info("Peristing join-via-remote %s", event) - new_event_context = await self._state_handler.compute_event_context( - event, old_state=state - ) + with nested_logging_context(suffix=event.event_id): + context = await self._state_handler.compute_event_context( + event, old_state=state + ) - return await self.persist_events_and_notify( - room_id, [(event, new_event_context)] - ) + context = await self._check_event_auth(origin, event, context) + if context.rejected: + raise SynapseError(400, "Join event was rejected") + + return await self.persist_events_and_notify(room_id, [(event, context)]) @log_function async def backfill( @@ -944,9 +947,15 @@ async def _process_received_pdu( ) -> None: """Called when we have a new non-outlier event. - This is called when we have a new event to add to the room DAG - either directly - via a /send request, retrieved via get_missing_events after a /send request, or - backfilled after a client request. + This is called when we have a new event to add to the room DAG. This can be + due to: + * events received directly via a /send request + * events retrieved via get_missing_events after a /send request + * events backfilled after a client request. + + It's not currently used for events received from incoming send_{join,knock,leave} + requests (which go via on_send_membership_event), nor for joins created by a + remote join dance (which go via process_remote_join). We need to do auth checks and put it through the StateHandler. From 32e923714064ad148418f26cdf33003a1feeed36 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 7 Oct 2021 01:30:26 +0100 Subject: [PATCH 5/6] changelog --- changelog.d/11012.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11012.bugfix diff --git a/changelog.d/11012.bugfix b/changelog.d/11012.bugfix new file mode 100644 index 000000000000..13b8e5983b73 --- /dev/null +++ b/changelog.d/11012.bugfix @@ -0,0 +1 @@ +Fix a long-standing bug which meant that events received over federation were sometimes incorrectly accepted into the room state. From 8c751f0adb01a737cfe9906c78ed7682a1b8dcf3 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 22 Oct 2021 11:10:43 +0100 Subject: [PATCH 6/6] Drop events with missing auth events As with populating state at a backwards extremity, we should drop events whose auth events cannot be found, rather than rejecting the whole response. --- synapse/handlers/federation_event.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index f4af512268fe..a7d935439fa4 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -445,22 +445,22 @@ async def process_remote_join( if room_version.identifier != room_version_id: raise SynapseError(400, "Room version mismatch") - # we should have all of the auth events in the chain. - for e in itertools.chain(auth_events, state, [event]): - for e_id in e.auth_event_ids(): - if e_id not in event_map: - logger.info( - "Auth chain incomplete: %s refers to missing event %s", e, e_id - ) - raise SynapseError(400, "Auth chain incomplete") - # filter out any events we have already seen seen_remotes = await self._store.have_seen_events(room_id, event_map.keys()) for s in seen_remotes: event_map.pop(s, None) # persist the auth chain and state events. + # # any invalid events here will be marked as rejected, and we'll carry on. + # + # any events whose auth events are missing (ie, not in the send_join response, + # and not already in our db) will just be ignored. This is correct behaviour, + # because the reason that auth_events are missing might be due to us being + # unable to validate their signatures. The fact that we can't validate their + # signatures right now doesn't mean that we will *never* be able to, so it + # is premature to reject them. + # await self._auth_and_persist_outliers(room_id, event_map.values()) # and now persist the join event itself. @@ -1304,14 +1304,14 @@ def prep(event: EventBase) -> Optional[Tuple[EventBase, EventContext]]: for auth_event_id in event.auth_event_ids(): ae = persisted_events.get(auth_event_id) if not ae: + # the fact we can't find the auth event doesn't mean it doesn't + # exist, which means it is premature to reject `event`. Instead we + # just ignore it for now. logger.warning( - "Event %s relies on auth_event %s, which could not be found.", + "Dropping event %s, which relies on auth_event %s, which could not be found", event, auth_event_id, ) - # the fact we can't find the auth event doesn't mean it doesn't - # exist, which means it is premature to reject `event`. Instead we - # just ignore it for now. return None auth.append(ae)