Skip to content

Commit 8c7406f

Browse files
addaleaxsam-github
authored andcommitted
tls: do not free cert in .getCertificate()
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 PR-URL: nodejs#25490 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
1 parent 2d25b65 commit 8c7406f

File tree

2 files changed

+10
-5
lines changed

2 files changed

+10
-5
lines changed

src/node_crypto.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1875,10 +1875,10 @@ void SSLWrap<Base>::GetCertificate(
18751875

18761876
Local<Object> result;
18771877

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

1880-
if (cert)
1881-
result = X509ToObject(env, cert.get());
1880+
if (cert != nullptr)
1881+
result = X509ToObject(env, cert);
18821882

18831883
args.GetReturnValue().Set(result);
18841884
}

test/parallel/test-tls-pfx-authorizationerror.js

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,13 @@ const server = tls
3737
rejectUnauthorized: false
3838
},
3939
function() {
40-
assert.strictEqual(client.getCertificate().serialNumber,
41-
'FAD50CC6A07F516C');
40+
for (let i = 0; i < 10; ++i) {
41+
// Calling this repeatedly is a regression test that verifies
42+
// that .getCertificate() does not accidentally decrease the
43+
// reference count of the X509* certificate on the native side.
44+
assert.strictEqual(client.getCertificate().serialNumber,
45+
'FAD50CC6A07F516C');
46+
}
4247
client.end();
4348
server.close();
4449
}

0 commit comments

Comments
 (0)