Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16659 )
Change subject: KUDU-3210 Add lock before verifying signature ...................................................................... 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 <aser...@cloudera.com> Reviewed-by: Grant Henke <granthe...@apache.org> Tested-by: Grant Henke <granthe...@apache.org> --- M src/kudu/security/CMakeLists.txt M src/kudu/security/crypto.cc M src/kudu/security/openssl_util.cc M src/kudu/security/openssl_util.h M src/kudu/security/tls_context.cc M src/kudu/security/tls_handshake.cc 6 files changed, 96 insertions(+), 13 deletions(-) Approvals: Wenzhe Zhou: Looks good to me, but someone else must approve Alexey Serbin: Looks good to me, approved Grant Henke: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/16659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ifafc7dcf91db910123276b657515e410bb7f6fcd Gerrit-Change-Number: 16659 Gerrit-PatchSet: 18 Gerrit-Owner: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
