maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx

> From: Sergei Golubchik
> Sent: Thursday, 18 November 2021 20:20
> To: Vladislav Vaintroub
> Cc: maria-developers@xxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxx
>