6
\$\begingroup\$
/**
 * credential
 *
 * Easy password hashing and verification in Node.
 * Protects against brute force, rainbow tables, and
 * timing attacks.
 *
 * Cryptographically secure per-password salts prevent
 * rainbow table attacks.
 *
 * Variable work unit key stretching prevents brute force.
 *
 * Constant time verification prevents hang man timing
 * attacks.
 *
 * Created by Eric Elliott for the book,
 * "Programming JavaScript Applications" (O'Reilly)
 *
 * MIT license http://opensource.org/licenses/MIT
 */

'use strict';
var crypto = require('crypto'),
  mixIn = require('mout/object/mixIn'),

  /**
   * pdkdf(password, salt, workUnits, workKey,
   *   keyLength, callback) callback(err, hash)
   *
   * A standard to employ hashing and key stretching to
   * prevent rainbow table and brute-force attacks, even
   * if an attacker steals your password database.
   *
   * This function is a thin wrapper around Node's built-in
   * crypto.pbkdf2().
   *
   * See Internet Engineering Task Force RFC 2898
   * 
   * @param  {String}   password
   * @param  {String}   salt
   * @param  {Number}   workUnits
   * @param  {Number}   workKey
   * @param  {Number}   keyLength
   * @param  {Function} callback
   * @return {undefined}
   */
  pbkdf2 = function pbkdf2(password, salt, workUnits,
      workKey, keyLength, callback) {
    var baseline = 1000,
      iterations = (baseline + workKey) * workUnits;

    crypto.pbkdf2(password, salt,
      iterations, keyLength, function (err, hash) {
        if (err) {
          return callback(err);
        }
        callback(null, new Buffer(hash).toString('base64'));
      });
  },

  hashMethods = {
    pbkdf2: pbkdf2
  },

  /**
   * createSalt(keylength, callback) callback(err, salt)
   *
   * Generates a cryptographically secure random string for
   * use as a password salt using Node's built-in
   * crypto.randomBytes().
   *
   * @param  {Number} keyLength
   * @param  {Function} callback 
   * @return {undefined}
   */
  createSalt = function createSalt(keyLength, callback) {
    crypto.randomBytes(keyLength, function (err, buff) {
      if (err) {
        return callback(err);
      }
      callback(null, buff.toString('base64'));
    });
  },

  /**
   * toHash(password, callback) callback(err, hash)
   *
   * Takes a new password and creates a unique hash. Passes
   * a JSON encoded object to the callback.
   *
   * @param  {[type]}   password
   * @param  {Function} callback
   */
  /**
   * callback
   * @param  {Error}  Error Error or null
   * @param  {String} hashObject JSON string
   * @param  {String} hashObject.hash
   * @param  {String} hashObject.salt
   * @param  {Number} hashObject.keyLength
   * @param  {String} hashObject.hashMethod
   * @param  {Number} hashObject.workUnits
   * @return {undefined}
   */
  toHash = function toHash(password,
      callback) {
    var hashMethod = this.hashMethod,
      keyLength = this.keyLength,
      workUnits = this.workUnits,
      workKey = this.workKey;

    // Create the salt
    createSalt(keyLength, function (err, salt) {
      if (err) {
        return callback(err);
      }

      // Then create the hash
      hashMethods[hashMethod](password, salt,
          workUnits, workKey, keyLength,
          function (err, hash) {

        if (err) {
          return callback(err);
        }

        callback(null, JSON.stringify({
          hash: hash,
          salt: salt,
          keyLength: keyLength,
          hashMethod: hashMethod,
          workUnits: workUnits
        }));

      });
    }.bind(this));
  },

  /**
   * constantEquals(x, y)
   *
   * Compare two strings, x and y with a constant-time
   * algorithm to prevent attacks based on timing statistics.
   * 
   * @param  {String} x
   * @param  {String} y
   * @return {Boolean}
   */
  constantEquals = function constantEquals(x, y) {
    var result = true,
      length = (x.length > y.length) ? x.length : y.length,
      i;

    for (i=0; i<length; i++) {
      if (x.charCodeAt(i) !== y.charCodeAt(i)) {
        result = false;
      }
    }
    return result;
  },

  parseHash = function parseHash(encodedHash) {
    try {
      return JSON.parse(encodedHash);
    } catch (err) {
      return err;
    }
  },

  /**
   * verify(hash, input, callback) callback(err, isValid)
   *
   * Takes a stored hash, password input from the user,
   * and a callback, and determines whether or not the
   * user's input matches the stored password.
   *
   * @param  {String}   hash stored JSON object
   * @param  {String}   input user's password input
   * @param  {Function} callback(err, isValid)
   */
  verify = function verify(hash, input, callback) {
    var storedHash = parseHash(hash),
      workKey = this.workKey;

    if (!hashMethods[storedHash.hashMethod]) {
      return callback(new Error('Couldn\'t parse stored ' +
        'hash.'));
    }

    hashMethods[storedHash.hashMethod](input, storedHash.salt,
        storedHash.workUnits, workKey, storedHash.keyLength,
        function (err, newHash) {

      if (err) {
        return callback(err);
      }
      callback(null, constantEquals(newHash, storedHash.hash));
    });
  },

  /**
   * configure(options)
   *
   * Alter settings or set your secret `workKey`. `Workkey`
   * is a secret value between one and 999, required to verify
   * passwords. This secret makes it harder to brute force
   * passwords from a stolen database by obscuring the number
   * of iterations required to test passwords.
   *
   * Warning: Decreasing `keyLength` or `work units`
   * can make your password database less secure.
   *
   * @param  {Object} options Options object.
   * @param  {Number} options.keyLength
   * @param  {Number} options.workUnits
   * @param  {Number} options.workKey secret
   * @return {Object} credential object
   */
  configure = function configure(options) {
    mixIn(this, this.defaults, options);
    return this;
  },

  defaults = {
    keyLength: 66,
    workUnits: 60,
    workKey: parseInt(process.env.credential_key, 10) || 388,
    hashMethod: 'pbkdf2'
  };

module.exports = mixIn({}, defaults, {
  hash: toHash,
  verify: verify,
  configure: configure
});
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

At first, disclaimer - I'm not a crypto expert so maybe another answer will be better than mine, but I hope that the my comments still will be useful.

  1. The code is not protected against incorrect values. So the property keyLength and workUnits may be unintentionally set to 0 and the algorithm will fail.
  2. verify actually disclose the reason of the failure which is not correct - there should not be absolutely no difference in timing and returned value. I would rather remove returns because of the err's and always call constantEquals.
  3. It is really doubtful whether the suggested method constantEquals really helps but anyway, the time of charCodeAt may be different depending on where the character is in the character list. So constantEquals can disclose the average character index. However, I'm not sure that this will help an attacker.
\$\endgroup\$
4
  • \$\begingroup\$ Thanks Alex. For 1, both keyLength and workUnits have default values. You would have to intentionally set keyLength and workUnits to 0 to make the code fail. Does this open up any attack vectors that you can see? Good point for 2. For 3, the hash length is the only thing I can think of that would be revealed by the average character index. Does that open up any potential attack vectors? \$\endgroup\$ Commented Oct 29, 2013 at 20:41
  • \$\begingroup\$ 1. Not direct, but it may be possible to target the configuration, not the algorythm itself. So for me it looks reasonable to somehow handle the situation when these values are low (throw exception or use hardcoded safe values). 3. While bruteforcing it may be enough to test the hashes with some specific average character. Consider simple 5 character hash - if atacker knows that the average is "j", then everything below jjjjj is not a hash. This greatly (n! vs x^n) narrows down the hash space. And 4. Node js caches modules,so it may be an issue if the paramereters are diferent in these places \$\endgroup\$ Commented Oct 30, 2013 at 1:05
  • \$\begingroup\$ But 3 is only under assumption that duration of the charCodeAt depens on the character code in position. If it is not or negligible (I almost sure in it, but it should be checked very carefully) then the method is safe. BTW JS compiler or minimizer can optimize this code so it become just "==". Maybe it is better to use sum of x - y and if it is 0 then hashes are equal. \$\endgroup\$ Commented Oct 30, 2013 at 1:18
  • \$\begingroup\$ !== is strict inequality, and can't be safely optimized to ==. If it ever did happen, that would be a bug... and minimizing is not likely, since this code runs on Node, and doesn't need to be transmitted to a browser. Thanks a lot for your insightful input. =) \$\endgroup\$ Commented Nov 8, 2013 at 1:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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