Merge branch 'rav/fix_event_sig_checks' into matrix-org/fix_event_sig_checks

This commit is contained in:
Richard van der Hoff 2018-09-05 15:21:36 +01:00
commit 86157b9743
9 changed files with 154 additions and 49 deletions

View file

@ -1,3 +1,12 @@
Synapse 0.33.3 (2018-08-22)
===========================
Bugfixes
--------
- Fix bug introduced in v0.33.3rc1 which made the ToS give a 500 error ([\#3732](https://github.com/matrix-org/synapse/issues/3732))
Synapse 0.33.3rc2 (2018-08-21) Synapse 0.33.3rc2 (2018-08-21)
============================== ==============================
@ -13,7 +22,7 @@ Synapse 0.33.3rc1 (2018-08-21)
Features Features
-------- --------
- Add support for the SNI extension to federation TLS connections ([\#1491](https://github.com/matrix-org/synapse/issues/1491)) - Add support for the SNI extension to federation TLS connections. Thanks to @vojeroen! ([\#3439](https://github.com/matrix-org/synapse/issues/3439))
- Add /_media/r0/config ([\#3184](https://github.com/matrix-org/synapse/issues/3184)) - Add /_media/r0/config ([\#3184](https://github.com/matrix-org/synapse/issues/3184))
- speed up /members API and add `at` and `membership` params as per MSC1227 ([\#3568](https://github.com/matrix-org/synapse/issues/3568)) - speed up /members API and add `at` and `membership` params as per MSC1227 ([\#3568](https://github.com/matrix-org/synapse/issues/3568))
- implement `summary` block in /sync response as per MSC688 ([\#3574](https://github.com/matrix-org/synapse/issues/3574)) - implement `summary` block in /sync response as per MSC688 ([\#3574](https://github.com/matrix-org/synapse/issues/3574))
@ -97,7 +106,7 @@ Features
Bugfixes Bugfixes
-------- --------
- Make /directory/list API return 404 for room not found instead of 400 ([\#2952](https://github.com/matrix-org/synapse/issues/2952)) - Make /directory/list API return 404 for room not found instead of 400. Thanks to @fuzzmz! ([\#3620](https://github.com/matrix-org/synapse/issues/3620))
- Default inviter_display_name to mxid for email invites ([\#3391](https://github.com/matrix-org/synapse/issues/3391)) - Default inviter_display_name to mxid for email invites ([\#3391](https://github.com/matrix-org/synapse/issues/3391))
- Don't generate TURN credentials if no TURN config options are set ([\#3514](https://github.com/matrix-org/synapse/issues/3514)) - Don't generate TURN credentials if no TURN config options are set ([\#3514](https://github.com/matrix-org/synapse/issues/3514))
- Correctly announce deleted devices over federation ([\#3520](https://github.com/matrix-org/synapse/issues/3520)) - Correctly announce deleted devices over federation ([\#3520](https://github.com/matrix-org/synapse/issues/3520))

View file

@ -1 +0,0 @@
Fix bug introduced in v0.33.3rc1 which made the ToS give a 500 error

View file

@ -17,4 +17,4 @@
""" This is a reference implementation of a Matrix home server. """ This is a reference implementation of a Matrix home server.
""" """
__version__ = "0.33.3rc2" __version__ = "0.33.3"

View file

@ -13,17 +13,20 @@
# See the License for the specific language governing permissions and # See the License for the specific language governing permissions and
# limitations under the License. # limitations under the License.
import logging import logging
from collections import namedtuple
import six import six
from twisted.internet import defer from twisted.internet import defer
from twisted.internet.defer import DeferredList
from synapse.api.constants import MAX_DEPTH from synapse.api.constants import MAX_DEPTH, EventTypes, Membership
from synapse.api.errors import Codes, SynapseError from synapse.api.errors import Codes, SynapseError
from synapse.crypto.event_signing import check_event_content_hash from synapse.crypto.event_signing import check_event_content_hash
from synapse.events import FrozenEvent from synapse.events import FrozenEvent
from synapse.events.utils import prune_event from synapse.events.utils import prune_event
from synapse.http.servlet import assert_params_in_dict from synapse.http.servlet import assert_params_in_dict
from synapse.types import get_domain_from_id
from synapse.util import logcontext, unwrapFirstError from synapse.util import logcontext, unwrapFirstError
logger = logging.getLogger(__name__) logger = logging.getLogger(__name__)
@ -133,34 +136,25 @@ class FederationBase(object):
* throws a SynapseError if the signature check failed. * throws a SynapseError if the signature check failed.
The deferreds run their callbacks in the sentinel logcontext. The deferreds run their callbacks in the sentinel logcontext.
""" """
deferreds = _check_sigs_on_pdus(self.keyring, pdus)
redacted_pdus = [
prune_event(pdu)
for pdu in pdus
]
deferreds = self.keyring.verify_json_objects_for_server([
(p.origin, p.get_pdu_json())
for p in redacted_pdus
])
ctx = logcontext.LoggingContext.current_context() ctx = logcontext.LoggingContext.current_context()
def callback(_, pdu, redacted): def callback(_, pdu):
with logcontext.PreserveLoggingContext(ctx): with logcontext.PreserveLoggingContext(ctx):
if not check_event_content_hash(pdu): if not check_event_content_hash(pdu):
logger.warn( logger.warn(
"Event content has been tampered, redacting %s: %s", "Event content has been tampered, redacting %s: %s",
pdu.event_id, pdu.get_pdu_json() pdu.event_id, pdu.get_pdu_json()
) )
return redacted return prune_event(pdu)
if self.spam_checker.check_event_for_spam(pdu): if self.spam_checker.check_event_for_spam(pdu):
logger.warn( logger.warn(
"Event contains spam, redacting %s: %s", "Event contains spam, redacting %s: %s",
pdu.event_id, pdu.get_pdu_json() pdu.event_id, pdu.get_pdu_json()
) )
return redacted return prune_event(pdu)
return pdu return pdu
@ -173,16 +167,116 @@ class FederationBase(object):
) )
return failure return failure
for deferred, pdu, redacted in zip(deferreds, pdus, redacted_pdus): for deferred, pdu in zip(deferreds, pdus):
deferred.addCallbacks( deferred.addCallbacks(
callback, errback, callback, errback,
callbackArgs=[pdu, redacted], callbackArgs=[pdu],
errbackArgs=[pdu], errbackArgs=[pdu],
) )
return deferreds return deferreds
class PduToCheckSig(namedtuple("PduToCheckSig", [
"pdu", "redacted_pdu_json", "event_id_domain", "sender_domain", "deferreds",
])):
pass
def _check_sigs_on_pdus(keyring, pdus):
"""Check that the given events are correctly signed
Args:
keyring (synapse.crypto.Keyring): keyring object to do the checks
pdus (Collection[EventBase]): the events to be checked
Returns:
List[Deferred]: a Deferred for each event in pdus, which will either succeed if
the signatures are valid, or fail (with a SynapseError) if not.
"""
# (currently this is written assuming the v1 room structure; we'll probably want a
# separate function for checking v2 rooms)
# we want to check that the event is signed by:
#
# (a) the server which created the event_id
#
# (b) 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
# invite, but the event may come from a different HS, for reasons that I don't
# entirely grok (why do the senders have to match? and if they do, why doesn't the
# joining server ask the inviting server to do the switcheroo with
# exchange_third_party_invite?).
#
# That's pretty awful, since redacting such an invite will render it invalid
# (because it will then look like a regular invite without a valid signature),
# 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.
#
# 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.
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)
]
more_deferreds = keyring.verify_json_objects_for_server([
(p.sender_domain, p.redacted_pdu_json)
for p in pdus_to_check_sender
])
for p, d in zip(pdus_to_check_sender, 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.
"""
if len(deferreds) > 1:
return DeferredList(deferreds, fireOnOneErrback=True, consumeErrors=True)
else:
assert len(deferreds) == 1
return deferreds[0]
def _is_invite_via_3pid(event):
return (
event.type == EventTypes.Member
and event.membership == Membership.INVITE
and "third_party_invite" in event.content
)
def event_from_pdu_json(pdu_json, outlier=False): def event_from_pdu_json(pdu_json, outlier=False):
"""Construct a FrozenEvent from an event json received over federation """Construct a FrozenEvent from an event json received over federation

View file

@ -99,7 +99,7 @@ class FederationServer(FederationBase):
@defer.inlineCallbacks @defer.inlineCallbacks
@log_function @log_function
def on_incoming_transaction(self, transaction_data): def on_incoming_transaction(self, origin, transaction_data):
# keep this as early as possible to make the calculated origin ts as # keep this as early as possible to make the calculated origin ts as
# accurate as possible. # accurate as possible.
request_time = self._clock.time_msec() request_time = self._clock.time_msec()
@ -108,34 +108,33 @@ class FederationServer(FederationBase):
if not transaction.transaction_id: if not transaction.transaction_id:
raise Exception("Transaction missing transaction_id") raise Exception("Transaction missing transaction_id")
if not transaction.origin:
raise Exception("Transaction missing origin")
logger.debug("[%s] Got transaction", transaction.transaction_id) logger.debug("[%s] Got transaction", transaction.transaction_id)
# use a linearizer to ensure that we don't process the same transaction # use a linearizer to ensure that we don't process the same transaction
# multiple times in parallel. # multiple times in parallel.
with (yield self._transaction_linearizer.queue( with (yield self._transaction_linearizer.queue(
(transaction.origin, transaction.transaction_id), (origin, transaction.transaction_id),
)): )):
result = yield self._handle_incoming_transaction( result = yield self._handle_incoming_transaction(
transaction, request_time, origin, transaction, request_time,
) )
defer.returnValue(result) defer.returnValue(result)
@defer.inlineCallbacks @defer.inlineCallbacks
def _handle_incoming_transaction(self, transaction, request_time): def _handle_incoming_transaction(self, origin, transaction, request_time):
""" Process an incoming transaction and return the HTTP response """ Process an incoming transaction and return the HTTP response
Args: Args:
origin (unicode): the server making the request
transaction (Transaction): incoming transaction transaction (Transaction): incoming transaction
request_time (int): timestamp that the HTTP request arrived at request_time (int): timestamp that the HTTP request arrived at
Returns: Returns:
Deferred[(int, object)]: http response code and body Deferred[(int, object)]: http response code and body
""" """
response = yield self.transaction_actions.have_responded(transaction) response = yield self.transaction_actions.have_responded(origin, transaction)
if response: if response:
logger.debug( logger.debug(
@ -149,7 +148,7 @@ class FederationServer(FederationBase):
received_pdus_counter.inc(len(transaction.pdus)) received_pdus_counter.inc(len(transaction.pdus))
origin_host, _ = parse_server_name(transaction.origin) origin_host, _ = parse_server_name(origin)
pdus_by_room = {} pdus_by_room = {}
@ -190,7 +189,7 @@ class FederationServer(FederationBase):
event_id = pdu.event_id event_id = pdu.event_id
try: try:
yield self._handle_received_pdu( yield self._handle_received_pdu(
transaction.origin, pdu origin, pdu
) )
pdu_results[event_id] = {} pdu_results[event_id] = {}
except FederationError as e: except FederationError as e:
@ -212,7 +211,7 @@ class FederationServer(FederationBase):
if hasattr(transaction, "edus"): if hasattr(transaction, "edus"):
for edu in (Edu(**x) for x in transaction.edus): for edu in (Edu(**x) for x in transaction.edus):
yield self.received_edu( yield self.received_edu(
transaction.origin, origin,
edu.edu_type, edu.edu_type,
edu.content edu.content
) )
@ -224,6 +223,7 @@ class FederationServer(FederationBase):
logger.debug("Returning: %s", str(response)) logger.debug("Returning: %s", str(response))
yield self.transaction_actions.set_response( yield self.transaction_actions.set_response(
origin,
transaction, transaction,
200, response 200, response
) )

View file

@ -36,7 +36,7 @@ class TransactionActions(object):
self.store = datastore self.store = datastore
@log_function @log_function
def have_responded(self, transaction): def have_responded(self, origin, transaction):
""" Have we already responded to a transaction with the same id and """ Have we already responded to a transaction with the same id and
origin? origin?
@ -50,11 +50,11 @@ class TransactionActions(object):
"transaction_id") "transaction_id")
return self.store.get_received_txn_response( return self.store.get_received_txn_response(
transaction.transaction_id, transaction.origin transaction.transaction_id, origin
) )
@log_function @log_function
def set_response(self, transaction, code, response): def set_response(self, origin, transaction, code, response):
""" Persist how we responded to a transaction. """ Persist how we responded to a transaction.
Returns: Returns:
@ -66,7 +66,7 @@ class TransactionActions(object):
return self.store.set_received_txn_response( return self.store.set_received_txn_response(
transaction.transaction_id, transaction.transaction_id,
transaction.origin, origin,
code, code,
response, response,
) )

View file

@ -353,7 +353,7 @@ class FederationSendServlet(BaseFederationServlet):
try: try:
code, response = yield self.handler.on_incoming_transaction( code, response = yield self.handler.on_incoming_transaction(
transaction_data origin, transaction_data,
) )
except Exception: except Exception:
logger.exception("on_incoming_transaction failed") logger.exception("on_incoming_transaction failed")

View file

@ -33,7 +33,7 @@ from ..utils import (
) )
def _expect_edu(destination, edu_type, content, origin="test"): def _expect_edu_transaction(edu_type, content, origin="test"):
return { return {
"origin": origin, "origin": origin,
"origin_server_ts": 1000000, "origin_server_ts": 1000000,
@ -42,8 +42,8 @@ def _expect_edu(destination, edu_type, content, origin="test"):
} }
def _make_edu_json(origin, edu_type, content): def _make_edu_transaction_json(edu_type, content):
return json.dumps(_expect_edu("test", edu_type, content, origin=origin)).encode( return json.dumps(_expect_edu_transaction(edu_type, content)).encode(
'utf8' 'utf8'
) )
@ -190,8 +190,7 @@ class TypingNotificationsTestCase(unittest.TestCase):
call( call(
"farm", "farm",
path="/_matrix/federation/v1/send/1000000/", path="/_matrix/federation/v1/send/1000000/",
data=_expect_edu( data=_expect_edu_transaction(
"farm",
"m.typing", "m.typing",
content={ content={
"room_id": self.room_id, "room_id": self.room_id,
@ -221,11 +220,10 @@ class TypingNotificationsTestCase(unittest.TestCase):
self.assertEquals(self.event_source.get_current_key(), 0) self.assertEquals(self.event_source.get_current_key(), 0)
yield self.mock_federation_resource.trigger( (code, response) = yield self.mock_federation_resource.trigger(
"PUT", "PUT",
"/_matrix/federation/v1/send/1000000/", "/_matrix/federation/v1/send/1000000/",
_make_edu_json( _make_edu_transaction_json(
"farm",
"m.typing", "m.typing",
content={ content={
"room_id": self.room_id, "room_id": self.room_id,
@ -233,7 +231,7 @@ class TypingNotificationsTestCase(unittest.TestCase):
"typing": True, "typing": True,
}, },
), ),
federation_auth=True, federation_auth_origin=b'farm',
) )
self.on_new_event.assert_has_calls( self.on_new_event.assert_has_calls(
@ -264,8 +262,7 @@ class TypingNotificationsTestCase(unittest.TestCase):
call( call(
"farm", "farm",
path="/_matrix/federation/v1/send/1000000/", path="/_matrix/federation/v1/send/1000000/",
data=_expect_edu( data=_expect_edu_transaction(
"farm",
"m.typing", "m.typing",
content={ content={
"room_id": self.room_id, "room_id": self.room_id,

View file

@ -307,7 +307,10 @@ class MockHttpResource(HttpServer):
@patch('twisted.web.http.Request') @patch('twisted.web.http.Request')
@defer.inlineCallbacks @defer.inlineCallbacks
def trigger(self, http_method, path, content, mock_request, federation_auth=False): def trigger(
self, http_method, path, content, mock_request,
federation_auth_origin=None,
):
""" Fire an HTTP event. """ Fire an HTTP event.
Args: Args:
@ -316,6 +319,7 @@ class MockHttpResource(HttpServer):
content : The HTTP body content : The HTTP body
mock_request : Mocked request to pass to the event so it can get mock_request : Mocked request to pass to the event so it can get
content. content.
federation_auth_origin (bytes|None): domain to authenticate as, for federation
Returns: Returns:
A tuple of (code, response) A tuple of (code, response)
Raises: Raises:
@ -336,8 +340,10 @@ class MockHttpResource(HttpServer):
mock_request.getClientIP.return_value = "-" mock_request.getClientIP.return_value = "-"
headers = {} headers = {}
if federation_auth: if federation_auth_origin is not None:
headers[b"Authorization"] = [b"X-Matrix origin=test,key=,sig="] headers[b"Authorization"] = [
b"X-Matrix origin=%s,key=,sig=" % (federation_auth_origin, )
]
mock_request.requestHeaders.getRawHeaders = mock_getRawHeaders(headers) mock_request.requestHeaders.getRawHeaders = mock_getRawHeaders(headers)
# return the right path if the event requires it # return the right path if the event requires it