> 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.
>
> 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.
>
> /* 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.
>
> 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);
--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
