From 9b7794262fa1a061f6668a6693a79b9bf8d3567a Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Tue, 17 Apr 2018 22:11:19 +0100
Subject: [PATCH 1/5] 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(-)
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:
From e585228860fe72ca8f2b95d49394a39f6329bf39 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Tue, 17 Apr 2018 23:41:12 +0100
Subject: [PATCH 2/5] Check events on backfill too
---
synapse/handlers/federation.py | 57 +++++++++++++++++++++++++---------
1 file changed, 42 insertions(+), 15 deletions(-)
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.
From 1f4b498b7313b9bff5bfc3eeeed25c7659276e82 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Tue, 17 Apr 2018 23:41:36 +0100
Subject: [PATCH 3/5] Add some comments
---
synapse/federation/federation_server.py | 25 +++++++++++++++++++++++--
synapse/handlers/federation.py | 15 ++++++++++++---
2 files changed, 35 insertions(+), 5 deletions(-)
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)
From 3de7d9fe99445b1229411e7a4706c46ea94ded00 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Fri, 20 Apr 2018 11:41:03 +0100
Subject: [PATCH 4/5] accept stupid events over backfill
---
synapse/handlers/federation.py | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
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(
From 9b9c38373cf9cdc57e106c8cb85c46e6dfd5c329 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Mon, 23 Apr 2018 12:00:06 +0100
Subject: [PATCH 5/5] Remove spurious param
---
synapse/handlers/federation.py | 1 -
1 file changed, 1 deletion(-)
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",
)