Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak in DiffieHellman.set{Privat,Public}Key() and some suggestions #8377

Closed
smilingthax opened this issue Sep 2, 2016 · 3 comments
Closed
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem.

Comments

@smilingthax
Copy link

smilingthax commented Sep 2, 2016

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...

@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Sep 2, 2016
@mscdex
Copy link
Contributor

mscdex commented Sep 2, 2016

/cc @nodejs/crypto

@Trott
Copy link
Member

Trott commented Jul 10, 2017

@nodejs/crypto Should this stay open?

@bnoordhuis
Copy link
Member

I added a commit to #14122 that fixes the memory leak (thanks for the bug report!)

As to the other suggestions, I don't have strong opinions except for the usual backwards compatibility concerns. Any API change or deprecation is going to be a drawn out process.

@bnoordhuis bnoordhuis added the confirmed-bug Issues with confirmed bugs. label Jul 10, 2017
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Jul 17, 2017
Fix a memory leak in dh.setPublicKey() and dh.setPrivateKey() where the
old keys weren't freed.

Fixes: nodejs#8377
addaleax pushed a commit that referenced this issue Jul 18, 2017
Fix a memory leak in dh.setPublicKey() and dh.setPrivateKey() where the
old keys weren't freed.

Fixes: #8377
PR-URL: #14122
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this issue Jul 19, 2017
Fix a memory leak in dh.setPublicKey() and dh.setPrivateKey() where the
old keys weren't freed.

Fixes: #8377
PR-URL: #14122
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants