maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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 <georg@xxxxxxxxxxx>
> 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 <openssl/evp.h>
> -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@xxxxxxxxxxx