[PATCH 1 of 4] SSL: switched to detect log level based on the last error

[Maxim Dounin]

Hello!

On Tue, Mar 07, 2023 at 06:40:50PM +0400, Roman Arutyunyan wrote:

> Hi,
> 
> On Wed, Mar 01, 2023 at 05:56:02PM +0300, Maxim Dounin wrote:
> > # HG changeset patch
> > # User Maxim Dounin <mdounin at mdounin.ru>
> > # Date 1677682263 -10800
> > #      Wed Mar 01 17:51:03 2023 +0300
> > # Node ID 4d0a265c1d20f22f196680dfcc9d044f9e711865
> > # Parent  2acb00b9b5fff8a97523b659af4377fc605abe6e
> > SSL: switched to detect log level based on the last error.
> > 
> > In some cases there might be multiple errors in the OpenSSL error queue,
> > notably when a libcrypto call fails, and then the SSL layer generates
> > an error itself.  For example, the following errors were observed
> > with OpenSSL 3.0.8 with TLSv1.3 enabled:
> > 
> > SSL_do_handshake() failed (SSL: error:02800066:Diffie-Hellman routines::invalid public key error:0A000132:SSL routines::bad ecpoint)
> > SSL_do_handshake() failed (SSL: error:08000066:elliptic curve routines::invalid encoding error:0A000132:SSL routines::bad ecpoint)
> > SSL_do_handshake() failed (SSL: error:0800006B:elliptic curve routines::point is not on curve error:0A000132:SSL routines::bad ecpoint)
> > 
> > In such cases it seems to be better to determine logging level based on
> > the last error in the error queue (the one added by the SSL layer,
> > SSL_R_BAD_ECPOINT in all of the above example example errors).  To do so,
> > the ngx_ssl_connection_error() function was changed to use
> > ERR_peek_last_error().
> > 
> > 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
> > @@ -3389,7 +3389,7 @@ ngx_ssl_connection_error(ngx_connection_
> >  
> >      } else if (sslerr == SSL_ERROR_SSL) {
> >  
> > -        n = ERR_GET_REASON(ERR_peek_error());
> > +        n = ERR_GET_REASON(ERR_peek_last_error());
> >  
> >              /* handshake failures */
> >          if (n == SSL_R_BAD_CHANGE_CIPHER_SPEC                        /*  103 */
> 
> Looks good.
> 
> Just for the record.  BoringSSL, LibreSSL and older versions of OpenSSL
> sometimes report SSL handshake errors directly from encryption libraries
> without adding an SSL-layer error.  In this case we do not change the log
> level and report these errors as critical.  Luckily, this does not seem to
> be the case with the newer OpenSSL versions.
> 
> OpenSSL 1.0.2u:
> 
> SSL_do_handshake() failed (SSL: error:0406506C:rsa routines:RSA_EAY_PRIVATE_DECRYPT:data greater than mod len) while SSL handshaking
> SSL_do_handshake() failed (SSL: error:04065084:rsa routines:RSA_EAY_PRIVATE_DECRYPT:data too large for modulus) while SSL handshaking
> 
> BoringSSL:
> 
> SSL_do_handshake() failed (SSL: error:04000070:RSA routines:OPENSSL_internal:DATA_LEN_NOT_EQUAL_TO_MOD_LEN) while SSL handshaking
> SSL_do_handshake() failed (SSL: error:04000073:RSA routines:OPENSSL_internal:DATA_TOO_LARGE_FOR_MODULUS) while SSL handshaking
> 
> LibreSSL:
> 
> SSL_do_handshake() failed (SSL: error:10FFF06B:elliptic curve routines:CRYPTO_internal:point is not on curve) while SSL handshaking
> SSL_do_handshake() failed (SSL: error:06FFF064:digital envelope routines:CRYPTO_internal:bad decrypt) while SSL handshaking

Sure.

As mentioned in the introduction to this patch series:

: Note that it doesn't try to quench all the errors observed, but only ones
: which clearly indicate misbehaving client.  Errors which might indicate
: issues in the library instead are preserved as is.  For example, logging
: level of the ERR_R_INTERNAL_ERROR and ERR_R_EC_LIB errors, which are also
: reported during tlsfuzzer runs with OpenSSL 3.0.8, are preserved as is.
: It's up to the library authors to either fix these, or return some better
: errors instead.

The RSA errors you've quoted are instead handled as internal 
errors in the OpenSSL 1.1.1 branch:

    decrypt_len = (int)RSA_private_decrypt((int)PACKET_remaining(&enc_premaster),
                                           PACKET_data(&enc_premaster),
                                           rsa_decrypt, rsa, RSA_NO_PADDING);
    if (decrypt_len < 0) {
        SSLfatal(s, SSL_AD_DECRYPT_ERROR, SSL_F_TLS_PROCESS_CKE_RSA,
                 ERR_R_INTERNAL_ERROR);
        goto err;
    }

And in OpenSSL 3.0.x this is fixed to generate proper 
SSL_R_DECRYPTION_FAILED error:

    if (EVP_PKEY_decrypt_init(ctx) <= 0
            || EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_WITH_TLS_PADDING) <= 0) {
        SSLfatal(s, SSL_AD_DECRYPT_ERROR, SSL_R_DECRYPTION_FAILED);
        goto err;
    }

Hopefully other libraries will follow.

-- 
Maxim Dounin
http://mdounin.ru/