[PATCH] SSL: consistent certificate verification depth

[Maxim Dounin]

Hello!

On Wed, Jul 20, 2022 at 05:58:26PM +0400, Sergey Kandaurov wrote:

> # HG changeset patch
> # User Sergey Kandaurov <pluknet at 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/