4
\$\begingroup\$

I use the native crypto . I use the pbkdf2 and the randomBytes for salting, and the timingSafeEqual to check for the password validity when logging in.

I wrote the following functions, based on various examples and the aforementioned APIs and functions. Here is my code

(stack : node 8.11.1 + express 4.16.3 + PostgreSQL 10)

const crypto = require('crypto');

var config = {
  hashBytes: 32,
  saltBytes: 16,
  iterations: 10000,
  digest : 'SHA512'
};

const  hashPassword = (password) =>{
  return new Promise((resolved, rejeted)=>{
    crypto.randomBytes(config.saltBytes, (err, salt) =>{
      if (err){return rejeted('Error while sign in');}
      salt = salt.toString('hex');
      crypto.pbkdf2(password, salt, config.iterations, config.hashBytes, config.digest,(err, hash) =>{
        if (err) {return rejeted('Error while sign in');}
        hash = hash.toString('hex');
        const combined = [salt, hash, config.iterations, config.hashBytes, config.digest].join('$');
        resolved(combined);
      });//closes pbkdf2
    });//closes randomBytes
  })//closes Promise
}//closes hashPassword

const  verifyPassword = (password,original) =>{
  return new Promise((resolved, rejeted)=>{
    let hash = original.split('$')[1];
    let salt = original.split('$')[0];
    let iterations = Number(original.split('$')[2]);
    let hashBytes = Number(original.split('$')[3]);
    let digest = original.split('$')[4];
    hash = Buffer.from(hash, 'hex');

    crypto.pbkdf2(password, salt, iterations, hashBytes, digest, (err, verify) =>{
      if (err){return rejeted('Error while logging in');}
      let equals = crypto.timingSafeEqual(hash, verify);
      resolved(equals);
    });
  })//closes Promise
}//closes verifyPassword

exports.hashPassword = hashPassword;
exports.verifyPassword = verifyPassword;

This works, but, here is what bothers me :

  1. Is it ok if I save the combined from the hashPassword as text in the DB? (column type text)
  2. I have to convert from text back in Buffer in the verifyPassword -- hash = Buffer.from(hash, 'hex'); part (that's because timingSafeEqual only accepts buffer). Is this ok? Does it take a lot of time?
  3. Is using text ok, or should I use and save buffer for this?

Any suggestions? Thank you

\$\endgroup\$
1
\$\begingroup\$
  1. Is it ok if I save the combined from the hashPassword as text in the DB? (column type text)

Yes, that's OK, if you use this to store password hashes. If you use it as a (encryption) key then you should avoid text, as it can be hard to destroy the result.

  1. I have to convert from text back in Buffer in the verifyPassword -- hash = Buffer.from(hash, 'hex'); part (timingSafeEqual only accepts buffer). Is this ok? Does it take a lot of time?

That's OK. Compared to PBKDF2 almost nothing takes a lot of time.

  1. Is using text ok, or should I use and save buffer for this?

No text is OK, see question #1.


With regards to the code, some notes:

  • Calling split multiple times is not a good idea, call it once and store the intermediate result.
  • As there are no checks on the results after the split, the hash string representation could be altered without notice (impact depends on how the code is used).
  • You could use just a counter to retrieve the various parts after split, and at least create the variables in order (e.g. salt before hash) - storing the hash last makes most sense to me.

I was afraid that you were implementing PBKDF2 yourself, but you seem to be correctly using the proper crypto calls.


A different idea of handling this (for you to ponder over).

Define your protocol somewhere and store a protocol version in your hash string. You could even use that to replace the salt size, iterations, hash type etc.. Then if you choose a higher iteration count you could just update your protocol version.

\$\endgroup\$
  • \$\begingroup\$ Hi there again. Can you please elaborate a little more on what you mean by "If you use it as a (encryption) key then you should avoid text, as it can be hard to destroy the result."? What an example would be? For now I am using text to save password, salt, the iterations, the hashBytes and the digest function. Also, what do you mean by "As there are no checks on the results after the split, the hash string representation could be altered without notice" ? How can the hash string representation could get altered, since it is saved in the database? Thanks \$\endgroup\$ – slevin Jun 1 '18 at 18:18
  • \$\begingroup\$ Also, what is the order that you suggest for the combined ? Should it be salt, config.iterations, config.hashBytes, config.digest, hash? Is there a common pattern here that I should follow for the order? Is this order more common (even though this is a PHP doc, I guess the order is common in all databases and implementations). Thanks again \$\endgroup\$ – slevin Jun 1 '18 at 18:23
  • \$\begingroup\$ 1. Strings in managed languages after often internalized and hard to destroy. A character array or byte array are generally not internalized and mutable do you can zero them after use. 2. Security wise it doesn't matter but yes the PHP order makes sense. Is goes from relatively constant to more variable and you need those values in the given order. \$\endgroup\$ – Maarten Bodewes Jun 1 '18 at 19:36
  • \$\begingroup\$ Sorry Maarten, but some details are still unclear. Let me rephrase. 1] What is an encryption key? Its used by the pbkdf2 to encrypt/decrypt data? 2] You say : " If you use it as a (encryption) key then you should avoid text, as it can be hard to destroy the result." I should avoid text if I use what as an encryption key? The "combined" of my "hashPassword"? 3] I still dont get how can a hash string representation could be altered without notice ? If there is any chance that it can get altered, then there is a serious issue in the "verifyPassword" function. Thanks again. \$\endgroup\$ – slevin Jun 2 '18 at 18:29
  • \$\begingroup\$ 1 & 2: PBKDF2 creates a key, which can be used as an encryption key. PBKDF2's output consists of bytes, which can be encoded as text. However, I'm warning you that key material should be destroyed when they're not required anymore, and strings are notoriously hard to destroy / zero. This is also why the password input is often supplied as a character array rather than a string, e.g. in the Java and .NET API's. \$\endgroup\$ – Maarten Bodewes Jun 6 '18 at 12:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.