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