From 95b3f952fa43e51feae166fa1678761c5e32d900 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 26 Jan 2022 12:02:54 +0000 Subject: Add a config flag to inhibit `M_USER_IN_USE` during registration (#11743) This is mostly motivated by the tchap use case, where usernames are automatically generated from the user's email address (in a way that allows figuring out the email address from the username). Therefore, it's an issue if we respond to requests on /register and /register/available with M_USER_IN_USE, because it can potentially leak email addresses (which include the user's real name and place of work). This commit adds a flag to inhibit the M_USER_IN_USE errors that are raised both by /register/available, and when providing a username early into the registration process. This error will still be raised if the user completes the registration process but the username conflicts. This is particularly useful when using modules (https://github.com/matrix-org/synapse/pull/11790 adds a module callback to set the username of users at registration) or SSO, since they can ensure the username is unique. More context is available in the PR that introduced this behaviour to synapse-dinsic: matrix-org/synapse-dinsic#48 - as well as the issue in the matrix-dinsic repo: matrix-org/matrix-dinsic#476 --- docs/sample_config.yaml | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'docs/sample_config.yaml') diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 1b86d0295d..b38e6d6c88 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1428,6 +1428,16 @@ account_threepid_delegates: # #auto_join_rooms_for_guests: false +# Whether to inhibit errors raised when registering a new account if the user ID +# already exists. If turned on, that requests to /register/available will always +# show a user ID as available, and Synapse won't raise an error when starting +# a registration with a user ID that already exists. However, Synapse will still +# raise an error if the registration completes and the username conflicts. +# +# Defaults to false. +# +#inhibit_user_in_use_error: true + ## Metrics ### -- cgit 1.5.1 From fd65139714433763e8ef5be6017ab6ba5988e766 Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 27 Jan 2022 11:06:29 +0100 Subject: Fix some indentation inconsistencies in the sample config (modules) (#11838) --- changelog.d/11838.misc | 1 + docs/sample_config.yaml | 10 +++++----- synapse/config/modules.py | 10 +++++----- 3 files changed, 11 insertions(+), 10 deletions(-) create mode 100644 changelog.d/11838.misc (limited to 'docs/sample_config.yaml') diff --git a/changelog.d/11838.misc b/changelog.d/11838.misc new file mode 100644 index 0000000000..4747bbe88b --- /dev/null +++ b/changelog.d/11838.misc @@ -0,0 +1 @@ +Fix some indentation inconsistencies in the sample config. \ No newline at end of file diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index b38e6d6c88..abf28e4490 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -41,11 +41,11 @@ # documentation on how to configure or create custom modules for Synapse. # modules: - # - module: my_super_module.MySuperClass - # config: - # do_thing: true - # - module: my_other_super_module.SomeClass - # config: {} + #- module: my_super_module.MySuperClass + # config: + # do_thing: true + #- module: my_other_super_module.SomeClass + # config: {} ## Server ## diff --git a/synapse/config/modules.py b/synapse/config/modules.py index 85fb05890d..2ef02b8f55 100644 --- a/synapse/config/modules.py +++ b/synapse/config/modules.py @@ -41,9 +41,9 @@ class ModulesConfig(Config): # documentation on how to configure or create custom modules for Synapse. # modules: - # - module: my_super_module.MySuperClass - # config: - # do_thing: true - # - module: my_other_super_module.SomeClass - # config: {} + #- module: my_super_module.MySuperClass + # config: + # do_thing: true + #- module: my_other_super_module.SomeClass + # config: {} """ -- cgit 1.5.1 From bf60da1a60096fac5fb778b732ff2214862ac808 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 28 Jan 2022 14:41:33 +0000 Subject: Configurable limits on avatars (#11846) Only allow files which file size and content types match configured limits to be set as avatar. Most of the inspiration from the non-test code comes from matrix-org/synapse-dinsic#19 --- changelog.d/11846.feature | 1 + docs/sample_config.yaml | 14 ++++ synapse/config/server.py | 27 +++++++ synapse/handlers/profile.py | 67 ++++++++++++++++ synapse/handlers/room_member.py | 6 ++ tests/handlers/test_profile.py | 94 ++++++++++++++++++++++- tests/rest/client/test_profile.py | 156 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 363 insertions(+), 2 deletions(-) create mode 100644 changelog.d/11846.feature (limited to 'docs/sample_config.yaml') diff --git a/changelog.d/11846.feature b/changelog.d/11846.feature new file mode 100644 index 0000000000..fcf6affdb5 --- /dev/null +++ b/changelog.d/11846.feature @@ -0,0 +1 @@ +Allow configuring a maximum file size as well as a list of allowed content types for avatars. diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index abf28e4490..689b207fc0 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -471,6 +471,20 @@ limit_remote_rooms: # #allow_per_room_profiles: false +# The largest allowed file size for a user avatar. Defaults to no restriction. +# +# Note that user avatar changes will not work if this is set without +# using Synapse's media repository. +# +#max_avatar_size: 10M + +# The MIME types allowed for user avatars. Defaults to no restriction. +# +# Note that user avatar changes will not work if this is set without +# using Synapse's media repository. +# +#allowed_avatar_mimetypes: ["image/png", "image/jpeg", "image/gif"] + # How long to keep redacted events in unredacted form in the database. After # this period redacted events get replaced with their redacted form in the DB. # diff --git a/synapse/config/server.py b/synapse/config/server.py index f200d0c1f1..a460cf25b4 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -489,6 +489,19 @@ class ServerConfig(Config): # events with profile information that differ from the target's global profile. self.allow_per_room_profiles = config.get("allow_per_room_profiles", True) + # The maximum size an avatar can have, in bytes. + self.max_avatar_size = config.get("max_avatar_size") + if self.max_avatar_size is not None: + self.max_avatar_size = self.parse_size(self.max_avatar_size) + + # The MIME types allowed for an avatar. + self.allowed_avatar_mimetypes = config.get("allowed_avatar_mimetypes") + if self.allowed_avatar_mimetypes and not isinstance( + self.allowed_avatar_mimetypes, + list, + ): + raise ConfigError("allowed_avatar_mimetypes must be a list") + self.listeners = [parse_listener_def(x) for x in config.get("listeners", [])] # no_tls is not really supported any more, but let's grandfather it in @@ -1168,6 +1181,20 @@ class ServerConfig(Config): # #allow_per_room_profiles: false + # The largest allowed file size for a user avatar. Defaults to no restriction. + # + # Note that user avatar changes will not work if this is set without + # using Synapse's media repository. + # + #max_avatar_size: 10M + + # The MIME types allowed for user avatars. Defaults to no restriction. + # + # Note that user avatar changes will not work if this is set without + # using Synapse's media repository. + # + #allowed_avatar_mimetypes: ["image/png", "image/jpeg", "image/gif"] + # How long to keep redacted events in unredacted form in the database. After # this period redacted events get replaced with their redacted form in the DB. # diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 6b5a6ded8b..36e3ad2ba9 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -31,6 +31,8 @@ from synapse.types import ( create_requester, get_domain_from_id, ) +from synapse.util.caches.descriptors import cached +from synapse.util.stringutils import parse_and_validate_mxc_uri if TYPE_CHECKING: from synapse.server import HomeServer @@ -64,6 +66,11 @@ class ProfileHandler: self.user_directory_handler = hs.get_user_directory_handler() self.request_ratelimiter = hs.get_request_ratelimiter() + self.max_avatar_size = hs.config.server.max_avatar_size + self.allowed_avatar_mimetypes = hs.config.server.allowed_avatar_mimetypes + + self.server_name = hs.config.server.server_name + if hs.config.worker.run_background_tasks: self.clock.looping_call( self._update_remote_profile_cache, self.PROFILE_UPDATE_MS @@ -286,6 +293,9 @@ class ProfileHandler: 400, "Avatar URL is too long (max %i)" % (MAX_AVATAR_URL_LEN,) ) + if not await self.check_avatar_size_and_mime_type(new_avatar_url): + raise SynapseError(403, "This avatar is not allowed", Codes.FORBIDDEN) + avatar_url_to_set: Optional[str] = new_avatar_url if new_avatar_url == "": avatar_url_to_set = None @@ -307,6 +317,63 @@ class ProfileHandler: await self._update_join_states(requester, target_user) + @cached() + async def check_avatar_size_and_mime_type(self, mxc: str) -> bool: + """Check that the size and content type of the avatar at the given MXC URI are + within the configured limits. + + Args: + mxc: The MXC URI at which the avatar can be found. + + Returns: + A boolean indicating whether the file can be allowed to be set as an avatar. + """ + if not self.max_avatar_size and not self.allowed_avatar_mimetypes: + return True + + server_name, _, media_id = parse_and_validate_mxc_uri(mxc) + + if server_name == self.server_name: + media_info = await self.store.get_local_media(media_id) + else: + media_info = await self.store.get_cached_remote_media(server_name, media_id) + + if media_info is None: + # Both configuration options need to access the file's metadata, and + # retrieving remote avatars just for this becomes a bit of a faff, especially + # if e.g. the file is too big. It's also generally safe to assume most files + # used as avatar are uploaded locally, or if the upload didn't happen as part + # of a PUT request on /avatar_url that the file was at least previewed by the + # user locally (and therefore downloaded to the remote media cache). + logger.warning("Forbidding avatar change to %s: avatar not on server", mxc) + return False + + if self.max_avatar_size: + # Ensure avatar does not exceed max allowed avatar size + if media_info["media_length"] > self.max_avatar_size: + logger.warning( + "Forbidding avatar change to %s: %d bytes is above the allowed size " + "limit", + mxc, + media_info["media_length"], + ) + return False + + if self.allowed_avatar_mimetypes: + # Ensure the avatar's file type is allowed + if ( + self.allowed_avatar_mimetypes + and media_info["media_type"] not in self.allowed_avatar_mimetypes + ): + logger.warning( + "Forbidding avatar change to %s: mimetype %s not allowed", + mxc, + media_info["media_type"], + ) + return False + + return True + async def on_profile_query(self, args: JsonDict) -> JsonDict: """Handles federation profile query requests.""" diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 6aa910dd10..3dd5e1b6e4 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -590,6 +590,12 @@ class RoomMemberHandler(metaclass=abc.ABCMeta): errcode=Codes.BAD_JSON, ) + if "avatar_url" in content: + if not await self.profile_handler.check_avatar_size_and_mime_type( + content["avatar_url"], + ): + raise SynapseError(403, "This avatar is not allowed", Codes.FORBIDDEN) + # The event content should *not* include the authorising user as # it won't be properly signed. Strip it out since it might come # back from a client updating a display name / avatar. diff --git a/tests/handlers/test_profile.py b/tests/handlers/test_profile.py index c153018fd8..60235e5699 100644 --- a/tests/handlers/test_profile.py +++ b/tests/handlers/test_profile.py @@ -11,12 +11,13 @@ # 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 Any, Dict from unittest.mock import Mock import synapse.types from synapse.api.errors import AuthError, SynapseError from synapse.rest import admin +from synapse.server import HomeServer from synapse.types import UserID from tests import unittest @@ -46,7 +47,7 @@ class ProfileTestCase(unittest.HomeserverTestCase): ) return hs - def prepare(self, reactor, clock, hs): + def prepare(self, reactor, clock, hs: HomeServer): self.store = hs.get_datastore() self.frank = UserID.from_string("@1234abcd:test") @@ -248,3 +249,92 @@ class ProfileTestCase(unittest.HomeserverTestCase): ), SynapseError, ) + + def test_avatar_constraints_no_config(self): + """Tests that the method to check an avatar against configured constraints skips + all of its check if no constraint is configured. + """ + # The first check that's done by this method is whether the file exists; if we + # don't get an error on a non-existing file then it means all of the checks were + # successfully skipped. + res = self.get_success( + self.handler.check_avatar_size_and_mime_type("mxc://test/unknown_file") + ) + self.assertTrue(res) + + @unittest.override_config({"max_avatar_size": 50}) + def test_avatar_constraints_missing(self): + """Tests that an avatar isn't allowed if the file at the given MXC URI couldn't + be found. + """ + res = self.get_success( + self.handler.check_avatar_size_and_mime_type("mxc://test/unknown_file") + ) + self.assertFalse(res) + + @unittest.override_config({"max_avatar_size": 50}) + def test_avatar_constraints_file_size(self): + """Tests that a file that's above the allowed file size is forbidden but one + that's below it is allowed. + """ + self._setup_local_files( + { + "small": {"size": 40}, + "big": {"size": 60}, + } + ) + + res = self.get_success( + self.handler.check_avatar_size_and_mime_type("mxc://test/small") + ) + self.assertTrue(res) + + res = self.get_success( + self.handler.check_avatar_size_and_mime_type("mxc://test/big") + ) + self.assertFalse(res) + + @unittest.override_config({"allowed_avatar_mimetypes": ["image/png"]}) + def test_avatar_constraint_mime_type(self): + """Tests that a file with an unauthorised MIME type is forbidden but one with + an authorised content type is allowed. + """ + self._setup_local_files( + { + "good": {"mimetype": "image/png"}, + "bad": {"mimetype": "application/octet-stream"}, + } + ) + + res = self.get_success( + self.handler.check_avatar_size_and_mime_type("mxc://test/good") + ) + self.assertTrue(res) + + res = self.get_success( + self.handler.check_avatar_size_and_mime_type("mxc://test/bad") + ) + self.assertFalse(res) + + def _setup_local_files(self, names_and_props: Dict[str, Dict[str, Any]]): + """Stores metadata about files in the database. + + Args: + names_and_props: A dictionary with one entry per file, with the key being the + file's name, and the value being a dictionary of properties. Supported + properties are "mimetype" (for the file's type) and "size" (for the + file's size). + """ + store = self.hs.get_datastore() + + for name, props in names_and_props.items(): + self.get_success( + store.store_local_media( + media_id=name, + media_type=props.get("mimetype", "image/png"), + time_now_ms=self.clock.time_msec(), + upload_name=None, + media_length=props.get("size", 50), + user_id=UserID.from_string("@rin:test"), + ) + ) diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index 2860579c2e..ead883ded8 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -13,8 +13,12 @@ # limitations under the License. """Tests REST events for /profile paths.""" +from typing import Any, Dict + +from synapse.api.errors import Codes from synapse.rest import admin from synapse.rest.client import login, profile, room +from synapse.types import UserID from tests import unittest @@ -25,6 +29,7 @@ class ProfileTestCase(unittest.HomeserverTestCase): admin.register_servlets_for_client_rest_resource, login.register_servlets, profile.register_servlets, + room.register_servlets, ] def make_homeserver(self, reactor, clock): @@ -150,6 +155,157 @@ class ProfileTestCase(unittest.HomeserverTestCase): self.assertEqual(channel.code, 200, channel.result) return channel.json_body.get("avatar_url") + @unittest.override_config({"max_avatar_size": 50}) + def test_avatar_size_limit_global(self): + """Tests that the maximum size limit for avatars is enforced when updating a + global profile. + """ + self._setup_local_files( + { + "small": {"size": 40}, + "big": {"size": 60}, + } + ) + + channel = self.make_request( + "PUT", + f"/profile/{self.owner}/avatar_url", + content={"avatar_url": "mxc://test/big"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 403, channel.result) + self.assertEqual( + channel.json_body["errcode"], Codes.FORBIDDEN, channel.json_body + ) + + channel = self.make_request( + "PUT", + f"/profile/{self.owner}/avatar_url", + content={"avatar_url": "mxc://test/small"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + @unittest.override_config({"max_avatar_size": 50}) + def test_avatar_size_limit_per_room(self): + """Tests that the maximum size limit for avatars is enforced when updating a + per-room profile. + """ + self._setup_local_files( + { + "small": {"size": 40}, + "big": {"size": 60}, + } + ) + + room_id = self.helper.create_room_as(tok=self.owner_tok) + + channel = self.make_request( + "PUT", + f"/rooms/{room_id}/state/m.room.member/{self.owner}", + content={"membership": "join", "avatar_url": "mxc://test/big"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 403, channel.result) + self.assertEqual( + channel.json_body["errcode"], Codes.FORBIDDEN, channel.json_body + ) + + channel = self.make_request( + "PUT", + f"/rooms/{room_id}/state/m.room.member/{self.owner}", + content={"membership": "join", "avatar_url": "mxc://test/small"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + @unittest.override_config({"allowed_avatar_mimetypes": ["image/png"]}) + def test_avatar_allowed_mime_type_global(self): + """Tests that the MIME type whitelist for avatars is enforced when updating a + global profile. + """ + self._setup_local_files( + { + "good": {"mimetype": "image/png"}, + "bad": {"mimetype": "application/octet-stream"}, + } + ) + + channel = self.make_request( + "PUT", + f"/profile/{self.owner}/avatar_url", + content={"avatar_url": "mxc://test/bad"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 403, channel.result) + self.assertEqual( + channel.json_body["errcode"], Codes.FORBIDDEN, channel.json_body + ) + + channel = self.make_request( + "PUT", + f"/profile/{self.owner}/avatar_url", + content={"avatar_url": "mxc://test/good"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + @unittest.override_config({"allowed_avatar_mimetypes": ["image/png"]}) + def test_avatar_allowed_mime_type_per_room(self): + """Tests that the MIME type whitelist for avatars is enforced when updating a + per-room profile. + """ + self._setup_local_files( + { + "good": {"mimetype": "image/png"}, + "bad": {"mimetype": "application/octet-stream"}, + } + ) + + room_id = self.helper.create_room_as(tok=self.owner_tok) + + channel = self.make_request( + "PUT", + f"/rooms/{room_id}/state/m.room.member/{self.owner}", + content={"membership": "join", "avatar_url": "mxc://test/bad"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 403, channel.result) + self.assertEqual( + channel.json_body["errcode"], Codes.FORBIDDEN, channel.json_body + ) + + channel = self.make_request( + "PUT", + f"/rooms/{room_id}/state/m.room.member/{self.owner}", + content={"membership": "join", "avatar_url": "mxc://test/good"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + def _setup_local_files(self, names_and_props: Dict[str, Dict[str, Any]]): + """Stores metadata about files in the database. + + Args: + names_and_props: A dictionary with one entry per file, with the key being the + file's name, and the value being a dictionary of properties. Supported + properties are "mimetype" (for the file's type) and "size" (for the + file's size). + """ + store = self.hs.get_datastore() + + for name, props in names_and_props.items(): + self.get_success( + store.store_local_media( + media_id=name, + media_type=props.get("mimetype", "image/png"), + time_now_ms=self.clock.time_msec(), + upload_name=None, + media_length=props.get("size", 50), + user_id=UserID.from_string("@rin:test"), + ) + ) + class ProfilesRestrictedTestCase(unittest.HomeserverTestCase): -- cgit 1.5.1