Tolerate encrypted or plaintext alerts

At certain points in the handshake we could receive either a plaintext or
an encrypted alert from the client. We should tolerate both where
appropriate.

Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6887)

6 files changed, 44 insertions, 14 deletions

diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index 10ac0936ed..d208695b16 100644
--- a/ssl/record/rec_layer_s3.c
+++ b/ssl/record/rec_layer_s3.c
@@ -816,8 +816,8 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
     /* Clear our SSL3_RECORD structures */
     memset(wr, 0, sizeof(wr));
     for (j = 0; j < numpipes; j++) {
-        unsigned int version = SSL_TREAT_AS_TLS13(s) ? TLS1_2_VERSION
-                                                     : s->version;
+        unsigned int version = (s->version == TLS1_3_VERSION) ? TLS1_2_VERSION
+                                                              : s->version;
         unsigned char *compressdata = NULL;
         size_t maxcomplen;
         unsigned int rectype;
diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c
index ad478bf375..a616bf0409 100644
--- a/ssl/record/ssl3_record.c
+++ b/ssl/record/ssl3_record.c
@@ -342,7 +342,10 @@ int ssl3_get_record(SSL *s)
                 if (SSL_IS_TLS13(s) && s->enc_read_ctx != NULL) {
                     if (thisrr->type != SSL3_RT_APPLICATION_DATA
                             && (thisrr->type != SSL3_RT_CHANGE_CIPHER_SPEC
-                                || !SSL_IS_FIRST_HANDSHAKE(s))) {
+                                || !SSL_IS_FIRST_HANDSHAKE(s))
+                            && (thisrr->type != SSL3_RT_ALERT
+                                || s->statem.enc_read_state
+                                   != ENC_READ_STATE_ALLOW_PLAIN_ALERTS)) {
                         SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE,
                                  SSL_F_SSL3_GET_RECORD, SSL_R_BAD_RECORD_TYPE);
                         return -1;
@@ -692,7 +695,9 @@ int ssl3_get_record(SSL *s)
             }
         }
 
-        if (SSL_IS_TLS13(s) && s->enc_read_ctx != NULL) {
+        if (SSL_IS_TLS13(s)
+                && s->enc_read_ctx != NULL
+                && thisrr->type != SSL3_RT_ALERT) {
             size_t end;
 
             if (thisrr->length == 0
diff --git a/ssl/record/ssl3_record_tls13.c b/ssl/record/ssl3_record_tls13.c
index cbf6a652e7..a11ed483e6 100644
--- a/ssl/record/ssl3_record_tls13.c
+++ b/ssl/record/ssl3_record_tls13.c
@@ -52,10 +52,13 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
         seq = RECORD_LAYER_get_read_sequence(&s->rlayer);
     }
 
-    if (ctx == NULL
-            || (rec->type == SSL3_RT_ALERT
-                && s->statem.enc_write_state
-                   == ENC_WRITE_STATE_WRITE_PLAIN_ALERTS)) {
+    /*
+     * If we're sending an alert and ctx != NULL then we must be forcing
+     * plaintext alerts. If we're reading and ctx != NULL then we allow
+     * plaintext alerts at certain points in the handshake. If we've got this
+     * far then we have already validated that a plaintext alert is ok here.
+     */
+    if (ctx == NULL || rec->type == SSL3_RT_ALERT) {
         memmove(rec->data, rec->input, rec->length);
         rec->input = rec->data;
         return 1;
diff --git a/ssl/statem/statem.h b/ssl/statem/statem.h
index 0799870178..144d930fc7 100644
--- a/ssl/statem/statem.h
+++ b/ssl/statem/statem.h
@@ -80,6 +80,13 @@ typedef enum {
     ENC_WRITE_STATE_WRITE_PLAIN_ALERTS
 } ENC_WRITE_STATES;
 
+typedef enum {
+    /* The enc_read_ctx can be used normally */
+    ENC_READ_STATE_VALID,
+    /* We may receive encrypted or plaintext alerts */
+    ENC_READ_STATE_ALLOW_PLAIN_ALERTS
+} ENC_READ_STATES;
+
 /*****************************************************************************
  *                                                                           *
  * This structure should be considered "opaque" to anything outside of the   *
@@ -110,6 +117,7 @@ struct ossl_statem_st {
     unsigned int no_cert_verify;
     int use_timer;
     ENC_WRITE_STATES enc_write_state;
+    ENC_READ_STATES enc_read_state;
 };
 typedef struct ossl_statem_st OSSL_STATEM;
 
diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index caed61aaac..8a7d178a51 100644
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -747,6 +747,12 @@ MSG_PROCESS_RETURN tls_process_finished(SSL *s, PACKET *pkt)
 
     /* This is a real handshake so make sure we clean it up at the end */
     if (s->server) {
+        /*
+        * To get this far we must have read encrypted data from the client. We
+        * no longer tolerate unencrypted alerts. This value is ignored if less
+        * than TLSv1.3
+        */
+        s->statem.enc_read_state = ENC_READ_STATE_VALID;
         if (s->post_handshake_auth != SSL_PHA_REQUESTED)
             s->statem.cleanuphand = 1;
         if (SSL_IS_TLS13(s) && !tls13_save_handshake_digest_for_pha(s)) {
diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index eb9070ecc4..db5aafe3be 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -848,12 +848,7 @@ WORK_STATE ossl_statem_server_post_work(SSL *s, WORK_STATE wst)
                 return WORK_MORE_A;
             break;
         }
-        /*
-         * TODO(TLS1.3): This actually causes a problem. We don't yet know
-         * whether the next record we are going to receive is an unencrypted
-         * alert, or an encrypted handshake message. We're going to need
-         * something clever in the record layer for this.
-         */
+
         if (SSL_IS_TLS13(s)) {
             if (!s->method->ssl3_enc->setup_key_block(s)
                 || !s->method->ssl3_enc->change_cipher_state(s,
@@ -868,6 +863,12 @@ WORK_STATE ossl_statem_server_post_work(SSL *s, WORK_STATE wst)
                 /* SSLfatal() already called */
                 return WORK_ERROR;
             }
+            /*
+             * We don't yet know whether the next record we are going to receive
+             * is an unencrypted alert, an encrypted alert, or an encrypted
+             * handshake message. We temporarily tolerate unencrypted alerts.
+             */
+            s->statem.enc_read_state = ENC_READ_STATE_ALLOW_PLAIN_ALERTS;
             break;
         }
 
@@ -3523,6 +3524,13 @@ MSG_PROCESS_RETURN tls_process_client_certificate(SSL *s, PACKET *pkt)
     size_t chainidx;
     SSL_SESSION *new_sess = NULL;
 
+    /*
+     * To get this far we must have read encrypted data from the client. We no
+     * longer tolerate unencrypted alerts. This value is ignored if less than
+     * TLSv1.3
+     */
+    s->statem.enc_read_state = ENC_READ_STATE_VALID;
+
     if ((sk = sk_X509_new_null()) == NULL) {
         SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PROCESS_CLIENT_CERTIFICATE,
                  ERR_R_MALLOC_FAILURE);