Skip to content

Commit c3409f5

Browse files
committed
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: #24261 Fixes: https://github.com/nodejs-private/security/issues/217 PR-URL: #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 1cbadd8 commit c3409f5

File tree

2 files changed

+10
-5
lines changed

2 files changed

+10
-5
lines changed

src/node_crypto.cc

+3-3
Original file line numberDiff line numberDiff line change
@@ -1935,10 +1935,10 @@ void SSLWrap<Base>::GetCertificate(
19351935

19361936
Local<Object> result;
19371937

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

1940-
if (cert)
1941-
result = X509ToObject(env, cert.get());
1940+
if (cert != nullptr)
1941+
result = X509ToObject(env, cert);
19421942

19431943
args.GetReturnValue().Set(result);
19441944
}

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

+7-2
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-
'ECC9B856270DA9A8');
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+
'ECC9B856270DA9A8');
46+
}
4247
client.end();
4348
server.close();
4449
}

0 commit comments

Comments
 (0)