From 7a523b3007673eb4e5bff29a3b265c395b28eeba Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 23 Mar 2020 09:08:23 -0400 Subject: [PATCH] Automatically calculate the comparator used to ensure that operation has not changed. --- synapse/handlers/auth.py | 32 +++++++++++++++--------- synapse/rest/client/v2_alpha/account.py | 17 +++---------- synapse/rest/client/v2_alpha/devices.py | 10 ++------ synapse/rest/client/v2_alpha/keys.py | 5 +--- synapse/rest/client/v2_alpha/register.py | 2 +- 5 files changed, 28 insertions(+), 38 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 790b3c5661..be072f3f7e 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -127,9 +127,9 @@ class AuthHandler(BaseHandler): def validate_user_via_ui_auth( self, requester: Requester, + request: SynapseRequest, request_body: Dict[str, Any], clientip: str, - action_data, ): """ Checks that the user is who they claim to be, via a UI auth. @@ -141,13 +141,12 @@ class AuthHandler(BaseHandler): Args: requester: The user, as given by the access token + request: The request sent by the client. + request_body: The body of the request sent by the client clientip: The IP address of the client. - action_data: An opaque object that should be provided initially and at the - end to ensure the request is not modified during a session. - Returns: defer.Deferred[dict]: the parameters for this request (which may have been given only in a previous call). @@ -180,7 +179,7 @@ class AuthHandler(BaseHandler): try: result, params, _ = yield self.check_auth( - flows, request_body, clientip, action_data + flows, request, request_body, clientip ) except LoginError: # Update the ratelimite to say we failed (`can_do_action` doesn't raise). @@ -222,9 +221,9 @@ class AuthHandler(BaseHandler): def check_auth( self, flows: List[List[str]], + request: SynapseRequest, clientdict: Dict[str, Any], clientip: str, - action_data, ): """ Takes a dictionary sent by the client in the login / registration @@ -244,6 +243,8 @@ class AuthHandler(BaseHandler): strings representing auth-types. At least one full flow must be completed in order for auth to be successful. + request: The request sent by the client. + clientdict: The dictionary from the client root level, not the 'auth' key: this method prompts for auth if none is sent. @@ -283,19 +284,26 @@ class AuthHandler(BaseHandler): # email auth link on there). It's probably too open to abuse # because it lets unauthenticated clients store arbitrary objects # on a homeserver. - # Revisit: Assumimg the REST APIs do sensible validation, the data + # Revisit: Assuming the REST APIs do sensible validation, the data # isn't arbintrary. session["clientdict"] = clientdict self._save_session(session) elif "clientdict" in session: clientdict = session["clientdict"] - # If ui_auth exists in the session this is a returning UI auth request. - # Validate that none of the requested information has changed. + # Ensure that the queried operation does not vary between stages of + # the UI authentication session. This is done by generating a stable + # comparator based on the URI, method, and body (minus the auth dict) + # and storing it during the initial query. Subsequent queries ensure + # that this comparator has not changed. + comparator = (request.uri, request.method, clientdict) if "ui_auth" not in session: - session["ui_auth"] = action_data - elif session["ui_auth"] != action_data: - raise SynapseError(403, "Foobar") + session["ui_auth"] = comparator + elif session["ui_auth"] != comparator: + raise SynapseError( + 403, + "Requested operation has changed during the UI authentication session.", + ) if not authdict: raise InteractiveAuthIncompleteError( diff --git a/synapse/rest/client/v2_alpha/account.py b/synapse/rest/client/v2_alpha/account.py index b94066b444..b1249b664c 100644 --- a/synapse/rest/client/v2_alpha/account.py +++ b/synapse/rest/client/v2_alpha/account.py @@ -234,19 +234,16 @@ class PasswordRestServlet(RestServlet): if self.auth.has_access_token(request): requester = await self.auth.get_user_by_req(request) params = await self.auth_handler.validate_user_via_ui_auth( - requester, - body, - self.hs.get_ip_from_request(request), - {"operation": "modify_password", "user": requester.user.to_string()}, + requester, request, body, self.hs.get_ip_from_request(request), ) user_id = requester.user.to_string() else: requester = None result, params, _ = await self.auth_handler.check_auth( [[LoginType.EMAIL_IDENTITY]], + request, body, self.hs.get_ip_from_request(request), - {"operation": "modify_password"}, # TODO ) if LoginType.EMAIL_IDENTITY in result: @@ -314,10 +311,7 @@ class DeactivateAccountRestServlet(RestServlet): return 200, {} await self.auth_handler.validate_user_via_ui_auth( - requester, - body, - self.hs.get_ip_from_request(request), - {"operation": "deactivate", "user": requester.user.to_string()}, + requester, request, body, self.hs.get_ip_from_request(request), ) result = await self._deactivate_account_handler.deactivate_account( requester.user.to_string(), erase, id_server=body.get("id_server") @@ -665,10 +659,7 @@ class ThreepidAddRestServlet(RestServlet): assert_valid_client_secret(client_secret) await self.auth_handler.validate_user_via_ui_auth( - requester, - body, - self.hs.get_ip_from_request(request), - {"operation": "add_3pid", "user": user_id}, + requester, request, body, self.hs.get_ip_from_request(request), ) validation_session = await self.identity_handler.validate_threepid_session( diff --git a/synapse/rest/client/v2_alpha/devices.py b/synapse/rest/client/v2_alpha/devices.py index 6fe508c11b..119d979052 100644 --- a/synapse/rest/client/v2_alpha/devices.py +++ b/synapse/rest/client/v2_alpha/devices.py @@ -81,10 +81,7 @@ class DeleteDevicesRestServlet(RestServlet): assert_params_in_dict(body, ["devices"]) await self.auth_handler.validate_user_via_ui_auth( - requester, - body, - self.hs.get_ip_from_request(request), - {"operation": "delete_devices", "devices": body["devices"]}, + requester, request, body, self.hs.get_ip_from_request(request), ) await self.device_handler.delete_devices( @@ -130,10 +127,7 @@ class DeviceRestServlet(RestServlet): raise await self.auth_handler.validate_user_via_ui_auth( - requester, - body, - self.hs.get_ip_from_request(request), - {"operation": "delete_device", "device": device_id}, + requester, request, body, self.hs.get_ip_from_request(request), ) await self.device_handler.delete_device(requester.user.to_string(), device_id) diff --git a/synapse/rest/client/v2_alpha/keys.py b/synapse/rest/client/v2_alpha/keys.py index 07a983e0ff..5eb7ef35a4 100644 --- a/synapse/rest/client/v2_alpha/keys.py +++ b/synapse/rest/client/v2_alpha/keys.py @@ -263,10 +263,7 @@ class SigningKeyUploadServlet(RestServlet): body = parse_json_object_from_request(request) await self.auth_handler.validate_user_via_ui_auth( - requester, - body, - self.hs.get_ip_from_request(request), - {"operation": "add_keys", "user": user_id}, + requester, request, body, self.hs.get_ip_from_request(request), ) result = await self.e2e_keys_handler.upload_signing_keys_for_user(user_id, body) diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index d916d2b126..6963d79310 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -500,9 +500,9 @@ class RegisterRestServlet(RestServlet): auth_result, params, session_id = await self.auth_handler.check_auth( self._registration_flows, + request, body, self.hs.get_ip_from_request(request), - {"operation": "register"}, ) # Check that we're not trying to register a denied 3pid.