maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Vladislav!

It's good to see how small the patch is, and that wolfssl is more
OpenSSL compatible than YaSSL.

Few comments below.

On May 19, Vladislav Vaintroub wrote:
> revision-id: 9e176e81b72 (mariadb-10.4.4-61-g9e176e81b72)
> parent(s): ea679c88c32
> author: Vladislav Vaintroub <wlad@xxxxxxxxxxx>
> committer: Vladislav Vaintroub <wlad@xxxxxxxxxxx>
> timestamp: 2019-05-01 11:28:56 +0100
> message:
> 
> MDEV-18531 : Use WolfSSL instead of YaSSL as "bundled" SSL/encryption library
> 
> - Add new submodule for WolfSSL
> - Build and use wolfssl and wolfcrypt instead of yassl/taocrypt
> - Use HAVE_WOLFSSL instead of HAVE_YASSL
> - Increase MY_AES_CTX_SIZE, to avoid compile time asserts in my_crypt.cc
>   (sizeof(EVP_CIPHER_CTX) is larger on WolfSSL)
>
> diff --git a/cmake/ssl.cmake b/cmake/ssl.cmake
> index aa42333a5c8..79a531ec323 100644
> --- a/cmake/ssl.cmake
> +++ b/cmake/ssl.cmake
> @@ -48,29 +48,19 @@ MACRO (CHANGE_SSL_SETTINGS string)
>  ENDMACRO()
>  
>  MACRO (MYSQL_USE_BUNDLED_SSL)
> -  SET(INC_DIRS 
> -    ${CMAKE_SOURCE_DIR}/extra/yassl/include
> -    ${CMAKE_SOURCE_DIR}/extra/yassl/taocrypt/include
> +  SET(INC_DIRS
> +    ${CMAKE_SOURCE_DIR}/extra/wolfssl/wolfssl
> +    ${CMAKE_SOURCE_DIR}/extra/wolfssl/wolfssl/wolfssl
> +    #${CMAKE_SOURCE_DIR}/extra/wolfssl/taocrypt/include

don't you want to remove these (and many below) commented lines?

>    )
> -  SET(SSL_LIBRARIES  yassl taocrypt)
> +  SET(SSL_LIBRARIES  wolfssl wolfcrypt)
>    SET(SSL_INCLUDE_DIRS ${INC_DIRS})
> -  SET(SSL_INTERNAL_INCLUDE_DIRS ${CMAKE_SOURCE_DIR}/extra/yassl/taocrypt/mySTL)
> +  SET(SSL_DEFINES "-DHAVE_OPENSSL -DHAVE_WOLFSSL  -DOPENSSL_ALL -DWOLFSSL_MYSQL_COMPATIBLE -DWC_NO_HARDEN")
> -  SET(SSL_DEFINES "-DHAVE_YASSL -DYASSL_PREFIX -DHAVE_OPENSSL -DMULTI_THREADED")
> -  SET(HAVE_ERR_remove_thread_state OFF CACHE INTERNAL "yassl doesn't have ERR_remove_thread_state")
> -  SET(HAVE_EncryptAes128Ctr OFF CACHE INTERNAL "yassl doesn't support AES-CTR")
> -  SET(HAVE_EncryptAes128Gcm OFF CACHE INTERNAL "yassl doesn't support AES-GCM")
> +  SET(HAVE_ERR_remove_thread_state ON CACHE INTERNAL "wolfssl doesn't have ERR_remove_thread_state")
> +  SET(HAVE_EncryptAes128Ctr ON CACHE INTERNAL "wolfssl does support AES-CTR")
> +  SET(HAVE_EncryptAes128Gcm OFF CACHE INTERNAL "wolfssl does not support AES-GCM")
>    CHANGE_SSL_SETTINGS("bundled")
> -  ADD_SUBDIRECTORY(extra/yassl)
> -  ADD_SUBDIRECTORY(extra/yassl/taocrypt)
> -  GET_TARGET_PROPERTY(src yassl SOURCES)
> -  FOREACH(file ${src})
> -    SET(SSL_SOURCES ${SSL_SOURCES} ${CMAKE_SOURCE_DIR}/extra/yassl/${file})
> -  ENDFOREACH()
> -  GET_TARGET_PROPERTY(src taocrypt SOURCES)
> -  FOREACH(file ${src})
> -    SET(SSL_SOURCES ${SSL_SOURCES}
> -      ${CMAKE_SOURCE_DIR}/extra/yassl/taocrypt/${file})
> -  ENDFOREACH()
> +  ADD_SUBDIRECTORY(extra/wolfssl)
>    MESSAGE_ONCE(SSL_LIBRARIES "SSL_LIBRARIES = ${SSL_LIBRARIES}")
>  ENDMACRO()
>  
> diff --git a/extra/wolfssl/CMakeLists.txt b/extra/wolfssl/CMakeLists.txt
> new file mode 100644
> index 00000000000..c1165ad6c72
> --- /dev/null
> +++ b/extra/wolfssl/CMakeLists.txt
> @@ -0,0 +1,119 @@
> +# CMakeLists.txt
> +#
> +# Copyright (C) 2006-2015 wolfSSL Inc.
> +#
> +# This file is part of wolfSSL. (formerly known as CyaSSL)
> +#
> +# wolfSSL is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# wolfSSL is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA
> +
> +SET(WOLFSSL_SRCDIR ${CMAKE_CURRENT_SOURCE_DIR}/wolfssl/src)
> +ADD_DEFINITIONS(${SSL_DEFINES})
> +ADD_DEFINITIONS(
> +  -DWOLFSSL_MYSQL_COMPATIBLE
> +  -DHAVE_ECC
> +  -DECC_TIMING_RESISTANT
> +  -DBUILDING_WOLFSSL
> +  -DHAVE_HASHDRBG
> +  -DWOLFSSL_AES_DIRECT
> +  -DWOLFSSL_SHA384
> +  -DWOLFSSL_SHA512
> +  -DWOLFSSL_SHA224
> +  -DSESSION_CERT
> +  -DKEEP_OUR_CERT
> +  -DWOLFSSL_STATIC_RSA
> +  -DWC_RSA_BLINDING
> +  -DHAVE_TLS_EXTENSIONS
> +  -DHAVE_AES_ECB
> +  -DWOLFSSL_AES_COUNTER
> +  -DNO_WOLFSSL_STUB)
> +
> +SET(WOLFSSL_SOURCES  
> +   ${WOLFSSL_SRCDIR}/crl.c
> +   ${WOLFSSL_SRCDIR}/internal.c
> +   ${WOLFSSL_SRCDIR}/keys.c
> +   ${WOLFSSL_SRCDIR}/tls.c
> +   ${WOLFSSL_SRCDIR}/wolfio.c
> +   ${WOLFSSL_SRCDIR}/ocsp.c
> +   ${WOLFSSL_SRCDIR}/ssl.c)
> +ADD_DEFINITIONS(-DWOLFSSL_LIB)
> +INCLUDE_DIRECTORIES(BEFORE ${CMAKE_CURRENT_SOURCE_DIR}/wolfssl)
> +IF(MSVC)
> +  # size_t to long truncation warning
> +  ADD_DEFINITIONS(-wd4267)
> +ENDIF()
> +
> +
> +ADD_CONVENIENCE_LIBRARY(wolfssl ${WOLFSSL_SOURCES})
> +
> +# Workaround linker crash with older Ubuntu binutils
> +# e.g aborting at ../../bfd/merge.c line 873 in _bfd_merged_section_offset
> +IF((CMAKE_SYSTEM_NAME MATCHES "Linux") AND (CMAKE_LINKER STREQUAL "/usr/bin/ld"))
> + EXECUTE_PROCESS(COMMAND ${CMAKE_LINKER} -v OUTPUT_VARIABLE LINKER_VERSION_STR)
> + STRING(REGEX MATCH "[0-9]+\\.[0-9]+" LINKER_VERSION
> +  "${LINKER_VERSION_STR}")
> + IF(LINKER_VERSION AND (LINKER_VERSION VERSION_LESS 2.25))
> +   STRING(REPLACE "-g " "-g1 " CMAKE_C_FLAGS_RELWITHDEBINFO 
> +     ${CMAKE_C_FLAGS_RELWITHDEBINFO})
> +   STRING(REPLACE "-g " "-g1 " CMAKE_C_FLAGS_DEBUG 
> +     ${CMAKE_C_FLAGS_DEBUG})
> +   STRING(REPLACE "-ggdb3 " " " CMAKE_C_FLAGS_RELWITHDEBINFO 
> +     ${CMAKE_C_FLAGS_RELWITHDEBINFO})
> +   STRING(REPLACE "-ggdb3 " " " CMAKE_C_FLAGS_DEBUG 
> +     ${CMAKE_C_FLAGS_DEBUG})
> + ENDIF()
> +ENDIF()

I would've just disabled -g\w+ unconditionally, for a simpler CMakeLists.txt
We aren't going to debug wolfssl anyway
(and if I'd need to debug it, for some weird reason, I know
how to enable debugging temporarily)

> +
> +
> +SET(WOLFCRYPT_SRCDIR ${CMAKE_CURRENT_SOURCE_DIR}/wolfssl/wolfcrypt/src)
> +SET(WOLFCRYPT_SOURCES
> +${WOLFCRYPT_SRCDIR}/aes.c
> +${WOLFCRYPT_SRCDIR}/arc4.c
> +${WOLFCRYPT_SRCDIR}/asn.c
> +#${WOLFCRYPT_SRCDIR}/blake2b.c
> +#${WOLFCRYPT_SRCDIR}/camellia.c
> +#${WOLFCRYPT_SRCDIR}/chacha.c
> +${WOLFCRYPT_SRCDIR}/coding.c
> +#${WOLFCRYPT_SRCDIR}/compress.c
> +${WOLFCRYPT_SRCDIR}/des3.c
> +${WOLFCRYPT_SRCDIR}/dh.c
> +${WOLFCRYPT_SRCDIR}/dsa.c
> +${WOLFCRYPT_SRCDIR}/ecc.c
> +${WOLFCRYPT_SRCDIR}/error.c
> +#${WOLFCRYPT_SRCDIR}/hc128.c
> +${WOLFCRYPT_SRCDIR}/hmac.c
> +${WOLFCRYPT_SRCDIR}/integer.c
> +${WOLFCRYPT_SRCDIR}/logging.c
> +#${WOLFCRYPT_SRCDIR}/md2.c
> +${WOLFCRYPT_SRCDIR}/md4.c
> +${WOLFCRYPT_SRCDIR}/md5.c
> +${WOLFCRYPT_SRCDIR}/memory.c
> +#${WOLFCRYPT_SRCDIR}/pkcs7.c
> +${WOLFCRYPT_SRCDIR}/pkcs12.c
> +#${WOLFCRYPT_SRCDIR}/poly1305.c
> +${WOLFCRYPT_SRCDIR}/pwdbased.c
> +${WOLFCRYPT_SRCDIR}/rabbit.c
> +${WOLFCRYPT_SRCDIR}/random.c
> +#${WOLFCRYPT_SRCDIR}/ripemd.c
> +${WOLFCRYPT_SRCDIR}/rsa.c
> +${WOLFCRYPT_SRCDIR}/sha.c
> +${WOLFCRYPT_SRCDIR}/sha256.c
> +${WOLFCRYPT_SRCDIR}/sha512.c
> +#${WOLFCRYPT_SRCDIR}/tfm.c
> +${WOLFCRYPT_SRCDIR}/wc_port.c
> +${WOLFCRYPT_SRCDIR}/wc_encrypt.c
> +${WOLFCRYPT_SRCDIR}/hash.c
> +${WOLFCRYPT_SRCDIR}/wolfmath.c)
> +ADD_CONVENIENCE_LIBRARY(wolfcrypt ${WOLFCRYPT_SOURCES})
> +
> diff --git a/extra/wolfssl/wolfssl b/extra/wolfssl/wolfssl
> new file mode 160000
> index 00000000000..dc313ccf6ee
> --- /dev/null
> +++ b/extra/wolfssl/wolfssl
> @@ -0,0 +1 @@
> +Subproject commit dc313ccf6ee0e99fb5a13793b82e412f396e11c6

I think we should only use wolfssl (any external project, actually)
only at release points, not at intermediate commits. 4.0.0-stable should
be ok, I guess.

And, generaly, pefer to use system globally installed library, if
available. Although in case of wolfssl (and libmarias3) I would not
bother.

> diff --git a/include/ssl_compat.h b/include/ssl_compat.h
> index c94b9671d5f..105405efff2 100644
> --- a/include/ssl_compat.h
> +++ b/include/ssl_compat.h
> @@ -17,9 +17,9 @@
>  #include <openssl/opensslv.h>
>  
>  /* OpenSSL version specific definitions */
> -#if !defined(HAVE_YASSL) && defined(OPENSSL_VERSION_NUMBER)
> +#if defined(OPENSSL_VERSION_NUMBER)
>  
> -#if OPENSSL_VERSION_NUMBER >= 0x10002000L && !defined(LIBRESSL_VERSION_NUMBER)
> +#if (OPENSSL_VERSION_NUMBER >= 0x10002000L && !defined(LIBRESSL_VERSION_NUMBER)) || defined (HAVE_WOLFSSL)

What's OPENSSL_VERSION_NUMBER in wolfssl?

>  #define HAVE_X509_check_host 1

May be move it to ssl.cmake? Where other such defines are.

>  #endif
>  
> diff --git a/include/sslopt-case.h b/include/sslopt-case.h
> index 4a8c65948cb..b129a97cc76 100644
> --- a/include/sslopt-case.h
> +++ b/include/sslopt-case.h
> @@ -29,8 +29,8 @@
>        One can disable SSL later by using --skip-ssl or --ssl=0
>      */
>        opt_use_ssl= 1;
> -    /* crl has no effect in yaSSL */  
> -#ifdef HAVE_YASSL
> +#ifdef HAVE_WOLFSSL
> +      /* CRL does not work with WolfSSL */ 

Still?

>        opt_ssl_crl= NULL;
>        opt_ssl_crlpath= NULL;
>  #endif
> diff --git a/mysys_ssl/my_crypt.cc b/mysys_ssl/my_crypt.cc
> index 2d6f5188034..1ce0c6bbc03 100644
> --- a/mysys_ssl/my_crypt.cc
> +++ b/mysys_ssl/my_crypt.cc
> @@ -18,14 +18,10 @@
>  #include <my_global.h>
>  #include <string.h>
>  
> -#ifdef HAVE_YASSL
> -#include "yassl.cc"

delete yassl.cc file, perhaps?

> -#else
>  #include <openssl/evp.h>
>  #include <openssl/aes.h>
>  #include <openssl/err.h>
>  #include <openssl/rand.h>
> -#endif
>  
>  #include <my_crypt.h>
>  #include <ssl_compat.h>
> @@ -70,8 +66,24 @@ class MyCTX
>    }
>    virtual int finish(uchar *dst, uint *dlen)
>    {
> +#ifdef HAVE_WOLFSSL
> +     /*
> +       Bug in WolfSSL - sometimes EVP_CipherFinal_ex
> +       returns success without setting destination length
> +       when it should return error.
> +       We catch it by presetting invalid value for length,
> +       and checking if it has changed after the call.
> +
> +       See https://github.com/wolfSSL/wolfssl/issues/2092
> +     */
> +    *dlen= UINT_MAX;
> +#endif

I suppose you can remove it. This is fixed as of 4.0.0-stable tag.

>      if (!EVP_CipherFinal_ex(ctx, dst, (int*)dlen))
>        return MY_AES_BAD_DATA;
> +#ifdef HAVE_WOLFSSL
> +    if (*dlen == UINT_MAX)
> +      return MY_AES_BAD_DATA;
> +#endif
>      return MY_AES_OK;
>    }
>  };
> diff --git a/plugin/file_key_management/parser.cc b/plugin/file_key_management/parser.cc
> index 13a9dfa0cb6..27277f62994 100644
> --- a/plugin/file_key_management/parser.cc
> +++ b/plugin/file_key_management/parser.cc
> @@ -103,7 +103,6 @@ openssl enc -aes-256-cbc -md sha1 -k "secret" -in keys.txt -out keys.enc
>      EVP_BytesToKey(EVP_aes_256_cbc(), EVP_sha1(), salt,
>                     secret, strlen(secret), 1, key, iv);
>  
> -   but alas! we want to support yassl too

does wolfssl have an equivalent of EVP_BytesToKey?
if not, the comment still holds.

>  */
>  
>  void Parser::bytes_to_key(const unsigned char *salt, const char *input,
> diff --git a/sql/mysqld.cc b/sql/mysqld.cc
> index bfa03aa57c1..8c3a59c55d1 100644
> --- a/sql/mysqld.cc
> +++ b/sql/mysqld.cc
> @@ -7317,13 +7315,13 @@ static int show_ssl_get_verify_mode(THD *thd, SHOW_VAR *var, char *buff,
>  {
>    var->type= SHOW_LONG;
>    var->value= buff;
> -#ifndef HAVE_YASSL
> +#ifndef HAVE_WOLFSSL
>    if( thd->net.vio && thd->net.vio->ssl_arg )
>      *((long *)buff)= (long)SSL_get_verify_mode((SSL*)thd->net.vio->ssl_arg);

no SSL_get_verify_mode or an analog in wolfssl?
I'm asking, as I see it has gone a long way towards OpenSSL compatibility

>    else
>      *((long *)buff)= 0;
>  #else
> -  *((long *)buff) = 0;
> +  *((long *)buff)= 0;
>  #endif
>    return 0;
>  }

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx