Memory leak in DiffieHellman.set{Privat,Public}Key() and some suggestions #8377
Description
var dh=require('crypto').createDiffieHellman(512);
var pubkey=dh.generateKeys();
while (true) {
for (var i=0; i<100000; i++) {
dh.setPublicKey(pubkey);
}
console.log(process.memoryUsage());
}In https://github.com/nodejs/node/blob/master/src/node_crypto.cc#L4757 (void DiffieHellman::SetPublicKey):
diffieHellman->dh->pub_key = BN_bin2bn(
reinterpret_cast<unsigned char*>(Buffer::Data(args[0])),
Buffer::Length(args[0]),
0);but ...->pub_key might already point to allocated memory, which is never freed.
man OpenSSL says:
BIGNUM *BN_bin2bn(const unsigned char *s, int len, BIGNUM *ret);BN_bin2bn() converts the positive integer in big-endian form of length len at s
into a BIGNUM and places it in ret. If ret is NULL, a new BIGNUM is created.
So it should be enough to pass diffieHellman->dh->pub_key as third parameter, instead of 0.
DiffieHellman::SetPrivateKey, a few lines later, has the same issue.
Alternatively, we could forbid calling set{Public,Private}Key after a key exists, because the DH api does not seem to encourage reuse of DiffieHellman classes: generateKeys() always returns the same key after the first call. According to OpenSSL documentation, this is caused by dh->priv_key being non-null.
Changing generateKeys() to return a new key on every call would be breaking API change; OTOH setPrivateKey() can (currently) not be used to reset dh->priv_key to null (while freeing possibly allocated memory).
All in all this suggests that the user should create a new DiffieHellman class for each key exchange, but the nodejs binding again thwarts such efforts:
var dh=crypto.getDiffieHellman('modp18');
// Clone DiffieHellman class
console.time();
var dh2=crypto.createDiffieHellman(dh.getPrime(),dh.getGenerator());
console.timeEnd(); // --> 1451ms, because of DH_check in DiffieHellman::VerifyContext()
Therefore I'd also suggest to add another "overload" to createDiffieHellman which takes an existing DiffieHellman class and creates a new one with the same Prime and Generator, with empty keys, but without calling (slow) DH_check (the parameters have already been checked earlier!) by just copying the .verifyError result.
A side note: Given the trivial conversion (via .getPrime, as shown above) of a DiffieHellmanGroup class to a DiffieHellman class and the same C++ class under the hood, the non-existence of .set{Public,Private}Key on the DiffieHellmanGroup class obtained by .getDiffieHellman is completely arbitrary and unnecessary.
BTW, why not add some way to directly use PEM-encoded ----BEGIN DH PARAMETERS----- ... END parameters via PEM_read_bio_DHparams and a memory BIO, e.g. by overloading getDiffieHellman()? E.g. RSA already reads keys in PEM format (via OpenSSL), and the natural format of new DiffieHellman Paramaters generated by (time-consuming!) openssl dhparams [bits] is PEM, which currently has be converted manually to a format readable by nodejs...