From c0e0740bef0db661abce352afaf6c958e276f11d Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Wed, 8 May 2019 18:26:56 +0100 Subject: add options to require an access_token to GET /profile and /publicRooms on CS API (#5083) This commit adds two config options: * `restrict_public_rooms_to_local_users` Requires auth to fetch the public rooms directory through the CS API and disables fetching it through the federation API. * `require_auth_for_profile_requests` When set to `true`, requires that requests to `/profile` over the CS API are authenticated, and only returns the user's profile if the requester shares a room with the profile's owner, as per MSC1301. MSC1301 also specifies a behaviour for federation (only returning the profile if the server asking for it shares a room with the profile's owner), but that's currently really non-trivial to do in a not too expensive way. Next step is writing down a MSC that allows a HS to specify which user sent the profile query. In this implementation, Synapse won't send a profile query over federation if it doesn't believe it already shares a room with the profile's owner, though. Groups have been intentionally omitted from this commit. --- synapse/handlers/profile.py | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) (limited to 'synapse/handlers/profile.py') diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index a65c98ff5c..91fc718ff8 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -53,6 +53,7 @@ class BaseProfileHandler(BaseHandler): @defer.inlineCallbacks def get_profile(self, user_id): target_user = UserID.from_string(user_id) + if self.hs.is_mine(target_user): try: displayname = yield self.store.get_profile_displayname( @@ -283,6 +284,48 @@ class BaseProfileHandler(BaseHandler): room_id, str(e) ) + @defer.inlineCallbacks + def check_profile_query_allowed(self, target_user, requester=None): + """Checks whether a profile query is allowed. If the + 'require_auth_for_profile_requests' config flag is set to True and a + 'requester' is provided, the query is only allowed if the two users + share a room. + + Args: + target_user (UserID): The owner of the queried profile. + requester (None|UserID): The user querying for the profile. + + Raises: + SynapseError(403): The two users share no room, or ne user couldn't + be found to be in any room the server is in, and therefore the query + is denied. + """ + # Implementation of MSC1301: don't allow looking up profiles if the + # requester isn't in the same room as the target. We expect requester to + # be None when this function is called outside of a profile query, e.g. + # when building a membership event. In this case, we must allow the + # lookup. + if not self.hs.config.require_auth_for_profile_requests or not requester: + return + + try: + requester_rooms = yield self.store.get_rooms_for_user( + requester.to_string() + ) + target_user_rooms = yield self.store.get_rooms_for_user( + target_user.to_string(), + ) + + # Check if the room lists have no elements in common. + if requester_rooms.isdisjoint(target_user_rooms): + raise SynapseError(403, "Profile isn't available", Codes.FORBIDDEN) + except StoreError as e: + if e.code == 404: + # This likely means that one of the users doesn't exist, + # so we act as if we couldn't find the profile. + raise SynapseError(403, "Profile isn't available", Codes.FORBIDDEN) + raise + class MasterProfileHandler(BaseProfileHandler): PROFILE_UPDATE_MS = 60 * 1000 -- cgit 1.5.1 From d16c6375fe39deaafd70b151e496f5e15fd7b29c Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Sat, 1 Jun 2019 10:42:33 +0100 Subject: Limit displaynames and avatar URLs These end up in join events everywhere, so let's limit them. Fixes #5079 --- changelog.d/5309.bugfix | 1 + synapse/handlers/profile.py | 13 +++++++++++++ synapse/handlers/register.py | 2 ++ 3 files changed, 16 insertions(+) create mode 100644 changelog.d/5309.bugfix (limited to 'synapse/handlers/profile.py') diff --git a/changelog.d/5309.bugfix b/changelog.d/5309.bugfix new file mode 100644 index 0000000000..97b3527266 --- /dev/null +++ b/changelog.d/5309.bugfix @@ -0,0 +1 @@ +Prevent users from setting huge displaynames and avatar URLs. diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 91fc718ff8..a5fc6c5dbf 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -31,6 +31,9 @@ from ._base import BaseHandler logger = logging.getLogger(__name__) +MAX_DISPLAYNAME_LEN = 100 +MAX_AVATAR_URL_LEN = 1000 + class BaseProfileHandler(BaseHandler): """Handles fetching and updating user profile information. @@ -162,6 +165,11 @@ class BaseProfileHandler(BaseHandler): if not by_admin and target_user != requester.user: raise AuthError(400, "Cannot set another user's displayname") + if len(new_displayname) > MAX_DISPLAYNAME_LEN: + raise SynapseError( + 400, "Displayname is too long (max %i)" % (MAX_DISPLAYNAME_LEN, ), + ) + if new_displayname == '': new_displayname = None @@ -217,6 +225,11 @@ class BaseProfileHandler(BaseHandler): if not by_admin and target_user != requester.user: raise AuthError(400, "Cannot set another user's avatar_url") + if len(new_avatar_url) > MAX_AVATAR_URL_LEN: + raise SynapseError( + 400, "Avatar URL is too long (max %i)" % (MAX_AVATAR_URL_LEN, ), + ) + yield self.store.set_profile_avatar_url( target_user.localpart, new_avatar_url ) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index e83ee24f10..9a388ea013 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -531,6 +531,8 @@ class RegistrationHandler(BaseHandler): A tuple of (user_id, access_token). Raises: RegistrationError if there was a problem registering. + + NB this is only used in tests. TODO: move it to the test package! """ if localpart is None: raise SynapseError(400, "Request must include user id") -- cgit 1.5.1 From a46ef1e3a4eea55919d86da322628a0713e6ba2d Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 7 Jun 2019 10:29:35 +0100 Subject: Handle HttpResponseException when using federation client. Otherwise we just log exceptions everywhere. --- synapse/groups/attestations.py | 4 ++-- synapse/handlers/groups_local.py | 4 +--- synapse/handlers/profile.py | 29 ++++++++++++++++------------- 3 files changed, 19 insertions(+), 18 deletions(-) (limited to 'synapse/handlers/profile.py') diff --git a/synapse/groups/attestations.py b/synapse/groups/attestations.py index e5dda1975f..cacc6026fa 100644 --- a/synapse/groups/attestations.py +++ b/synapse/groups/attestations.py @@ -42,7 +42,7 @@ from signedjson.sign import sign_json from twisted.internet import defer -from synapse.api.errors import RequestSendFailed, SynapseError +from synapse.api.errors import HttpResponseException, RequestSendFailed, SynapseError from synapse.metrics.background_process_metrics import run_as_background_process from synapse.types import get_domain_from_id from synapse.util.logcontext import run_in_background @@ -194,7 +194,7 @@ class GroupAttestionRenewer(object): yield self.store.update_attestation_renewal( group_id, user_id, attestation ) - except RequestSendFailed as e: + except (RequestSendFailed, HttpResponseException) as e: logger.warning( "Failed to renew attestation of %r in %r: %s", user_id, group_id, e, diff --git a/synapse/handlers/groups_local.py b/synapse/handlers/groups_local.py index 02c508acec..f60ace02e8 100644 --- a/synapse/handlers/groups_local.py +++ b/synapse/handlers/groups_local.py @@ -49,9 +49,7 @@ def _create_rerouter(func_name): def http_response_errback(failure): failure.trap(HttpResponseException) e = failure.value - if e.code == 403: - raise e.to_synapse_error() - return failure + raise e.to_synapse_error() def request_failed_errback(failure): failure.trap(RequestSendFailed) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index a5fc6c5dbf..3e04233394 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -15,12 +15,15 @@ import logging +from six import raise_from + from twisted.internet import defer from synapse.api.errors import ( AuthError, - CodeMessageException, Codes, + HttpResponseException, + RequestSendFailed, StoreError, SynapseError, ) @@ -85,10 +88,10 @@ class BaseProfileHandler(BaseHandler): ignore_backoff=True, ) defer.returnValue(result) - except CodeMessageException as e: - if e.code != 404: - logger.exception("Failed to get displayname") - raise + except RequestSendFailed as e: + raise_from(SynapseError(502, "Failed to fetch profile"), e) + except HttpResponseException as e: + raise e.to_synapse_error() @defer.inlineCallbacks def get_profile_from_cache(self, user_id): @@ -142,10 +145,10 @@ class BaseProfileHandler(BaseHandler): }, ignore_backoff=True, ) - except CodeMessageException as e: - if e.code != 404: - logger.exception("Failed to get displayname") - raise + except RequestSendFailed as e: + raise_from(SynapseError(502, "Failed to fetch profile"), e) + except HttpResponseException as e: + raise e.to_synapse_error() defer.returnValue(result["displayname"]) @@ -208,10 +211,10 @@ class BaseProfileHandler(BaseHandler): }, ignore_backoff=True, ) - except CodeMessageException as e: - if e.code != 404: - logger.exception("Failed to get avatar_url") - raise + except RequestSendFailed as e: + raise_from(SynapseError(502, "Failed to fetch profile"), e) + except HttpResponseException as e: + raise e.to_synapse_error() defer.returnValue(result["avatar_url"]) -- cgit 1.5.1