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?

>
> 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.

> +
> + 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.

> + }
> +
> #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.

> 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);
>
> @@ -1016,7 +1026,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 +1067,7 @@ ngx_ssl_verify_callback(int ok, X509_STO
> if (issuer) {
> OPENSSL_free(issuer);
> }
> +
> #endif
>
> return 1;
>

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org