Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

In view of the recent LinkedIn breach, would anyone care to review my data access routines relating to user access?

This is a standard node.js module, accessing a Postgres database.

(function () {

    'use strict';

    var crypto = require('crypto'),
        hash = function (pass, salt) {
            var h = crypto.createHash('sha512');

            h.update(pass);
            h.update(salt);

            return h.digest('base64');
        };

    module.exports.getUser = function (conn, email, password, callback) {
        conn.query({
            name: 'getUser',
            text: 'SELECT "id", "passwordHash" FROM "user" WHERE "email" = $1 LIMIT 1',
            values: [email]
        }, function (err, result) {
            if (err) {
                throw err;
            }
            if (result.rows.length === 0) {
                callback(null);
            } else {
                var newHash = hash(password, email),
                    row = result.rows[0];

                if (row.passwordHash === newHash) {
                    callback({id: row.id, email: email});
                } else {
                    callback(null);
                }
            }
        });
    };

    module.exports.addUser = function (conn, email, password, callback) {
        var newHash = hash(password, email);

        conn.query({
            name: 'addUser',
            text: 'INSERT INTO "user" ("email", "passwordHash") VALUES ($1, $2) RETURNING "id"',
            values: [email, newHash]
        }, function (err, result) {
            var id;

            if (err) {
                console.error(err);
                callback(null);
            } else {
                id = result.rows[0].id;

                callback({id: id, email: email});
            }
        });
    };

}());
share|improve this question
1  
The code looks fine to me, but I wouldn't use the email as the hash salt because it doesn't provide enough entropy. I always use a GUID as a salt because it ensures that bits(password+salt) > bits(hash()) – Bill Barry Jun 6 '12 at 14:33
    
OK, so perhaps where I do h.update(pass); h.update(salt);, I should also add h.update(<secret-guid>); – stusmith Jun 6 '12 at 15:07
up vote 7 down vote accepted

I do not see anything immediately wrong with your addUser() and getUser() implementations.
Your hash() implementation could be improved in two areas though.

  1. Salt
    As already indicated by Bill Barry in the comments, your should use a better salt.
    Some rules to keep in mind when handling salts,

    • Salts should be unique per password.
      Every password should have a unique salt. Sharing a salt reduces its effectiveness.

    • Salts should be unpredictable
      You should generate a salt using a Cryptographically Secure Pseudo-Random Number Generator (CSPRNG).

      You may want to try using crypto.randomBytes(size, [callback]) if available.

    • Salts should be large
      PKCS#5 recommends a salt be no smaller than 64-bits (8 bytes).
      I like to start with 128-bit salts (16 bytes), favoring higher salt-lengths for more passwords and better security.

    • Salts are not secret
      Although purposefully disclosing the salt isn't a good idea, you can store them in cleartext.

  2. Key stretching / Iterations
    Wikipedia has a good description on Key Stretching,

    key stretching refers to techniques used to make a possibly weak key, typically a password or passphrase, more secure against a brute force attack by increasing the time it takes to test each possible key.

    The crypto module does not appear to offer key stretching natively for sha512.

    You should look into using crypto.pbkdf2(password, salt, iterations, keylen, callback) which uses HMAC-SHA1. You shouldn't use SHA1 on its own, but pbkdf2 using HMAC-SHA1 is still secure.
    See Password-Based Key Derivation Function 2 for more information.

    You will want to set the number of iterations as high as possible while still maintaining an acceptable responsiveness for your application. Though PKCS#5 recommends a minimum of 1000 iterations, 10000+ iterations is fairly typical; computers are only getting faster.

share|improve this answer

@Jared Paolin's answer is good.

Expanding on it a little:

  1. The salt should be exactly the same length as the hash.
  2. When you verify, always use a constant-time algorithm so that attackers can't play hangman to guess passwords. If it takes longer to get a response for a mostly right password, attackers can use that data to progressively guess the password one letter at a time.
  3. For 2014, you need at least 128,000 iterations to be safe with PDKDF2 / HMAC-SHA1. Make sure to pass the work load in as a parameter to PDKDF2, so you can change it as processors get faster. Double the iterations every 2 years.
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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