Re: [Maria-developers] Review for MDEV-10332
![](https://secure.gravatar.com/avatar/39b623a1559cf9c69ac3d9d4fb44e7fe.jpg?s=120&d=mm&r=g)
Hi, Georg! First, I've looked a bit at EVP_CIPHER_CTX_new/etc. With the CRYPTO_set_mem_functions() one can overwrite malloc/realloc/free functions that OpenSSL is using. We absolutely should do it. At the very least, OpenSSL should use my_malloc/my_realloc/my_free. But a more interesting approach would be to allocate a buffer on the stack and let our openssl m/r/f helpers take the memory from there. And print a warning with a fallback to malloc if that buffer isn't enough. This way openssl won't need mallocs is most cases. Still, I'd suggest to do this in a separate commit. In your main "OpenSSL 1.1 support" commit, just use CRYPTO_set_mem_functions to put OpenSSL through my_malloc/my_realloc/my_free. On Nov 15, Georg Richter wrote:
From 6103b311c4c5740998d342afbba9ac70f8ec5731 Mon Sep 17 00:00:00 2001 From: Georg Richter
Date: Tue, 15 Nov 2016 18:39:54 +0100 Subject: [PATCH] MDEV-10332: Added support for OpenSSL 1.1 diff --git a/mysql-test/r/openssl_6975,tlsv10.result b/mysql-test/r/openssl_6975,tlsv10.result index a65167f..e216ec2 100644 --- a/mysql-test/r/openssl_6975,tlsv10.result +++ b/mysql-test/r/openssl_6975,tlsv10.result @@ -1,11 +1,11 @@ create user ssl_sslv3@localhost; -grant select on test.* to ssl_sslv3@localhost require cipher "RC4-SHA"; +grant select on test.* to ssl_sslv3@localhost require cipher "DES-CBC3-SHA";
did you verify that after your changes all tests also work with older openssl versions and with yassl?
create user ssl_tls12@localhost; grant select on test.* to ssl_tls12@localhost require cipher "AES128-SHA256"; TLS1.2 ciphers: user is ok with any cipher ERROR 2026 (HY000): SSL connection error: sslv3 alert handshake failure diff --git a/mysys_ssl/my_crypt.cc b/mysys_ssl/my_crypt.cc index 49bd9af..d72bc72 100644 --- a/mysys_ssl/my_crypt.cc +++ b/mysys_ssl/my_crypt.cc @@ -36,9 +36,20 @@ class MyCTX { public: - EVP_CIPHER_CTX ctx; - MyCTX() { EVP_CIPHER_CTX_init(&ctx); } - virtual ~MyCTX() { EVP_CIPHER_CTX_cleanup(&ctx); ERR_remove_state(0); } + EVP_CIPHER_CTX *ctx; +#if defined(HAVE_YASSL) + MyCTX() { ctx= new EVP_CIPHER_CTX; EVP_CIPHER_CTX_init(ctx); } + virtual ~MyCTX() { EVP_CIPHER_CTX_cleanup(ctx); delete ctx; ERR_remove_state(0); } +#else + MyCTX() {ctx= EVP_CIPHER_CTX_new(); EVP_CIPHER_CTX_init(ctx); } + virtual ~MyCTX() { EVP_CIPHER_CTX_free(ctx); +#if OPENSSL_VERSION_NUMBER < 0x10100000L + ERR_remove_state(0); +#else + ERR_clear_error(); +#endif + } +#endif
No #ifdef's here, please. YaSSL-specific code from mysys_ssl/yassl.cc makes yassl to look sufficiently like openssl, so that mysys_ssl/my_crypt.cc wouldn't need any ifdefs. Please, keep it that way. And, certainly, don't put any #if's that check openssl version in the code, keep them in the headers or in the beginning of the file (there is, already #define ERR_remove_state(X) there)
virtual int init(const EVP_CIPHER *cipher, int encrypt, const uchar *key, uint klen, const uchar *iv, uint ivlen) @@ -72,7 +83,10 @@ class MyCTX_nopad : public MyCTX { public: const uchar *key; + const uchar *src; int klen; + uchar oiv[90];
90 bytes for an IV?
+ uint extra_bytes;
MyCTX_nopad() : MyCTX() { } ~MyCTX_nopad() { } @@ -285,7 +310,11 @@ int my_random_bytes(uchar *buf, int num) instead of whatever random engine is currently set in OpenSSL. That way we are guaranteed to have a non-blocking random. */ +#if OPENSSL_VERSION_NUMBER < 0x10100000L RAND_METHOD *rand = RAND_SSLeay(); +#else + RAND_METHOD *rand = RAND_OpenSSL(); +#endif
no #if's in the code, please
if (rand == NULL || rand->bytes(buf, num) != 1) return MY_AES_OPENSSL_ERROR; return MY_AES_OK; diff --git a/mysys_ssl/my_md5.cc b/mysys_ssl/my_md5.cc index 7139ea9..4885033 100644 --- a/mysys_ssl/my_md5.cc +++ b/mysys_ssl/my_md5.cc @@ -58,10 +57,11 @@ static void md5_result(MD5_CONTEXT *context, uchar digest[MD5_HASH_SIZE])
#elif defined(HAVE_OPENSSL) #include
-typedef EVP_MD_CTX MD5_CONTEXT; +typedef void MD5_CONTEXT; -static void md5_init(MD5_CONTEXT *context) +static void md5_init(MD5_CONTEXT *ctx) { + EVP_MD_CTX *context= (EVP_MD_CTX *)ctx; EVP_MD_CTX_init(context); #ifdef EVP_MD_CTX_FLAG_NON_FIPS_ALLOW /* Ok to ignore FIPS: MD5 is not used for crypto here */ @@ -123,21 +143,43 @@ void my_md5_multi(uchar *digest, ...) { va_list args; va_start(args, digest); - - MD5_CONTEXT md5_context; +#ifdef HAVE_YASSL + MD5_CONTEXT *md5_context= new MD5_CONTEXT; +#else +#if OPENSSL_VERSION_NUMBER < 0x10100000L + EVP_MD_CTX *md5_context= (EVP_MD_CTX *)alloca(sizeof(EVP_MD_CTX)); +#else + EVP_MD_CTX *md5_context= EVP_MD_CTX_new(); +#endif +#endif const uchar *str;
- md5_init_fast(&md5_context); + md5_init_fast(md5_context); for (str= va_arg(args, const uchar*); str; str= va_arg(args, const uchar*)) - md5_input(&md5_context, str, va_arg(args, size_t)); + md5_input(md5_context, str, va_arg(args, size_t));
- md5_result(&md5_context, digest); + md5_result(md5_context, digest); va_end(args); +#if defined(HAVE_YASSL) + delete md5_context; +#else +#if OPENSSL_VERSION_NUMBER >= 0x10100000L + EVP_MD_CTX_free(md5_context); +#endif +#endif }
size_t my_md5_context_size() { +#if defined(HAVE_YASSL) return sizeof(MD5_CONTEXT); +#elif OPENSSL_VERSION_NUMBER < 0x10100000L + return sizeof(EVP_MD_CTX); +#else + /* Since OpenSSL 1.1 we can't determine the length of EVP_MD_CTX anymore, + so we need to return a number (48 bytes + 16 bytes reserved) */ + return 64;
How can you be sure that it's enough? I'd just return sizeof(void*) and in md5_init stored a return value of EVP_MD_CTX_new() there. Until we do that optimization that I mentioned in the beginning of the email. And get rid of #if's please.
+#endif }
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
participants (1)
-
Sergei Golubchik