crypto: Fix "leak" in SecureContext::SetCert()

[lsegal]

Node.js borrows a function SSL_CTX_use_certificate_chain from OpenSSL which handles loading of the certificate bundle provided from a buffer object. However, it looks like the code taken from OpenSSL was originally hanging onto the certificate resource created by SSL_CTX_use_certificate, but it does not seem to be used by Node.js, and not freeing it causes a large amount of memory to be generated and lost (though eventually partially returned in a full GC run). I'm not sure if this is necessarily a "memory leak", but at the very least it is causing less-than-efficient memory usage. The patch frees the resource creating a much more stable memory usage footprint.

In order to illustrate the "leak" I wrote a minimal reproduction case listed below which causes system memory usage to skyrocket when creating crypto credentials. Note that SetCert() is exposed through crypto.createCredentials(), but for clarity, this minimal test case shows the actual code path within createCredentials() that causes the "leak":

var crypto = require('crypto');
var fs = require('fs');
var binding = process.binding('crypto');
var SecureContext = binding.SecureContext;

// load any cert bundle (we use one similar to Firefox)
var cert = fs.readFileSync(__dirname + '/ca-bundle.crt');

var numTimes = 1;
function run() {
  var mem = process.memoryUsage();
  console.log(numTimes + ' ' + mem.rss + ' ' + mem.heapUsed +
              ' ' + mem.heapTotal);

  // the following 3 lines is equivalent to calling:
  //   crypto.createCredentials({cert: cert});
  var context = new SecureContext();
  context.init();
  context.setCert(cert);

  if (numTimes++ < 5000) setTimeout(run, 0);
}

console.log("Attempt RSS \"Heap Used\" \"Heap Total\"");
run();

To clarify, the leak is only caused when crypto.createCredentials() is called with the cert option filled, which causes it to call SecureContext.setCert(). Without this, the memory usage looks normal.

The following graphs show memory usage when running the above script before and after applying the patch in this pull request:

(Note: these graphs are on a logarithmic scale)

Before patch:

After patch:

I also stress tested this implementation to ensure that freeing memory in the implementation does not cause any segfault/side effects-- I cannot guarantee that this will not cause a segfault, but in my experimentation, it did not cause any. Please try this out on your machines and verify my patch! :)

Finally, my pull request applies this patch on master, but given that it is a backwards compatible change, I believe that it can (and probably should) be backported, since the current behaviour can cause critically poor memory performance.

[Nodejs-Jenkins]

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

• Loren Segal

Please see CONTRIBUTING.md for more information

[lsegal]

(Note that I just submitted the CLA form prior to submitting the PR, did it go through?)

[bnoordhuis]

Have you verified that it actually works? The way I read your patch, you're basically doing a SSL_CTX_clear_extra_chain_certs after every SSL_CTX_add_extra_chain_cert call, which kind of defeats the purpose.

[indutny]

Additional nit: this stuff is performed only once at server initialization and won't really leak anything in production, unless you're using thousands of TLS-servers on one physical machine.

[lsegal]

@bnoordhuis yes I have tested this. Admittedly the fix seems odd to me, but I did verify the patch as illustrated by the pre/post graphs above. I also verified that it actually validates a valid and invalid certificate by sending live requests to servers with correct and incorrect SSL certs. I will do some more tests on this to verify that I'm not missing something here.

@indutny each call to crypto.createCredentials() calls the above function. You can see this and even test it yourself in the above minimal reproduction case. Note that crypto.createCredentials can be called many times in an application, either directly, since it's a public function of the crypto module, or internally by https.request(), which calls this whenever an Agent uses a custom certificate chain. It's very easy to reproduce this leak in production (we actually found this leak from users who reported it in production environments) just by sending multiple client requests over HTTPS in a long running process.

[bnoordhuis]

I've been grepping through openssl and I'm pretty sure my previous comment is correct: your patch immediately throws away the new certificate chain, effectively making it a null operation.

Now, I'm willing to believe that hanging on to the certificate chain after the SSL handshake is complete "leaks" memory (in the sense that it's not reclaimed until the SecureContext object is) but I don't see a way around that. Freeing it after the handshake is probably not the solution because you're going to need it again when you or the peer renegotiates the SSL session.

I wonder how much of an issue this is in the real world: the overhead per certificate is a few hundred bytes plus the size of the certificate, probably somewhere between 1 and 8 kB.

It's presumably more noticeable in v0.10 due to how V8's garbage collector works now (in a nutshell: more lazily). We could give it a hint with V8::AdjustAmountOfExternalAllocatedMemory() so it knows to GC more often. The issue of course is that we don't know the exact amount of memory a certificate uses. It would be a guess on top of a heuristic.