Move MAC removal responsibility to the various protocol "enc" functions

For CBC ciphersuites using Mac-then-encrypt we have to be careful about
removing the MAC from the record in constant time. Currently that happens
immediately before MAC verification. Instead we move this responsibility
to the various protocol "enc" functions so that MAC removal is handled at
the same time as padding removal.

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/12288)

8 files changed, 375 insertions, 400 deletions

diff --git a/ssl/record/rec_layer_d1.c b/ssl/record/rec_layer_d1.c
index 9a82e3ffa2..866ef18381 100644
--- a/ssl/record/rec_layer_d1.c
+++ b/ssl/record/rec_layer_d1.c
@@ -939,7 +939,7 @@ int do_dtls1_write(SSL *s, int type, const unsigned char *buf,
     if (eivlen)
         SSL3_RECORD_add_length(&wr, eivlen);
 
-    if (s->method->ssl3_enc->enc(s, &wr, 1, 1) < 1) {
+    if (s->method->ssl3_enc->enc(s, &wr, 1, 1, NULL, mac_size) < 1) {
         if (!ossl_statem_in_error(s)) {
             SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_DO_DTLS1_WRITE,
                      ERR_R_INTERNAL_ERROR);
diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index fac3506b19..8ea16672b6 100644
--- a/ssl/record/rec_layer_s3.c
+++ b/ssl/record/rec_layer_s3.c
@@ -1044,7 +1044,7 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
          * We haven't actually negotiated the version yet, but we're trying to
          * send early data - so we need to use the tls13enc function.
          */
-        if (tls13_enc(s, wr, numpipes, 1) < 1) {
+        if (tls13_enc(s, wr, numpipes, 1, NULL, mac_size) < 1) {
             if (!ossl_statem_in_error(s)) {
                 SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_DO_SSL3_WRITE,
                          ERR_R_INTERNAL_ERROR);
@@ -1053,7 +1053,8 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
         }
     } else {
         if (!BIO_get_ktls_send(s->wbio)) {
-            if (s->method->ssl3_enc->enc(s, wr, numpipes, 1) < 1) {
+            if (s->method->ssl3_enc->enc(s, wr, numpipes, 1, NULL,
+                                         mac_size) < 1) {
                 if (!ossl_statem_in_error(s)) {
                     SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_DO_SSL3_WRITE,
                             ERR_R_INTERNAL_ERROR);
diff --git a/ssl/record/record.h b/ssl/record/record.h
index 0504a6f959..234656bf93 100644
--- a/ssl/record/record.h
+++ b/ssl/record/record.h
@@ -178,6 +178,12 @@ typedef struct record_layer_st {
  *                                                                           *
  *****************************************************************************/
 
+struct ssl_mac_buf_st {
+    unsigned char *mac;
+    int alloced;
+};
+typedef struct ssl_mac_buf_st SSL_MAC_BUF;
+
 #define MIN_SSL2_RECORD_LEN     9
 
 #define RECORD_LAYER_set_read_ahead(rl, ra)     ((rl)->read_ahead = (ra))
@@ -213,13 +219,16 @@ __owur int ssl3_read_bytes(SSL *s, int type, int *recvd_type,
                            unsigned char *buf, size_t len, int peek,
                            size_t *readbytes);
 __owur int ssl3_setup_buffers(SSL *s);
-__owur int ssl3_enc(SSL *s, SSL3_RECORD *inrecs, size_t n_recs, int send);
+__owur int ssl3_enc(SSL *s, SSL3_RECORD *inrecs, size_t n_recs, int send,
+                    SSL_MAC_BUF *mac, size_t macsize);
 __owur int n_ssl3_mac(SSL *ssl, SSL3_RECORD *rec, unsigned char *md, int send);
 __owur int ssl3_write_pending(SSL *s, int type, const unsigned char *buf, size_t len,
                               size_t *written);
-__owur int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int send);
+__owur int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending,
+                    SSL_MAC_BUF *mac, size_t macsize);
 __owur int tls1_mac(SSL *ssl, SSL3_RECORD *rec, unsigned char *md, int send);
-__owur int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int send);
+__owur int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int send,
+                     SSL_MAC_BUF *mac, size_t macsize);
 int DTLS_RECORD_LAYER_new(RECORD_LAYER *rl);
 void DTLS_RECORD_LAYER_free(RECORD_LAYER *rl);
 void DTLS_RECORD_LAYER_clear(RECORD_LAYER *rl);
diff --git a/ssl/record/record_local.h b/ssl/record/record_local.h
index f7734d832b..dc92732243 100644
--- a/ssl/record/record_local.h
+++ b/ssl/record/record_local.h
@@ -107,13 +107,16 @@ void SSL3_RECORD_set_seq_num(SSL3_RECORD *r, const unsigned char *seq_num);
 int ssl3_get_record(SSL *s);
 __owur int ssl3_do_compress(SSL *ssl, SSL3_RECORD *wr);
 __owur int ssl3_do_uncompress(SSL *ssl, SSL3_RECORD *rr);
-int ssl3_cbc_copy_mac(unsigned char *out,
-                       const SSL3_RECORD *rec, size_t md_size);
-__owur int ssl3_cbc_remove_padding(SSL3_RECORD *rec,
-                                   size_t block_size, size_t mac_size);
-__owur int tls1_cbc_remove_padding(const SSL *s,
-                                   SSL3_RECORD *rec,
-                                   size_t block_size, size_t mac_size);
+__owur int ssl3_cbc_remove_padding_and_mac(SSL *s,
+                                           SSL3_RECORD *rec,
+                                           unsigned char **mac,
+                                           int *alloced,
+                                           size_t block_size, size_t mac_size);
+__owur int tls1_cbc_remove_padding_and_mac(const SSL *s,
+                                           SSL3_RECORD *rec,
+                                           unsigned char **mac,
+                                           int *alloced,
+                                           size_t block_size, size_t mac_size);
 int dtls1_process_record(SSL *s, DTLS1_BITMAP *bitmap);
 __owur int dtls1_get_record(SSL *s);
 int early_data_count_ok(SSL *s, size_t length, size_t overhead, int send);
diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c
index a2f7f848d1..3b1007f574 100644
--- a/ssl/record/ssl3_record.c
+++ b/ssl/record/ssl3_record.c
@@ -32,6 +32,14 @@ static const unsigned char ssl3_pad_2[48] = {
     0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c
 };
 
+static int ssl3_cbc_copy_mac(const SSL *s,
+                             SSL3_RECORD *rec,
+                             unsigned char **mac,
+                             int *alloced,
+                             size_t block_size,
+                             size_t mac_size,
+                             size_t good);
+
 /*
  * Clear the contents of an SSL3_RECORD but retain any memory allocated
  */
@@ -182,12 +190,13 @@ int ssl3_get_record(SSL *s)
     unsigned char *p;
     unsigned char md[EVP_MAX_MD_SIZE];
     unsigned int version;
-    size_t mac_size;
+    size_t mac_size = 0;
     int imac_size;
     size_t num_recs = 0, max_recs, j;
     PACKET pkt, sslv2pkt;
-    size_t first_rec_len;
     int is_ktls_left;
+    SSL_MAC_BUF *macbufs = NULL;
+    int ret = -1;
 
     rr = RECORD_LAYER_get_rrec(&s->rlayer);
     rbuf = RECORD_LAYER_get_rbuf(&s->rlayer);
@@ -526,20 +535,28 @@ int ssl3_get_record(SSL *s)
     if (BIO_get_ktls_recv(s->rbio) && !is_ktls_left)
         goto skip_decryption;
 
+    /* TODO(size_t): convert this to do size_t properly */
+    if (s->read_hash != NULL) {
+        const EVP_MD *tmpmd = EVP_MD_CTX_md(s->read_hash);
+
+        if (tmpmd != NULL) {
+            imac_size = EVP_MD_size(tmpmd);
+            if (!ossl_assert(imac_size >= 0 && imac_size <= EVP_MAX_MD_SIZE)) {
+                    SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_SSL3_GET_RECORD,
+                             ERR_LIB_EVP);
+                    return -1;
+            }
+            mac_size = (size_t)imac_size;
+        }
+    }
+
     /*
      * If in encrypt-then-mac mode calculate mac from encrypted record. All
      * the details below are public so no timing details can leak.
      */
     if (SSL_READ_ETM(s) && s->read_hash) {
         unsigned char *mac;
-        /* TODO(size_t): convert this to do size_t properly */
-        imac_size = EVP_MD_CTX_size(s->read_hash);
-        if (!ossl_assert(imac_size >= 0 && imac_size <= EVP_MAX_MD_SIZE)) {
-                SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_SSL3_GET_RECORD,
-                         ERR_LIB_EVP);
-                return -1;
-        }
-        mac_size = (size_t)imac_size;
+
         for (j = 0; j < num_recs; j++) {
             thisrr = &rr[j];
 
@@ -557,27 +574,39 @@ int ssl3_get_record(SSL *s)
                 return -1;
             }
         }
+        /*
+         * We've handled the mac now - there is no MAC inside the encrypted
+         * record
+         */
+        mac_size = 0;
     }
 
-    first_rec_len = rr[0].length;
+    if (mac_size > 0) {
+        macbufs = OPENSSL_zalloc(sizeof(*macbufs) * num_recs);
+        if (macbufs == NULL) {
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_SSL3_GET_RECORD,
+                     ERR_R_MALLOC_FAILURE);
+            return -1;
+        }
+    }
 
-    enc_err = s->method->ssl3_enc->enc(s, rr, num_recs, 0);
+    enc_err = s->method->ssl3_enc->enc(s, rr, num_recs, 0, macbufs, mac_size);
 
     /*-
      * enc_err is:
-     *    0: (in non-constant time) if the record is publicly invalid.
-     *    1: if the padding is valid
-     *    -1: if the padding is invalid
+     *    0: if the record is publicly invalid, or an internal error, or AEAD
+     *       decryption failed, or ETM decryption failed.
+     *    1: Success or MTE decryption failed (MAC will be randomised)
      */
     if (enc_err == 0) {
         if (ossl_statem_in_error(s)) {
             /* SSLfatal() already got called */
-            return -1;
+            goto end;
         }
         if (num_recs == 1 && ossl_statem_skip_early_data(s)) {
             /*
-             * Valid early_data that we cannot decrypt might fail here as
-             * publicly invalid. We treat it like an empty record.
+             * Valid early_data that we cannot decrypt will fail here. We treat
+             * it like an empty record.
              */
 
             thisrr = &rr[0];
@@ -585,18 +614,19 @@ int ssl3_get_record(SSL *s)
             if (!early_data_count_ok(s, thisrr->length,
                                      EARLY_DATA_CIPHERTEXT_OVERHEAD, 0)) {
                 /* SSLfatal() already called */
-                return -1;
+                goto end;
             }
 
             thisrr->length = 0;
             thisrr->read = 1;
             RECORD_LAYER_set_numrpipes(&s->rlayer, 1);
             RECORD_LAYER_reset_read_sequence(&s->rlayer);
-            return 1;
+            ret = 1;
+            goto end;
         }
         SSLfatal(s, SSL_AD_BAD_RECORD_MAC, SSL_F_SSL3_GET_RECORD,
-                 SSL_R_BLOCK_CIPHER_PAD_IS_WRONG);
-        return -1;
+                 SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC);
+        goto end;
     }
     OSSL_TRACE_BEGIN(TLS) {
         BIO_printf(trc_out, "dec %lu\n", (unsigned long)rr[0].length);
@@ -608,93 +638,24 @@ int ssl3_get_record(SSL *s)
         (s->enc_read_ctx != NULL) &&
         (!SSL_READ_ETM(s) && EVP_MD_CTX_md(s->read_hash) != NULL)) {
         /* s->read_hash != NULL => mac_size != -1 */
-        unsigned char *mac = NULL;
-        unsigned char mac_tmp[EVP_MAX_MD_SIZE];
-
-        mac_size = EVP_MD_CTX_size(s->read_hash);
-        if (!ossl_assert(mac_size <= EVP_MAX_MD_SIZE)) {
-            SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_SSL3_GET_RECORD,
-                     ERR_R_INTERNAL_ERROR);
-            return -1;
-        }
 
         for (j = 0; j < num_recs; j++) {
+            SSL_MAC_BUF *thismb = &macbufs[j];
             thisrr = &rr[j];
-            /*
-             * orig_len is the length of the record before any padding was
-             * removed. This is public information, as is the MAC in use,
-             * therefore we can safely process the record in a different amount
-             * of time if it's too short to possibly contain a MAC.
-             */
-            if (thisrr->orig_len < mac_size ||
-                /* CBC records must have a padding length byte too. */
-                (EVP_CIPHER_CTX_mode(s->enc_read_ctx) == EVP_CIPH_CBC_MODE &&
-                 thisrr->orig_len < mac_size + 1)) {
-                SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_F_SSL3_GET_RECORD,
-                         SSL_R_LENGTH_TOO_SHORT);
-                return -1;
-            }
-
-            if (EVP_CIPHER_CTX_mode(s->enc_read_ctx) == EVP_CIPH_CBC_MODE) {
-                /*
-                 * We update the length so that the TLS header bytes can be
-                 * constructed correctly but we need to extract the MAC in
-                 * constant time from within the record, without leaking the
-                 * contents of the padding bytes.
-                 */
-                mac = mac_tmp;
-                if (!ssl3_cbc_copy_mac(mac_tmp, thisrr, mac_size)) {
-                    SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_SSL3_GET_RECORD,
-                             ERR_R_INTERNAL_ERROR);
-                    return -1;
-                }
-                thisrr->length -= mac_size;
-            } else {
-                /*
-                 * In this case there's no padding, so |rec->orig_len| equals
-                 * |rec->length| and we checked that there's enough bytes for
-                 * |mac_size| above.
-                 */
-                thisrr->length -= mac_size;
-                mac = &thisrr->data[thisrr->length];
-            }
 
             i = s->method->ssl3_enc->mac(s, thisrr, md, 0 /* not send */ );
-            if (i == 0 || mac == NULL
-                || CRYPTO_memcmp(md, mac, (size_t)mac_size) != 0)
-                enc_err = -1;
+            if (i == 0 || thismb == NULL || thismb->mac == NULL
+                || CRYPTO_memcmp(md, thismb->mac, (size_t)mac_size) != 0)
+                enc_err = 0;
             if (thisrr->length > SSL3_RT_MAX_COMPRESSED_LENGTH + mac_size)
-                enc_err = -1;
+                enc_err = 0;
         }
     }
 
-    if (enc_err < 0) {
+    if (enc_err == 0) {
         if (ossl_statem_in_error(s)) {
             /* We already called SSLfatal() */
-            return -1;
-        }
-        if (num_recs == 1 && ossl_statem_skip_early_data(s)) {
-            /*
-             * We assume this is unreadable early_data - we treat it like an
-             * empty record
-             */
-
-            /*
-             * The record length may have been modified by the mac check above
-             * so we use the previously saved value
-             */
-            if (!early_data_count_ok(s, first_rec_len,
-                                     EARLY_DATA_CIPHERTEXT_OVERHEAD, 0)) {
-                /* SSLfatal() already called */
-                return -1;
-            }
-
-            thisrr = &rr[0];
-            thisrr->length = 0;
-            thisrr->read = 1;
-            RECORD_LAYER_set_numrpipes(&s->rlayer, 1);
-            RECORD_LAYER_reset_read_sequence(&s->rlayer);
-            return 1;
+            goto end;
         }
         /*
          * A separate 'decryption_failed' alert was introduced with TLS 1.0,
@@ -705,7 +666,7 @@ int ssl3_get_record(SSL *s)
          */
         SSLfatal(s, SSL_AD_BAD_RECORD_MAC, SSL_F_SSL3_GET_RECORD,
                  SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC);
-        return -1;
+        goto end;
     }
 
  skip_decryption:
@@ -718,12 +679,12 @@ int ssl3_get_record(SSL *s)
             if (thisrr->length > SSL3_RT_MAX_COMPRESSED_LENGTH) {
                 SSLfatal(s, SSL_AD_RECORD_OVERFLOW, SSL_F_SSL3_GET_RECORD,
                          SSL_R_COMPRESSED_LENGTH_TOO_LONG);
-                return -1;
+                goto end;
             }
             if (!ssl3_do_uncompress(s, thisrr)) {
                 SSLfatal(s, SSL_AD_DECOMPRESSION_FAILURE, SSL_F_SSL3_GET_RECORD,
                          SSL_R_BAD_DECOMPRESSION);
-                return -1;
+                goto end;
             }
         }
 
@@ -736,7 +697,7 @@ int ssl3_get_record(SSL *s)
                     || thisrr->type != SSL3_RT_APPLICATION_DATA) {
                 SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_F_SSL3_GET_RECORD,
                          SSL_R_BAD_RECORD_TYPE);
-                return -1;
+                goto end;
             }
 
             /* Strip trailing padding */
@@ -751,7 +712,7 @@ int ssl3_get_record(SSL *s)
                     && thisrr->type != SSL3_RT_HANDSHAKE) {
                 SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_F_SSL3_GET_RECORD,
                          SSL_R_BAD_RECORD_TYPE);
-                return -1;
+                goto end;
             }
             if (s->msg_callback)
                 s->msg_callback(0, s->version, SSL3_RT_INNER_CONTENT_TYPE,
@@ -768,13 +729,13 @@ int ssl3_get_record(SSL *s)
                 && thisrr->length == 0) {
             SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_F_SSL3_GET_RECORD,
                      SSL_R_BAD_LENGTH);
-            return -1;
+            goto end;
         }
 
         if (thisrr->length > SSL3_RT_MAX_PLAIN_LENGTH && !BIO_get_ktls_recv(s->rbio)) {
             SSLfatal(s, SSL_AD_RECORD_OVERFLOW, SSL_F_SSL3_GET_RECORD,
                      SSL_R_DATA_LENGTH_TOO_LONG);
-            return -1;
+            goto end;
         }
 
         /* If received packet overflows current Max Fragment Length setting */
@@ -783,7 +744,7 @@ int ssl3_get_record(SSL *s)
                 && !BIO_get_ktls_recv(s->rbio)) {
             SSLfatal(s, SSL_AD_RECORD_OVERFLOW, SSL_F_SSL3_GET_RECORD,
                      SSL_R_DATA_LENGTH_TOO_LONG);
-            return -1;
+            goto end;
         }
 
         thisrr->off = 0;
@@ -802,7 +763,7 @@ int ssl3_get_record(SSL *s)
                 > MAX_EMPTY_RECORDS) {
                 SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_F_SSL3_GET_RECORD,
                          SSL_R_RECORD_TOO_SMALL);
-                return -1;
+                goto end;
             }
         } else {
             RECORD_LAYER_reset_empty_record_count(&s->rlayer);
@@ -814,12 +775,21 @@ int ssl3_get_record(SSL *s)
         if (thisrr->type == SSL3_RT_APPLICATION_DATA
                 && !early_data_count_ok(s, thisrr->length, 0, 0)) {
             /* SSLfatal already called */
-            return -1;
+            goto end;
         }
     }
 
     RECORD_LAYER_set_numrpipes(&s->rlayer, num_recs);
-    return 1;
+    ret = 1;
+ end:
+    if (macbufs != NULL) {
+        for (j = 0; j < num_recs; j++) {
+            if (macbufs[j].alloced)
+                OPENSSL_free(macbufs[j].mac);
+        }
+        OPENSSL_free(macbufs);
+    }
+    return ret;
 }
 
 int ssl3_do_uncompress(SSL *ssl, SSL3_RECORD *rr)
@@ -866,23 +836,21 @@ int ssl3_do_compress(SSL *ssl, SSL3_RECORD *wr)
 }
 
 /*-
- * ssl3_enc encrypts/decrypts |n_recs| records in |inrecs|.  Will call
- * SSLfatal() for internal errors, but not otherwise.
+ * ssl3_enc encrypts/decrypts |n_recs| records in |inrecs|. Calls SSLfatal on
+ * internal error, but not otherwise. It is the responsibility of the caller to
+ * report a bad_record_mac
  *
  * Returns:
- *   0: (in non-constant time) if the record is publicly invalid (i.e. too
- *       short etc).
- *   1: if the record's padding is valid / the encryption was successful.
- *   -1: if the record's padding is invalid or, if sending, an internal error
- *       occurred.
+ *    0: if the record is publicly invalid, or an internal error
+ *    1: Success or Mac-then-encrypt decryption failed (MAC will be randomised)
  */
-int ssl3_enc(SSL *s, SSL3_RECORD *inrecs, size_t n_recs, int sending)
+int ssl3_enc(SSL *s, SSL3_RECORD *inrecs, size_t n_recs, int sending,
+             SSL_MAC_BUF *mac, size_t macsize)
 {
     SSL3_RECORD *rec;
     EVP_CIPHER_CTX *ds;
     size_t l, i;
-    size_t bs, mac_size = 0;
-    int imac_size;
+    size_t bs;
     const EVP_CIPHER *enc;
 
     rec = inrecs;
@@ -930,52 +898,50 @@ int ssl3_enc(SSL *s, SSL3_RECORD *inrecs, size_t n_recs, int sending)
         }
 
         if (!sending) {
-            if (l == 0 || l % bs != 0)
+            if (l == 0 || l % bs != 0) {
+                /* Publicly invalid */
                 return 0;
+            }
             /* otherwise, rec->length >= bs */
         }
 
         /* TODO(size_t): Convert this call */
-        if (EVP_Cipher(ds, rec->data, rec->input, (unsigned int)l) < 1)
-            return -1;
-
-        if (EVP_MD_CTX_md(s->read_hash) != NULL) {
-            /* TODO(size_t): convert me */
-            imac_size = EVP_MD_CTX_size(s->read_hash);
-            if (imac_size < 0) {
-                SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_SSL3_ENC,
-                         ERR_R_INTERNAL_ERROR);
-                return -1;
-            }
-            mac_size = (size_t)imac_size;
+        if (EVP_Cipher(ds, rec->data, rec->input, (unsigned int)l) < 1) {
+            /* Shouldn't happen */
+            SSLfatal(s, SSL_AD_BAD_RECORD_MAC, 0, ERR_R_INTERNAL_ERROR);
+            return 0;
         }
-        if ((bs != 1) && !sending)
-            return ssl3_cbc_remove_padding(rec, bs, mac_size);
+
+        if (!sending)
+            return !ssl3_cbc_remove_padding_and_mac(s, rec,
+                                        (mac != NULL) ? &mac->mac : NULL,
+                                        (mac != NULL) ? &mac->alloced : NULL,
+                                        bs,
+                                        macsize);
     }
     return 1;
 }
 
 #define MAX_PADDING 256
 /*-
- * tls1_enc encrypts/decrypts |n_recs| in |recs|.  Will call SSLfatal() for
- * internal errors, but not otherwise.
+ * tls1_enc encrypts/decrypts |n_recs| in |recs|. Calls SSLfatal on internal
+ * error, but not otherwise. It is the responsibility of the caller to report
+ * a bad_record_mac - if appropriate (DTLS just drops the record).
  *
  * Returns:
- *   0: (in non-constant time) if the record is publicly invalid (i.e. too
- *       short etc).
- *   1: if the record's padding is valid / the encryption was successful.
- *   -1: if the record's padding/AEAD-authenticator is invalid or, if sending,
- *       an internal error occurred.
+ *    0: if the record is publicly invalid, or an internal error, or AEAD
+ *       decryption failed, or Encrypt-then-mac decryption failed.
+ *    1: Success or Mac-then-encrypt decryption failed (MAC will be randomised)
  */
-int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
+int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending,
+             SSL_MAC_BUF *macs, size_t macsize)
 {
     EVP_CIPHER_CTX *ds;
     size_t reclen[SSL_MAX_PIPELINES];
     unsigned char buf[SSL_MAX_PIPELINES][EVP_AEAD_TLS1_AAD_LEN];
-    int i, pad = 0, ret, tmpr;
-    size_t bs, mac_size = 0, ctr, padnum, loop;
+    int i, pad = 0, tmpr;
+    size_t bs, ctr, padnum, loop;
     unsigned char padval;
-    int imac_size;
     const EVP_CIPHER *enc;
     int tlstree_enc = sending ? (s->mac_flags & SSL_MAC_FLAG_WRITE_MAC_TLSTREE)
                               : (s->mac_flags & SSL_MAC_FLAG_READ_MAC_TLSTREE);
@@ -992,7 +958,7 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
             if (!ossl_assert(n >= 0)) {
                 SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
                          ERR_R_INTERNAL_ERROR);
-                return -1;
+                return 0;
             }
         }
         ds = s->enc_write_ctx;
@@ -1016,12 +982,12 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
                          */
                         SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
                                  ERR_R_INTERNAL_ERROR);
-                        return -1;
+                        return 0;
                     } else if (RAND_bytes_ex(s->ctx->libctx, recs[ctr].input,
                                              ivlen) <= 0) {
                         SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
                                  ERR_R_INTERNAL_ERROR);
-                        return -1;
+                        return 0;
                     }
                 }
             }
@@ -1032,7 +998,7 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
             if (!ossl_assert(n >= 0)) {
                 SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
                          ERR_R_INTERNAL_ERROR);
-                return -1;
+                return 0;
             }
         }
         ds = s->enc_read_ctx;
@@ -1047,7 +1013,6 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
             memmove(recs[ctr].data, recs[ctr].input, recs[ctr].length);
             recs[ctr].input = recs[ctr].data;
         }
-        ret = 1;
     } else {
         bs = EVP_CIPHER_block_size(EVP_CIPHER_CTX_cipher(ds));
 
@@ -1060,7 +1025,7 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
                  */
                 SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
                          SSL_R_PIPELINE_FAILURE);
-                return -1;
+                return 0;
             }
         }
         for (ctr = 0; ctr < n_recs; ctr++) {
@@ -1100,7 +1065,7 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
                 if (pad <= 0) {
                     SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
                              ERR_R_INTERNAL_ERROR);
-                    return -1;
+                    return 0;
                 }
 
                 if (sending) {
@@ -1116,7 +1081,7 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
                 if (padnum > MAX_PADDING) {
                     SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
                              ERR_R_INTERNAL_ERROR);
-                    return -1;
+                    return 0;
                 }
                 /* we need to add 'padnum' padding bytes of value padval */
                 padval = (unsigned char)(padnum - 1);
@@ -1127,8 +1092,10 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
             }
 
             if (!sending) {
-                if (reclen[ctr] == 0 || reclen[ctr] % bs != 0)
+                if (reclen[ctr] == 0 || reclen[ctr] % bs != 0) {
+                    /* Publicly invalid */
                     return 0;
+                }
             }
         }
         if (n_recs > 1) {
@@ -1142,7 +1109,7 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
                                     (int)n_recs, data) <= 0) {
                 SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
                          SSL_R_PIPELINE_FAILURE);
-                return -1;
+                return 0;
             }
             /* Set the input buffers */
             for (ctr = 0; ctr < n_recs; ctr++) {
@@ -1154,7 +1121,7 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
                                        (int)n_recs, reclen) <= 0) {
                 SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
                          SSL_R_PIPELINE_FAILURE);
-                return -1;
+                return 0;
             }
         }
 
@@ -1175,7 +1142,7 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
             if (EVP_CIPHER_CTX_ctrl(ds, EVP_CTRL_TLSTREE, decrement_seq, seq) <= 0) {
                 SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
                          ERR_R_INTERNAL_ERROR);
-              return -1;
+                return 0;
             }
         }
 
@@ -1185,8 +1152,10 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
         if ((EVP_CIPHER_flags(EVP_CIPHER_CTX_cipher(ds))
              & EVP_CIPH_FLAG_CUSTOM_CIPHER)
             ? (tmpr < 0)
-            : (tmpr == 0))
-            return -1;          /* AEAD can fail to verify MAC */
+            : (tmpr == 0)) {
+            /* AEAD can fail to verify MAC */
+            return 0;
+        }
 
         if (sending == 0) {
             if (EVP_CIPHER_mode(enc) == EVP_CIPH_GCM_MODE) {
@@ -1204,29 +1173,18 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
             }
         }
 
-        ret = 1;
-        if (!SSL_READ_ETM(s) && EVP_MD_CTX_md(s->read_hash) != NULL) {
-            imac_size = EVP_MD_CTX_size(s->read_hash);
-            if (imac_size < 0) {
-                SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
-                         ERR_R_INTERNAL_ERROR);
-                return -1;
-            }
-            mac_size = (size_t)imac_size;
-        }
-        if ((bs != 1) && !sending) {
-            int tmpret;
+        if (!sending) {
             for (ctr = 0; ctr < n_recs; ctr++) {
-                tmpret = tls1_cbc_remove_padding(s, &recs[ctr], bs, mac_size);
                 /*
-                 * If tmpret == 0 then this means publicly invalid so we can
-                 * short circuit things here. Otherwise we must respect constant
-                 * time behaviour.
+                 * If using Mac-then-encrypt, then this will succeed but with a
+                 * random MAC if padding is invalid
                  */
-                if (tmpret == 0)
+                if (!tls1_cbc_remove_padding_and_mac(s, &recs[ctr],
+                                     (macs != NULL) ? &macs[ctr].mac : NULL,
+                                     (macs != NULL) ? &macs[ctr].alloced : NULL,
+                                     bs,
+                                     macsize))
                     return 0;
-                ret = constant_time_select_int(constant_time_eq_int(tmpret, 1),
-                                               ret, -1);
             }
         }
         if (pad && !sending) {
@@ -1235,7 +1193,7 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
             }
         }
     }
-    return ret;
+    return 1;
 }
 
 int n_ssl3_mac(SSL *ssl, SSL3_RECORD *rec, unsigned char *md, int sending)
@@ -1448,16 +1406,20 @@ int tls1_mac(SSL *ssl, SSL3_RECORD *rec, unsigned char *md, int sending)
 
 /*-
  * ssl3_cbc_remove_padding removes padding from the decrypted, SSLv3, CBC
- * record in |rec| by updating |rec->length| in constant time.
+ * record in |rec| by updating |rec->length| in constant time. It also extracts
+ * the MAC from the underlying record.
  *
  * block_size: the block size of the cipher used to encrypt the record.
  * returns:
- *   0: (in non-constant time) if the record is publicly invalid.
- *   1: if the padding was valid
- *  -1: otherwise.
+ *   0: if the record is publicly invalid.
+ *   1: if the record is publicly valid. If the padding removal fails then the
+ *      MAC returned is random.
  */
-int ssl3_cbc_remove_padding(SSL3_RECORD *rec,
-                            size_t block_size, size_t mac_size)
+int ssl3_cbc_remove_padding_and_mac(SSL *s,
+                                    SSL3_RECORD *rec,
+                                    unsigned char **mac,
+                                    int *alloced,
+                                    size_t block_size, size_t mac_size)
 {
     size_t padding_length;
     size_t good;
@@ -1474,86 +1436,95 @@ int ssl3_cbc_remove_padding(SSL3_RECORD *rec,
     /* SSLv3 requires that the padding is minimal. */
     good &= constant_time_ge_s(block_size, padding_length + 1);
     rec->length -= good & (padding_length + 1);
-    return constant_time_select_int_s(good, 1, -1);
+
+    return ssl3_cbc_copy_mac(s, rec, mac, alloced, block_size, mac_size, good);
 }
 
 /*-
  * tls1_cbc_remove_padding removes the CBC padding from the decrypted, TLS, CBC
- * record in |rec| in constant time and returns 1 if the padding is valid and
- * -1 otherwise. It also removes any explicit IV from the start of the record
- * without leaking any timing about whether there was enough space after the
- * padding was removed.
+ * record in |rec| in constant time. It also removes any explicit IV from the
+ * start of the record without leaking any timing about whether there was enough
+ * space after the padding was removed, as well as extracting the embedded MAC
+ * (also in constant time). For Mac-then-encrypt, if the padding is invalid then
+ * a success result will occur and a randomised MAC will be returned.
  *
  * block_size: the block size of the cipher used to encrypt the record.
  * returns:
- *   0: (in non-constant time) if the record is publicly invalid.
- *   1: if the padding was valid
- *  -1: otherwise.
+ *    0: if the record is publicly invalid, or an internal error
+ *    1: Success or Mac-then-encrypt decryption failed (MAC will be randomised)
  */
-int tls1_cbc_remove_padding(const SSL *s,
-                            SSL3_RECORD *rec,
-                            size_t block_size, size_t mac_size)
+int tls1_cbc_remove_padding_and_mac(const SSL *s,
+                                    SSL3_RECORD *rec,
+                                    unsigned char **mac,
+                                    int *alloced,
+                                    size_t block_size, size_t mac_size)
 {
     size_t good;
     size_t padding_length, to_check, i;
-    const size_t overhead = 1 /* padding length byte */  + mac_size;
-    /* Check if version requires explicit IV */
-    if (SSL_USE_EXPLICIT_IV(s)) {
-        /*
-         * These lengths are all public so we can test them in non-constant
-         * time.
-         */
-        if (overhead + block_size > rec->length)
-            return 0;
-        /* We can now safely skip explicit IV */
-        rec->data += block_size;
-        rec->input += block_size;
-        rec->length -= block_size;
-        rec->orig_len -= block_size;
-    } else if (overhead > rec->length)
+    size_t overhead = ((block_size == 1) ? 0 : 1) /* padding length byte */
+                      + (SSL_USE_EXPLICIT_IV(s) ? block_size : 0)
+                      + mac_size;
+
+    /*
+     * These lengths are all public so we can test them in non-constant
+     * time.
+     */
+    if (overhead > rec->length)
         return 0;
 
-    padding_length = rec->data[rec->length - 1];
+    if (block_size != 1) {
+        if (SSL_USE_EXPLICIT_IV(s)) {
+            rec->data += block_size;
+            rec->input += block_size;
+            rec->length -= block_size;
+            rec->orig_len -= block_size;
+            overhead -= block_size;
+        }
 
-    if (EVP_CIPHER_flags(EVP_CIPHER_CTX_cipher(s->enc_read_ctx)) &
-        EVP_CIPH_FLAG_AEAD_CIPH