hashPassword should not rehash a password that is already a bcrypt hash

[kplatter]

No description provided.

[tjwebb]

detecting a hash is not really possible. some longer version of "abc123" is a possible hash, but could also be someone's password. This also creates a potential vulnerability, where passwords that appear to be hashed are actually stored in plaintext.

[kplatter]

A bcyrpt hash has a defined format see http://stackoverflow.com/questions/5393803/can-someone-explain-how-bcrypt-verifies-a-hash/10933491#10933491

[tjwebb]

The problem isn't whether bcrypt has a defined format, it's whether user passwords have a defined format. They don't. So if my user password is something like 1$2$3$mypass then there's a problem.

https://github.com/tjwebb/sails-auth/blob/master/api/models/Passport.js#L12

What if a password is hashed with a different salt, or salt length? It'll look like a bcrypt password, but the comparison wouldn't work. There's just no way I'm going to conditionally allow some passwords to enter the database un-hashed. The security of the system then starts to depend on programmer skill at writing logic to discern password formats, rather than encryption.

If your goal is to never store the sails-permissions admin password in plaintext, send it in as an environment variable when you start your Sails app for the first time:

$ ADMIN_PASSWORD=mypassword sails lift

[kplatter]

I was basing the detection off of $2a$%%$ and then any length after that, which would not care about salt or salt length. Yes it is possible that a user could have a password of $2a$10$.... but how likely is that? I have other users other than admin that I need to add and I don't store their password unhashed. As it is there is no option for adding them to the database without having their unhashed password. What if there was an attribute on the passport that indicated whether or not it should be hashed and then "delete passport.passwordAlreadyHashed"?

[tjwebb]

I have other users other than admin that I need to add and I don't store their password unhashed.

You can send it in through the REST interface (/user/create, etc) and create users in the typical way. Nothing needs to be stored unhashed anywhere. Is there some other web service that you're used to which lets you send it pre-hashed user passwords? I've never seen this before.

Yes it is possible that a user could have a password of $2a$10$.... but how likely is that?

That's not up to me to decide. People use all kinds of passwords with all kinds of special characters and various craziness. Creating a separate, special process for already-hashed passwords just opens up an additional attack vector. It only has the potential to create problems. The reasons I've mentioned are probably some of the same reasons that no databases or other popular web services allow administering uses in this way.

There's nothing stopping you from implementing this yourself in an extension to this module, but I very strongly advise against it.

[kplatter]

SQL Server supports it see https://msdn.microsoft.com/en-us/library/ms189751.aspx

Oracle supports it see http://docs.oracle.com/cd/E17952_01/refman-5.5-en/create-user.html

MySQL supports it see http://www.techonthenet.com/mysql/users/create_user.php

I'm not sure how supplying a hashed password opens an attack vector any more than sending a clear text one does. Each of the cases above provide options for supplying it either way, but it is stored as hashed.

[tjwebb]

From the SQL Server docs:

This option should only be used for migrating databases from one server to another. Do not use the HASHED option to create new logins.

There's a good reason for this, which I've mentioned already. Here, you are migrating data, which is fine for you. This functionality is not appropriate as a general feature of this module.

Again, you're free to extend this module to suit your own custom requirements. A simpler option is to manually insert the data into the database; conceptually it makes sense: by going through sails-auth, you're trying to register a user who's technically already registered and has a password.

[kplatter]

They still provide the option, and these are not new logings but existing ones. In any case, I'm not sure how to extend since the function is on the Passport model. I guess I can create a new model for the Passport named PassportHash that has the same attributes and update the password without the rehashed password after the user is registered?

[kplatter]

I overrode the beforeCreate, and beforeUpdate functions in api\models\Passport.js, is there some other more sailsish way to do it?

[tjwebb]

Yea, currently the hashing is done in Passport.beforeCreate. You can override the beforeCreate function just by defining one in your local api/models/Passport.js inside the merge block. My implementation might look something like this:

{
  // ...
  beforeCreate: function (passport, next) {
    if (!passport.skipHash && passport.matchBcryptFormat(passport.password) {
      hashPassword(passport, next);
    }
    else {
      next();
    }
  }
}