0

So, I used the crypto module in node.js to implement a secure key generator and encrypt/decrypt functions. Knowing that security's best friend is a spotlight on the implementation, I want to throw it out here and make sure I didn't fall into any holes.

A little about my use case: I'm building an app that communicates over the internet using tcp sockets. For the time being, key exchange and identity verification are beyond my scope, I'll be generating keys and manually copying them to each end of the communication.

I plan on releasing this code as a node.js module, explicitly laying out the assumptions and limitations inherent in the choices I've made... this is intended to be a one trick pony, done right.

I've copied the relevant functions below. You can see the full code at https://github.com/jmshinn/onecrypt/blob/master/lib/onecrypt.js, which includes several other stubbed out functions, including some for managing user passwords, you can look at that stuff if you're interested but all I'm asking about here is the stuff I've copied below. I've slightly re-written these functions to post here to cut out a little optional cruft, I understand the devil could be in the details which is why I've included the link above to the actual source.

I've made comments where appropriate to detail what I'm doing and why. Please feel free to correct any erroneous assumptions I've laid out in addition to evaluating the implementation itself.

key generation:

// gen_key() is intended for manual use on the command line.
// It's entirely synchronous and not built for high volume use

var gen_key = exports.gen_key = function(params) {
    params = params || {};

    // ASSERTION: I'm using AES 256, therefore the resulting
    // ... key length should be 256 bits
    var keylen = 256; // 256 bits
    // the following are relevant to deriving keys from passwords
    // ASSUMPTION: I've universally seen a salt length of 128
    // ... bits recommended, so I went with it
    var saltlen = 128; // 128 bits
    // ASSERTION: This number is obviously dependent on the
    // ... current and recent state of hardware, though for
    // ... our purposes overkill is a virtue
    var iterations = 1048576; // default to 2^20 iterations

    // all of the relevant functions take input in bytes
    var byte_length = params.byte_length ?
        parseInt(params.byte_length) :
            (params.bit_length ?
            (parseInt(params.bit_length)/8) :
            (keylen/8));

    if (params.password) {
        // crypto.randomBytes is a CSPRNG
        var salt = params.salt?
            params.salt:
            crypto.randomBytes(saltlen/8);
        if (params.iterations) iterations =
            params.iterations;

        // NOTE: The native pbkdf2 function uses sha1, which is
        // ... perhaps not ideal.
        var key = crypto.pbkdf2Sync(params.password, salt,
            iterations, byte_length);

        // NOTE: This is tuned to output a string which can
        // ... be copied and saved, but allows for other types
        // ... of use
        return params.return_params ?
            [
                (params.raw?key:key.toString('base64')),
                {
                    salt: (params.raw?salt:salt.toString('base64')),
                    iterations: iterations,
                    keylen: byte_length,
                    algo: 'pbkdf2',
                    hash: 'hmac-sha1'
                }
            ] :
            (params.raw?key:key.toString('base64'));
    }
    else {
        // crypto.randomBytes is a CSPRNG
        var key = crypto.randomBytes(byte_length);
        // ASSUMPTION: A string of random bytes generated by a
        // ... CSPRNG is sufficient to use as a cryptographically
        // ... secure key without further processing
        return params.raw?key:key.toString('base64');
    }
};

symmetric encryption:

var encipher = exports.encipher = function(payload, key, mackey, opts, cb) {
    opts = opts || {};

    // ASSUMPTION: I've universally seen an iv length of 128
    // ... bits recommended, so I went with it
    var iv_length = 128; // bit length

    // this is intended for moderate volume usage, so make it asynchronous
    // it's not optimized for large payload sizes at this time

    // ASSUMPTION: A random IV is still preferred over a simple one even
    // ... though we're using CTR (see below), which can effectively cope
    // ... with a simple IV
    crypto.randomBytes(iv_length/8, function(err, iv) {
        if (err) cb(err);
        // for now, force the cipher used and mode
        opts.algorithm = 'aes-256';
        // ASSUMPTION: CTR mode is preferred over CBC or another mode
        // ... with padding, because it eliminates the padding-oracle
        // ... attack. I use CTR over GCM because the authentication
        // ... properties of GCM are not natively available in node.js
        opts.mode = 'ctr';
        // QUESTION: I mostly just chose this with the bigger-is-better
        // ... mindset, any reason to choose something different?
        opts.hmac_algo = 'sha512';

        // convert our payload and keys to buffers, particularly to
        // allow us to specify the encoding of our keys
        if (!Buffer.isBuffer(payload)) payload = opts.payload_encoding ?
            new Buffer(payload, opts.payload_encoding) :
            new Buffer(payload);
        if (!Buffer.isBuffer(key)) key = opts.key_encoding ?
            new Buffer(key, opts.key_encoding) :
            new Buffer(key);
        if (!Buffer.isBuffer(mackey)) mackey = opts.mackey_encoding ?
            new Buffer(mackey, opts.mackey_encoding) :
            new Buffer(mackey);

        var cipher = crypto.createCipheriv(opts.algorithm+'-'+opts.mode,
            key, iv);
        cipher.write(payload);
        cipher.end();
        var ciphertext = cipher.read();

        // ASSERTION: the key used to generate the HMAC should be
        // ... different than the key used to generate the ciphertext
        // ... though it's not explicitly enforced
        var hmac = crypto.createHmac(opts.hmac_algo, mackey);
        // ASSERTION: The HMAC should be produced from the iv+ciphertext
        hmac.write(iv);
        hmac.write(ciphertext);
        hmac.end();
        var mac = hmac.read();

        // send all of the public data needed to decrypt the message,
        // the calling application can handle how they're packaged together
        cb(null, mac, iv, ciphertext);
    });
};

symmetric decryption:

// This is strictly a functional reversal of the encipher method
// Only question has to do with timing

var decipher = exports.decipher = function(payload, key, mackey, mac,
    iv, opts) {
    opts = opts || {};

    // we pass in the pieces individually, the calling application
    // manages how the iv and mac are packaged together
    // for now, force the cipher used and mode
    opts.algorithm = 'aes-256';
    opts.mode = 'ctr';
    opts.hmac_algo = 'sha512';

    if (!Buffer.isBuffer(payload)) payload = opts.payload_encoding ?
        new Buffer(payload, opts.payload_encoding) :
        new Buffer(payload);
    if (!Buffer.isBuffer(key)) key = opts.key_encoding ?
        new Buffer(key, opts.key_encoding) :
        new Buffer(key);
    if (!Buffer.isBuffer(mackey)) mackey = opts.mackey_encoding ?
        new Buffer(mackey, opts.mackey_encoding) :
        new Buffer(mackey);
    if (!Buffer.isBuffer(iv)) iv = opts.iv_encoding ?
        new Buffer(iv, opts.iv_encoding) :
        new Buffer(iv);
    if (!Buffer.isBuffer(mac)) mac = opts.mac_encoding ?
        new Buffer(mac, opts.mac_encoding) :
        new Buffer(mac);

    var hmac = crypto.createHmac(opts.hmac_algo, mackey);
    hmac.write(iv);
    hmac.write(payload);
    hmac.end();
    // if we haven't authenticated, then we've got a problem
    // QUESTION: Presumably the calling application needs to
    // ... behave carefully to avoid enabling a timing-oracle
    // ... attack?
    if (hmac.read().toString(opts.mac_encoding) !==
        mac.toString(opts.mac_encoding))
        return new Error('Message failed to authenticate');

    var decipher = crypto.createDecipheriv(opts.algorithm+'-'+opts.mode,
        key, iv);
    decipher.write(payload);
    decipher.end();
    var plaintext = decipher.read();
    return plaintext; // return a raw buffer of our decrypted text
};

EDIT - and a use case:

> var onecrypt = require('./lib/onecrypt');
> var key = onecrypt.gen_key({ raw: true });
> var mackey = onecrypt.gen_key({ raw: true});
> var result;
> onecrypt.encipher('secret message YAY!', key, mackey, null, function(err, mac, iv, ciphertext) { result = [mac, iv, ciphertext]; });

-- in my use-case, I would then send the mac, iv, and ciphertext over an unencrypted TCP socket

> var plain = onecrypt.decipher(result[2], key, mackey, result[0], result[1]);
> console.log(plain.toString());

-- outputs:
'secret message YAY!'
7
  • Just to make sure I understand: you are using AES to encrypt a message, HMACing the message, then sending that message across a TCP connection? Are you also using TLS or SSH underneath, or are you relying on your communications protocol to be secure?
    – atk
    Jan 21, 2014 at 16:44
  • I am not using any other method for security, this is it.
    – Jason
    Jan 21, 2014 at 16:46
  • To be a little more specific, I'm using Zero-MQ sockets, and encrypting certain message frames.
    – Jason
    Jan 21, 2014 at 16:48
  • 1
    Why not use SSL? Makes your life easier, it's more vetted and you don't have to spend hours and hours trying to develop your own crypto protocol (which will likely not be completely secure). Jan 21, 2014 at 17:10
  • I started down that path, because I'm aware of the ubiquitous advice to do so. Perhaps it's just the foolhardy nature of someone who hasn't lived and breathed security, but manually implementing SSL as would be necessary in my case gets more complicated then doing it this way, and adds more overhead and complexity to implement handshaking that goes beyond what I need.
    – Jason
    Jan 21, 2014 at 18:32

2 Answers 2

3

From a cursory look, this seems reasonable or at least most of the potential implementation flaws would reside in the javascript crypto library.

As you say, it doesn't handle key management or identity authentication, which probably limits its widespread use. But if you have just 2 machines that need to communicate and can manage the keys through another means, this may be reasonable.

Other than that it seems to rely primarily on AES CTR mode with a sha-512 HMAC, which seems like a reasonable choice.

As potential improvements, I would add a handshake process where the message sender requests a random nonce from the server (in plaintext), the receiver generates and sends random nonce, and that random nonce must be present inside the message body (and this is checked to be the same as the sent nonce before the decrypted ciphertext is sent to the application). The intention of this improvement, is to prevent replay attacks (e.g., if the application says transfer $100 from Alice to Bob, you don't want replay attacks to allow an network eavesdropper to resend that message 1000 times and have it go to the application, so Alice transfers $100,000 to Bob).

(EDIT: Originally I proposed using the sender's nonce as the IV, but that presents an obvious attack; an eavesdropper keeps intercepting and sending the same nonce. Then they build up a catalog of messages that act like a reused one-time-pad, which standard attacks can be used to recover the pad (even if not recover the key) and decrypt the traffic).

Also, I would change (hmac.read().toString(opts.mac_encoding) !== mac.toString(opts.mac_encoding)) to be a constant-time string comparison, so failure time is independent of how many bytes of the MAC match.

I'd write a constant time string comparison in the following way:

function constant_time_str_cmp(str1, str2) {
    if(str1.length !== str2.length) { return false; }
    var result = 0;
    for(var i = 0; i < str1.length; i++) {
       result |= (str1.charCodeAt(i) ^ str2.charCodeAt(i));
    }
    return result === 0
}

Note this compares each letter of the two strings character by character (using xor ^ for each character comparison) and only returns 0 if all of them are identical. It doesn't early terminate like str1 === str2 and str1 !== str2 typically do, if the first differing character occurs early in the strings.

I'd also be wary of some attack on node.js that displays debugging information and inadvertantly leaks your key.

5
  • Thanks! Your handshake method actually dovetails perfectly with the base messaging protocol I'm working with (and gives me an idea to further research using SSL BIOs to see if all my work here might yet be relegated to educational purposes). And thanks also for the string compare, that's an area I was unsure in. If node is attacked in such a way, my data itself would be compromised, not just the key, no?
    – Jason
    Jan 21, 2014 at 22:03
  • I wouldn't be using the handshake to avoid replay attacks, because my intention is to send multiple messages without performing a handshake every time. Replay attacks are avoided in my application by enforcing a counter in the encrypted and mac-ed message. If the last message was 1, the next message must be 2 or it's an error. Duplicates will likely be dropped with no fanfare, missed messages will be handled in some other way, likely requesting the current state of the affected data.
    – Jason
    Jan 21, 2014 at 22:12
  • @Jason - fair enough on avoiding the handshake if you avoid replay attacks by another means (you may have said this earlier, but I didn't see it). I'm not that familiar with node or the library invoked by require('crypto'), so I was completely speculative on their flaws. Granted other web frameworks I am familiar with do often leak variables on errors in debug mode during development (and in production mode often send emails to admin's with these variables in plaintext potentially in an eavesdroppable manner); just something to think about and try checking.
    – dr jimbob
    Jan 21, 2014 at 23:20
  • Sure. My exposure to that should be fairly minimal, as I'm not using this crypto code in a web context (though there is a web context as part of the overall app, the keys should be out of scope... regardless, I'll be mindful)
    – Jason
    Jan 22, 2014 at 15:23
  • To clarify: All debugging/error logging output from the code (in normal operating context, where this code will be relevant) either goes to STDOUT or STDERR, or if I catch it, logged on the back end for access over a web interface, which will itself be accessed over HTTPS. No direct feedback will occur as a result of an encrypted message, it's a fully asynchronous messaging system. A message failure may produce a new message as a response, but only if the failure is completely understood and can be handled, otherwise it's logged at the receiving end and dropped.
    – Jason
    Jan 22, 2014 at 15:38
2

Based on the comments, your protocol is broken. Not that I actually saw a specific problem, but you have invented a new secure communications protocol. There are many books that explain how to build secure communications protocols and how they can be built wrong. Even the experts who build secure protocols make mistakes - see the most popular in use, TLS and SSL, which periodically need to be fixed.

Additionally, you don't have a threat model, protocol model, list of security goals, list of security goals not achieved, etc. All of these things are needed before a security protocol can be evaluated. If you just use a pre-existing secure protocol, you don't need to create these.

I haven't looked at your code, but here's a short list of a few errors you might have and you must address in your threat model in order to have any chance of being secure:

  • message replay
  • message re-ordering
  • message injection (mid-stream)
  • key revocation
  • key management
  • BEAST, CRIME, BREACH
  • unprotected initialization vector
  • insecure initialization vector
  • reused initialization vector



Use TLS or SSL.

17
  • I don't intend this code to handle those elements. Details to follow in subsequent comments, to make sure it all fits.
    – Jason
    Jan 21, 2014 at 18:10
  • message replay, ordering, injection are all handled inherently as part of the scope of my application, even if I was doing it over completely secure internal connections with no crypto at all.
    – Jason
    Jan 21, 2014 at 18:10
  • key revocation & management - as stated in my original topic, handled manually
    – Jason
    Jan 21, 2014 at 18:11
  • @Jason, the trouble is, if you don't have your security protocol handle all those things, then you're not secure from a malicious network node doing any of them. For example, if you don't handle message injection, I can take a previous conversation and inject it in the middle of this conversation. If the current conversation is storing data, and the previous conversation was deleting the database, now I can make you delete the database.
    – atk
    Jan 21, 2014 at 18:14
  • 1
    BEAST relies on CBC, from what I can tell. CRIME and BREACH aren't relevant specifically because I'm not using TLS compression
    – Jason
    Jan 21, 2014 at 18:17

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .