KUDU-3210 Add lock before verifying signature

It seems there is a race condition somewhere in OpenSSL FIPS Object
Module 2.0, or at least in SafeLogic CryptoComply for Servers
1.0.2v-fips, as a certificate can get corrupted when multiple
certificates are being verified in the same time, throwing an error
during TLS handshake similar to this:

error:0407006A:rsa routines:RSA_padding_check_PKCS1_type_1:block type is not 01:rsa_pk1.c:102 error:04067072:rsa routines:RSA_EAY_PUBLIC_DECRYPT:padding check failed:rsa_eay.c:786 error:1408D07B:SSL routines:ssl3_get_key_exchange:bad signature:s3_clnt.c:2038

This means OpenSSL is trying to verify a signature with a public key
that doesn't match the private key, resulting in an invalid message.
According to a mailing list thread[1] this can happen in multi-threaded
applications, such as ours, even though we seem to have proper locking
in our verify signature function and the OpenSSL locking callbacks are
properly registered too.

Unfortunately, OpenSSL 1.0.2 doesn't guarantee complete thread
safety[2]:

"OpenSSL can generally be used safely in multi-threaded applications
provided that at least two callback functions are set, the
locking_function and threadid_func. Note that OpenSSL is not completely
thread-safe, and unfortunately not all global resources have the
necessary locks. Further, the thread-safety does not extend to things
like multiple threads using the same SSL object at the same time."

In addition to the TLS negotiation this can also get triggered when
verifying authn token signatures, further suggesting a race condition in
the underlying OpenSSL library.

This commit adds additional locking conditionally depending on a hidden
runtime flag "openssl_defensive_locking" to crypto and TLS
context/handshake which seems to prevent this from happening in both
places.

[1] https://groups.google.com/u/1/g/mailing.openssl.users/c/u0JyAuogrc0
[2] https://www.openssl.org/docs/man1.0.2/man3/threads.html

Change-Id: Ifafc7dcf91db910123276b657515e410bb7f6fcd
Reviewed-on: http://gerrit.cloudera.org:8080/16659
Reviewed-by: Wenzhe Zhou <[email protected]>
Reviewed-by: Alexey Serbin <[email protected]>
Reviewed-by: Grant Henke <[email protected]>
Tested-by: Grant Henke <[email protected]>

src/kudu/security/CMakeLists.txt

@@ -67,9 +67,9 @@ set(SECURITY_SRCS
   ca/cert_management.cc
   cert.cc
   crypto.cc
-  kerberos_util.cc
   gssapi.cc
   init.cc
+  kerberos_util.cc
   openssl_util.cc
   ${PORTED_X509_CHECK_HOST_CC}
   security_flags.cc

src/kudu/security/crypto.cc

@@ -27,6 +27,9 @@
 
 #include <functional>
 #include <memory>
+#if OPENSSL_VERSION_NUMBER < 0x10100000L
+#include <mutex>
+#endif
 #include <ostream>
 #include <string>
 
@@ -66,6 +69,10 @@ int DerWritePublicKey(BIO* bio, EVP_PKEY* key) {
   return i2d_RSA_PUBKEY_bio(bio, rsa.get());
 }
 
+#if OPENSSL_VERSION_NUMBER < 0x10100000L
+OpenSSLMutex mutex;
+#endif
+
 } // anonymous namespace
 
 template<> struct SslTypeTraits<BIGNUM> {
@@ -135,6 +142,9 @@ Status PublicKey::VerifySignature(DigestType digest,
   const EVP_MD* md = GetMessageDigest(digest);
   auto md_ctx = ssl_make_unique(EVP_MD_CTX_create());
 
+#if OPENSSL_VERSION_NUMBER < 0x10100000L
+  std::unique_lock<OpenSSLMutex> l(mutex);
+#endif
   OPENSSL_RET_NOT_OK(EVP_DigestVerifyInit(md_ctx.get(), nullptr, md, nullptr, GetRawData()),
                      "error initializing verification digest");
   OPENSSL_RET_NOT_OK(EVP_DigestVerifyUpdate(md_ctx.get(), data.data(), data.size()),
@@ -227,6 +237,9 @@ Status PrivateKey::MakeSignature(DigestType digest,
   const EVP_MD* md = GetMessageDigest(digest);
   auto md_ctx = ssl_make_unique(EVP_MD_CTX_create());
 
+#if OPENSSL_VERSION_NUMBER < 0x10100000L
+  std::unique_lock<OpenSSLMutex> l(mutex);
+#endif
   OPENSSL_RET_NOT_OK(EVP_DigestSignInit(md_ctx.get(), nullptr, md, nullptr, GetRawData()),
                      "error initializing signing digest");
   OPENSSL_RET_NOT_OK(EVP_DigestSignUpdate(md_ctx.get(), data.data(), data.size()),

src/kudu/security/openssl_util.cc

@@ -28,15 +28,18 @@
 #include <string>
 #include <vector>
 
+#include <gflags/gflags.h>
 #include <glog/logging.h>
 
+#include "kudu/gutil/macros.h"
 #include "kudu/gutil/strings/split.h"
 #include "kudu/gutil/strings/strip.h"
 #include "kudu/gutil/strings/substitute.h"
 #if OPENSSL_VERSION_NUMBER < 0x10100000L
 #include "kudu/util/debug/leakcheck_disabler.h"
 #endif
 #include "kudu/util/errno.h"
+#include "kudu/util/flag_tags.h"
 #include "kudu/util/flags.h"
 #if OPENSSL_VERSION_NUMBER < 0x10100000L
 #include "kudu/util/mutex.h"
@@ -45,6 +48,15 @@
 #include "kudu/util/status.h"
 #include "kudu/util/subprocess.h"
 
+DEFINE_bool(openssl_defensive_locking,
+            false,
+            "If enabled, cryptographic methods lock more defensively to work around thread safety "
+            "issues in certain OpenSSL versions. This makes Kudu servers or clients more stable "
+            "when running on an affected OpenSSL version, sacrificing some performance.");
+TAG_FLAG(openssl_defensive_locking, unsafe);
+TAG_FLAG(openssl_defensive_locking, hidden);
+TAG_FLAG(openssl_defensive_locking, runtime);
+
 using std::ostringstream;
 using std::string;
 using std::vector;
@@ -358,5 +370,26 @@ Status GetPasswordFromShellCommand(const string& cmd, string* password) {
   return Status::OK();
 }
 
+OpenSSLMutex::OpenSSLMutex() : locking_enabled_(FLAGS_openssl_defensive_locking) {}
+
+void OpenSSLMutex::lock() {
+  if (locking_enabled_) {
+    mutex_.lock();
+  }
+}
+
+bool OpenSSLMutex::try_lock() {
+  if (locking_enabled_) {
+    return mutex_.try_lock();
+  }
+  return true;
+}
+
+void OpenSSLMutex::unlock() {
+  if (locking_enabled_) {
+    mutex_.unlock();
+  }
+}
+
 } // namespace security
 } // namespace kudu

src/kudu/security/openssl_util.h

@@ -24,6 +24,7 @@
 
 #include <functional>
 #include <memory>
+#include <mutex>
 #include <ostream>
 #include <string>
 
@@ -198,29 +199,39 @@ class RawDataWrapper {
   c_unique_ptr<RawDataType> data_;
 };
 
+// Wrapper around std::mutex that only locks if a special
+// 'openssl_defensive_locking' flag is set to true. See the description of the
+// flag for more details.
+class OpenSSLMutex {
+ public:
+  OpenSSLMutex();
+  void lock();
+  bool try_lock();
+  void unlock();
 
-namespace internal {
+ private:
+  std::mutex mutex_;
+  const bool locking_enabled_;
+};
 
+namespace internal {
 // Implementation of SCOPED_OPENSSL_NO_PENDING_ERRORS. Use the macro form
 // instead of directly instantiating the implementation class.
 struct ScopedCheckNoPendingSSLErrors {
  public:
-  explicit ScopedCheckNoPendingSSLErrors(const char* func)
-      : func_(func) {
-    DCHECK_EQ(ERR_peek_error(), 0)
-        << "Expected no pending OpenSSL errors on " << func_
-        << " entry, but had: " << GetOpenSSLErrors();
+  explicit ScopedCheckNoPendingSSLErrors(const char* func) : func_(func) {
+    DCHECK_EQ(ERR_peek_error(), 0) << "Expected no pending OpenSSL errors on " << func_
+                                   << " entry, but had: " << GetOpenSSLErrors();
   }
   ~ScopedCheckNoPendingSSLErrors() {
-    DCHECK_EQ(ERR_peek_error(), 0)
-        << "Expected no pending OpenSSL errors on " << func_
-        << " exit, but had: " << GetOpenSSLErrors();
+    DCHECK_EQ(ERR_peek_error(), 0) << "Expected no pending OpenSSL errors on " << func_
+                                   << " exit, but had: " << GetOpenSSLErrors();
   }
 
  private:
   const char* const func_;
 };
 
-} // namespace internal
-} // namespace security
+}  // namespace internal
+}  // namespace security
 } // namespace kudu

src/kudu/security/tls_context.cc

@@ -94,6 +94,12 @@ DEFINE_int32(openssl_security_level_override, -1,
 TAG_FLAG(openssl_security_level_override, hidden);
 TAG_FLAG(openssl_security_level_override, unsafe);
 
+#if OPENSSL_VERSION_NUMBER < 0x10100000L
+namespace {
+kudu::security::OpenSSLMutex mutex;
+}  // anonymous namespace
+#endif
+
 namespace kudu {
 namespace security {
 
@@ -460,6 +466,10 @@ boost::optional<CertSignRequest> TlsContext::GetCsrIfNecessary() const {
 
 Status TlsContext::AdoptSignedCert(const Cert& cert) {
   SCOPED_OPENSSL_NO_PENDING_ERRORS;
+
+#if OPENSSL_VERSION_NUMBER < 0x10100000L
+  unique_lock<OpenSSLMutex> lock_global(mutex);
+#endif
   unique_lock<RWMutex> lock(lock_);
 
   if (!csr_) {

src/kudu/security/tls_handshake.cc

@@ -22,11 +22,15 @@
 #include <openssl/x509.h>
 
 #include <memory>
+#if OPENSSL_VERSION_NUMBER < 0x10002000L
+#include <mutex>
+#endif
 #include <string>
 
 #include "kudu/gutil/strings/strip.h"
 #include "kudu/gutil/strings/substitute.h"
 #include "kudu/security/cert.h"
+#include "kudu/security/openssl_util.h"
 #include "kudu/security/tls_socket.h"
 #include "kudu/util/net/socket.h"
 #include "kudu/util/status.h"
@@ -40,6 +44,12 @@ using std::string;
 using std::unique_ptr;
 using strings::Substitute;
 
+#if OPENSSL_VERSION_NUMBER < 0x10100000L
+namespace {
+kudu::security::OpenSSLMutex mutex;
+}  // anonymous namespace
+#endif
+
 namespace kudu {
 namespace security {
 
@@ -92,7 +102,13 @@ Status TlsHandshake::Continue(const string& recv, string* send) {
   DCHECK(n == recv.size() || (n == -1 && recv.empty()));
   DCHECK_EQ(BIO_ctrl_pending(rbio), recv.size());
 
-  int rc = SSL_do_handshake(ssl_.get());
+  int rc;
+  {
+#if OPENSSL_VERSION_NUMBER < 0x10100000L
+    std::unique_lock<OpenSSLMutex> lock(mutex);
+#endif
+    rc = SSL_do_handshake(ssl_.get());
+  }
   if (rc != 1) {
     int ssl_err = SSL_get_error(ssl_.get(), rc);
     // WANT_READ and WANT_WRITE indicate that the handshake is not yet complete.