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]>
nodejs · Jan 23, 2019 · c3409f5 · c3409f5
1 parent 1cbadd8
commit c3409f5
Show file tree

Hide file tree

Showing 2 changed files with 10 additions and 5 deletions.
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -1935,10 +1935,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);
 }

diff --git a/test/parallel/test-tls-pfx-authorizationerror.js b/test/parallel/test-tls-pfx-authorizationerror.js
@@ -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();
       }