From 9dc6f3075aea7c76c3d6a201f8a78ace76f99a3e Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 20 May 2020 09:48:03 -0400 Subject: [PATCH] Hash passwords earlier in the password reset process (#7538) This now matches the logic of the registration process as modified in 56db0b1365965c02ff539193e26c333b7f70d101 / #7523. --- changelog.d/7538.bugfix | 1 + synapse/handlers/set_password.py | 5 +---- synapse/rest/admin/users.py | 13 +++++++++++-- synapse/rest/client/v2_alpha/account.py | 21 ++++++++++++++++++--- synapse/rest/client/v2_alpha/register.py | 4 ++-- 5 files changed, 33 insertions(+), 11 deletions(-) create mode 100644 changelog.d/7538.bugfix diff --git a/changelog.d/7538.bugfix b/changelog.d/7538.bugfix new file mode 100644 index 000000000..4a614a9e6 --- /dev/null +++ b/changelog.d/7538.bugfix @@ -0,0 +1 @@ + Hash passwords as early as possible during password reset. diff --git a/synapse/handlers/set_password.py b/synapse/handlers/set_password.py index 63d8f9aa0..4d245b618 100644 --- a/synapse/handlers/set_password.py +++ b/synapse/handlers/set_password.py @@ -35,16 +35,13 @@ class SetPasswordHandler(BaseHandler): async def set_password( self, user_id: str, - new_password: str, + password_hash: str, logout_devices: bool, requester: Optional[Requester] = None, ): if not self.hs.config.password_localdb_enabled: raise SynapseError(403, "Password change disabled", errcode=Codes.FORBIDDEN) - self._password_policy_handler.validate_password(new_password) - password_hash = await self._auth_handler.hash(new_password) - try: await self.store.user_set_password_hash(user_id, password_hash) except StoreError as e: diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index 326682fbd..e7f6928c8 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -222,8 +222,14 @@ class UserRestServletV2(RestServlet): else: new_password = body["password"] logout_devices = True + + new_password_hash = await self.auth_handler.hash(new_password) + await self.set_password_handler.set_password( - target_user.to_string(), new_password, logout_devices, requester + target_user.to_string(), + new_password_hash, + logout_devices, + requester, ) if "deactivated" in body: @@ -523,6 +529,7 @@ class ResetPasswordRestServlet(RestServlet): self.store = hs.get_datastore() self.hs = hs self.auth = hs.get_auth() + self.auth_handler = hs.get_auth_handler() self._set_password_handler = hs.get_set_password_handler() async def on_POST(self, request, target_user_id): @@ -539,8 +546,10 @@ class ResetPasswordRestServlet(RestServlet): new_password = params["new_password"] logout_devices = params.get("logout_devices", True) + new_password_hash = await self.auth_handler.hash(new_password) + await self._set_password_handler.set_password( - target_user_id, new_password, logout_devices, requester + target_user_id, new_password_hash, logout_devices, requester ) return 200, {} diff --git a/synapse/rest/client/v2_alpha/account.py b/synapse/rest/client/v2_alpha/account.py index 1bd023477..d4f721b6b 100644 --- a/synapse/rest/client/v2_alpha/account.py +++ b/synapse/rest/client/v2_alpha/account.py @@ -220,12 +220,27 @@ class PasswordRestServlet(RestServlet): self.auth = hs.get_auth() self.auth_handler = hs.get_auth_handler() self.datastore = self.hs.get_datastore() + self.password_policy_handler = hs.get_password_policy_handler() self._set_password_handler = hs.get_set_password_handler() @interactive_auth_handler async def on_POST(self, request): body = parse_json_object_from_request(request) + # we do basic sanity checks here because the auth layer will store these + # in sessions. Pull out the new password provided to us. + if "new_password" in body: + new_password = body.pop("new_password") + if not isinstance(new_password, str) or len(new_password) > 512: + raise SynapseError(400, "Invalid password") + self.password_policy_handler.validate_password(new_password) + + # If the password is valid, hash it and store it back on the body. + # This ensures that only the hashed password is handled everywhere. + if "new_password_hash" in body: + raise SynapseError(400, "Unexpected property: new_password_hash") + body["new_password_hash"] = await self.auth_handler.hash(new_password) + # there are two possibilities here. Either the user does not have an # access token, and needs to do a password reset; or they have one and # need to validate their identity. @@ -276,12 +291,12 @@ class PasswordRestServlet(RestServlet): logger.error("Auth succeeded but no known type! %r", result.keys()) raise SynapseError(500, "", Codes.UNKNOWN) - assert_params_in_dict(params, ["new_password"]) - new_password = params["new_password"] + assert_params_in_dict(params, ["new_password_hash"]) + new_password_hash = params["new_password_hash"] logout_devices = params.get("logout_devices", True) await self._set_password_handler.set_password( - user_id, new_password, logout_devices, requester + user_id, new_password_hash, logout_devices, requester ) return 200, {} diff --git a/synapse/rest/client/v2_alpha/register.py b/synapse/rest/client/v2_alpha/register.py index c26927f27..addd4cae1 100644 --- a/synapse/rest/client/v2_alpha/register.py +++ b/synapse/rest/client/v2_alpha/register.py @@ -431,8 +431,8 @@ class RegisterRestServlet(RestServlet): raise SynapseError(400, "Invalid password") self.password_policy_handler.validate_password(password) - # If the password is valid, hash it and store it back on the request. - # This ensures the hashed password is handled everywhere. + # If the password is valid, hash it and store it back on the body. + # This ensures that only the hashed password is handled everywhere. if "password_hash" in body: raise SynapseError(400, "Unexpected property: password_hash") body["password_hash"] = await self.auth_handler.hash(password)