feathersjs-ecosystem/feathers-authentication-management

Securely store password reset tokens

Closed this issue · 9 comments

As displayed in https://github.com/feathersjs/feathers-authentication-management/blob/master/src/sendResetPwd.js#L37, it doesn't look like this module hashes the password reset tokens when it adds them to the user's data store. However, reset tokens are a credential useful to an adversary in the case of a data breach, similar to that of passwords themselves. It is a lot less work to use a reset token to take over a user's account than it is to brute-force bcrypt hashes.

I think that the reset tokens should be sent, and then the user model patched with the hashed variant of the sent token. This will mitigate a number of situations in which the database can be read (and not necessarily written to) by an adversary. A realistic situation I brought up in a recent blog post is a situation in which the database is directly exposed for reading to the internet due to misconfiguration. An adversary can issue password resets for users and hijack accounts without needing write access to the user models of the applications.

I didn't issue a PR for this as I'm not entirely familiar with the architecture of Feathers, and it seems that a change of this nature would be a breaking change for existing reset tokens unless the user model was flagged or migrated in some fashion, a non-trivial task when the hashing algorithm and underlying data store can be changed. These are architectural decisions best left to maintainers.

Ways I can see this happening:

  1. In new versions of feathers-authentication-management, patchUser also patches user models with isSecureToken: true as a stopgap, and the code branches to check old, active reset tokens. New reset tokens are hashed and checked against the hashed variant.
  2. Don't worry about it. Tokens have a short expiration date, and the token not matching just forces a new reset for some users stuck during the migration process.
  3. Migrate old plaintext tokens upon upgrade to the new module. This would likely need some way to detect whether an existing token was hashed, which is fuzzy at best given that the password hash algorithm can be changed out by a user.

A PR would be welcome.

Does that mean you are indifferent at this stage as to which way it goes? If I'm writing the PR for this I'm going to choose the "break old reset tokens" option because it's by far the easiest to develop and test for. 😀

I prefer being backward compatible as often as possible, but there are limits. Adding code to detect previous versions of tokens is a smell imho; requiring patching of the DB is problematic.

My thought would be to go with your option #2 ("break old reset tokens"), and bump the major semver on the repo. Plus you adding a migration note in the feathers-docs describing how the DB could be patched if desired and the relatively acceptable consequentces of not doing

I'm been trying to get a pull request together but there is a complication.

The current way it works is it actually queries the user collection using the reset token, so if it's stored as a secure hash there's no way to search for users using it. Bcrypt seems to not replicate 1 way hashes, I suppose it forces you to use compare.

This means your feathersjs developers will need to reimplement their server to send other information in the email link along with the reset password to identify the user, such as their email.

What do you recommend?

Unless we store the user id in the reset token such as id___token. Perhaps separated by 3 underscores. Could that be insecure?

I'm posting a bug here instead of opening a new issue because this issue is still open and it's related.
The implementation for for sendResetPwd uses the hashPassword hook from authentication-local, but it uses the "password" key instead of getting "passwordField" from the config. The hashPassword hook does check the config, so if passwordField is set to something other than "password", there is a key mismatch and the unhashed resetToken is stored in the db. This results in an error when calling the resetPwdLong action, because bcrypt.compare is being passed 2 identical strings.

@codingfriend1 I think this is related to your PR. Any comments?

@eddyystop I looked into it briefly. Unfortunately I don't have time to fix this right now. It's actually not a bug introduced by my changes, this bug seems to have existed before my PR.

See https://github.com/feathers-plus/feathers-authentication-management/blob/6b21d5ef5039d0da4b6c4939baabe542f1e9747e/src/helpers.js for proof, which the commit right before my changes.

Essentially the problem seems to be that the feathers-authentication repo looks for the passwordField in the user config, but the feathers-authentication-management hashPassword helper is hard coding the property "password" instead of checking the site options for the password field name.

Take a look at
feathers-authentication-management/src/helpers.js

On line 14 it's hard coding data: { password },. It may be as simple as importing the config and using the password field from options rather than the hard coded value.

The a-l-m rewrite has an option.passwordfield. The default is obtained from config/default.json.

Full details at https://github.com/feathers-plus/authentication-local-management/blob/master/misc/upgrading.md