From 770b8234450cae5a89173e9269deac9793059aa5 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 29 Jan 2019 16:09:29 +0000 Subject: [PATCH 1/9] Only check event IDs domain signed event for V1 and V2 Since newer versions of events don't have the same format for event ID. --- synapse/event_auth.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index 9adedbbb02..9199a6055f 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -20,7 +20,13 @@ from signedjson.key import decode_verify_key_bytes from signedjson.sign import SignatureVerifyException, verify_signed_json from unpaddedbase64 import decode_base64 -from synapse.api.constants import KNOWN_ROOM_VERSIONS, EventTypes, JoinRules, Membership +from synapse.api.constants import ( + KNOWN_ROOM_VERSIONS, + EventTypes, + JoinRules, + Membership, + RoomVersions, +) from synapse.api.errors import AuthError, EventSizeError, SynapseError from synapse.types import UserID, get_domain_from_id @@ -49,7 +55,6 @@ def check(room_version, event, auth_events, do_sig_check=True, do_size_check=Tru if do_sig_check: sender_domain = get_domain_from_id(event.sender) - event_id_domain = get_domain_from_id(event.event_id) is_invite_via_3pid = ( event.type == EventTypes.Member @@ -66,9 +71,13 @@ def check(room_version, event, auth_events, do_sig_check=True, do_size_check=Tru if not is_invite_via_3pid: raise AuthError(403, "Event not signed by sender's server") - # Check the event_id's domain has signed the event - if not event.signatures.get(event_id_domain): - raise AuthError(403, "Event not signed by sending server") + if event.format_version in (RoomVersions.V1, RoomVersions.V2): + # Only older room versions have event IDs to check. + event_id_domain = get_domain_from_id(event.event_id) + + # Check the origin domain has signed the event + if not event.signatures.get(event_id_domain): + raise AuthError(403, "Event not signed by sending server") if auth_events is None: # Oh, we don't know what the state of the room was, so we From b1fffca3458c6ba26da1e61e4e70ac6f7419d839 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 29 Jan 2019 16:11:19 +0000 Subject: [PATCH 2/9] Remove event ID usage when checking if new room The event ID is changing, so we can no longer get the domain from it. On the other hand, the check is unnecessary. --- synapse/handlers/room_member.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 9ed5a05cca..2beffdf41e 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -940,7 +940,8 @@ class RoomMemberHandler(object): # first member event? create_event_id = current_state_ids.get(("m.room.create", "")) if len(current_state_ids) == 1 and create_event_id: - defer.returnValue(self.hs.is_mine_id(create_event_id)) + # We can only get here if we're in the process of creating the room + defer.returnValue(True) for etype, state_key in current_state_ids: if etype != EventTypes.Member or not self.hs.is_mine_id(state_key): From 55d90248356b0068b201c5be7298e0f3ae1c8ace Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 29 Jan 2019 16:15:16 +0000 Subject: [PATCH 3/9] Use snder and not event ID domain to check if ours The transaction queue only sends out events that we generate. This was done by checking domain of event ID, but that can no longer be used. Instead, we may as well use the sender field. --- synapse/federation/transaction_queue.py | 2 +- synapse/handlers/federation.py | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/synapse/federation/transaction_queue.py b/synapse/federation/transaction_queue.py index fe787abaeb..1f0b67f5f8 100644 --- a/synapse/federation/transaction_queue.py +++ b/synapse/federation/transaction_queue.py @@ -175,7 +175,7 @@ class TransactionQueue(object): def handle_event(event): # Only send events for this server. send_on_behalf_of = event.internal_metadata.get_send_on_behalf_of() - is_mine = self.is_mine_id(event.event_id) + is_mine = self.is_mine_id(event.sender) if not is_mine and send_on_behalf_of is None: return diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index fcaf7530b0..f89dabb9eb 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -2293,6 +2293,10 @@ class FederationHandler(BaseHandler): EventValidator().validate_new(event) + # We need to tell the transaction queue to send this out, even + # though the sender isn't a local user. + event.internal_metadata.send_on_behalf_of = self.hs.hostname + try: yield self.auth.check_from_context(room_version, event, context) except AuthError as e: @@ -2342,6 +2346,10 @@ class FederationHandler(BaseHandler): raise e yield self._check_signature(event, context) + # We need to tell the transaction queue to send this out, even + # though the sender isn't a local user. + event.internal_metadata.send_on_behalf_of = get_domain_from_id(event.sender) + # XXX we send the invite here, but send_membership_event also sends it, # so we end up making two requests. I think this is redundant. returned_invite = yield self.send_invite(origin, event) From 8e3d34e3c58374ab32f3aaace916ddcb4b1a150c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 29 Jan 2019 16:26:40 +0000 Subject: [PATCH 4/9] Use event origin for filtering incoming events We only process events sent to us from a server if the event ID matches the server, to help guard against federation storms. We replace this with a check against the event origin. --- synapse/federation/federation_server.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 6681614232..5c3784c560 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -25,7 +25,7 @@ from twisted.internet import defer from twisted.internet.abstract import isIPAddress from twisted.python import failure -from synapse.api.constants import EventTypes +from synapse.api.constants import EventTypes, Membership from synapse.api.errors import ( AuthError, FederationError, @@ -620,16 +620,19 @@ class FederationServer(FederationBase): """ # 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): + if origin != get_domain_from_id(pdu.sender): # We continue to accept join events from any server; this is # necessary for the federation join dance to work correctly. # (When we join over federation, the "helper" server is # responsible for sending out the join event, rather than the - # origin. See bug #1893). + # origin. See bug #1893. This is also true for some third party + # invites). if not ( pdu.type == 'm.room.member' and pdu.content and - pdu.content.get("membership", None) == 'join' + pdu.content.get("membership", None) in ( + Membership.JOIN, Membership.INVITE, + ) ): logger.info( "Discarding PDU %s from invalid origin %s", From 840068bd787dbf4a8640549578af5ad39b8fb156 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 29 Jan 2019 17:21:48 +0000 Subject: [PATCH 5/9] Only check event ID domain for signatures for V1 events In future version events won't have an event ID, so we won't be able to do this check. --- synapse/federation/federation_base.py | 64 ++++++++++++++----------- synapse/federation/federation_client.py | 6 +-- synapse/federation/federation_server.py | 5 +- 3 files changed, 44 insertions(+), 31 deletions(-) diff --git a/synapse/federation/federation_base.py b/synapse/federation/federation_base.py index 5c31e5f85f..0bff8686e0 100644 --- a/synapse/federation/federation_base.py +++ b/synapse/federation/federation_base.py @@ -20,7 +20,7 @@ import six from twisted.internet import defer from twisted.internet.defer import DeferredList -from synapse.api.constants import MAX_DEPTH, EventTypes, Membership +from synapse.api.constants import KNOWN_ROOM_VERSIONS, MAX_DEPTH, EventTypes, Membership from synapse.api.errors import Codes, SynapseError from synapse.crypto.event_signing import check_event_content_hash from synapse.events import event_type_from_format_version @@ -66,7 +66,7 @@ class FederationBase(object): Returns: Deferred : A list of PDUs that have valid signatures and hashes. """ - deferreds = self._check_sigs_and_hashes(pdus) + deferreds = self._check_sigs_and_hashes(room_version, pdus) @defer.inlineCallbacks def handle_check_result(pdu, deferred): @@ -121,16 +121,17 @@ class FederationBase(object): else: defer.returnValue([p for p in valid_pdus if p]) - def _check_sigs_and_hash(self, pdu): + def _check_sigs_and_hash(self, room_version, pdu): return logcontext.make_deferred_yieldable( - self._check_sigs_and_hashes([pdu])[0], + self._check_sigs_and_hashes(room_version, [pdu])[0], ) - def _check_sigs_and_hashes(self, pdus): + def _check_sigs_and_hashes(self, room_version, pdus): """Checks that each of the received events is correctly signed by the sending server. Args: + room_version (str): The room version of the PDUs pdus (list[FrozenEvent]): the events to be checked Returns: @@ -141,7 +142,7 @@ class FederationBase(object): * throws a SynapseError if the signature check failed. The deferreds run their callbacks in the sentinel logcontext. """ - deferreds = _check_sigs_on_pdus(self.keyring, pdus) + deferreds = _check_sigs_on_pdus(self.keyring, room_version, pdus) ctx = logcontext.LoggingContext.current_context() @@ -203,16 +204,17 @@ class FederationBase(object): class PduToCheckSig(namedtuple("PduToCheckSig", [ - "pdu", "redacted_pdu_json", "event_id_domain", "sender_domain", "deferreds", + "pdu", "redacted_pdu_json", "sender_domain", "deferreds", ])): pass -def _check_sigs_on_pdus(keyring, pdus): +def _check_sigs_on_pdus(keyring, room_version, pdus): """Check that the given events are correctly signed Args: keyring (synapse.crypto.Keyring): keyring object to do the checks + room_version (str): the room version of the PDUs pdus (Collection[EventBase]): the events to be checked Returns: @@ -243,32 +245,22 @@ def _check_sigs_on_pdus(keyring, pdus): # # let's start by getting the domain for each pdu, and flattening the event back # to JSON. + pdus_to_check = [ PduToCheckSig( pdu=p, redacted_pdu_json=prune_event(p).get_pdu_json(), - event_id_domain=get_domain_from_id(p.event_id), sender_domain=get_domain_from_id(p.sender), deferreds=[], ) for p in pdus ] - # first make sure that the event is signed by the event_id's domain - deferreds = keyring.verify_json_objects_for_server([ - (p.event_id_domain, p.redacted_pdu_json) - for p in pdus_to_check - ]) - - for p, d in zip(pdus_to_check, deferreds): - p.deferreds.append(d) - - # now let's look for events where the sender's domain is different to the - # event id's domain (normally only the case for joins/leaves), and add additional - # checks. + # First we check that the sender event is signed by the sender's domain + # (except if its a 3pid invite, in which case it may be sent by any server) pdus_to_check_sender = [ p for p in pdus_to_check - if p.sender_domain != p.event_id_domain and not _is_invite_via_3pid(p.pdu) + if not _is_invite_via_3pid(p.pdu) ] more_deferreds = keyring.verify_json_objects_for_server([ @@ -279,19 +271,37 @@ def _check_sigs_on_pdus(keyring, pdus): for p, d in zip(pdus_to_check_sender, more_deferreds): p.deferreds.append(d) + # now let's look for events where the sender's domain is different to the + # event id's domain (normally only the case for joins/leaves), and add additional + # checks. Only do this if the room version has a concept of event ID domain + if room_version in KNOWN_ROOM_VERSIONS: + pdus_to_check_event_id = [ + p for p in pdus_to_check + if p.sender_domain != get_domain_from_id(p.pdu.event_id) + ] + + more_deferreds = keyring.verify_json_objects_for_server([ + (get_domain_from_id(p.pdu.event_id), p.redacted_pdu_json) + for p in pdus_to_check_event_id + ]) + + for p, d in zip(pdus_to_check_event_id, more_deferreds): + p.deferreds.append(d) + # replace lists of deferreds with single Deferreds return [_flatten_deferred_list(p.deferreds) for p in pdus_to_check] def _flatten_deferred_list(deferreds): - """Given a list of one or more deferreds, either return the single deferred, or - combine into a DeferredList. + """Given a list of deferreds, either return the single deferred, + combine into a DeferredList, or return an already resolved deferred. """ if len(deferreds) > 1: return DeferredList(deferreds, fireOnOneErrback=True, consumeErrors=True) - else: - assert len(deferreds) == 1 + elif len(deferreds) == 1: return deferreds[0] + else: + return defer.succeed(None) def _is_invite_via_3pid(event): @@ -319,7 +329,7 @@ def event_from_pdu_json(pdu_json, event_format_version, outlier=False): """ # we could probably enforce a bunch of other fields here (room_id, sender, # origin, etc etc) - assert_params_in_dict(pdu_json, ('event_id', 'type', 'depth')) + assert_params_in_dict(pdu_json, ('type', 'depth')) depth = pdu_json['depth'] if not isinstance(depth, six.integer_types): diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 9b4acd2ed7..4e4f58b418 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -205,7 +205,7 @@ class FederationClient(FederationBase): # FIXME: We should handle signature failures more gracefully. pdus[:] = yield logcontext.make_deferred_yieldable(defer.gatherResults( - self._check_sigs_and_hashes(pdus), + self._check_sigs_and_hashes(room_version, pdus), consumeErrors=True, ).addErrback(unwrapFirstError)) @@ -268,7 +268,7 @@ class FederationClient(FederationBase): pdu = pdu_list[0] # Check signatures are correct. - signed_pdu = yield self._check_sigs_and_hash(pdu) + signed_pdu = yield self._check_sigs_and_hash(room_version, pdu) break @@ -757,7 +757,7 @@ class FederationClient(FederationBase): pdu = event_from_pdu_json(pdu_dict, format_ver) # Check signatures are correct. - pdu = yield self._check_sigs_and_hash(pdu) + pdu = yield self._check_sigs_and_hash(room_version, pdu) # FIXME: We should handle signature failures more gracefully. diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 5c3784c560..aeadc9c564 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -645,9 +645,12 @@ class FederationServer(FederationBase): pdu.event_id, origin ) + # We've already checked that we know the room version by this point + room_version = yield self.store.get_room_version(pdu.room_id) + # Check signature. try: - pdu = yield self._check_sigs_and_hash(pdu) + pdu = yield self._check_sigs_and_hash(room_version, pdu) except SynapseError as e: raise FederationError( "ERROR", From 610f0830b09fd1ef33c1ef9aa4c32d4eb411c289 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 29 Jan 2019 17:23:47 +0000 Subject: [PATCH 6/9] Don't assert an event must have an event ID --- synapse/events/validator.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/synapse/events/validator.py b/synapse/events/validator.py index c53bf44e51..a072674b02 100644 --- a/synapse/events/validator.py +++ b/synapse/events/validator.py @@ -15,7 +15,7 @@ from six import string_types -from synapse.api.constants import EventTypes, Membership +from synapse.api.constants import EventFormatVersions, EventTypes, Membership from synapse.api.errors import SynapseError from synapse.types import EventID, RoomID, UserID @@ -29,7 +29,8 @@ class EventValidator(object): """ self.validate_builder(event) - EventID.from_string(event.event_id) + if event.format_version == EventFormatVersions.V1: + EventID.from_string(event.event_id) required = [ "auth_events", From b40abe07243d1c7d66bffc2b5b6bd36861750525 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 29 Jan 2019 17:28:10 +0000 Subject: [PATCH 7/9] Newsfile --- changelog.d/4514.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4514.misc diff --git a/changelog.d/4514.misc b/changelog.d/4514.misc new file mode 100644 index 0000000000..43f8963614 --- /dev/null +++ b/changelog.d/4514.misc @@ -0,0 +1 @@ +Add infrastructure to support different event formats From 655ce037fd30589a587c8d3e5994e818a7ef381b Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 29 Jan 2019 22:33:43 +0000 Subject: [PATCH 8/9] check event format version not room version --- synapse/event_auth.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index 9199a6055f..e8da5310ed 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -22,10 +22,10 @@ from unpaddedbase64 import decode_base64 from synapse.api.constants import ( KNOWN_ROOM_VERSIONS, + EventFormatVersions, EventTypes, JoinRules, Membership, - RoomVersions, ) from synapse.api.errors import AuthError, EventSizeError, SynapseError from synapse.types import UserID, get_domain_from_id @@ -71,7 +71,7 @@ def check(room_version, event, auth_events, do_sig_check=True, do_size_check=Tru if not is_invite_via_3pid: raise AuthError(403, "Event not signed by sender's server") - if event.format_version in (RoomVersions.V1, RoomVersions.V2): + if event.format_version in (EventFormatVersions.V1,): # Only older room versions have event IDs to check. event_id_domain = get_domain_from_id(event.event_id) From ff2f65d737b3d36e2aef8c296d1ce731156d3847 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 29 Jan 2019 22:35:36 +0000 Subject: [PATCH 9/9] Update comment --- synapse/federation/federation_base.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/federation/federation_base.py b/synapse/federation/federation_base.py index 0bff8686e0..a400091db7 100644 --- a/synapse/federation/federation_base.py +++ b/synapse/federation/federation_base.py @@ -227,9 +227,7 @@ def _check_sigs_on_pdus(keyring, room_version, pdus): # we want to check that the event is signed by: # - # (a) the server which created the event_id - # - # (b) the sender's server. + # (a) the sender's server # # - except in the case of invites created from a 3pid invite, which are exempt # from this check, because the sender has to match that of the original 3pid @@ -243,6 +241,8 @@ def _check_sigs_on_pdus(keyring, room_version, pdus): # and signatures are *supposed* to be valid whether or not an event has been # redacted. But this isn't the worst of the ways that 3pid invites are broken. # + # (b) for V1 and V2 rooms, the server which created the event_id + # # let's start by getting the domain for each pdu, and flattening the event back # to JSON.