Bug#35833719 Consume PEM and SSL errors after failed function call

To prevent errors from failed OpenSSL library function piling up always
consume error directly after failing function call.

The piled up old errors caused confusion when checking the errors for
later function calls.

An example was errors like:

  NDB TLS SSL_read: error:0480006C:PEM routines::no start line

When SSL_read was called all certificate handling have already completed
and there should be no reasone for PEM function errors. But this was due
to old errors reported, errors that should have been consumed earlier
during certificate handling.

In Certificate::read there certificates are read in a loop from a file
an explicit check that "no start line" error aborted the loop and not
some other error.

Change-Id: Icfabe1f06d362489551e6ec7b8d42a829f65878c

Author: zmur

storage/ndb/include/util/NodeCertificate.hpp

@@ -173,7 +173,7 @@ class Certificate {
   static bool remove(const char * dir, const char * file);
 
   /* Read from file */
-  static void read(struct stack_st_X509 *, FILE *);
+  static bool read(struct stack_st_X509 *, FILE *);
   static struct stack_st_X509 * open(const char * path);
   static struct stack_st_X509 * open(const PkiFile::PathName &);
   static struct x509_st * open_one(const char * path);  // first cert in file

storage/ndb/src/common/util/NdbSocket.cpp

@@ -130,22 +130,6 @@ int NdbSocket::set_nonblocking(int on) const {
 
 /* NdbSocket private instance methods */
 
-// ssl_close
-void NdbSocket::ssl_close() {
-  Guard guard(mutex);
-  const int mode = SSL_get_shutdown(ssl);
-  if (!(mode & SSL_SENT_SHUTDOWN)) {
-    /*
-     * Do not call SSL_shutdown again if it already been called in
-     * NdbSocket::shutdown. In that case it could block waiting on
-     * SSL_RECEIVED_SHUTDOWN.
-     */
-    SSL_shutdown(ssl);
-  }
-  SSL_free(ssl);
-  ssl = nullptr;
-}
-
 #if OPENSSL_VERSION_NUMBER >= NDB_TLS_MINIMUM_OPENSSL
 
 static void log_ssl_error(const char * fn_name)
@@ -204,6 +188,28 @@ static ssize_t handle_ssl_error(int err, const char * fn) {
   }
 }
 
+// ssl_close
+void NdbSocket::ssl_close() {
+  Guard guard(mutex);
+  const int mode = SSL_get_shutdown(ssl);
+  if (!(mode & SSL_SENT_SHUTDOWN)) {
+    /*
+     * Do not call SSL_shutdown again if it already been called in
+     * NdbSocket::shutdown. In that case it could block waiting on
+     * SSL_RECEIVED_SHUTDOWN.
+     */
+    int r = SSL_shutdown(ssl);
+    if (r < 0) {
+      // Clear errors
+      int err = SSL_get_error(ssl, r);
+      Debug_Log("SSL_shutdown(): ERR %d", err);
+      handle_ssl_error(err, "SSL_close");
+    }
+  }
+  SSL_free(ssl);
+  ssl = nullptr;
+}
+
 bool NdbSocket::update_keys(bool req_peer) const {
   if(ssl && (SSL_version(ssl) == TLS1_3_VERSION)) {
     Guard guard(mutex);
@@ -310,6 +316,7 @@ bool NdbSocket::key_update_pending() const { return false; }
 ssize_t NdbSocket::ssl_recv(char *, size_t) const { return too_old; }
 ssize_t NdbSocket::ssl_peek(char *, size_t) const { return too_old; }
 ssize_t NdbSocket::ssl_send(const char *, size_t) const { return too_old; }
+void NdbSocket::ssl_close() { }
 int NdbSocket::ssl_shutdown() const { return -1; }
 #endif
 

storage/ndb/src/common/util/NodeCertificate.cpp

@@ -37,6 +37,7 @@
 #include "ndb_global.h"  // DIR_SEPARATOR, access()
 #include "ndb_limits.h"
 
+#include "debugger/EventLogger.hpp"
 #include "portlib/ndb_localtime.h"
 #include "util/File.hpp"
 #include "util/ndb_openssl3_compat.h"
@@ -57,6 +58,30 @@ inline void _putenv(const char *a) { putenv(const_cast<char *>(a)); }
 */
 static constexpr size_t CN_max_length = 65;
 
+static void handle_pem_error(const char fn_name[])
+{
+  int err = ERR_peek_last_error();
+  if (err != 0)
+  {
+    char buffer[256];
+    ERR_error_string_n(err, buffer, 256);
+    g_eventLogger->error("NDB TLS %s: %s", fn_name, buffer);
+  }
+  else
+  {
+    // Expected some error
+    g_eventLogger->error("NDB TLS %s: Expected error but found none.", fn_name);
+  }
+  /*
+   * Check that this function are called for every failed function call that
+   * have set an error.
+   */
+#if defined(VM_TRACE) || !defined(NDEBUG) || defined(ERROR_INSERT)
+  require(ERR_get_error() != 0); // At least one error
+#endif
+  while (ERR_get_error() != 0) /* clear SSL error stack without checks */ ;
+}
+
 /*
  * Implementation of certificate and key file naming conventions
  */
@@ -299,6 +324,7 @@ EVP_PKEY * PrivateKey::open(const char * path, char * passphrase) {
   FILE * fp = fopen(path, "r");
   if(fp != nullptr) {
     PEM_read_PrivateKey(fp, &key, nullptr, passphrase);
+    if(!key) handle_pem_error("PEM_read_PrivateKey");
     fclose(fp);
   }
   return key;
@@ -319,6 +345,7 @@ bool PrivateKey::store(EVP_PKEY * key, const PkiFile::PathName & path,
     return true;
   }
   else {
+    handle_pem_error("PEM_write_PKCS8PrivateKey");
     fclose(fp);
     PkiFile::remove(path);
     return false;
@@ -488,6 +515,7 @@ SigningRequest * SigningRequest::open(const char * file) {
 SigningRequest * SigningRequest::read(FILE * fp) {
   X509_REQ * req = nullptr;
   PEM_read_X509_REQ(fp, &req, nullptr, nullptr);
+  if (req == nullptr) handle_pem_error("PEM_read_X509_REQ");
   return req ? new SigningRequest(req) : nullptr;
 }
 
@@ -514,7 +542,9 @@ bool SigningRequest::store(const char * dir) const {
 }
 
 bool SigningRequest::write(FILE * fp) const {
-  return PEM_write_X509_REQ(fp, m_req);
+  bool ok = PEM_write_X509_REQ(fp, m_req);
+  if (!ok) handle_pem_error("PEM_write_X509_REQ");
+  return ok;
 }
 
 bool SigningRequest::verify() const {
@@ -621,6 +651,7 @@ bool Certificate::write(STACK_OF(X509) * certs, FILE * fp) {
   int r = 1;
   for(int i = 0 ; i < sk_X509_num(certs) && r == 1 ; i++)
     r = PEM_write_X509(fp, sk_X509_value(certs, i));
+  if (r != 1) handle_pem_error("PEM_writeX509");
   return r;
 }
 
@@ -669,22 +700,30 @@ STACK_OF(X509) * Certificate::open(const char * path) {
 
   if(fp) {
     certs = sk_X509_new_null();
-    Certificate::read(certs, fp);
+    bool ok = Certificate::read(certs, fp);
     fclose(fp);
-  }
 
-  if(sk_X509_num(certs) == 0) {
-    sk_X509_free(certs);
-    certs = nullptr;
+    if(!ok || sk_X509_num(certs) == 0) {
+      sk_X509_pop_free(certs, X509_free);
+      certs = nullptr;
+    }
   }
 
   return certs;
 }
 
-void Certificate::read(STACK_OF(X509) * certs, FILE * fp) {
+bool Certificate::read(STACK_OF(X509) * certs, FILE * fp) {
   X509 * cert;
   while((cert = PEM_read_X509(fp, nullptr, nullptr, nullptr)) != nullptr)
     sk_X509_push(certs, cert);
+  // Expect PEM_R_NO_START_LINE error
+  int err = ERR_peek_last_error();
+  if (ERR_GET_REASON(err) == PEM_R_NO_START_LINE) {
+    while (ERR_get_error() != 0) /* clear ssl errors */ ;
+    return true;
+  }
+  handle_pem_error("PEM_read_X509");
+  return false;
 }
 
 X509 * Certificate::open_one(const char * path) {
@@ -1277,10 +1316,7 @@ int NodeCertificate::stderr_callback(int result, X509_STORE_CTX * ctx) {
 bool NodeCertificate::verify_signature(EVP_PKEY * CA_key) const {
   int r0 = X509_verify(m_x509, CA_key);
   if (r0 != 1) {
-    char error_buf[256];
-    int er = ERR_get_error();
-    ERR_error_string_n(er, error_buf, 256);
-    fprintf(stderr, "X509_verify: %d (err %d: %s)\n", r0, er, error_buf);
+    handle_pem_error("X509_verify");
     return false;
   }
 

storage/ndb/tools/sign_keys.cpp

@@ -1202,14 +1202,18 @@ int remote_key_signing(const SigningRequest * csr, EVP_PKEY * key,
   fclose(wfp);
 
   /* Read certificate chain file from coprocess */
-  Certificate::read(all_certs, rfp);
+  bool read_certs_ok = Certificate::read(all_certs, rfp);
   fclose(rfp);
 
   /* Wait up to 10 seconds for coprocess to exit.
      Return value 137 will indicate that wait() has failed. */
   int r1 = 137;
   proc->wait(r1, 10000);
 
+  /* Check if any failure when certs were read */
+  if (!read_certs_ok)
+    return 138;
+
   /* Check if any certs were read */
   int ncerts = sk_X509_num(all_certs);
   if(ncerts == 0)
@@ -1247,6 +1251,7 @@ const NodeCertificate * sign_remote(const SigningRequest * csr,
   int rs = remote_key_signing(csr, csr->key(), cluster_certs, all_certs);
   if(rs == 0)
     return NodeCertificate::from_credentials(all_certs, csr->key());
+  sk_X509_pop_free(all_certs, X509_free);
   fprintf(stderr, "Remote key signing error: %d\n", rs);
   return nullptr;
 }