Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

1 Answer

up vote 4 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

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.