[PATCH PoC] secure password reset implementation
Mads Kiilerich
mads at kiilerich.com
Tue May 19 11:10:46 EDT 2015
On 05/17/2015 02:07 AM, 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 8d43a8174c960779437c2d8de7a0906a8cd14128
> # Parent cb911e90e205bdb18fc2e2bd66549ea388d00413
> 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.
Very cool. It would be nice if you stated more clearly what is wrong
about the old implementation.
> The idea behind is to generate a 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 the the 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 not
> implemented),
Please just hardcode something like 24 hours.
> 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.
Are you or others aware of "standard" ways of handling password resets?
Something that indicates we are doing it correctly or that we can reuse?
> 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 (note: email template needs to be improved).
Yes - please fix the email template ;-) This change without a change of
the template would be wrong.
> 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.
>
> diff --git a/kallithea/controllers/login.py b/kallithea/controllers/login.py
> --- a/kallithea/controllers/login.py
> +++ b/kallithea/controllers/login.py
> @@ -43,7 +43,8 @@ from kallithea.lib.auth_modules import i
> from kallithea.lib.base import BaseController, render
> from kallithea.lib.exceptions import UserCreationError
> 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
>
> @@ -241,12 +242,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'),
> category='success')
> - return redirect(url('login_home'))
> + return redirect(redirect_link)
>
> - except formencode.Invalid, errors:
> + except formencode.Invalid as errors:
> return htmlfill.render(
> render('/password_reset.html'),
> defaults=errors.value,
> @@ -258,17 +259,49 @@ 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(
> + username = request.GET.get('username'),
> + timestamp = request.GET.get('timestamp'),
> + token = request.GET.get('token')
> + )
> + if c.data['token']:
> + try:
> + log.debug("data = %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'))
> + except Exception as e:
> + log.error(e)
> + return redirect(url('reset_password'))
I don't like the pattern of redirecting on serious internal errors and
have been trying to get rid of it in other places. I think it generally
is much better to just show an error message so the error can be fixed
... and the user easily can use back to go back and retry.
> + else:
> + return render('/password_reset_confirmation.html')
> + elif request.POST:
> + password_reset_confirmation_form = PasswordResetConfirmationForm()()
> + c.data = dict(
> + username = request.POST.get('username'),
> + password = request.POST.get('password'),
> + password_confirm = request.POST.get('password_confirm'),
> + timestamp = request.POST.get('timestamp'),
> + token = request.POST.get('token')
> + )
> 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("data = %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/model/forms.py b/kallithea/model/forms.py
> --- a/kallithea/model/forms.py
> +++ b/kallithea/model/forms.py
> @@ -205,6 +205,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))
> +
> + 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,7 +27,11 @@ Original author and date, and relevant c
>
>
> import logging
> +import hashlib
> +import time
> import traceback
> +from pylons import config
> +from pylons import session
> from pylons.i18n.translation import _
>
> from sqlalchemy.exc import DatabaseError
> @@ -296,10 +300,19 @@ class UserModel(BaseModel):
>
> user_email = data['email']
> user = User.get_by_email(user_email)
> + timestamp = int(time.time())
> if user:
> log.debug('password reset user found %s' % user)
> + token = hashlib.sha1('%s%s%s%s%s' % (user.user_id,
> + timestamp,
> + user.password,
> + session.id,
> + config.get('app_instance_uuid'))
> + ).hexdigest()
Now we are reinventing crypto. That is dangerous. One thing: One thing
is that hashes could give conflicts in the hash but we should make sure
that we never got "collisions" in the input string - such collisions
could perhaps be used as attacks. Perhaps use \n as delimiter of the fields?
The computation should probably be moved to a function so it can be reused.
> link = h.canonical_url('reset_password_confirmation',
> - key=user.api_key)
> + username=user.username,
> + timestamp=timestamp,
> + token=token)
> reg_type = EmailNotificationModel.TYPE_PASSWORD_RESET
> body = EmailNotificationModel().get_email_tmpl(
> reg_type, 'txt',
> @@ -316,27 +329,45 @@ class UserModel(BaseModel):
> else:
> log.debug("password reset email %s not found" % user_email)
>
> - return True
> + return h.canonical_url('reset_password_confirmation',
> + username=user.username,
> + timestamp=timestamp)
> +
> + def reset_password_confirm(self, data):
> + from kallithea.lib.celerylib import tasks, run_task
> + from kallithea.lib import auth
> + user = User.get_by_username(data['username'])
> + if not user:
> + log.debug("user %s not found" % data['username'])
> + return False
> +
> + token = hashlib.sha1('%s%s%s%s%s' % (user.user_id,
> + data['timestamp'],
> + user.password,
> + session.id,
> + config.get('app_instance_uuid'))
> + ).hexdigest()
> + 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
> from kallithea.lib import auth
> - user_email = data['email']
> - user = User.get_by_email(user_email)
> - new_passwd = auth.PasswordGenerator().gen_password(
> - 8, auth.PasswordGenerator.ALPHABETS_BIG_SMALL)
> - if user:
> - user.password = auth.get_crypt_password(new_passwd)
> + user = User.get_by_username(data['username'])
> + if not user:
> + log.debug("user %s not found" % data['username'])
> +
> + if data['password']:
> + user.password = auth.get_crypt_password(data['password'])
> Session().add(user)
> Session().commit()
> - log.info('change password for %s' % user_email)
> - if new_passwd is None:
> - raise Exception('unable to generate new password')
> + log.info('change password for %s' % user.username)
>
> - 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)
> + run_task(tasks.send_email, [user.email],
> + _('Password change notification'),
> + _('Your Kallithea has been changed'))
Your Kallithea _password_ has been changed
> + log.info('send password reset mail to %s' % user.email)
>
> return True
>
> 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')}
> +</%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
We should probably also show the canonical url to make sure the user
recognizes which site we are talking about.
> + </div>
> + <div class="inner">
> + %if c.data['username']:
> + <p>${_('You are about to reset password for the user %s') % c.data['username']}</p>
Reset _the_ password
We should however not reveal the username - at most the email address
the user provided. We should also make sure the same message is shown,
no matter if the address is recognized or not.
> + ${h.form(h.canonical_url('reset_password_confirmation'), method='post' if c.data['token'] else 'get')}
> + <div style="display: none;">
> + ${h.hidden('username',value=c.data['username'])}
> + %if c.data['token']:
> + ${h.hidden('token',value=c.data['token'])}
> + %endif
> + %if c.data['timestamp']:
> + ${h.hidden('timestamp',value=c.data['timestamp'])}
> + %endif
> + </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>
But there is no information about any code in the email - there is only
a URL?
We should either explicitly show the code in the mail or just ask the
user to click the link in the mail and not show the "code" input field.
> + </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>
>
/Mads
More information about the kallithea-general
mailing list