Hello! On Fri, Dec 22, 2023 at 05:53:30PM +0400, Sergey Kandaurov wrote:
> > > On 21 Dec 2023, at 02:40, Maxim Dounin <[email protected]> wrote: > > > > Hello! > > > > On Tue, Dec 19, 2023 at 10:44:00PM +0400, Sergey Kandaurov wrote: > > > >> > >>> On 19 Dec 2023, at 12:58, Maxim Dounin <[email protected]> wrote: > >>> > >>> Hello! > >>> > >>> On Tue, Dec 19, 2023 at 02:09:10AM +0400, Sergey Kandaurov wrote: > >>> > >>>>> On 24 Nov 2023, at 00:29, Ilya Shipitsin <[email protected]> wrote: > >>>>> > >>>>> # HG changeset patch > >>>>> # User Ilya Shipitsin <[email protected]> > >>>>> # Date 1700769135 -3600 > >>>>> # Thu Nov 23 20:52:15 2023 +0100 > >>>>> # Node ID 2001e73ce136d5bfc9bde27d338865b14b8ad436 > >>>>> # Parent 7ec761f0365f418511e30b82e9adf80bc56681df > >>>>> ssl: SSL_get0_verified_chain is available for LibreSSL >= 3.3.6 > >>>> > >>>> style: SSL prefix should be uppercase. > >>>> > >>>>> > >>>>> diff -r 7ec761f0365f -r 2001e73ce136 > >>>>> src/event/ngx_event_openssl_stapling.c > >>>>> --- a/src/event/ngx_event_openssl_stapling.c Thu Oct 26 23:35:09 > >>>>> 2023 +0300 > >>>>> +++ b/src/event/ngx_event_openssl_stapling.c Thu Nov 23 20:52:15 > >>>>> 2023 +0100 > >>>>> @@ -893,7 +893,8 @@ > >>>>> ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD; > >>>>> ocsp->conf = ocf; > >>>>> > >>>>> -#if (OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined > >>>>> LIBRESSL_VERSION_NUMBER) > >>>>> +/* minimum OpenSSL 1.1.1 & LibreSSL 3.3.6 */ > >>>>> +#if (OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined > >>>>> LIBRESSL_VERSION_NUMBER) || (defined(LIBRESSL_VERSION_NUMBER) && > >>>>> (LIBRESSL_VERSION_NUMBER >= 0x3030600L)) > >>>>> > >>>>> ocsp->certs = SSL_get0_verified_chain(c->ssl->connection); > >>>>> > >>>> > >>>> Testing "defined(LIBRESSL_VERSION_NUMBER)" is superfluous. > >>>> The macro test suffers from a very long line. > >>>> > >>>> The correct version test seems to be against LibreSSL 3.5.0, see > >>>> https://ftp.openbsd.org/pub/OpenBSD/LibreSSL/libressl-3.5.0-relnotes.txt > >>>> > >>>> So, the resulting change would be as follows: > >>>> > >>>> diff --git a/src/event/ngx_event_openssl_stapling.c > >>>> b/src/event/ngx_event_openssl_stapling.c > >>>> --- a/src/event/ngx_event_openssl_stapling.c > >>>> +++ b/src/event/ngx_event_openssl_stapling.c > >>>> @@ -893,7 +893,9 @@ ngx_ssl_ocsp_validate(ngx_connection_t * > >>>> ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD; > >>>> ocsp->conf = ocf; > >>>> > >>>> -#if (OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined > >>>> LIBRESSL_VERSION_NUMBER) > >>>> +#if (OPENSSL_VERSION_NUMBER >= 0x10100000L \ > >>>> + && !defined LIBRESSL_VERSION_NUMBER) \ > >>>> + || LIBRESSL_VERSION_NUMBER >= 0x3050000fL > >>> > >>> Rather, > >>> > >>> +#if (OPENSSL_VERSION_NUMBER >= 0x10100000L > >>> \ > >>> + && (!defined LIBRESSL_VERSION_NUMBER > >>> \ > >>> + || LIBRESSL_VERSION_NUMBER >= 0x3050000fL)) > >>> > >> > >> Agree. For the sake of completeness: > >> > >> # HG changeset patch > >> # User Sergey Kandaurov <[email protected]> > >> # Date 1703004490 -14400 > >> # Tue Dec 19 20:48:10 2023 +0400 > >> # Node ID 267cee796462f4f6bacf825c8fd24d13845d36f4 > >> # Parent 7a6d52990f2e2d88460a3dc6cc84aac89b7329ea > >> SSL: using SSL_get0_verified_chain() with LibreSSL 3.5.0+. > >> > >> Prodded by Ilya Shipitsin. > >> > >> diff --git a/src/event/ngx_event_openssl_stapling.c > >> b/src/event/ngx_event_openssl_stapling.c > >> --- a/src/event/ngx_event_openssl_stapling.c > >> +++ b/src/event/ngx_event_openssl_stapling.c > >> @@ -893,7 +893,9 @@ ngx_ssl_ocsp_validate(ngx_connection_t * > >> ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD; > >> ocsp->conf = ocf; > >> > >> -#if (OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined > >> LIBRESSL_VERSION_NUMBER) > >> +#if (OPENSSL_VERSION_NUMBER >= 0x10100000L > >> \ > >> + && (!defined LIBRESSL_VERSION_NUMBER > >> \ > >> + || LIBRESSL_VERSION_NUMBER >= 0x3050000fL)) > >> > >> ocsp->certs = SSL_get0_verified_chain(c->ssl->connection); > >> > >> > >>>> > >>>> ocsp->certs = SSL_get0_verified_chain(c->ssl->connection); > >>>> > >>>> > >>>> On the other hand, I don't like the resulting style mudness. > >>>> It may have sense just to drop old LibreSSL versions support: > >>>> maintaining one or two most recent stable branches should be enough. > >>> > >>> +1 on this. > >>> > >>> We certainly don't want to maintain support for ancient LibreSSL > >>> versions. IMO, just two branches is more than enough (and this is > >>> what I use in my testing, which usually means latest development > >>> release and latest stable release). > >>> > >>> At most, this probably can be three branches - which seems to > >>> match LibreSSL support policy, > >>> https://www.libressl.org/releases.html: > >>> > >>> : LibreSSL transitions to a new stable release branch every 6 months > >>> : in coordination with the OpenBSD development schedule. LibreSSL > >>> : stable branches are updated for 1 year after their corresponding > >>> : OpenBSD branch is tagged for release. See below for the current > >>> : stable release branches. > >>> > >>> In either case, LibreSSL versions below 3.5.0 are already not > >>> supported. If I understand correctly, right now the oldest > >>> supported branch is 3.7.x. > >> > >> Agree. Also, Repology shows that modern popular distributions, > >> such as Alpine Linux and FreeBSD, have at least LibreSSL 3.5.x: > >> https://repology.org/project/libressl/versions > >> > >>> > >>>> But anyway, I don't see an obvious win over the existing code: > >>>> the certificate chain is reconstructed if SSL_get0_verified_chain() > >>>> is (detected to be) not present, which should be fine in most cases. > >>>> > >>>> That said, it doesn't seem to deserve introducing 3-line macro test, > >>>> or (see OTOH note) breaking old LibreSSL support for no apparent reason. > >>> > >>> Reconstruction of the chain implies verification of signatures > >>> along the chain and can be costly. As such, it certainly would be > >>> better to use SSL_get0_verified_chain() as long as it is > >>> available. > >> > >> Agree. > >> My point is that not using SSL_get0_verified_chain() should not result > >> in a broken functionality, as in the OCSP cert validation. > >> So, intention to start using it in LibreSSL may look an insufficient > >> argument per se to drop old LibreSSL versions. > >> Though, dropping them may be orthogonal to SSL_get0_verified_chain(). > >> > >>> > >>> Also, removing the "!defined LIBRESSL_VERSION_NUMBER" check might > >>> be seen as positive even without any additional benefits. > >>> > >>> Along with that, however, we might want to adjust the > >>> LIBRESSL_VERSION_NUMBER check in the ngx_event_openssl.h file, so > >>> OPENSSL_VERSION_NUMBER is set to a better value for old LibreSSL > >>> versions - for example, to only set OPENSSL_VERSION_NUMBER to > >>> 0x1010000fL for LibreSSL 3.5.0 or above. > >> > >> Sounds like a plan if we are fine to drop older LibreSSL versions. > >> > >>> This might allow to > >>> preserve limited compatibility with ancient LibreSSL versions > >>> without additional efforts (not tested though). > >>> > >> > >> This won't build with any LibreSSL version in the [2.8.0, 3.5.0) range. > >> Particularly, SSL_CTX_sess_set_get_cb() has got a const argument in > >> LibreSSL 2.8.0, which is not backward compatible, see 7337:cab37803ebb3. > >> Another reason is SSL was made opaque by default in 3.4.x. > > > > The const argument can be easily ignored by using > > -Wno-error=incompatible-function-pointer-types (or just > > -Wno-error), which seems to be reasonable when trying to build > > things with ancient libraries. This makes it possible to build > > with LibreSSL 2.8.0-3.3.6 with minimal efforts. > > Ok, that makes sense. > For the record, GCC uses another warning option name: > > src/event/ngx_event_openssl.c:3770:43: error: passing argument 2 of > 'SSL_CTX_sess_set_get_cb' from incompatible pointer type > [-Werror=incompatible-pointer-type] > > There is a typo in GCC 12 diagnostics, the actual option name > appears to be -Wno-incompatible-pointer-types (note "s"). > BTW, it appears to be a valid option name in Clang as well > to suppress such warnings. Yep, obviously enough required options might be different in different compilers. > > For LibreSSL 3.4.x, the dependency on the SSL internals can be > > easily eliminated by testing SSL_OP_NO_CLIENT_RENEGOTIATION, which > > anyway seems to be a reasonable change. > > Agree, this is really true as tested on various LibreSSL versions, > from 2.5.0 to 3.8.2 (SSL_OP_NO_CLIENT_RENEGOTIATION appeared in 2.6.0). > > > > >> > >> (Others seem not to affect building on older versions if pick up 3.5.0: > >> - X509_up_ref appeared in LibreSSL 2.6.0, X509 made opaque in 3.5.0; > >> - X509_get0_notAfter appeared in 2.7.0, X509_get_notAfter still there.) > >> > >> Personally I'm fine to drop ancient LibreSSL versions, because it has > >> to happen someday and we don't want to maintain them eternally. > >> Alternative patch for your consideration: > >> > >> # HG changeset patch > >> # User Sergey Kandaurov <[email protected]> > >> # Date 1703011348 -14400 > >> # Tue Dec 19 22:42:28 2023 +0400 > >> # Node ID 94d4ef4a2316da66fea084952913ff2b0f84827d > >> # Parent 7a6d52990f2e2d88460a3dc6cc84aac89b7329ea > >> SSL: removed compatibility with LibreSSL < 3.5.0. > >> > >> OPENSSL_VERSION_NUMBER is now redefined to 0x1010000fL for LibreSSL 3.5.0+. > >> As older versions starting from LibreSSL 2.8.0 won't build with a lesser > >> OPENSSL_VERSION_NUMBER value (see 7337:cab37803ebb3 for details), they are > >> now explicitly unsupported. > >> > >> Besides that, this allows to start using SSL_get0_verified_chain() > >> with LibreSSL without additional macro tests. > >> > >> diff --git a/src/event/ngx_event_openssl.h b/src/event/ngx_event_openssl.h > >> --- a/src/event/ngx_event_openssl.h > >> +++ b/src/event/ngx_event_openssl.h > >> @@ -45,10 +45,10 @@ > >> > >> #if (defined LIBRESSL_VERSION_NUMBER && OPENSSL_VERSION_NUMBER == > >> 0x20000000L) > >> #undef OPENSSL_VERSION_NUMBER > >> -#if (LIBRESSL_VERSION_NUMBER >= 0x2080000fL) > >> +#if (LIBRESSL_VERSION_NUMBER >= 0x3050000fL) > >> #define OPENSSL_VERSION_NUMBER 0x1010000fL > >> #else > >> -#define OPENSSL_VERSION_NUMBER 0x1000107fL > >> +#error LibreSSL too old, need at least 3.5.0 > >> #endif > >> #endif > > > > I'm certainly against the idea of explicitly rejecting old > > versions. As demonstrated above, even versions affected by > > various changes can be used with minimal efforts, such as > > disabling -Werror. > > > > For the record, here is a patch I tested with: > > > > diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c > > --- a/src/event/ngx_event_openssl.c > > +++ b/src/event/ngx_event_openssl.c > > @@ -1105,7 +1105,8 @@ ngx_ssl_info_callback(const ngx_ssl_conn > > BIO *rbio, *wbio; > > ngx_connection_t *c; > > > > -#ifndef SSL_OP_NO_RENEGOTIATION > > +#if (!defined SSL_OP_NO_RENEGOTIATION > > \ > > + && !defined SSL_OP_NO_CLIENT_RENEGOTIATION) > > > > if ((where & SSL_CB_HANDSHAKE_START) > > && SSL_is_server((ngx_ssl_conn_t *) ssl_conn)) > > @@ -1838,9 +1839,10 @@ ngx_ssl_handshake(ngx_connection_t *c) > > c->read->ready = 1; > > c->write->ready = 1; > > > > -#ifndef SSL_OP_NO_RENEGOTIATION > > -#if OPENSSL_VERSION_NUMBER < 0x10100000L > > -#ifdef SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS > > +#if (!defined SSL_OP_NO_RENEGOTIATION > > \ > > + && !defined SSL_OP_NO_CLIENT_RENEGOTIATION > > \ > > + && defined SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS > > \ > > + && OPENSSL_VERSION_NUMBER < 0x10100000L) > > Keeping macro tests nested allows to add/remove them with minimal diff. > I don't mind though to combine primaries into a single test, this is > what nginx happens to use in the rest style. The only justified > exceptions I found that use nested tests are "#if (NGX_GNU_HURD)" > and recently introduced "#if (NGX_QUIC)" in ngx_event_openssl.h. Here nesting is mostly historic due to different tests added over time. Given there are 4 tests now, and SSL_OP_NO_RENEGOTIATION is combined with SSL_OP_NO_CLIENT_RENEGOTIATION in other places, combining all the tests looks like the way to go. > > > > > /* initial handshake done, disable renegotiation (CVE-2009-3555) */ > > if (c->ssl->connection->s3 && SSL_is_server(c->ssl->connection)) { > > @@ -1848,8 +1850,6 @@ ngx_ssl_handshake(ngx_connection_t *c) > > } > > > > #endif > > -#endif > > -#endif > > > > #if (defined BIO_get_ktls_send && !NGX_WIN32) > > > > @@ -2483,7 +2483,8 @@ ngx_ssl_handle_recv(ngx_connection_t *c, > > int sslerr; > > ngx_err_t err; > > > > -#ifndef SSL_OP_NO_RENEGOTIATION > > +#if (!defined SSL_OP_NO_RENEGOTIATION > > \ > > + && !defined SSL_OP_NO_CLIENT_RENEGOTIATION) > > > > if (c->ssl->renegotiation) { > > /* > > diff --git a/src/event/ngx_event_openssl.h b/src/event/ngx_event_openssl.h > > --- a/src/event/ngx_event_openssl.h > > +++ b/src/event/ngx_event_openssl.h > > @@ -45,7 +45,7 @@ > > > > #if (defined LIBRESSL_VERSION_NUMBER && OPENSSL_VERSION_NUMBER == > > 0x20000000L) > > #undef OPENSSL_VERSION_NUMBER > > -#if (LIBRESSL_VERSION_NUMBER >= 0x2080000fL) > > +#if (LIBRESSL_VERSION_NUMBER >= 0x3050000fL) > > #define OPENSSL_VERSION_NUMBER 0x1010000fL > > #else > > #define OPENSSL_VERSION_NUMBER 0x1000107fL > > diff --git a/src/event/ngx_event_openssl_stapling.c > > b/src/event/ngx_event_openssl_stapling.c > > --- a/src/event/ngx_event_openssl_stapling.c > > +++ b/src/event/ngx_event_openssl_stapling.c > > @@ -893,7 +893,7 @@ ngx_ssl_ocsp_validate(ngx_connection_t * > > ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD; > > ocsp->conf = ocf; > > > > -#if (OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined > > LIBRESSL_VERSION_NUMBER) > > +#if (OPENSSL_VERSION_NUMBER >= 0x10100000L) > > Not sure if brackets are needed here, since it's no longer combined. > Currently, there are 17 occurrences in nginx that don't use brackets, > and 3 simple cases that do. I think that brackets are generally needed, but OPENSSL_VERSION_NUMBER used to be checked without brackets in most cases. As such, I don't object either variant. If needed, brackets can be added to all affected #if's by a separate style-only commit. > > > > > ocsp->certs = SSL_get0_verified_chain(c->ssl->connection); > > > > Overall, things look good to me. > As discussed, broke this into two changes: > > # HG changeset patch > # User Sergey Kandaurov <[email protected]> > # Date 1703252997 -14400 > # Fri Dec 22 17:49:57 2023 +0400 > # Node ID 7fc017b776047b26c7e42b355a1bf142cf968537 > # Parent 500071f3265d259eb1917cd8367828834ff0ae14 > SSL: disabled renegotiation checks with LibreSSL. > > Similar to 7356:e3ba4026c02d, as long as SSL_OP_NO_CLIENT_RENEGOTIATION > is defined, it is the library responsibility to prevent renegotiation. > > Additionally, this allows to raise LibreSSL version used to redefine > OPENSSL_VERSION_NUMBER to 0x1010000fL, such that this won't result in > attempts to dereference SSL objects made opaque in LibreSSL 3.4.0. > > Patch by Maxim Dounin. > > diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c > --- a/src/event/ngx_event_openssl.c > +++ b/src/event/ngx_event_openssl.c > @@ -1105,7 +1105,8 @@ ngx_ssl_info_callback(const ngx_ssl_conn > BIO *rbio, *wbio; > ngx_connection_t *c; > > -#ifndef SSL_OP_NO_RENEGOTIATION > +#if (!defined SSL_OP_NO_RENEGOTIATION > \ > + && !defined SSL_OP_NO_CLIENT_RENEGOTIATION) > > if ((where & SSL_CB_HANDSHAKE_START) > && SSL_is_server((ngx_ssl_conn_t *) ssl_conn)) > @@ -1838,9 +1839,10 @@ ngx_ssl_handshake(ngx_connection_t *c) > c->read->ready = 1; > c->write->ready = 1; > > -#ifndef SSL_OP_NO_RENEGOTIATION > -#if OPENSSL_VERSION_NUMBER < 0x10100000L > -#ifdef SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS > +#if (!defined SSL_OP_NO_RENEGOTIATION > \ > + && !defined SSL_OP_NO_CLIENT_RENEGOTIATION > \ > + && defined SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS > \ > + && OPENSSL_VERSION_NUMBER < 0x10100000L) > > /* initial handshake done, disable renegotiation (CVE-2009-3555) */ > if (c->ssl->connection->s3 && SSL_is_server(c->ssl->connection)) { > @@ -1848,8 +1850,6 @@ ngx_ssl_handshake(ngx_connection_t *c) > } > > #endif > -#endif > -#endif > > #if (defined BIO_get_ktls_send && !NGX_WIN32) > > @@ -2483,7 +2483,8 @@ ngx_ssl_handle_recv(ngx_connection_t *c, > int sslerr; > ngx_err_t err; > > -#ifndef SSL_OP_NO_RENEGOTIATION > +#if (!defined SSL_OP_NO_RENEGOTIATION > \ > + && !defined SSL_OP_NO_CLIENT_RENEGOTIATION) > > if (c->ssl->renegotiation) { > /* > # HG changeset patch > # User Sergey Kandaurov <[email protected]> > # Date 1703253041 -14400 > # Fri Dec 22 17:50:41 2023 +0400 > # Node ID 44266e0651c44f530c4aa66e68c1b9464a9acee7 > # Parent 7fc017b776047b26c7e42b355a1bf142cf968537 > SSL: reasonable version for LibreSSL adjusted. > > OPENSSL_VERSION_NUMBER is now redefined to 0x1010000fL for LibreSSL 3.5.0 > and above. Building with older LibreSSL versions, such as 2.8.0, may now > produce warnings (see cab37803ebb3) and may require appropriate compiler > options to suppress them. > > Notably, this allows to start using SSL_get0_verified_chain() appeared > in OpenSSL 1.1.0 and LibreSSL 3.5.0, without additional macro tests. > > Prodded by Ilya Shipitsin. > > diff --git a/src/event/ngx_event_openssl.h b/src/event/ngx_event_openssl.h > --- a/src/event/ngx_event_openssl.h > +++ b/src/event/ngx_event_openssl.h > @@ -45,7 +45,7 @@ > > #if (defined LIBRESSL_VERSION_NUMBER && OPENSSL_VERSION_NUMBER == > 0x20000000L) > #undef OPENSSL_VERSION_NUMBER > -#if (LIBRESSL_VERSION_NUMBER >= 0x2080000fL) > +#if (LIBRESSL_VERSION_NUMBER >= 0x3050000fL) > #define OPENSSL_VERSION_NUMBER 0x1010000fL > #else > #define OPENSSL_VERSION_NUMBER 0x1000107fL > diff --git a/src/event/ngx_event_openssl_stapling.c > b/src/event/ngx_event_openssl_stapling.c > --- a/src/event/ngx_event_openssl_stapling.c > +++ b/src/event/ngx_event_openssl_stapling.c > @@ -893,7 +893,7 @@ ngx_ssl_ocsp_validate(ngx_connection_t * > ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD; > ocsp->conf = ocf; > > -#if (OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined > LIBRESSL_VERSION_NUMBER) > +#if OPENSSL_VERSION_NUMBER >= 0x10100000L > > ocsp->certs = SSL_get0_verified_chain(c->ssl->connection); > Looks good. -- Maxim Dounin http://mdounin.ru/ _______________________________________________ nginx-devel mailing list [email protected] https://mailman.nginx.org/mailman/listinfo/nginx-devel
