Fix insecure use of AES-256-CTR cipher (#296)

[bnoordhuis]

Using the same IV twice with a counter mode cipher breaks the security
of the cipher. Switch to AES-256-CBC.

I made the cipher nonconfigurable because it seems more likely than not
that people will get it wrong otherwise.

This commit also drops the dependency on the secure-keys module, which
appears to be unmaintained and didn't really live up to its name.

[bnoordhuis]

Caveat emptor: CBC mode pads up to the block size so the value doesn't really look like this, it'll be longer.

[bnoordhuis]

Forgot to add, this PR uses a CBC cipher with a fixed IV. I wrote it that way to keep code that is already using aes-256-cbc from breaking but ideally a unique IV is used every time (more details).

[mhamann]

@bnoordhuis thanks for the PR!

Given that this will go into nconf 1.x, my thought is that we have a chance to do "crypto" properly. Since that forcing the CBC cipher is already a breaking change, I would recommend that we force users to either pass in a key + iv that they've derived, or at least allow a password + salt and run that through crypto.pbkdf2() before handing to createCipheriv()

Thoughts?

[bnoordhuis]

forcing the CBC cipher is already a breaking change

This PR respects existing .alg keys. It should be backwards compatible unless I botched something. I can add a test to that effect.

Of course fixing it properly is best but I figured this could go into a v0.x release and fix the immediate issue.

For v1.0, I'd recommend switching to aes-256-gcm: that provides encryption and integrity, not just encryption.

allow a password + salt and run that through crypto.pbkdf2() before handing to createCipheriv()

Ouch, we say that in our crypto docs, don't we? Yeah, so that's not quite correct in this case: it's okay to key-stretch the secret to fit the cipher's passphrase size but the IV should be unique every time, otherwise identical plaintexts encrypt to identical ciphertexts.

The logic would look something like this:

// ...
var passphrase = crypto.pbkdf2Sync(secret, salt, 1e5, 32, 'sha256');
var iv = crypto.randomBytes(16);
var ciphertext = encrypt(passphrase, iv, data);
var value = iv.toString('hex') + ciphertext.toString('hex');
// ...

I'll open a nodejs/node issue to get the documentation updated.

[bnoordhuis]

Ping. It's been five months.

[mhamann]

@bnoordhuis Thanks for the reminder. I was a bit out of commission shortly after you opened this PR, so apologies for the delay.

I want to get this merged ASAP. Will try to do some testing on my own just to ensure backwards compatibility, but would be good if you can include a test to that effect as well.

Additionally, a PR fixing this "the right way" for v1.x would be greatly appreciated as well, if you get the time/opportunity. :-)

[bnoordhuis]

but would be good if you can include a test to that effect as well

@mhamann Done. I don't know if I'll have time to write a proper fix anytime soon.

[mhamann]

Thanks @bnoordhuis. I figured you stay pretty busy ;-)

I'll see if I can get my security hat on long enough to work something up and then maybe send a review request your way.

[mhamann]

This has been resolved via #322 due to some other changes that occurred upstream. Thanks for providing the building blocks for getting this fixed!