diff --git a/CHANGES.md b/CHANGES.md index f8da8771a..fab27b874 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,35 @@ +Synapse 1.41.1 (2021-08-31) +=========================== + +Due to the two security issues highlighted below, server administrators are encouraged to update Synapse. We are not aware of these vulnerabilities being exploited in the wild. + +Security advisory +----------------- + +The following issues are fixed in v1.41.1. + +- **[GHSA-3x4c-pq33-4w3q](https://github.com/matrix-org/synapse/security/advisories/GHSA-3x4c-pq33-4w3q) / [CVE-2021-39164](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-39164): Enumerating a private room's list of members and their display names.** + + If an unauthorized user both knows the Room ID of a private room *and* that room's history visibility is set to `shared`, then they may be able to enumerate the room's members, including their display names. + + The unauthorized user must be on the same homeserver as a user who is a member of the target room. + + Fixed by [52c7a51cf](https://github.com/matrix-org/synapse/commit/52c7a51cf). + +- **[GHSA-jj53-8fmw-f2w2](https://github.com/matrix-org/synapse/security/advisories/GHSA-jj53-8fmw-f2w2) / [CVE-2021-39163](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-39163): Disclosing a private room's name, avatar, topic, and number of members.** + + If an unauthorized user knows the Room ID of a private room, then its name, avatar, topic, and number of members may be disclosed through Group / Community features. + + The unauthorized user must be on the same homeserver as a user who is a member of the target room, and their homeserver must allow non-administrators to create groups (`enable_group_creation` in the Synapse configuration; off by default). + + Fixed by [cb35df940a](https://github.com/matrix-org/synapse/commit/cb35df940a), [\#10723](https://github.com/matrix-org/synapse/issues/10723). + +Bugfixes +-------- + +- Fix a regression introduced in Synapse 1.41 which broke email transmission on systems using older versions of the Twisted library. ([\#10713](https://github.com/matrix-org/synapse/issues/10713)) + + Synapse 1.41.0 (2021-08-24) =========================== diff --git a/changelog.d/10713.bugfix b/changelog.d/10713.bugfix new file mode 100644 index 000000000..e8caf3d23 --- /dev/null +++ b/changelog.d/10713.bugfix @@ -0,0 +1 @@ +Fix a regression introduced in Synapse 1.41 which broke email transmission on Systems using older versions of the Twisted library. diff --git a/changelog.d/10723.bugfix b/changelog.d/10723.bugfix new file mode 100644 index 000000000..e6ffdc951 --- /dev/null +++ b/changelog.d/10723.bugfix @@ -0,0 +1 @@ +Fix unauthorised exposure of room metadata to communities. diff --git a/debian/changelog b/debian/changelog index 4da4bc018..5f7a795b6 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +matrix-synapse-py3 (1.41.1) stable; urgency=high + + * New synapse release 1.41.1. + + -- Synapse Packaging team Tue, 31 Aug 2021 12:59:10 +0100 + matrix-synapse-py3 (1.41.0) stable; urgency=medium * New synapse release 1.41.0. diff --git a/mypy.ini b/mypy.ini index e1b9405da..349efe37b 100644 --- a/mypy.ini +++ b/mypy.ini @@ -87,6 +87,7 @@ files = tests/test_utils, tests/handlers/test_password_providers.py, tests/handlers/test_room_summary.py, + tests/handlers/test_send_email.py, tests/rest/client/v1/test_login.py, tests/rest/client/v2_alpha/test_auth.py, tests/util/test_itertools.py, diff --git a/synapse/__init__.py b/synapse/__init__.py index ef3770262..06d80f79b 100644 --- a/synapse/__init__.py +++ b/synapse/__init__.py @@ -47,7 +47,7 @@ try: except ImportError: pass -__version__ = "1.41.0" +__version__ = "1.41.1" if bool(os.environ.get("SYNAPSE_TEST_PATCH_LOG_CONTEXTS", False)): # We import here so that we don't have to install a bunch of deps when diff --git a/synapse/groups/groups_server.py b/synapse/groups/groups_server.py index 3dc55ab86..d6b75ac27 100644 --- a/synapse/groups/groups_server.py +++ b/synapse/groups/groups_server.py @@ -332,6 +332,13 @@ class GroupsServerWorkerHandler: requester_user_id, group_id ) + # Note! room_results["is_public"] is about whether the room is considered + # public from the group's point of view. (i.e. whether non-group members + # should be able to see the room is in the group). + # This is not the same as whether the room itself is public (in the sense + # of being visible in the room directory). + # As such, room_results["is_public"] itself is not sufficient to determine + # whether any given user is permitted to see the room's metadata. room_results = await self.store.get_rooms_in_group( group_id, include_private=is_user_in_group ) @@ -341,8 +348,15 @@ class GroupsServerWorkerHandler: room_id = room_result["room_id"] joined_users = await self.store.get_users_in_room(room_id) + + # check the user is actually allowed to see the room before showing it to them + allow_private = requester_user_id in joined_users + entry = await self.room_list_handler.generate_room_entry( - room_id, len(joined_users), with_alias=False, allow_private=True + room_id, + len(joined_users), + with_alias=False, + allow_private=allow_private, ) if not entry: @@ -354,7 +368,7 @@ class GroupsServerWorkerHandler: chunk.sort(key=lambda e: -e["num_joined_members"]) - return {"chunk": chunk, "total_room_count_estimate": len(room_results)} + return {"chunk": chunk, "total_room_count_estimate": len(chunk)} class GroupsServerHandler(GroupsServerWorkerHandler): diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 525effeda..d0e8d38da 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -183,20 +183,37 @@ class MessageHandler: if not last_events: raise NotFoundError("Can't find event for token %s" % (at_token,)) + last_event = last_events[0] + + # check whether the user is in the room at that time to determine + # whether they should be treated as peeking. + state_map = await self.state_store.get_state_for_event( + last_event.event_id, + StateFilter.from_types([(EventTypes.Member, user_id)]), + ) + + joined = False + membership_event = state_map.get((EventTypes.Member, user_id)) + if membership_event: + joined = membership_event.membership == Membership.JOIN + + is_peeking = not joined visible_events = await filter_events_for_client( self.storage, user_id, last_events, filter_send_to_client=False, + is_peeking=is_peeking, ) - event = last_events[0] if visible_events: room_state_events = await self.state_store.get_state_for_events( - [event.event_id], state_filter=state_filter + [last_event.event_id], state_filter=state_filter ) - room_state: Mapping[Any, EventBase] = room_state_events[event.event_id] + room_state: Mapping[Any, EventBase] = room_state_events[ + last_event.event_id + ] else: raise AuthError( 403, diff --git a/synapse/handlers/send_email.py b/synapse/handlers/send_email.py index dda9659c1..a31fe3e3c 100644 --- a/synapse/handlers/send_email.py +++ b/synapse/handlers/send_email.py @@ -19,9 +19,12 @@ from email.mime.text import MIMEText from io import BytesIO from typing import TYPE_CHECKING, Optional +from pkg_resources import parse_version + +import twisted from twisted.internet.defer import Deferred -from twisted.internet.interfaces import IReactorTCP -from twisted.mail.smtp import ESMTPSenderFactory +from twisted.internet.interfaces import IOpenSSLContextFactory, IReactorTCP +from twisted.mail.smtp import ESMTPSender, ESMTPSenderFactory from synapse.logging.context import make_deferred_yieldable @@ -30,6 +33,19 @@ if TYPE_CHECKING: logger = logging.getLogger(__name__) +_is_old_twisted = parse_version(twisted.__version__) < parse_version("21") + + +class _NoTLSESMTPSender(ESMTPSender): + """Extend ESMTPSender to disable TLS + + Unfortunately, before Twisted 21.2, ESMTPSender doesn't give an easy way to disable + TLS, so we override its internal method which it uses to generate a context factory. + """ + + def _getContextFactory(self) -> Optional[IOpenSSLContextFactory]: + return None + async def _sendmail( reactor: IReactorTCP, @@ -42,7 +58,7 @@ async def _sendmail( password: Optional[bytes] = None, require_auth: bool = False, require_tls: bool = False, - tls_hostname: Optional[str] = None, + enable_tls: bool = True, ) -> None: """A simple wrapper around ESMTPSenderFactory, to allow substitution in tests @@ -57,24 +73,37 @@ async def _sendmail( password: password to give when authenticating require_auth: if auth is not offered, fail the request require_tls: if TLS is not offered, fail the reqest - tls_hostname: TLS hostname to check for. None to disable TLS. + enable_tls: True to enable TLS. If this is False and require_tls is True, + the request will fail. """ msg = BytesIO(msg_bytes) - d: "Deferred[object]" = Deferred() - factory = ESMTPSenderFactory( - username, - password, - from_addr, - to_addr, - msg, - d, - heloFallback=True, - requireAuthentication=require_auth, - requireTransportSecurity=require_tls, - hostname=tls_hostname, - ) + def build_sender_factory(**kwargs) -> ESMTPSenderFactory: + return ESMTPSenderFactory( + username, + password, + from_addr, + to_addr, + msg, + d, + heloFallback=True, + requireAuthentication=require_auth, + requireTransportSecurity=require_tls, + **kwargs, + ) + + if _is_old_twisted: + # before twisted 21.2, we have to override the ESMTPSender protocol to disable + # TLS + factory = build_sender_factory() + + if not enable_tls: + factory.protocol = _NoTLSESMTPSender + else: + # for twisted 21.2 and later, there is a 'hostname' parameter which we should + # set to enable TLS. + factory = build_sender_factory(hostname=smtphost if enable_tls else None) # the IReactorTCP interface claims host has to be a bytes, which seems to be wrong reactor.connectTCP(smtphost, smtpport, factory, timeout=30, bindAddress=None) # type: ignore[arg-type] @@ -154,5 +183,5 @@ class SendEmailHandler: password=self._smtp_pass, require_auth=self._smtp_user is not None, require_tls=self._require_transport_security, - tls_hostname=self._smtp_host if self._enable_tls else None, + enable_tls=self._enable_tls, ) diff --git a/tests/handlers/test_send_email.py b/tests/handlers/test_send_email.py new file mode 100644 index 000000000..6f77b1237 --- /dev/null +++ b/tests/handlers/test_send_email.py @@ -0,0 +1,112 @@ +# Copyright 2021 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +from typing import List, Tuple + +from zope.interface import implementer + +from twisted.internet import defer +from twisted.internet.address import IPv4Address +from twisted.internet.defer import ensureDeferred +from twisted.mail import interfaces, smtp + +from tests.server import FakeTransport +from tests.unittest import HomeserverTestCase + + +@implementer(interfaces.IMessageDelivery) +class _DummyMessageDelivery: + def __init__(self): + # (recipient, message) tuples + self.messages: List[Tuple[smtp.Address, bytes]] = [] + + def receivedHeader(self, helo, origin, recipients): + return None + + def validateFrom(self, helo, origin): + return origin + + def record_message(self, recipient: smtp.Address, message: bytes): + self.messages.append((recipient, message)) + + def validateTo(self, user: smtp.User): + return lambda: _DummyMessage(self, user) + + +@implementer(interfaces.IMessageSMTP) +class _DummyMessage: + """IMessageSMTP implementation which saves the message delivered to it + to the _DummyMessageDelivery object. + """ + + def __init__(self, delivery: _DummyMessageDelivery, user: smtp.User): + self._delivery = delivery + self._user = user + self._buffer: List[bytes] = [] + + def lineReceived(self, line): + self._buffer.append(line) + + def eomReceived(self): + message = b"\n".join(self._buffer) + b"\n" + self._delivery.record_message(self._user.dest, message) + return defer.succeed(b"saved") + + def connectionLost(self): + pass + + +class SendEmailHandlerTestCase(HomeserverTestCase): + def test_send_email(self): + """Happy-path test that we can send email to a non-TLS server.""" + h = self.hs.get_send_email_handler() + d = ensureDeferred( + h.send_email( + "foo@bar.com", "test subject", "Tests", "HTML content", "Text content" + ) + ) + # there should be an attempt to connect to localhost:25 + self.assertEqual(len(self.reactor.tcpClients), 1) + (host, port, client_factory, _timeout, _bindAddress) = self.reactor.tcpClients[ + 0 + ] + self.assertEqual(host, "localhost") + self.assertEqual(port, 25) + + # wire it up to an SMTP server + message_delivery = _DummyMessageDelivery() + server_protocol = smtp.ESMTP() + server_protocol.delivery = message_delivery + # make sure that the server uses the test reactor to set timeouts + server_protocol.callLater = self.reactor.callLater # type: ignore[assignment] + + client_protocol = client_factory.buildProtocol(None) + client_protocol.makeConnection(FakeTransport(server_protocol, self.reactor)) + server_protocol.makeConnection( + FakeTransport( + client_protocol, + self.reactor, + peer_address=IPv4Address("TCP", "127.0.0.1", 1234), + ) + ) + + # the message should now get delivered + self.get_success(d, by=0.1) + + # check it arrived + self.assertEqual(len(message_delivery.messages), 1) + user, msg = message_delivery.messages.pop() + self.assertEqual(str(user), "foo@bar.com") + self.assertIn(b"Subject: test subject", msg) diff --git a/tests/rest/client/v1/test_rooms.py b/tests/rest/client/v1/test_rooms.py index 0c9cbb9af..50100a5ae 100644 --- a/tests/rest/client/v1/test_rooms.py +++ b/tests/rest/client/v1/test_rooms.py @@ -29,7 +29,7 @@ from synapse.api.constants import EventContentFields, EventTypes, Membership from synapse.api.errors import HttpResponseException from synapse.handlers.pagination import PurgeStatus from synapse.rest import admin -from synapse.rest.client import account, directory, login, profile, room +from synapse.rest.client import account, directory, login, profile, room, sync from synapse.types import JsonDict, RoomAlias, UserID, create_requester from synapse.util.stringutils import random_string @@ -381,6 +381,8 @@ class RoomPermissionsTestCase(RoomBase): class RoomsMemberListTestCase(RoomBase): """Tests /rooms/$room_id/members/list REST events.""" + servlets = RoomBase.servlets + [sync.register_servlets] + user_id = "@sid1:red" def test_get_member_list(self): @@ -397,6 +399,86 @@ class RoomsMemberListTestCase(RoomBase): channel = self.make_request("GET", "/rooms/%s/members" % room_id) self.assertEquals(403, channel.code, msg=channel.result["body"]) + def test_get_member_list_no_permission_with_at_token(self): + """ + Tests that a stranger to the room cannot get the member list + (in the case that they use an at token). + """ + room_id = self.helper.create_room_as("@someone.else:red") + + # first sync to get an at token + channel = self.make_request("GET", "/sync") + self.assertEquals(200, channel.code) + sync_token = channel.json_body["next_batch"] + + # check that permission is denied for @sid1:red to get the + # memberships of @someone.else:red's room. + channel = self.make_request( + "GET", + f"/rooms/{room_id}/members?at={sync_token}", + ) + self.assertEquals(403, channel.code, msg=channel.result["body"]) + + def test_get_member_list_no_permission_former_member(self): + """ + Tests that a former member of the room can not get the member list. + """ + # create a room, invite the user and the user joins + room_id = self.helper.create_room_as("@alice:red") + self.helper.invite(room_id, "@alice:red", self.user_id) + self.helper.join(room_id, self.user_id) + + # check that the user can see the member list to start with + channel = self.make_request("GET", "/rooms/%s/members" % room_id) + self.assertEquals(200, channel.code, msg=channel.result["body"]) + + # ban the user + self.helper.change_membership(room_id, "@alice:red", self.user_id, "ban") + + # check the user can no longer see the member list + channel = self.make_request("GET", "/rooms/%s/members" % room_id) + self.assertEquals(403, channel.code, msg=channel.result["body"]) + + def test_get_member_list_no_permission_former_member_with_at_token(self): + """ + Tests that a former member of the room can not get the member list + (in the case that they use an at token). + """ + # create a room, invite the user and the user joins + room_id = self.helper.create_room_as("@alice:red") + self.helper.invite(room_id, "@alice:red", self.user_id) + self.helper.join(room_id, self.user_id) + + # sync to get an at token + channel = self.make_request("GET", "/sync") + self.assertEquals(200, channel.code) + sync_token = channel.json_body["next_batch"] + + # check that the user can see the member list to start with + channel = self.make_request( + "GET", "/rooms/%s/members?at=%s" % (room_id, sync_token) + ) + self.assertEquals(200, channel.code, msg=channel.result["body"]) + + # ban the user (Note: the user is actually allowed to see this event and + # state so that they know they're banned!) + self.helper.change_membership(room_id, "@alice:red", self.user_id, "ban") + + # invite a third user and let them join + self.helper.invite(room_id, "@alice:red", "@bob:red") + self.helper.join(room_id, "@bob:red") + + # now, with the original user, sync again to get a new at token + channel = self.make_request("GET", "/sync") + self.assertEquals(200, channel.code) + sync_token = channel.json_body["next_batch"] + + # check the user can no longer see the updated member list + channel = self.make_request( + "GET", "/rooms/%s/members?at=%s" % (room_id, sync_token) + ) + self.assertEquals(403, channel.code, msg=channel.result["body"]) + def test_get_member_list_mixed_memberships(self): room_creator = "@some_other_guy:red" room_id = self.helper.create_room_as(room_creator) diff --git a/tests/rest/client/v2_alpha/test_groups.py b/tests/rest/client/v2_alpha/test_groups.py new file mode 100644 index 000000000..ad0425ae6 --- /dev/null +++ b/tests/rest/client/v2_alpha/test_groups.py @@ -0,0 +1,56 @@ +# Copyright 2021 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from synapse.rest.client import groups, room + +from tests import unittest +from tests.unittest import override_config + + +class GroupsTestCase(unittest.HomeserverTestCase): + user_id = "@alice:test" + room_creator_user_id = "@bob:test" + + servlets = [room.register_servlets, groups.register_servlets] + + @override_config({"enable_group_creation": True}) + def test_rooms_limited_by_visibility(self): + group_id = "+spqr:test" + + # Alice creates a group + channel = self.make_request("POST", "/create_group", {"localpart": "spqr"}) + self.assertEquals(channel.code, 200, msg=channel.text_body) + self.assertEquals(channel.json_body, {"group_id": group_id}) + + # Bob creates a private room + room_id = self.helper.create_room_as(self.room_creator_user_id, is_public=False) + self.helper.auth_user_id = self.room_creator_user_id + self.helper.send_state( + room_id, "m.room.name", {"name": "bob's secret room"}, tok=None + ) + self.helper.auth_user_id = self.user_id + + # Alice adds the room to her group. + channel = self.make_request( + "PUT", f"/groups/{group_id}/admin/rooms/{room_id}", {} + ) + self.assertEquals(channel.code, 200, msg=channel.text_body) + self.assertEquals(channel.json_body, {}) + + # Alice now tries to retrieve the room list of the space. + channel = self.make_request("GET", f"/groups/{group_id}/rooms") + self.assertEquals(channel.code, 200, msg=channel.text_body) + self.assertEquals( + channel.json_body, {"chunk": [], "total_room_count_estimate": 0} + ) diff --git a/tests/server.py b/tests/server.py index 6fddd3b30..b861c7b86 100644 --- a/tests/server.py +++ b/tests/server.py @@ -10,9 +10,10 @@ from zope.interface import implementer from twisted.internet import address, threads, udp from twisted.internet._resolver import SimpleResolverComplexifier -from twisted.internet.defer import Deferred, fail, succeed +from twisted.internet.defer import Deferred, fail, maybeDeferred, succeed from twisted.internet.error import DNSLookupError from twisted.internet.interfaces import ( + IAddress, IHostnameResolver, IProtocol, IPullProducer, @@ -511,6 +512,9 @@ class FakeTransport: will get called back for connectionLost() notifications etc. """ + _peer_address: Optional[IAddress] = attr.ib(default=None) + """The value to be returend by getPeer""" + disconnecting = False disconnected = False connected = True @@ -519,7 +523,7 @@ class FakeTransport: autoflush = attr.ib(default=True) def getPeer(self): - return None + return self._peer_address def getHost(self): return None @@ -572,7 +576,12 @@ class FakeTransport: self.producerStreaming = streaming def _produce(): - d = self.producer.resumeProducing() + if not self.producer: + # we've been unregistered + return + # some implementations of IProducer (for example, FileSender) + # don't return a deferred. + d = maybeDeferred(self.producer.resumeProducing) d.addCallback(lambda x: self._reactor.callLater(0.1, _produce)) if not streaming: