You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by gr...@apache.org on 2020/11/04 01:52:40 UTC
[kudu] branch master updated: KUDU-3210 Add lock before verifying
signature
This is an automated email from the ASF dual-hosted git repository.
granthenke pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new f9f3189 KUDU-3210 Add lock before verifying signature
f9f3189 is described below
commit f9f3189a6dbe0636d578d80b1d8e60cf7b2e6686
Author: Attila Bukor <ab...@apache.org>
AuthorDate: Tue Oct 20 18:04:38 2020 +0200
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 <wz...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Grant Henke <gr...@apache.org>
Tested-by: Grant Henke <gr...@apache.org>
---
src/kudu/security/CMakeLists.txt | 2 +-
src/kudu/security/crypto.cc | 13 +++++++++++++
src/kudu/security/openssl_util.cc | 33 +++++++++++++++++++++++++++++++++
src/kudu/security/openssl_util.h | 33 ++++++++++++++++++++++-----------
src/kudu/security/tls_context.cc | 10 ++++++++++
src/kudu/security/tls_handshake.cc | 18 +++++++++++++++++-
6 files changed, 96 insertions(+), 13 deletions(-)
diff --git a/src/kudu/security/CMakeLists.txt b/src/kudu/security/CMakeLists.txt
index 7abaabb..bfe7f87 100644
--- a/src/kudu/security/CMakeLists.txt
+++ b/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
diff --git a/src/kudu/security/crypto.cc b/src/kudu/security/crypto.cc
index 2deb348..a6acbc6 100644
--- a/src/kudu/security/crypto.cc
+++ b/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()),
diff --git a/src/kudu/security/openssl_util.cc b/src/kudu/security/openssl_util.cc
index 8967557..7198db3 100644
--- a/src/kudu/security/openssl_util.cc
+++ b/src/kudu/security/openssl_util.cc
@@ -28,8 +28,10 @@
#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"
@@ -37,6 +39,7 @@
#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
diff --git a/src/kudu/security/openssl_util.h b/src/kudu/security/openssl_util.h
index e5b522e..113e7a9 100644
--- a/src/kudu/security/openssl_util.h
+++ b/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
diff --git a/src/kudu/security/tls_context.cc b/src/kudu/security/tls_context.cc
index 6f4d19c..76c9a52 100644
--- a/src/kudu/security/tls_context.cc
+++ b/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_) {
diff --git a/src/kudu/security/tls_handshake.cc b/src/kudu/security/tls_handshake.cc
index 52162c1..3c3b80e 100644
--- a/src/kudu/security/tls_handshake.cc
+++ b/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.