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@mariadb.com> committer: Vladislav Vaintroub <wlad@mariadb.com> 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@mariadb.org