Skip to content

Commit

Permalink
tls: do not free cert in .getCertificate()
Browse files Browse the repository at this point in the history
The documentation of `SSL_get_certificate` states that it returns
an internal pointer that must not be freed by the caller.

Therefore, using a smart pointer to take ownership is incorrect.

Refs: https://man.openbsd.org/SSL_get_certificate.3
Refs: nodejs#24261
Fixes: https://github.com/nodejs-private/security/issues/217
  • Loading branch information
addaleax committed Jan 14, 2019
1 parent 27dc74f commit 7611f1b
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 5 deletions.
6 changes: 3 additions & 3 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1963,10 +1963,10 @@ void SSLWrap<Base>::GetCertificate(

Local<Object> result;

X509Pointer cert(SSL_get_certificate(w->ssl_.get()));
X509* cert = SSL_get_certificate(w->ssl_.get());

if (cert)
result = X509ToObject(env, cert.get());
if (cert != nullptr)
result = X509ToObject(env, cert);

args.GetReturnValue().Set(result);
}
Expand Down
9 changes: 7 additions & 2 deletions test/parallel/test-tls-pfx-authorizationerror.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,13 @@ const server = tls
rejectUnauthorized: false
},
function() {
assert.strictEqual(client.getCertificate().serialNumber,
'ECC9B856270DA9A8');
for (let i = 0; i < 10; ++i) {
// Calling this repeatedly is a regression test that verifies
// that .getCertificate() does not accidentally decrease the
// reference count of the X509* certificate on the native side.
assert.strictEqual(client.getCertificate().serialNumber,
'ECC9B856270DA9A8');
}
client.end();
server.close();
}
Expand Down

0 comments on commit 7611f1b

Please sign in to comment.