[PATCH PoC] secure password reset implementation

[Andrew Shadura]

Hello,

On Tue, 19 May 2015 17:10:46 +0200
Mads Kiilerich <mads at kiilerich.com> wrote:

> > 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.

Okay, I will.

> > 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?

I looked at what other frameworks do, and from what I saw it seems they
do more or less the same thing.

> >       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.

Okay, I will fail with 500 or a better code (if applicable) here.

> > @@ -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?

Well, none of the data we use here is accepted directly from user
(except timestamp — when verifying the token; I should probably
validate it before using). From what I saw in similar cases elsewhere
(for example, OAuth 1.0), sometimes the data is escaped to avoid
collisions.

> The computation should probably be moved to a function so it can be
> reused.

Agree.

> > +    </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.

Well, yes, you're right. I shouldn't do email → username conversion.

> 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.

The template isn't perfect, that's the problem :) The idea that you
receive an email with a link (with token) and a token separately from a
link, so you can choose to either follow a link, or to type a code
manually. The password reset form redirects you to a form where the
only thing missing is the token.

I'll post a revised version of a patch soon.

-- 
Cheers,
  Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.sfconservancy.org/pipermail/kallithea-general/attachments/20150520/fe1ba48f/attachment-0001.sig>