Hello!

On Fri, Dec 22, 2023 at 05:53:30PM +0400, Sergey Kandaurov wrote:

>
> > On 21 Dec 2023, at 02:40, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > Hello!
> >
> > On Tue, Dec 19, 2023 at 10:44:00PM +0400, Sergey Kandaurov wrote:
> >
> >>
> >>> On 19 Dec 2023, at 12:58, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >>>
> >>> Hello!
> >>>
> >>> On Tue, Dec 19, 2023 at 02:09:10AM +0400, Sergey Kandaurov wrote:
> >>>
> >>>>> On 24 Nov 2023, at 00:29, Ilya Shipitsin <chipitsine@gmail.com> wrote:
> >>>>>
> >>>>> # HG changeset patch
> >>>>> # User Ilya Shipitsin <chipitsine@gmail.com>
> >>>>> # 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 <pluknet@nginx.com>
> >> # 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 <pluknet@nginx.com>
> >> # 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 <pluknet@nginx.com>
> # 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 <pluknet@nginx.com>
> # 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
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel