From 9b7794262fa1a061f6668a6693a79b9bf8d3567a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 17 Apr 2018 22:11:19 +0100 Subject: Reject events which have too many auth_events or prev_events ... this should protect us from being dossed by people making silly events (deliberately or otherwise) --- synapse/handlers/federation.py | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) (limited to 'synapse') diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 080aca3d71..b7b0816449 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -15,8 +15,14 @@ # limitations under the License. """Contains handlers for federation events.""" + +import httplib +import itertools +import logging + from signedjson.key import decode_verify_key_bytes from signedjson.sign import verify_signed_json +from twisted.internet import defer from unpaddedbase64 import decode_base64 from ._base import BaseHandler @@ -43,10 +49,6 @@ from synapse.util.retryutils import NotRetryingDestination from synapse.util.distributor import user_joined_room -from twisted.internet import defer - -import itertools -import logging logger = logging.getLogger(__name__) @@ -115,6 +117,28 @@ class FederationHandler(BaseHandler): logger.debug("Already seen pdu %s", pdu.event_id) return + # do some initial sanity-checking of the event. In particular, make + # sure it doesn't have hundreds of prev_events or auth_events, which + # could cause a huge state resolution or cascade of event fetches + if len(pdu.prev_events) > 20: + logger.warn("Rejecting event %s which has %i prev_events", + pdu.event_id, len(pdu.prev_events)) + raise FederationError( + "ERROR", + httplib.BAD_REQUEST, + "Too many prev_events", + affected=pdu.event_id, + ) + if len(pdu.auth_events) > 10: + logger.warn("Rejecting event %s which has %i auth_events", + pdu.event_id, len(pdu.auth_events)) + raise FederationError( + "ERROR", + httplib.BAD_REQUEST, + "Too many auth_events", + affected=pdu.event_id, + ) + # If we are currently in the process of joining this room, then we # queue up events for later processing. if pdu.room_id in self.room_queues: -- cgit 1.4.1 From e585228860fe72ca8f2b95d49394a39f6329bf39 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 17 Apr 2018 23:41:12 +0100 Subject: Check events on backfill too --- synapse/handlers/federation.py | 57 +++++++++++++++++++++++++++++++----------- 1 file changed, 42 insertions(+), 15 deletions(-) (limited to 'synapse') diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index b7b0816449..310db3fb46 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -119,23 +119,14 @@ class FederationHandler(BaseHandler): # do some initial sanity-checking of the event. In particular, make # sure it doesn't have hundreds of prev_events or auth_events, which - # could cause a huge state resolution or cascade of event fetches - if len(pdu.prev_events) > 20: - logger.warn("Rejecting event %s which has %i prev_events", - pdu.event_id, len(pdu.prev_events)) + # could cause a huge state resolution or cascade of event fetches. + try: + self._sanity_check_event(pdu) + except SynapseError as err: raise FederationError( "ERROR", - httplib.BAD_REQUEST, - "Too many prev_events", - affected=pdu.event_id, - ) - if len(pdu.auth_events) > 10: - logger.warn("Rejecting event %s which has %i auth_events", - pdu.event_id, len(pdu.auth_events)) - raise FederationError( - "ERROR", - httplib.BAD_REQUEST, - "Too many auth_events", + err.code, + err.msg, affected=pdu.event_id, ) @@ -565,6 +556,9 @@ class FederationHandler(BaseHandler): extremities=extremities, ) + for ev in events: + self._sanity_check_event(ev) + # Don't bother processing events we already have. seen_events = yield self.store.have_events_in_timeline( set(e.event_id for e in events) @@ -867,6 +861,39 @@ class FederationHandler(BaseHandler): defer.returnValue(False) + def _sanity_check_event(self, ev): + """ + Do some early sanity checks of a received event + + In particular, checks it doesn't have an excessive number of + prev_events or auth_events, which could cause a huge state resolution + or cascade of event fetches. + + Args: + ev (synapse.events.EventBase): event to be checked + + Returns: None + + Raises: + SynapseError if the event does not pass muster + """ + if len(ev.prev_events) > 20: + logger.warn("Rejecting event %s which has %i prev_events", + ev.event_id, len(ev.prev_events)) + raise SynapseError( + httplib.BAD_REQUEST, + "Too many prev_events", + ) + + if len(ev.auth_events) > 10: + logger.warn("Rejecting event %s which has %i auth_events", + ev.event_id, len(ev.auth_events)) + raise SynapseError( + "ERROR", + httplib.BAD_REQUEST, + "Too many auth_events", + ) + @defer.inlineCallbacks def send_invite(self, target_host, event): """ Sends the invite to the remote server for signing. -- cgit 1.4.1 From 1f4b498b7313b9bff5bfc3eeeed25c7659276e82 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 17 Apr 2018 23:41:36 +0100 Subject: Add some comments --- synapse/federation/federation_server.py | 25 +++++++++++++++++++++++-- synapse/handlers/federation.py | 15 ++++++++++++--- 2 files changed, 35 insertions(+), 5 deletions(-) (limited to 'synapse') diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index e4ce037acf..682721cec7 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- # Copyright 2015, 2016 OpenMarket Ltd +# Copyright 2018 New Vector Ltd # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -494,13 +495,33 @@ class FederationServer(FederationBase): def _handle_received_pdu(self, origin, pdu): """ Process a PDU received in a federation /send/ transaction. + If the event is invalid, then this method throws a FederationError. + (The error will then be logged and sent back to the sender (which + probably won't do anything with it), and other events in the + transaction will be processed as normal). + + It is likely that we'll then receive other events which refer to + this rejected_event in their prev_events, etc. When that happens, + we'll attempt to fetch the rejected event again, which will presumably + fail, so those second-generation events will also get rejected. + + Eventually, we get to the point where there are more than 10 events + between any new events and the original rejected event. Since we + only try to backfill 10 events deep on received pdu, we then accept the + new event, possibly introducing a discontinuity in the DAG, with new + forward extremities, so normal service is approximately returned, + until we try to backfill across the discontinuity. + Args: origin (str): server which sent the pdu pdu (FrozenEvent): received pdu Returns (Deferred): completes with None - Raises: FederationError if the signatures / hash do not match - """ + + Raises: FederationError if the signatures / hash do not match, or + if the event was unacceptable for any other reason (eg, too large, + too many prev_events, couldn't find the prev_events) + """ # check that it's actually being sent from a valid destination to # workaround bug #1753 in 0.18.5 and 0.18.6 if origin != get_domain_from_id(pdu.event_id): diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 310db3fb46..1296e22af6 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -542,9 +542,16 @@ class FederationHandler(BaseHandler): def backfill(self, dest, room_id, limit, extremities): """ Trigger a backfill request to `dest` for the given `room_id` - This will attempt to get more events from the remote. This may return - be successfull and still return no events if the other side has no new - events to offer. + This will attempt to get more events from the remote. If the other side + has no new events to offer, this will return an empty list. + + As the events are received, we check their signatures, and also do some + sanity-checking on them. If any of the backfilled events are invalid, + this method throws a SynapseError. + + TODO: make this more useful to distinguish failures of the remote + server from invalid events (there is probably no point in trying to + re-fetch invalid events from every other HS in the room.) """ if dest == self.server_name: raise SynapseError(400, "Can't backfill from self.") @@ -556,6 +563,8 @@ class FederationHandler(BaseHandler): extremities=extremities, ) + # do some sanity-checking of the received events, before we go and + # do state resolution across 1000 events. for ev in events: self._sanity_check_event(ev) -- cgit 1.4.1 From 3de7d9fe99445b1229411e7a4706c46ea94ded00 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 20 Apr 2018 11:41:03 +0100 Subject: accept stupid events over backfill --- synapse/handlers/federation.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) (limited to 'synapse') diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 1296e22af6..2523ae4a09 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -563,10 +563,15 @@ class FederationHandler(BaseHandler): extremities=extremities, ) - # do some sanity-checking of the received events, before we go and - # do state resolution across 1000 events. - for ev in events: - self._sanity_check_event(ev) + # ideally we'd sanity check the events here for excess prev_events etc, + # but it's hard to reject events at this point without completely + # breaking backfill in the same way that it is currently broken by + # events whose signature we cannot verify (#3121). + # + # So for now we accept the events anyway. #3124 tracks this. + # + # for ev in events: + # self._sanity_check_event(ev) # Don't bother processing events we already have. seen_events = yield self.store.have_events_in_timeline( -- cgit 1.4.1 From 9b9c38373cf9cdc57e106c8cb85c46e6dfd5c329 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 23 Apr 2018 12:00:06 +0100 Subject: Remove spurious param --- synapse/handlers/federation.py | 1 - 1 file changed, 1 deletion(-) (limited to 'synapse') diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 2523ae4a09..b662d480b6 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -903,7 +903,6 @@ class FederationHandler(BaseHandler): logger.warn("Rejecting event %s which has %i auth_events", ev.event_id, len(ev.auth_events)) raise SynapseError( - "ERROR", httplib.BAD_REQUEST, "Too many auth_events", ) -- cgit 1.4.1