> On 20 Jul 2022, at 22:04, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> Hello!
>
> On Wed, Jul 20, 2022 at 05:58:26PM +0400, Sergey Kandaurov wrote:
>
>> # HG changeset patch
>> # User Sergey Kandaurov <pluknet@nginx.com>
>> # Date 1658325446 -14400
>> # Wed Jul 20 17:57:26 2022 +0400
>> # Node ID 5a9cc2a846c9ea4c0af03109ab186af1ac28e222
>> # Parent 069a4813e8d6d7ec662d282a10f5f7062ebd817f
>> SSL: consistent certificate verification depth.
>>
>> Originally, certificate verification depth was used to limit the number
>> of signatures to validate, that is, to limit chains with intermediate
>> certificates one less. The semantics was changed in OpenSSL 1.1.0, and
>> instead it limits now the number of intermediate certificates allowed.
>> This makes it not possible to limit certificate checking to self-signed
>> certificates with verify depth 0 in OpenSSL 1.1.0+, and is inconsistent
>> compared with former behaviour in BoringSSL and older OpenSSL versions.
>>
>> This change restores former verification logic when using OpenSSL 1.1.0+.
>> The verify callback is adjusted to emit the "certificate chain too long"
>> error when the certificate chain exceeds the verification depth. It has
>> no effect to other SSL libraries, where this is limited by other means.
>> Also, this fixes verification checks when using LibreSSL 3.4.0+, where
>> a chain depth is not limited except by X509_VERIFY_MAX_CHAIN_CERTS (32).
>
> This (highly questionable) OpenSSL behaviour seems to be status
> quo for a while, also recorded in tests (see ssl_verify_depth.t).
> Any specific reasons for the nginx-side workaround?

As explained in the commit message, main motivation is to eliminate
annoying difference in behaviour among various SSL libraries
(aside from working around the arguably broken LibreSSL verifier).
Nothing specific behind that.
Disambiguating ssl_verify_depth.t is a good demo of net result.
The downside is that this can potentially break previously working
configurations when using with modern OpenSSL versions.
So the patch is to seek feedback on whether this makes sense.

>
>>
>> diff -r 069a4813e8d6 -r 5a9cc2a846c9 src/event/ngx_event_openssl.c
>> --- a/src/event/ngx_event_openssl.c Tue Jul 19 17:05:27 2022 +0300
>> +++ b/src/event/ngx_event_openssl.c Wed Jul 20 17:57:26 2022 +0400
>> @@ -997,16 +997,26 @@ ngx_ssl_crl(ngx_conf_t *cf, ngx_ssl_t *s
>> static int
>> ngx_ssl_verify_callback(int ok, X509_STORE_CTX *x509_store)
>> {
>> + int depth, verify_depth;
>> + ngx_ssl_conn_t *ssl_conn;
>> +
>> + ssl_conn = X509_STORE_CTX_get_ex_data(x509_store,
>> + SSL_get_ex_data_X509_STORE_CTX_idx());
>> +
>> + depth = X509_STORE_CTX_get_error_depth(x509_store);
>> + verify_depth = SSL_CTX_get_verify_depth(SSL_get_SSL_CTX(ssl_conn));
>
> s/verify_depth/limit/?
>
> Also, using SSL_get_verify_depth() instead of going through SSL
> context might be easier.

Fixed, thanks. Some remnants left after r&d.

>
>> +
>> + if (depth > verify_depth) {
>> + X509_STORE_CTX_set_error(x509_store, X509_V_ERR_CERT_CHAIN_TOO_LONG);
>
> This is going to overwrite earlier errors, if any. Does not look like a good
> idea.

Agree, it can actually - not what I want to do within this change.
Also, refined commit message.

>
>> + }
>> +
>> #if (NGX_DEBUG)
>> +
>> + int err;
>
> This is not going to work, variables have to be defined at the
> start of a block unless you assume C99. At least MSVC 2010 won't
> be able to compile this.

Fixed, thanks.

# HG changeset patch
# User Sergey Kandaurov <pluknet@nginx.com>
# Date 1658412531 -14400
# Thu Jul 21 18:08:51 2022 +0400
# Node ID d064df4a9eb7f74f328d01337e7743adb6468dd5
# Parent 069a4813e8d6d7ec662d282a10f5f7062ebd817f
SSL: consistent certificate verification depth.

Originally, certificate verification depth was used to limit the number
of signatures to validate, that is, to limit chains with intermediate
certificates one less. The semantics was changed in OpenSSL 1.1.0, and
instead it limits now the number of intermediate certificates allowed.
This makes it not possible to limit certificate checking to self-signed
certificates with verify depth 0 in OpenSSL 1.1.0+, and is inconsistent
compared with BoringSSL and former behaviour in older OpenSSL versions.

This change restores former verification logic when using OpenSSL 1.1.0+.
The verify callback is adjusted to set the "certificate chain too long"
error if the certificate chain exceeds the verification depth and the
error wasn't previously set by other means.

Also, this fixes verification checks when using LibreSSL 3.4.0+, where
a chain depth is not limited except by X509_VERIFY_MAX_CHAIN_CERTS (32).

diff -r 069a4813e8d6 -r d064df4a9eb7 src/event/ngx_event_openssl.c
--- a/src/event/ngx_event_openssl.c Tue Jul 19 17:05:27 2022 +0300
+++ b/src/event/ngx_event_openssl.c Thu Jul 21 18:08:51 2022 +0400
@@ -997,16 +997,28 @@ ngx_ssl_crl(ngx_conf_t *cf, ngx_ssl_t *s
static int
ngx_ssl_verify_callback(int ok, X509_STORE_CTX *x509_store)
{
+ int err, depth, limit;
+ ngx_ssl_conn_t *ssl_conn;
+
+ ssl_conn = X509_STORE_CTX_get_ex_data(x509_store,
+ SSL_get_ex_data_X509_STORE_CTX_idx());
+
+ err = X509_STORE_CTX_get_error(x509_store);
+ depth = X509_STORE_CTX_get_error_depth(x509_store);
+
+ limit = SSL_get_verify_depth(ssl_conn);
+
+ if (depth > limit && err == X509_V_OK) {
+ err = X509_V_ERR_CERT_CHAIN_TOO_LONG;
+ X509_STORE_CTX_set_error(x509_store, err);
+ }
+
#if (NGX_DEBUG)
+ {
char *subject, *issuer;
- int err, depth;
X509 *cert;
X509_NAME *sname, *iname;
ngx_connection_t *c;
- ngx_ssl_conn_t *ssl_conn;
-
- ssl_conn = X509_STORE_CTX_get_ex_data(x509_store,
- SSL_get_ex_data_X509_STORE_CTX_idx());

c = ngx_ssl_get_connection(ssl_conn);

@@ -1015,8 +1027,6 @@ ngx_ssl_verify_callback(int ok, X509_STO
}

cert = X509_STORE_CTX_get_current_cert(x509_store);
- err = X509_STORE_CTX_get_error(x509_store);
- depth = X509_STORE_CTX_get_error_depth(x509_store);

sname = X509_get_subject_name(cert);

@@ -1058,6 +1068,7 @@ ngx_ssl_verify_callback(int ok, X509_STO
if (issuer) {
OPENSSL_free(issuer);
}
+ }
#endif

return 1;


--
Sergey Kandaurov

_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org