From 402d6039a982c9c2b83458147af119098c12cd64 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 11 Mar 2020 15:12:32 -0400 Subject: [PATCH 01/10] Add additional information to the session dictionary about what is being authenticated. --- synapse/handlers/auth.py | 31 +++++++++++++++++++++--- synapse/rest/client/v2_alpha/account.py | 20 ++++++++++++--- synapse/rest/client/v2_alpha/devices.py | 12 +++++++-- synapse/rest/client/v2_alpha/keys.py | 2 +- synapse/rest/client/v2_alpha/register.py | 6 ++++- 5 files changed, 60 insertions(+), 11 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 7860f9625e5e..088bc920efe3 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -125,7 +125,12 @@ def __init__(self, hs): @defer.inlineCallbacks def validate_user_via_ui_auth( - self, requester: Requester, request_body: Dict[str, Any], clientip: str + self, + requester: Requester, + request_body: Dict[str, Any], + clientip: str, + action_type, + action_id, ): """ Checks that the user is who they claim to be, via a UI auth. @@ -172,7 +177,9 @@ def validate_user_via_ui_auth( flows = [[login_type] for login_type in self._supported_login_types] try: - result, params, _ = yield self.check_auth(flows, request_body, clientip) + result, params, _ = yield self.check_auth( + flows, request_body, clientip, action_type, action_id + ) except LoginError: # Update the ratelimite to say we failed (`can_do_action` doesn't raise). self._failed_uia_attempts_ratelimiter.can_do_action( @@ -211,7 +218,12 @@ def get_enabled_auth_types(self): @defer.inlineCallbacks def check_auth( - self, flows: List[List[str]], clientdict: Dict[str, Any], clientip: str + self, + flows: List[List[str]], + clientdict: Dict[str, Any], + clientip: str, + action_type, + action_id, ): """ Takes a dictionary sent by the client in the login / registration @@ -277,6 +289,19 @@ def check_auth( 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. + if "ui_auth" not in session: + session["ui_auth"] = { + "action_type": action_type, + "action_id": action_id, + } + elif ( + session["ui_auth"]["action_type"] != action_type + or session["ui_auth"]["action_id"] != action_id + ): + raise SynapseError(403, "Foobar") + if not authdict: raise InteractiveAuthIncompleteError( self._auth_dict_for_flows(flows, session) diff --git a/synapse/rest/client/v2_alpha/account.py b/synapse/rest/client/v2_alpha/account.py index 631cc74cb42f..780f862163d6 100644 --- a/synapse/rest/client/v2_alpha/account.py +++ b/synapse/rest/client/v2_alpha/account.py @@ -234,13 +234,21 @@ async def on_POST(self, request): 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) + requester, + body, + self.hs.get_ip_from_request(request), + "modify_password", + "", # TODO ) user_id = requester.user.to_string() else: requester = None result, params, _ = await self.auth_handler.check_auth( - [[LoginType.EMAIL_IDENTITY]], body, self.hs.get_ip_from_request(request) + [[LoginType.EMAIL_IDENTITY]], + body, + self.hs.get_ip_from_request(request), + "modify_password", + "", # TODO ) if LoginType.EMAIL_IDENTITY in result: @@ -308,7 +316,11 @@ async def on_POST(self, request): return 200, {} await self.auth_handler.validate_user_via_ui_auth( - requester, body, self.hs.get_ip_from_request(request) + requester, + body, + self.hs.get_ip_from_request(request), + "deactivate", + requester.user.to_string(), ) result = await self._deactivate_account_handler.deactivate_account( requester.user.to_string(), erase, id_server=body.get("id_server") @@ -656,7 +668,7 @@ async def on_POST(self, request): assert_valid_client_secret(client_secret) await self.auth_handler.validate_user_via_ui_auth( - requester, body, self.hs.get_ip_from_request(request) + requester, body, self.hs.get_ip_from_request(request), "add_3pid", user_id ) 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 94ff73f384e1..2ef5ec19a63a 100644 --- a/synapse/rest/client/v2_alpha/devices.py +++ b/synapse/rest/client/v2_alpha/devices.py @@ -81,7 +81,11 @@ async def on_POST(self, request): assert_params_in_dict(body, ["devices"]) await self.auth_handler.validate_user_via_ui_auth( - requester, body, self.hs.get_ip_from_request(request) + requester, + body, + self.hs.get_ip_from_request(request), + "delete_devices", + "", # TODO ) await self.device_handler.delete_devices( @@ -127,7 +131,11 @@ async def on_DELETE(self, request, device_id): raise await self.auth_handler.validate_user_via_ui_auth( - requester, body, self.hs.get_ip_from_request(request) + requester, + body, + self.hs.get_ip_from_request(request), + "delete_device", + device_id, ) 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 f7ed4daf90a7..0e9cf8172937 100644 --- a/synapse/rest/client/v2_alpha/keys.py +++ b/synapse/rest/client/v2_alpha/keys.py @@ -263,7 +263,7 @@ async def on_POST(self, request): 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) + requester, body, self.hs.get_ip_from_request(request), "add_keys", user_id ) 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 a09189b1b469..950177075ec2 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -499,7 +499,11 @@ async def on_POST(self, request): ) auth_result, params, session_id = await self.auth_handler.check_auth( - self._registration_flows, body, self.hs.get_ip_from_request(request) + self._registration_flows, + body, + self.hs.get_ip_from_request(request), + "register", + "", # TODO ) # Check that we're not trying to register a denied 3pid. From dab5cf183655d65909c32ccbe7b90e97943b596f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 11 Mar 2020 15:12:59 -0400 Subject: [PATCH 02/10] Clear the session on successful authentication. --- synapse/handlers/auth.py | 15 +++++++++++++++ tests/rest/client/v2_alpha/test_auth.py | 2 +- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 088bc920efe3..2f5739c7335a 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -347,6 +347,10 @@ def check_auth( creds, list(clientdict), ) + + # Blow away the session so it can not be re-used. + self._invalidate_session(session["id"]) + return creds, clientdict, session["id"] ret = self._auth_dict_for_flows(flows, session) @@ -519,6 +523,17 @@ def _get_session_info(self, session_id: Optional[str]) -> dict: return self.sessions[session_id] + def _invalidate_session(self, session_id) -> None: + """Invalidate session information for session ID""" + session = self.sessions.get(session_id, None) + if session and "ui_auth" in session: + # Set the items in the ui_auth session to sentinel values that can + # never be equaled. + session["ui_auth"] = { + "action_type": object(), + "action_id": object(), + } + @defer.inlineCallbacks def get_access_token_for_user_id( self, user_id: str, device_id: Optional[str], valid_until_ms: Optional[int] diff --git a/tests/rest/client/v2_alpha/test_auth.py b/tests/rest/client/v2_alpha/test_auth.py index b6df1396ad66..3e9edc368083 100644 --- a/tests/rest/client/v2_alpha/test_auth.py +++ b/tests/rest/client/v2_alpha/test_auth.py @@ -104,7 +104,7 @@ def test_fallback_captcha(self): ) self.render(request) - # Now we should have fufilled a complete auth flow, including + # Now we should have fulfilled a complete auth flow, including # the recaptcha fallback step, we can then send a # request to the register API with the session in the authdict. request, channel = self.make_request( From 7adeb252403b3621351b95e2880b284acebd2ec9 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 20 Mar 2020 13:03:57 -0400 Subject: [PATCH 03/10] Add changelog entry. --- changelog.d/7068.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/7068.bugfix diff --git a/changelog.d/7068.bugfix b/changelog.d/7068.bugfix new file mode 100644 index 000000000000..d1693a7f2248 --- /dev/null +++ b/changelog.d/7068.bugfix @@ -0,0 +1 @@ +Ensure that a user inteactive authentication session is tied to a single request. From afe0a36920c91b76a3ef383c097b7e48f4c1f2d5 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 20 Mar 2020 13:28:20 -0400 Subject: [PATCH 04/10] Handle review feedback. --- synapse/handlers/auth.py | 26 ++++++++---------------- synapse/rest/client/v2_alpha/account.py | 14 ++++++------- synapse/rest/client/v2_alpha/devices.py | 6 ++---- synapse/rest/client/v2_alpha/keys.py | 5 ++++- synapse/rest/client/v2_alpha/register.py | 3 +-- 5 files changed, 23 insertions(+), 31 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 2f5739c7335a..d582b9571b03 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -129,8 +129,7 @@ def validate_user_via_ui_auth( requester: Requester, request_body: Dict[str, Any], clientip: str, - action_type, - action_id, + action_data, ): """ Checks that the user is who they claim to be, via a UI auth. @@ -146,6 +145,9 @@ def validate_user_via_ui_auth( 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). @@ -178,7 +180,7 @@ def validate_user_via_ui_auth( try: result, params, _ = yield self.check_auth( - flows, request_body, clientip, action_type, action_id + flows, request_body, clientip, action_data ) except LoginError: # Update the ratelimite to say we failed (`can_do_action` doesn't raise). @@ -222,8 +224,7 @@ def check_auth( flows: List[List[str]], clientdict: Dict[str, Any], clientip: str, - action_type, - action_id, + action_data, ): """ Takes a dictionary sent by the client in the login / registration @@ -292,14 +293,8 @@ def check_auth( # If ui_auth exists in the session this is a returning UI auth request. # Validate that none of the requested information has changed. if "ui_auth" not in session: - session["ui_auth"] = { - "action_type": action_type, - "action_id": action_id, - } - elif ( - session["ui_auth"]["action_type"] != action_type - or session["ui_auth"]["action_id"] != action_id - ): + session["ui_auth"] = action_data + elif session["ui_auth"] != action_data: raise SynapseError(403, "Foobar") if not authdict: @@ -529,10 +524,7 @@ def _invalidate_session(self, session_id) -> None: if session and "ui_auth" in session: # Set the items in the ui_auth session to sentinel values that can # never be equaled. - session["ui_auth"] = { - "action_type": object(), - "action_id": object(), - } + session["ui_auth"] = object() @defer.inlineCallbacks def get_access_token_for_user_id( diff --git a/synapse/rest/client/v2_alpha/account.py b/synapse/rest/client/v2_alpha/account.py index 780f862163d6..b94066b444c7 100644 --- a/synapse/rest/client/v2_alpha/account.py +++ b/synapse/rest/client/v2_alpha/account.py @@ -237,8 +237,7 @@ async def on_POST(self, request): requester, body, self.hs.get_ip_from_request(request), - "modify_password", - "", # TODO + {"operation": "modify_password", "user": requester.user.to_string()}, ) user_id = requester.user.to_string() else: @@ -247,8 +246,7 @@ async def on_POST(self, request): [[LoginType.EMAIL_IDENTITY]], body, self.hs.get_ip_from_request(request), - "modify_password", - "", # TODO + {"operation": "modify_password"}, # TODO ) if LoginType.EMAIL_IDENTITY in result: @@ -319,8 +317,7 @@ async def on_POST(self, request): requester, body, self.hs.get_ip_from_request(request), - "deactivate", - requester.user.to_string(), + {"operation": "deactivate", "user": requester.user.to_string()}, ) result = await self._deactivate_account_handler.deactivate_account( requester.user.to_string(), erase, id_server=body.get("id_server") @@ -668,7 +665,10 @@ async def on_POST(self, request): assert_valid_client_secret(client_secret) await self.auth_handler.validate_user_via_ui_auth( - requester, body, self.hs.get_ip_from_request(request), "add_3pid", user_id + requester, + body, + self.hs.get_ip_from_request(request), + {"operation": "add_3pid", "user": user_id}, ) 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 2ef5ec19a63a..6fe508c11b3d 100644 --- a/synapse/rest/client/v2_alpha/devices.py +++ b/synapse/rest/client/v2_alpha/devices.py @@ -84,8 +84,7 @@ async def on_POST(self, request): requester, body, self.hs.get_ip_from_request(request), - "delete_devices", - "", # TODO + {"operation": "delete_devices", "devices": body["devices"]}, ) await self.device_handler.delete_devices( @@ -134,8 +133,7 @@ async def on_DELETE(self, request, device_id): requester, body, self.hs.get_ip_from_request(request), - "delete_device", - device_id, + {"operation": "delete_device", "device": device_id}, ) 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 0e9cf8172937..07a983e0ff4d 100644 --- a/synapse/rest/client/v2_alpha/keys.py +++ b/synapse/rest/client/v2_alpha/keys.py @@ -263,7 +263,10 @@ async def on_POST(self, request): 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), "add_keys", user_id + requester, + body, + self.hs.get_ip_from_request(request), + {"operation": "add_keys", "user": user_id}, ) 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 950177075ec2..d916d2b1266f 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -502,8 +502,7 @@ async def on_POST(self, request): self._registration_flows, body, self.hs.get_ip_from_request(request), - "register", - "", # TODO + {"operation": "register"}, ) # Check that we're not trying to register a denied 3pid. From 90c2b6ca0cf0fed61d83a5a607fd714cd66b39b9 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 20 Mar 2020 15:56:01 -0400 Subject: [PATCH 05/10] Only clear the session once the final request comes through without an auth flow. --- synapse/handlers/auth.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index d582b9571b03..790b3c566177 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -343,8 +343,11 @@ def check_auth( list(clientdict), ) - # Blow away the session so it can not be re-used. - self._invalidate_session(session["id"]) + # If the authentication flow is complete and this is the + # subsequent request, mark this session as invalid, so it cannot + # be re-used. + if "type" not in authdict: + self._remove_session(session["id"]) return creds, clientdict, session["id"] @@ -518,13 +521,9 @@ def _get_session_info(self, session_id: Optional[str]) -> dict: return self.sessions[session_id] - def _invalidate_session(self, session_id) -> None: - """Invalidate session information for session ID""" - session = self.sessions.get(session_id, None) - if session and "ui_auth" in session: - # Set the items in the ui_auth session to sentinel values that can - # never be equaled. - session["ui_auth"] = object() + def _remove_session(self, session_id) -> None: + """Remove a session (if it exists).""" + self.sessions.pop(session_id, None) @defer.inlineCallbacks def get_access_token_for_user_id( From 7a523b3007673eb4e5bff29a3b265c395b28eeba Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 23 Mar 2020 09:08:23 -0400 Subject: [PATCH 06/10] 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 790b3c566177..be072f3f7e02 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -127,9 +127,9 @@ def __init__(self, hs): 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 @@ def validate_user_via_ui_auth( 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 @@ def validate_user_via_ui_auth( 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 @@ def get_enabled_auth_types(self): 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 @@ def check_auth( 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 @@ def check_auth( # 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 b94066b444c7..b1249b664ccf 100644 --- a/synapse/rest/client/v2_alpha/account.py +++ b/synapse/rest/client/v2_alpha/account.py @@ -234,19 +234,16 @@ async def on_POST(self, request): 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 @@ async def on_POST(self, request): 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 @@ async def on_POST(self, request): 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 6fe508c11b3d..119d9790524a 100644 --- a/synapse/rest/client/v2_alpha/devices.py +++ b/synapse/rest/client/v2_alpha/devices.py @@ -81,10 +81,7 @@ async def on_POST(self, request): 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 @@ async def on_DELETE(self, request, device_id): 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 07a983e0ff4d..5eb7ef35a4ba 100644 --- a/synapse/rest/client/v2_alpha/keys.py +++ b/synapse/rest/client/v2_alpha/keys.py @@ -263,10 +263,7 @@ async def on_POST(self, request): 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 d916d2b1266f..6963d793108f 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -500,9 +500,9 @@ async def on_POST(self, request): 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. From ac262188b6b3106175c79164c907dc8129b10b0b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 23 Mar 2020 09:10:03 -0400 Subject: [PATCH 07/10] Clarify a comment. --- synapse/handlers/auth.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index be072f3f7e02..a71e032f17b7 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -351,9 +351,9 @@ def check_auth( list(clientdict), ) - # If the authentication flow is complete and this is the - # subsequent request, mark this session as invalid, so it cannot - # be re-used. + # Once the authentication flow has completed and the final + # operation is requested, the session should be removed so it + # cannot be re-used. if "type" not in authdict: self._remove_session(session["id"]) From 5111d86e706f353a60998f14486824bdf324460b Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 23 Mar 2020 09:39:42 -0400 Subject: [PATCH 08/10] Add a test to ensure that modifying the operation fails. --- tests/rest/client/v2_alpha/test_auth.py | 66 +++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/tests/rest/client/v2_alpha/test_auth.py b/tests/rest/client/v2_alpha/test_auth.py index 3e9edc368083..624bf5ada23f 100644 --- a/tests/rest/client/v2_alpha/test_auth.py +++ b/tests/rest/client/v2_alpha/test_auth.py @@ -115,3 +115,69 @@ def test_fallback_captcha(self): # We're given a registered user. self.assertEqual(channel.json_body["user_id"], "@user:test") + + def test_cannot_change_operation(self): + """ + The initial requested operation cannot be modified during the user interactive authentication session. + """ + + # Make the initial request to register. (Later on a different password + # will be used.) + request, channel = self.make_request( + "POST", + "register", + {"username": "user", "type": "m.login.password", "password": "bar"}, + ) + self.render(request) + + # Returns a 401 as per the spec + self.assertEqual(request.code, 401) + # Grab the session + session = channel.json_body["session"] + # Assert our configured public key is being given + self.assertEqual( + channel.json_body["params"]["m.login.recaptcha"]["public_key"], "brokencake" + ) + + request, channel = self.make_request( + "GET", "auth/m.login.recaptcha/fallback/web?session=" + session + ) + self.render(request) + self.assertEqual(request.code, 200) + + request, channel = self.make_request( + "POST", + "auth/m.login.recaptcha/fallback/web?session=" + + session + + "&g-recaptcha-response=a", + ) + self.render(request) + self.assertEqual(request.code, 200) + + # The recaptcha handler is called with the response given + attempts = self.recaptcha_checker.recaptcha_attempts + self.assertEqual(len(attempts), 1) + self.assertEqual(attempts[0][0]["response"], "a") + + # also complete the dummy auth + request, channel = self.make_request( + "POST", "register", {"auth": {"session": session, "type": "m.login.dummy"}} + ) + self.render(request) + + # Now we should have fulfilled a complete auth flow, including + # the recaptcha fallback step. Make the initial request again, but + # with a different password. This causes the request to fail since the + # operaiton was modified during the ui auth session. + request, channel = self.make_request( + "POST", + "register", + { + "username": "user", + "type": "m.login.password", + "password": "foo", # Note this doesn't match the original request. + "auth": {"session": session}, + }, + ) + self.render(request) + self.assertEqual(channel.code, 403) From d0ece9edfd9c4e36de88bb591ee64158099b9fb5 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 23 Mar 2020 10:50:30 -0400 Subject: [PATCH 09/10] Update a test to provide the same data throughout the UI Auth. --- tests/test_terms_auth.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_terms_auth.py b/tests/test_terms_auth.py index 5ec5d2b358fd..a3f98a141252 100644 --- a/tests/test_terms_auth.py +++ b/tests/test_terms_auth.py @@ -53,7 +53,8 @@ def prepare(self, reactor, clock, hs): def test_ui_auth(self): # Do a UI auth request - request, channel = self.make_request(b"POST", self.url, b"{}") + request_data = json.dumps({"username": "kermit", "password": "monkey"}) + request, channel = self.make_request(b"POST", self.url, request_data) self.render(request) self.assertEquals(channel.result["code"], b"401", channel.result) From ebc7245adedafc4e75ca28dc8ffc741ff0391a07 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 25 Mar 2020 15:17:35 -0400 Subject: [PATCH 10/10] Remove the code to delete a session after it is used. --- synapse/handlers/auth.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index a71e032f17b7..2ce1425dfaec 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -351,12 +351,6 @@ def check_auth( list(clientdict), ) - # Once the authentication flow has completed and the final - # operation is requested, the session should be removed so it - # cannot be re-used. - if "type" not in authdict: - self._remove_session(session["id"]) - return creds, clientdict, session["id"] ret = self._auth_dict_for_flows(flows, session) @@ -529,10 +523,6 @@ def _get_session_info(self, session_id: Optional[str]) -> dict: return self.sessions[session_id] - def _remove_session(self, session_id) -> None: - """Remove a session (if it exists).""" - self.sessions.pop(session_id, None) - @defer.inlineCallbacks def get_access_token_for_user_id( self, user_id: str, device_id: Optional[str], valid_until_ms: Optional[int]