Re: [Maria-developers] c80991c79f7: MDEV-25785 Add support for OpenSSL 3.0
Hi, Vladislav! few questions below: On Nov 18, Vladislav Vaintroub wrote:
revision-id: c80991c79f7 (mariadb-10.6.1-213-gc80991c79f7) parent(s): cee33f1ab7c author: Vladislav Vaintroub committer: Vladislav Vaintroub timestamp: 2021-11-09 02:04:22 +0100 message:
MDEV-25785 Add support for OpenSSL 3.0
Summary of changes
- MD_CTX_SIZE is increased
- EVP_CIPHER_CTX_buf_noconst(ctx) does not work anymore, points to nobody knows where. The assumption made previously was that (since the function does not seem to be documented) was that it points to the last partial source block. Add own partial block buffer for NOPAD encryption instead
- SECLEVEL in CipherString in openssl.cnf had been downgraded to 0, from 1, to make TLSv1.0 and TLSv1.1 possible
The definition of SECLEVEL is: Level 0 Everything is permitted. This retains compatibility with previous versions of OpenSSL. Level 1 The security level corresponds to a minimum of 80 bits of security. Any parameters offering below 80 bits of security are excluded. As a result RSA, DSA and DH keys shorter than 1024 bits and ECC keys shorter than 160 bits are prohibited. All export cipher suites are prohibited since they all offer less than 80 bits of security. SSL version 2 is prohibited. Any cipher suite using MD5 for the MAC is also prohibited. Note that signatures using SHA1 and MD5 are also forbidden at this level as they have less than 80 security bits. Only at level 3 does it say "TLS versions below 1.1 are not permitted." I don't see why you had to change from 1 to 0. Do we have "signatures using SHA1 and MD5"?
- ctx_buf buffer now must be aligned to 16 bytes with openssl( previously with WolfSSL only), ot crashes will happen
- updated aes-t , to be better debuggable using function, rather than a huge multiline macro added test that does "nopad" encryption piece-wise, to test replacement of EVP_CIPHER_CTX_buf_noconst
diff --git a/cmake/ssl.cmake b/cmake/ssl.cmake index 7c2488be8bd..64c93ff9b4f 100644 --- a/cmake/ssl.cmake +++ b/cmake/ssl.cmake @@ -139,9 +139,20 @@ MACRO (MYSQL_CHECK_SSL) SET(SSL_INTERNAL_INCLUDE_DIRS "") SET(SSL_DEFINES "-DHAVE_OPENSSL")
+ FOREACH(x INCLUDES LIBRARIES DEFINITIONS) + SET(SAVE_CMAKE_REQUIRED_${x} ${CMAKE_REQUIRED_${x}}) + ENDFOREACH()
why do you set/restore them?
+ # Silence "deprecated in OpenSSL 3.0" + IF((NOT OPENSSL_VERSION) # 3.0 not determined by older cmake + OR NOT(OPENSSL_VERSION VERSION_LESS "3.0.0")) + SET(SSL_DEFINES "${SSL_DEFINES} -DOPENSSL_API_COMPAT=0x10100000L") + SET(CMAKE_REQUIRED_DEFINITIONS -DOPENSSL_API_COMPAT=0x10100000L) + ENDIF()
This is just postponing the inevitable. They'll drop the old API eventually. As far as I understand the internals were changed in a way that doesn't fit the old API. Is there some safe subset of OpenSSL API that works both in 1.0 and in 3.0 ? It might be more future proof to use only that. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Vladislav! On Nov 19, Vladislav Vaintroub wrote:
Hi Serg,
Re. The SECLEVEL 0 is , as I wrote necessary to get tests TLSv1.0 and TLSv1.1 running
I read in openssl/NEWS.md at master · openssl/openssl (github.com) that “SSL 3, TLS 1.0, TLS 1.1, and DTLS 1.0 only work at security level 0”
this means the documentation is wrong? https://www.openssl.org/docs/man3.0/man3/SSL_CTX_get_security_level.html says what I quoted, TLS versions < 1.1 are not permitted only at level 3. Do you mean if you change from 0 to 1, tests start to fail? If the answer is "yes", please mention this in the commit comment, like - SECLEVEL in CipherString in openssl.cnf had been downgraded to 0, from 1, to make TLSv1.0 and TLSv1.1 possible (according to https://github.com/openssl/openssl/blob/openssl-3.0.0/NEWS.md even though the manual for SSL_CTX_get_security_level claims that it should not be necessary)
+ FOREACH(x INCLUDES LIBRARIES DEFINITIONS) + SET(SAVE_CMAKE_REQUIRED_${x} ${CMAKE_REQUIRED_${x}}) + ENDFOREACH()
why do you set/restore them?
Why do I change CMAKE_REQUIRED_{INCLUDES,LIBRARIES,DEFINITIONS}, and restore them is because of how CHECK_SYMBOL_EXISTS work. It is not like I added it just for OpenSSL 3.0 . I added CMAKE_REQUIRED_DEFINITIONS, because I use that -D at runtime, and it might, at least theoretically affect the behaviour . Whether the outcome of the test is really the same, I did not even check, and I did not think much about it. It is just the safest thing is to use the same flag for the system checks, as it would be used later during compilation. I save and restore the CMake variables, because it is again, safer than former “overwrite variables, later make them empty”, which could lead to unwanted side-effects during cmake run,
I think the "former" (or, rather existing, conventional) approach is easier to use - assume all CMAKE_REQUIRED_* variables are empty, set what you must, and clear them afterwards. We do it everywhere. Why would one ever want for them to be set to some specific non-empty values between CHECK_SYMBOL_EXISTS invocations?
This is just postponing the inevitable. They'll drop the old API eventually. As far as I understand the internals were changed in a way that doesn't fit the old API.
Somehow it did fit, as it works, and the build passes, and the tests pass.
The unique warnings are about SHA{1|224|256|384|512}_Init/Final/Update, DES_{ede3_cbc_encrypt|set_key_unchecked}, and DH_new|DH_free.
Once they remove it, we can remove it, too, not a huge deal. I propose to keep this patch at minimum, delaying the beautifications and clean-ups are until next time, when the inevitable problem arises, say, in 5 years, I do not expect that to be sooner.
It's not that exciting a task to do it many times, I think. Might be better just to do it once, and be done with it. A good argument not to do it (that is, to do it in two steps) could be, that new API that one has to use in 3.0 is incompatible with 1.1, so we'd need to double the number of #ifdef's If this is the case then, indeed, better to wait, say, 5 years, as you suggest, for OpenSSL 1.x to reach EOL
Note- there does not seem to be an non-deprecated replacement for DES_set_key_unchecked, so we have to have this anti-deprecate setting this or that way, anyway.
I think we should rather deprecate DES_ENCRYPT and DES_DECRYPT functions. It should've been done long time ago. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
From: Sergei Golubchik Sent: Thursday, 18 November 2021 20:20 To: Vladislav Vaintroub Cc: maria-developers@lists.launchpad.net Subject: Re: [Maria-developers] c80991c79f7: MDEV-25785 Add support for OpenSSL 3.0
Hi, Vladislav!
few questions below:
On Nov 18, Vladislav Vaintroub wrote:
revision-id: c80991c79f7 (mariadb-10.6.1-213-gc80991c79f7) parent(s): cee33f1ab7c author: Vladislav Vaintroub committer: Vladislav Vaintroub timestamp: 2021-11-09 02:04:22 +0100 message:
MDEV-25785 Add support for OpenSSL 3.0
Summary of changes
- MD_CTX_SIZE is increased
- EVP_CIPHER_CTX_buf_noconst(ctx) does not work anymore, points to nobody knows where. The assumption made previously was that (since the function does not seem to be documented) was that it points to the last partial source block. Add own partial block buffer for NOPAD encryption instead
- SECLEVEL in CipherString in openssl.cnf had been downgraded to 0, from 1, to make TLSv1.0 and TLSv1.1 possible
The definition of SECLEVEL is:
Level 0
Everything is permitted. This retains compatibility with previous versions of OpenSSL.
Level 1
The security level corresponds to a minimum of 80 bits of security. Any parameters offering below 80 bits of security are excluded. As a result RSA, DSA and DH keys shorter than 1024 bits and ECC keys shorter than 160 bits are prohibited. All export cipher suites are prohibited since they all offer less than 80 bits of security. SSL version 2 is prohibited. Any cipher suite using MD5 for the MAC is also prohibited. Note that signatures using SHA1 and MD5 are also forbidden at this level as they have less than 80 security bits.
Only at level 3 does it say "TLS versions below 1.1 are not permitted."
I don't see why you had to change from 1 to 0. Do we have "signatures using SHA1 and MD5"?
- ctx_buf buffer now must be aligned to 16 bytes with openssl( previously with WolfSSL only), ot crashes will happen
- updated aes-t , to be better debuggable using function, rather than a huge multiline macro added test that does "nopad" encryption piece-wise, to test replacement of EVP_CIPHER_CTX_buf_noconst
diff --git a/cmake/ssl.cmake b/cmake/ssl.cmake index 7c2488be8bd..64c93ff9b4f 100644 --- a/cmake/ssl.cmake +++ b/cmake/ssl.cmake @@ -139,9 +139,20 @@ MACRO (MYSQL_CHECK_SSL) SET(SSL_INTERNAL_INCLUDE_DIRS "") SET(SSL_DEFINES "-DHAVE_OPENSSL") + FOREACH(x INCLUDES LIBRARIES DEFINITIONS) + SET(SAVE_CMAKE_REQUIRED_${x} ${CMAKE_REQUIRED_${x}}) + ENDFOREACH()
why do you set/restore them?
+ # Silence "deprecated in OpenSSL 3.0" + IF((NOT OPENSSL_VERSION) # 3.0 not determined by older cmake + OR NOT(OPENSSL_VERSION VERSION_LESS "3.0.0")) + SET(SSL_DEFINES "${SSL_DEFINES} -DOPENSSL_API_COMPAT=0x10100000L") + SET(CMAKE_REQUIRED_DEFINITIONS -DOPENSSL_API_COMPAT=0x10100000L) + ENDIF()
This is just postponing the inevitable. They'll drop the old API eventually. As far as I understand the internals were changed in a way that doesn't fit the old API.
Is there some safe subset of OpenSSL API that works both in 1.0 and in 3.0 ? It might be more future proof to use only that.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Vladislav! On Nov 21, Vladislav Vaintroub wrote:
H Serg,
I read in openssl/NEWS.md at master · openssl/openssl (github.com) that “SSL 3, TLS 1.0, TLS 1.1, and DTLS 1.0 only work at security level 0” this means the documentation is wrong?
This means exactly that. I changed th comment like you suggested.
I think the "former" (or, rather existing, conventional) approach is easier to use - assume all CMAKE_REQUIRED_* variables are empty, set
I’m not doing to dive into discussion here, so I changed it the way you like more, especially since this makes the patch even smaller
It's not that exciting a task to do it many times, I think. Might be better just to do it once, and be done with it. A good argument not to do it (that is, to do it in two steps) could be, that new API that one has to use in 3.0 is incompatible with 1.1, so we'd need to double the number of #ifdef's If this is the case then, indeed, better to wait, say, 5 years, as you suggest, for OpenSSL 1.x to reach EOL
A good argument is not to change something if it is not broken, and also keep the patch sizes to minimum, which I think happened here.
This patch is hopefully applicable back to 10.2 . I do not think we’d have a zero porting work for any new version of OpenSSL, since they like to change API to the new API-du-jour.
That's true. 1.0->1.1 wasn't exactly trivial.
But, a good argument to change anything is to remove the ifdefs. I think it would be possible for 10.4+, in case WolfSSL’s OpenSSL compatibility is good enough. I think ideally, it would be done when 10.3 is no more supported, so that YASSL , at least is no more concern. By that time, OpenSSL 1.0 might no be supported either.
Okay. I don't have a strong opinion about it. Let's use your minimal approach, as long as it works. Which, I guess, means practically, as long as no major distro ships openssl 3.0 with compatibility API disabled.
I also think too much time is spend on discussing SHAx hashing here, it is a relatively small thing, after all.
I don't understand what you mean, I didn't touch SHAx topic at all.
I think we should rather deprecate DES_ENCRYPT and DES_DECRYPT functions. It should've been done long time ago.
Ok, but there is also a deprecation procedure for us. We can’t remove those functions right now, and DES_XXX would be in deprecated mode for a while until they would be removed, in a couple of years.
MDEV-27104 Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (3)
-
Sergei Golubchik -
Vladislav Vaintroub -
Vladislav Vaintroub