[PATCH v2] auth: secure password reset implementation
Mads Kiilerich
mads at kiilerich.com
Sun Aug 9 02:43:16 UTC 2015
On 08/08/2015 05:25 PM, Andrew Shadura wrote:
> # HG changeset patch
> # User Andrew Shadura <andrew at shadura.me>
> # Date 1431821238 -7200
> # Sun May 17 02:07:18 2015 +0200
> # Node ID c4ad7e6deed43b0f725b60b37ae1d2161eead200
> # Parent e84c2738fbd84569492c56e5a9361ee94af5d420
> auth: secure password reset implementation
>
> This is a better implementation of password reset function, which
> doesn't involve sending a new password to the user's email address
> in clear text, and at the same time doesn't require state storing.
/me thinks "is stateless"
> The old implementation generated a new password and sent it
> in clear text to whatever email assigned to the user currently,
> invalidating the old password the very moment a new one is requested.
This sentence is hard to read. I guess the crucial part is that any user
(unauthenticated, obviously) could request a reset for any username/email.
> Apart from potential insecurity, this makes it possible for anyone
> to disrupt users' workflow by repeatedly resetting their passwords.
>
> The idea behind this implementation is to generate
> an authentication token which is dependent on the user state
> at the time before the password change takes place, so the token
> is one-time and can't be reused, and also to bind the token to
> the session.
perhaps more clearly: to the browser session
> The token is calculated as SHA1 hash of the following:
>
> * user's identifier (number, not a name)
> * timestamp
> * hashed user's password
> * session identifier
> * per-application secret
>
> We use numeric user's identifier, as it's fixed and doesn't change,
> so renaming users doesn't affect the mechanism. Timestamp is added
> to make it possible to limit the token's validness (currently hard
> coded to 24h), and we don't want users to be able to fake that field
> easily. Hashed user's password is needed to prevent using the token
> again once the password has been changed. Session identifier is
> an additional security measure to ensure someone else stealing the
> token can't use it. Finally, per-application secret is just another
> way to make it harder for an attacker to guess all values in an
> attempt to generate a valid token.
>
> When the token is generated, an anonymous user is directed to a
> confirmation page where the timestamp and the usernames are already
> preloaded, so the user needs to specify the token. User can either
> click the link in the email if it's really them reading it, or to type
> the token manually.
>
> Using the right token in the same session as it was requested directs
> the user to a password change form, where the user is supposed to
> specify a new password (twice, of course). Upon completing the form
> (which is POSTed) the password change happens and a notification
> mail is sent.
>
> The test is updated to test the basic functionality with a bad and
> a good token, but it doesn't (yet) cover all code paths.
>
> diff --git a/kallithea/controllers/login.py b/kallithea/controllers/login.py
> --- a/kallithea/controllers/login.py
> +++ b/kallithea/controllers/login.py
> @@ -42,7 +42,8 @@ from kallithea.lib.base import BaseContr
> from kallithea.lib.exceptions import UserCreationError
> from kallithea.lib.utils2 import safe_str
> from kallithea.model.db import User, Setting
> -from kallithea.model.forms import LoginForm, RegisterForm, PasswordResetForm
> +from kallithea.model.forms import \
> + LoginForm, RegisterForm, PasswordResetForm, PasswordResetConfirmationForm
> from kallithea.model.user import UserModel
> from kallithea.model.meta import Session
>
> @@ -197,12 +198,12 @@ class LoginController(BaseController):
> error_dict = {'recaptcha_field': _msg}
> raise formencode.Invalid(_msg, _value, None,
> error_dict=error_dict)
> - UserModel().reset_password_link(form_result)
> + redirect_link = UserModel().reset_password_link(form_result)
> h.flash(_('Your password reset link was sent'),
I guess it would be more correct to explain it as "a confirmation code
has been sent".
> category='success')
> - return redirect(url('login_home'))
> + return redirect(redirect_link)
>
> - except formencode.Invalid, errors:
> + except formencode.Invalid as errors:
I pushed a separate changeset fixing all(?) exception namings to use
'as'. This change will thus go away when rebasing your change (quite
trivial, but also available on
https://bitbucket.org/Unity-Technologies/kallithea/commits/dfa4f7c701aed5ae00e45bbbd6f24ab80a83d320
)
> return htmlfill.render(
> render('/password_reset.html'),
This template still says: "Password reset link will be sent to the email
address matching your username" ... but there is no 'username' involved
here.
I think the page should be updated to explain what is going to happen.
It should perhaps say something like "a confirmation mail will be sent
to the specified email address if it is registered in the system".
> defaults=errors.value,
> @@ -214,17 +215,45 @@ class LoginController(BaseController):
> return render('/password_reset.html')
>
> def password_reset_confirmation(self):
> - if request.GET and request.GET.get('key'):
> + if request.GET:
> + c.data = dict(
> + email = request.GET.get('email'),
> + timestamp = request.GET.get('timestamp'),
> + token = request.GET.get('token'),
As below, perhaps use a form to validate it and avoid getting None.
> + )
> + if c.data['token']:
> + log.debug("password reset confirmation (GET): %s", c.data)
> + if UserModel().reset_password_confirm(c.data):
> + return render('/password_reset_confirmation.html')
> + else:
> + h.flash(_('Invalid password reset token'),
> + category='error')
> + return redirect(url('reset_password'))
> + else:
> + return render('/password_reset_confirmation.html')
> + elif request.POST:
> + password_reset_confirmation_form = PasswordResetConfirmationForm()()
> + c.data = dict(
> + email = request.POST.get('email'),
> + password = request.POST.get('password'),
> + password_confirm = request.POST.get('password_confirm'),
> + timestamp = request.POST.get('timestamp'),
> + token = request.POST.get('token'),
When we have a form (which should validate all data), why not take the
data from the form instead of using POST.get which can return None and
cause weird crashes.
The system already has other kinds of "tokens" so how about consistently
calling this something more clear. Perhaps "verification token" (or
'reset token' as it is called in a few places).
> + )
> try:
> - user = User.get_by_api_key(request.GET.get('key'))
> - data = dict(email=user.email)
> - UserModel().reset_password(data)
> - h.flash(_('Your password reset was successful, '
> - 'new password has been sent to your email'),
> - category='success')
> - except Exception, e:
> - log.error(e)
> - return redirect(url('reset_password'))
> + log.debug("password reset confirmation (POST): %s", c.data)
> + form_result = password_reset_confirmation_form.to_python(dict(request.POST))
> + UserModel().reset_password(c.data)
> + h.flash(_('Successfully updated password'),
> + category='success')
> + except formencode.Invalid as errors:
> + return htmlfill.render(
> + render('/password_reset_confirmation.html'),
> + defaults=errors.value,
> + errors=errors.error_dict or {},
> + prefix_error=False,
> + encoding="UTF-8",
> + force_defaults=False)
>
> return redirect(url('login_home'))
>
> diff --git a/kallithea/lib/utils2.py b/kallithea/lib/utils2.py
> --- a/kallithea/lib/utils2.py
> +++ b/kallithea/lib/utils2.py
> @@ -34,6 +34,7 @@ import uuid
> import datetime
> import urllib
> import binascii
> +import hashlib
>
> import webob
> import urlobject
> @@ -170,6 +171,14 @@ def generate_api_key():
> return binascii.hexlify(os.urandom(20))
>
>
> +def hash_values(*args):
> + """
> + Calculate SHA1 for the string representations of the arguments,
> + concatenated and joined using NUL characters. Returns a hexadecimal
> + representation of the hash.
> + """
> + return hashlib.sha1('\0'.join(str(arg) for arg in args)).hexdigest()
> +
> def safe_int(val, default=None):
> """
> Returns int() of val if val is not convertable to int use default
> diff --git a/kallithea/model/forms.py b/kallithea/model/forms.py
> --- a/kallithea/model/forms.py
> +++ b/kallithea/model/forms.py
> @@ -212,6 +212,17 @@ def PasswordResetForm():
> email = v.Email(not_empty=True)
> return _PasswordResetForm
>
> +def PasswordResetConfirmationForm():
> + class _PasswordResetConfirmationForm(formencode.Schema):
> + allow_extra_fields = True
> + filter_extra_fields = True
> +
> + password = All(v.ValidPassword(), v.UnicodeString(strip=False, min=6))
> + password_confirm = All(v.ValidPassword(), v.UnicodeString(strip=False, min=6))
I suggest declaring other (hidden) parameters here too and thus getting
some early basic verification on the server side.
> +
> + chained_validators = [v.ValidPasswordsMatch('password',
> + 'password_confirm')]
> + return _PasswordResetConfirmationForm
>
> def RepoForm(edit=False, old_data={}, supported_backends=BACKENDS.keys(),
> repo_groups=[], landing_revs=[]):
> diff --git a/kallithea/model/user.py b/kallithea/model/user.py
> --- a/kallithea/model/user.py
> +++ b/kallithea/model/user.py
> @@ -27,13 +27,15 @@ Original author and date, and relevant c
>
>
> import logging
> +import time
> import traceback
> +from pylons import config
> from pylons.i18n.translation import _
>
> from sqlalchemy.exc import DatabaseError
>
> from kallithea import EXTERN_TYPE_INTERNAL
> -from kallithea.lib.utils2 import safe_unicode, generate_api_key, get_current_authuser
> +from kallithea.lib.utils2 import safe_unicode, generate_api_key, get_current_authuser, hash_values
> from kallithea.lib.caching_query import FromCache
> from kallithea.model import BaseModel
> from kallithea.model.db import User, UserToPerm, Notification, \
> @@ -271,6 +273,33 @@ class UserModel(BaseModel):
> from kallithea.lib.hooks import log_delete_user
> log_delete_user(user.get_dict(), cur_user)
>
> + def reset_password_token(self, user, timestamp, session_id):
> + """
> + The token is calculated as SHA1 hash of the following:
> +
> + * user's identifier (number, not a name)
> + * timestamp
> + * hashed user's password
> + * session identifier
> + * per-application secret
> +
> + We use numeric user's identifier, as it's fixed and doesn't change,
> + so renaming users doesn't affect the mechanism. Timestamp is added
> + to make it possible to limit the token's validness (currently hard
> + coded to 24h), and we don't want users to be able to fake that field
> + easily. Hashed user's password is needed to prevent using the token
> + again once the password has been changed. Session identifier is
> + an additional security measure to ensure someone else stealing the
> + token can't use it. Finally, per-application secret is just another
> + way to make it harder for an attacker to guess all values in an
> + attempt to generate a valid token.
> + """
> + return hash_values(user.user_id,
> + timestamp,
> + user.password,
> + session_id,
> + config.get('app_instance_uuid'))
> +
> def reset_password_link(self, data):
> from kallithea.lib.celerylib import tasks, run_task
> from kallithea.model.notification import EmailNotificationModel
> @@ -278,27 +307,65 @@ class UserModel(BaseModel):
>
> user_email = data['email']
> user = User.get_by_email(user_email)
> + timestamp = int(time.time())
> if user is not None:
> - log.debug('password reset user found %s' % user)
> + log.debug('password reset user %s found', user)
> + token = self.reset_password_token(user,
> + timestamp,
> + h.authentication_token)
> link = h.canonical_url('reset_password_confirmation',
> - key=user.api_key)
> + email=user_email,
> + timestamp=timestamp,
> + token=token)
> reg_type = EmailNotificationModel.TYPE_PASSWORD_RESET
> body = EmailNotificationModel().get_email_tmpl(
> reg_type, 'txt',
> user=user.short_contact,
> + reset_token=token,
> reset_url=link)
> html_body = EmailNotificationModel().get_email_tmpl(
> reg_type, 'html',
> user=user.short_contact,
> + reset_token=token,
> reset_url=link)
> log.debug('sending email')
> run_task(tasks.send_email, [user_email],
> _("Password reset link"), body, html_body)
> - log.info('send new password mail to %s' % user_email)
> + log.info('send new password mail to %s', user_email)
Changes like this has also been fixed in the whole code base.
> else:
> - log.debug("password reset email %s not found" % user_email)
> + log.debug("password reset email %s not found", user_email)
>
> - return True
> + return h.canonical_url('reset_password_confirmation',
> + email=user_email,
> + timestamp=timestamp)
I wonder if it could be a problem to have email addresses in the URL and
thus in browser history and access logs ... but I guess it is ok and a
deliberate design decision.
> + def reset_password_confirm(self, data):
> + from kallithea.lib.celerylib import tasks, run_task
> + from kallithea.lib import auth
> + import kallithea.lib.helpers as h
> + user = User.get_by_email(data['email'])
> + if not user:
I would prefer to be more spot-on and and say 'if user is None'.
> + log.debug("user with email %s not found", data['email'])
> + return False
> +
> + now = int(time.time())
> + timestamp = int(data['timestamp'])
> +
> + if (timestamp is None) or (timestamp > now):
At this point we know that timestamp is an int and cannot be None. But I
would suggest to use a form to convert it to int safely.
> + log.debug('timestamp is None or from the future')
> + return False
> +
> + # TODO for now, hardcode 24h, need to be a setting
> + if (now - timestamp) > 86400:
> + log.debug('token expired')
> + return False
> +
> + token = self.reset_password_token(user,
> + data['timestamp'],
> + h.authentication_token)
> + log.debug('computed token: %s', token)
> + log.debug('received token: %s', data['token'])
> + return token == data['token']
>
> def reset_password(self, data):
> from kallithea.lib.celerylib import tasks, run_task
> @@ -311,14 +378,14 @@ class UserModel(BaseModel):
> user.password = auth.get_crypt_password(new_passwd)
> Session().add(user)
> Session().commit()
> - log.info('change password for %s' % user_email)
> + log.info('change password for %s', user_email)
> if new_passwd is None:
> raise Exception('unable to generate new password')
>
> run_task(tasks.send_email, [user_email],
> _('Your new password'),
> _('Your new Kallithea password:%s') % (new_passwd,))
> - log.info('send new password mail to %s' % user_email)
> + log.info('send new password mail to %s', user_email)
>
> return True
>
> diff --git a/kallithea/templates/email_templates/password_reset.html b/kallithea/templates/email_templates/password_reset.html
> --- a/kallithea/templates/email_templates/password_reset.html
> +++ b/kallithea/templates/email_templates/password_reset.html
> @@ -3,8 +3,10 @@
>
> <h4>${_('Hello %s') % user}</h4>
>
> -<p>${_('We received a request to create a new password for your account.')}</p>
> -<p>${_('You can generate it by clicking following URL')}:</p>
> +<p>${_('We have received a request to reset the password for your account.')}</p>
> +<p>${_('To set a new password, click the following link')}:</p>
> <p><a href="${reset_url}">${reset_url}</a></p>
>
> -<p>${_("Please ignore this email if you did not request a new password .")}</p>
> +<p>${_("Should you not be able to use the link above, please type the following code into the password reset form")}: <code>${reset_token}</code></p>
> +
> +<p>${_("If it weren't you who requested the password reset, just disregard this message.")}</p>
> diff --git a/kallithea/templates/email_templates/password_reset.txt b/kallithea/templates/email_templates/password_reset.txt
> --- a/kallithea/templates/email_templates/password_reset.txt
> +++ b/kallithea/templates/email_templates/password_reset.txt
> @@ -3,9 +3,11 @@
>
> ${_('Hello %s') % user|n,unicode}
>
> -${_('We received a request to create a new password for your account.')|n,unicode}
> -${_('You can generate it by clicking following URL')|n,unicode}:
> +${_('We have received a request to reset the password for your account..')|n,unicode}
> +${_('To set a new password, click the following link')|n,unicode}:
>
> ${reset_url|n,unicode}
>
> -${_("Please ignore this email if you did not request a new password .")|n,unicode}
> +${_("Should you not be able to use the link above, please type the following code into the password reset form")|n,unicode}: ${reset_token|n,unicode}
> +
> +${_("If it weren't you who requested the password reset, just disregard this message.")|n,unicode}
> diff --git a/kallithea/templates/password_reset_confirmation.html b/kallithea/templates/password_reset_confirmation.html
> --- a/kallithea/templates/password_reset_confirmation.html
> +++ b/kallithea/templates/password_reset_confirmation.html
> @@ -0,0 +1,73 @@
> +## -*- coding: utf-8 -*-
> +<%inherit file="base/root.html"/>
> +
> +<%block name="title">
> + ${_('Password Reset Confirmation')}
I think the 'confirmation' name is a bit misleading. This page is for
setting/changing the password. When the old password can't be used as
proof of being in a position to change the password, we need this mail
verification token instead.
> +</%block>
> +
> +<div id="register">
> + <%include file="/base/flash_msg.html"/>
> + <div class="title withlogo">
> + %if c.site_name:
> + <h5>${_('Reset Your Password to %s') % c.site_name}</h5>
> + %else:
> + <h5>${_('Reset Your Password')}</h5>
> + %endif
> + </div>
> + <div class="inner">
> + %if c.data['email']:
> + <p>${_('You are about to set a new password for the email address %s') % c.data['email']}</p>
> + ${h.form(h.canonical_url('reset_password_confirmation'), method='post' if c.data['token'] else 'get')}
Why not just always POST and let the user fill out the verification
token if it not already is given in the URL?
> + <div style="display: none;">
> + ${h.hidden('email',value=c.data['email'])}
> + %if c.data['token']:
> + ${h.hidden('token',value=c.data['token'])}
> + %endif
> + %if c.data['timestamp']:
> + ${h.hidden('timestamp',value=c.data['timestamp'])}
> + %endif
Perhaps make this page simpler by always showing the verification token
field but let it be pre-filled if it is given in the URL from the mail.
And in which cases can the timestamp be missing?
> + </div>
> + %endif
> + <div class="form">
> + <!-- fields -->
> + <div class="fields">
> + %if c.data['token']:
> + <div class="field">
> + <div class="label">
> + <label for="password">${_('New Password')}:</label>
> + </div>
> + <div class="input">
> + ${h.password('password',class_='focus')}
> + </div>
> + </div>
> +
> + <div class="field">
> + <div class="label">
> + <label for="password_confirm">${_('Confirm New Password')}:</label>
> + </div>
> + <div class="input">
> + ${h.password('password_confirm',class_='focus')}
> + </div>
> + </div>
> + %else:
> + <div class="field">
> + <div class="label">
> + <label for="token">${_('Code you received in the email')}:</label>
> + </div>
> + <div class="input">
> + ${h.text('token',class_='focus')}
> + </div>
> + </div>
> +
> + %endif
> +
> + <div class="buttons">
> + <div class="nohighlight">
> + ${h.submit('send',_('Confirm'),class_="btn")}
> + </div>
> + </div>
> + </div>
> + </div>
> + ${h.end_form()}
> + </div>
> + </div>
> diff --git a/kallithea/tests/functional/test_login.py b/kallithea/tests/functional/test_login.py
> --- a/kallithea/tests/functional/test_login.py
> +++ b/kallithea/tests/functional/test_login.py
> @@ -12,6 +12,7 @@ from kallithea.model.api_key import ApiK
> from kallithea.model import validators
> from kallithea.model.db import User, Notification
> from kallithea.model.meta import Session
> +from kallithea.model.user import UserModel
>
> fixture = Fixture()
>
> @@ -336,6 +337,7 @@ class TestLoginController(TestController
> response.mustcontain('An email address must contain a single @')
>
> def test_forgot_password(self):
> + import time
Just import globally?
> response = self.app.get(url(controller='login',
> action='password_reset'))
> self.assertEqual(response.status, '200 OK')
> @@ -345,6 +347,7 @@ class TestLoginController(TestController
> email = 'username at python-works.com'
> name = 'passwd'
> lastname = 'reset'
> + timestamp = int(time.time())
>
> new = User()
> new.username = username
> @@ -364,27 +367,45 @@ class TestLoginController(TestController
>
> response = response.follow()
>
> - # BAD KEY
> + # BAD TOKEN
>
> - key = "bad"
> + token = "bad"
> response = self.app.get(url(controller='login',
> action='password_reset_confirmation',
> - key=key))
> + email=email,
> + timestamp=timestamp,
> + token=token))
> self.assertEqual(response.status, '302 Found')
> self.assertTrue(response.location.endswith(url('reset_password')))
>
> + response = response.follow()
> +
> # GOOD KEY
>
> - key = User.get_by_username(username).api_key
> + token = UserModel().reset_password_token(User.get_by_username(username),
> + timestamp,
> + h.authentication_token)
> +
> response = self.app.get(url(controller='login',
> action='password_reset_confirmation',
> - key=key))
> + email=email,
> + timestamp=timestamp,
> + token=token))
> + self.assertEqual(response.status, '200 OK')
> + response.mustcontain("You are about to set a new password for the email address %s" % email)
> +
> + response = self.app.post(url(controller='login',
> + action='password_reset_confirmation'),
> + {'email': email,
> + 'timestamp': timestamp,
> + 'password': "p at ssw0rd",
> + 'password_confirm': "p at ssw0rd",
> + 'token': token,
> + })
> self.assertEqual(response.status, '302 Found')
> - self.assertTrue(response.location.endswith(url('login_home')))
>
> self.checkSessionFlash(response,
> - ('Your password reset was successful, '
> - 'new password has been sent to your email'))
> + ('Successfully updated password'))
>
> response = response.follow()
>
> _______________________________________________
> kallithea-general mailing list
> kallithea-general at sfconservancy.org
> http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
More information about the kallithea-general
mailing list