The branch master has been updated via bcf2907c685cf1bde9eb92928fad5e85c483563b (commit) via fb62e47c782397cadf607b92ce50f2bbe250d12e (commit) via 4aa5a5669c69a66fbd8b31c52014356f1e960501 (commit) from 2285c0f624b2f5fd16b590511dc35f427053f89f (commit)
- Log ----------------------------------------------------------------- commit bcf2907c685cf1bde9eb92928fad5e85c483563b Author: Matt Caswell <m...@openssl.org> Date: Fri Jun 8 17:18:03 2018 +0100 Remodel the if sequence for handling alerts Reviewed-by: Andy Polyakov <ap...@openssl.org> (Merged from https://github.com/openssl/openssl/pull/6370) commit fb62e47c782397cadf607b92ce50f2bbe250d12e Author: Matt Caswell <m...@openssl.org> Date: Fri May 18 09:08:19 2018 +0100 Don't send a warning alert in TLSv1.3 TLSv1.3 ignores the alert level, so we should suppress sending of warning only alerts. Fixes #6211 Reviewed-by: Andy Polyakov <ap...@openssl.org> (Merged from https://github.com/openssl/openssl/pull/6370) commit 4aa5a5669c69a66fbd8b31c52014356f1e960501 Author: Matt Caswell <m...@openssl.org> Date: Fri May 18 09:07:42 2018 +0100 Fix TLSv1.3 alert handling In TLSv1.3 we should ignore the severity level of an alert according to the spec. Reviewed-by: Andy Polyakov <ap...@openssl.org> (Merged from https://github.com/openssl/openssl/pull/6370) ----------------------------------------------------------------------- Summary of changes: ssl/record/rec_layer_s3.c | 63 ++++++++++++++++++++++------------------------- ssl/statem/extensions.c | 4 ++- 2 files changed, 33 insertions(+), 34 deletions(-) diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index 61010f4..75b506b 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -1209,6 +1209,7 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, SSL3_RECORD *rr; SSL3_BUFFER *rbuf; void (*cb) (const SSL *ssl, int type2, int val) = NULL; + int is_tls13 = SSL_IS_TLS13(s); rbuf = &s->rlayer.rbuf; @@ -1340,7 +1341,7 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, if (type == SSL3_RECORD_get_type(rr) || (SSL3_RECORD_get_type(rr) == SSL3_RT_CHANGE_CIPHER_SPEC && type == SSL3_RT_HANDSHAKE && recvd_type != NULL - && !SSL_IS_TLS13(s))) { + && !is_tls13)) { /* * SSL3_RT_APPLICATION_DATA or * SSL3_RT_HANDSHAKE or @@ -1524,7 +1525,8 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, cb(s, SSL_CB_READ_ALERT, j); } - if (alert_level == SSL3_AL_WARNING) { + if (alert_level == SSL3_AL_WARNING + || (is_tls13 && alert_descr == SSL_AD_USER_CANCELLED)) { s->s3->warn_alert = alert_descr; SSL3_RECORD_set_read(rr); @@ -1534,34 +1536,19 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, SSL_R_TOO_MANY_WARN_ALERTS); return -1; } + } - if (alert_descr == SSL_AD_CLOSE_NOTIFY) { - s->shutdown |= SSL_RECEIVED_SHUTDOWN; - return 0; - } - /* - * Apart from close_notify the only other warning alert in TLSv1.3 - * is user_cancelled - which we just ignore. - */ - if (SSL_IS_TLS13(s) && alert_descr != SSL_AD_USER_CANCELLED) { - SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_F_SSL3_READ_BYTES, - SSL_R_UNKNOWN_ALERT_TYPE); - return -1; - } - /* - * This is a warning but we receive it if we requested - * renegotiation and the peer denied it. Terminate with a fatal - * alert because if application tried to renegotiate it - * presumably had a good reason and expects it to succeed. In - * future we might have a renegotiation where we don't care if - * the peer refused it where we carry on. - */ - if (alert_descr == SSL_AD_NO_RENEGOTIATION) { - SSLfatal(s, SSL_AD_HANDSHAKE_FAILURE, SSL_F_SSL3_READ_BYTES, - SSL_R_NO_RENEGOTIATION); - return -1; - } - } else if (alert_level == SSL3_AL_FATAL) { + /* + * Apart from close_notify the only other warning alert in TLSv1.3 + * is user_cancelled - which we just ignore. + */ + if (is_tls13 && alert_descr == SSL_AD_USER_CANCELLED) { + goto start; + } else if (alert_descr == SSL_AD_CLOSE_NOTIFY + && (is_tls13 || alert_level == SSL3_AL_WARNING)) { + s->shutdown |= SSL_RECEIVED_SHUTDOWN; + return 0; + } else if (alert_level == SSL3_AL_FATAL || is_tls13) { char tmp[16]; s->rwstate = SSL_NOTHING; @@ -1574,13 +1561,23 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, SSL3_RECORD_set_read(rr); SSL_CTX_remove_session(s->session_ctx, s->session); return 0; - } else { - SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_F_SSL3_READ_BYTES, - SSL_R_UNKNOWN_ALERT_TYPE); + } else if (alert_descr == SSL_AD_NO_RENEGOTIATION) { + /* + * This is a warning but we receive it if we requested + * renegotiation and the peer denied it. Terminate with a fatal + * alert because if application tried to renegotiate it + * presumably had a good reason and expects it to succeed. In + * future we might have a renegotiation where we don't care if + * the peer refused it where we carry on. + */ + SSLfatal(s, SSL_AD_HANDSHAKE_FAILURE, SSL_F_SSL3_READ_BYTES, + SSL_R_NO_RENEGOTIATION); return -1; } - goto start; + SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_F_SSL3_READ_BYTES, + SSL_R_UNKNOWN_ALERT_TYPE); + return -1; } if (s->shutdown & SSL_SENT_SHUTDOWN) { /* but we have not received a diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index 8885e5e..496039e 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -984,7 +984,9 @@ static int final_server_name(SSL *s, unsigned int context, int sent) return 0; case SSL_TLSEXT_ERR_ALERT_WARNING: - ssl3_send_alert(s, SSL3_AL_WARNING, altmp); + /* TLSv1.3 doesn't have warning alerts so we suppress this */ + if (!SSL_IS_TLS13(s)) + ssl3_send_alert(s, SSL3_AL_WARNING, altmp); return 1; case SSL_TLSEXT_ERR_NOACK: _____ openssl-commits mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-commits