Remodel the if sequence for handling alerts

Reviewed-by: Andy Polyakov <appro@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6370)

1 files changed, 28 insertions, 36 deletions

diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index a3dee2de81..75b506bd85 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
@@ -1497,7 +1498,6 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
 
     if (SSL3_RECORD_get_type(rr) == SSL3_RT_ALERT) {
         unsigned int alert_level, alert_descr;
-        int tls13_user_cancelled;
         unsigned char *alert_bytes = SSL3_RECORD_get_data(rr)
                                      + SSL3_RECORD_get_off(rr);
         PACKET alert;
@@ -1525,9 +1525,8 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
             cb(s, SSL_CB_READ_ALERT, j);
         }
 
-        tls13_user_cancelled = SSL_IS_TLS13(s)
-                               && alert_descr == SSL_AD_USER_CANCELLED;
-        if (alert_level == SSL3_AL_WARNING || tls13_user_cancelled) {
+        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);
 
@@ -1537,38 +1536,19 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
                          SSL_R_TOO_MANY_WARN_ALERTS);
                 return -1;
             }
-
-            /*
-             * Apart from close_notify the only other warning alert in TLSv1.3
-             * is user_cancelled - which we just ignore.
-             */
-            if (tls13_user_cancelled)
-                goto start;
-        }
-
-        if (alert_descr == SSL_AD_CLOSE_NOTIFY
-                && (SSL_IS_TLS13(s) || alert_level == SSL3_AL_WARNING)) {
-            s->shutdown |= SSL_RECEIVED_SHUTDOWN;
-            return 0;
         }
 
         /*
-         * 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.
+         * Apart from close_notify the only other warning alert in TLSv1.3
+         * is user_cancelled - which we just ignore.
          */
-        if (alert_descr == SSL_AD_NO_RENEGOTIATION
-                && !SSL_IS_TLS13(s)
-                && alert_level == SSL3_AL_WARNING) {
-            SSLfatal(s, SSL_AD_HANDSHAKE_FAILURE, SSL_F_SSL3_READ_BYTES,
-                     SSL_R_NO_RENEGOTIATION);
-            return -1;
-        }
-
-        if (alert_level == SSL3_AL_FATAL || SSL_IS_TLS13(s)) {
+        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;
@@ -1581,11 +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;
         }
+
+        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