commit 29fb3464c948f26474a69cf345d29584f15513a0
author Mario Emmenlauer <mario@emmenlauer.de> Fri Jun 04 11:50:38 2021 +0200
committer GitHub <noreply@github.com> Fri Jun 04 11:50:38 2021 +0200
tree 3361491b93d0b88019f63c8e63dc023b088b891a
parent f726ae31b9c49b4b3ce9c2f8261760ff9692641a
parent 016dbac94d5fd1ca82e78663416c221388e26db3

Merge pull request #2382 from BioDataAnalysis/bda_add_openssl_membuffer_loading

Robustness improvements when loading OpenSSL certificates

lib/cpp/src/thrift/transport/TSSLSocket.cpp

diff --git a/lib/cpp/src/thrift/transport/TSSLSocket.cpp b/lib/cpp/src/thrift/transport/TSSLSocket.cpp
index 9efc5fc..665f8f6 100644
--- a/lib/cpp/src/thrift/transport/TSSLSocket.cpp
+++ b/lib/cpp/src/thrift/transport/TSSLSocket.cpp
@@ -489,7 +489,7 @@
               && (errno_copy != THRIFT_EAGAIN)) {
             break;
           }
-        // fallthrough        
+        // fallthrough
         case SSL_ERROR_WANT_READ:
         case SSL_ERROR_WANT_WRITE:
           if (isLibeventSafe()) {
@@ -970,10 +970,12 @@
   if (strcmp(format, "PEM") == 0) {
     BIO* mem = BIO_new(BIO_s_mem());
     BIO_puts(mem, aCertificate);
-    X509* cert = PEM_read_bio_X509(mem, nullptr, 0, nullptr);
+    X509* cert = PEM_read_bio_X509(mem, nullptr, nullptr, nullptr);
     BIO_free(mem);
+    const int status = SSL_CTX_use_certificate(ctx_->get(), cert);
+    X509_free(cert);
 
-    if (SSL_CTX_use_certificate(ctx_->get(), cert) == 0) {
+    if (status != 1) {
       int errno_copy = THRIFT_GET_SOCKET_ERROR;
       string errors;
       buildErrors(errors, errno_copy);
@@ -1005,12 +1007,18 @@
                               "loadPrivateKey: either <path> or <format> is nullptr");
   }
   if (strcmp(format, "PEM") == 0) {
-    BIO* mem = BIO_new(BIO_s_mem());
+    EVP_PKEY* cert;
+    BIO* mem;
+
+    mem = BIO_new(BIO_s_mem());
     BIO_puts(mem, aPrivateKey);
-    EVP_PKEY* cert = PEM_read_bio_PrivateKey(mem, nullptr, nullptr, nullptr);
+    cert = PEM_read_bio_PrivateKey(mem, nullptr, nullptr, nullptr);
 
     BIO_free(mem);
-    if (SSL_CTX_use_PrivateKey(ctx_->get(), cert) == 0) {
+    const int status = SSL_CTX_use_PrivateKey(ctx_->get(), cert);
+    EVP_PKEY_free(cert);
+
+    if (status == 0) {
       int errno_copy = THRIFT_GET_SOCKET_ERROR;
       string errors;
       buildErrors(errors, errno_copy);
@@ -1040,12 +1048,15 @@
                               "loadTrustedCertificates: aCertificate is empty");
   }
   X509_STORE* vX509Store = SSL_CTX_get_cert_store(ctx_->get());
+
   BIO* mem = BIO_new(BIO_s_mem());
   BIO_puts(mem, aCertificate);
-  X509* cert = PEM_read_bio_X509(mem, nullptr, 0, nullptr);
+  X509* cert = PEM_read_bio_X509(mem, nullptr, nullptr, nullptr);
   BIO_free(mem);
+  const int status = X509_STORE_add_cert(vX509Store, cert);
+  X509_free(cert);
 
-  if (X509_STORE_add_cert(vX509Store, cert) == 0) {
+  if (status != 1) {
     int errno_copy = THRIFT_GET_SOCKET_ERROR;
     string errors;
     buildErrors(errors, errno_copy);
@@ -1055,10 +1066,14 @@
   if (aChain) {
     mem = BIO_new(BIO_s_mem());
     BIO_puts(mem, aChain);
-    cert = PEM_read_bio_X509(mem, nullptr, 0, nullptr);
+    cert = PEM_read_bio_X509(mem, nullptr, nullptr, nullptr);
     BIO_free(mem);
 
+    // NOTE: The x509 certificate provided to SSL_CTX_add_extra_chain_cert()
+    // will be freed by the library when the SSL_CTX is destroyed. Do not free
+    // the x509 object manually here.
     if (SSL_CTX_add_extra_chain_cert(ctx_->get(), cert) == 0) {
+      X509_free(cert);
       int errno_copy = THRIFT_GET_SOCKET_ERROR;
       string errors;
       buildErrors(errors, errno_copy);

lib/cpp/test/SecurityTest.cpp

diff --git a/lib/cpp/test/SecurityTest.cpp b/lib/cpp/test/SecurityTest.cpp
index 982a4f3..b70729c 100644
--- a/lib/cpp/test/SecurityTest.cpp
+++ b/lib/cpp/test/SecurityTest.cpp
@@ -219,6 +219,7 @@
     try
     {
         // matrix of connection success between client and server with different SSLProtocol selections
+        static_assert(apache::thrift::transport::LATEST == 5, "Mismatch in assumed number of ssl protocols");
         bool matrix[apache::thrift::transport::LATEST + 1][apache::thrift::transport::LATEST + 1] =
         {
     //   server    = SSLTLS   SSLv2    SSLv3    TLSv1_0  TLSv1_1  TLSv1_2