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)

1 files changed, 22 insertions, 19 deletions

diff --git a/ssl/record/ssl3_record_tls13.c b/ssl/record/ssl3_record_tls13.c
index f18da2db74..910b6a5862 100644
--- a/ssl/record/ssl3_record_tls13.c
+++ b/ssl/record/ssl3_record_tls13.c
@@ -12,17 +12,16 @@
 #include "internal/cryptlib.h"
 
 /*-
- * tls13_enc encrypts/decrypts |n_recs| in |recs|. Will call SSLfatal() for
- * internal errors, but not otherwise.
+ * tls13_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.
  *
  * Returns:
- *    0: (in non-constant time) if the record is publicly invalid (i.e. too
- *        short etc).
- *    1: if the record encryption was successful.
- *   -1: if the record's AEAD-authenticator is invalid or, if sending,
- *       an internal error occurred.
+ *    0: On failure
+ *    1: if the record encryption/decryption was successful.
  */
-int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
+int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending,
+              SSL_MAC_BUF *mac, size_t macsize)
 {
     EVP_CIPHER_CTX *ctx;
     unsigned char iv[EVP_MAX_IV_LENGTH], recheader[SSL3_RT_HEADER_LENGTH];
@@ -39,7 +38,7 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
         /* TODO(TLS1.3): Support pipelining */
         SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_ENC,
                  ERR_R_INTERNAL_ERROR);
-        return -1;
+        return 0;
     }
 
     if (sending) {
@@ -75,7 +74,7 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
                              && s->psksession->ext.max_early_data > 0)) {
                 SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_ENC,
                          ERR_R_INTERNAL_ERROR);
-                return -1;
+                return 0;
             }
             alg_enc = s->psksession->cipher->algorithm_enc;
         }
@@ -87,7 +86,7 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
         if (!ossl_assert(s->s3.tmp.new_cipher != NULL)) {
             SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_ENC,
                      ERR_R_INTERNAL_ERROR);
-            return -1;
+            return 0;
         }
         alg_enc = s->s3.tmp.new_cipher->algorithm_enc;
     }
@@ -101,7 +100,7 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
                                          NULL) <= 0) {
             SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_ENC,
                      ERR_R_INTERNAL_ERROR);
-            return -1;
+            return 0;
         }
     } else if (alg_enc & SSL_AESGCM) {
         taglen = EVP_GCM_TLS_TAG_LEN;
@@ -110,7 +109,7 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
     } else {
         SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_ENC,
                  ERR_R_INTERNAL_ERROR);
-        return -1;
+        return 0;
     }
 
     if (!sending) {
@@ -128,7 +127,7 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
         /* Should not happen */
         SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_ENC,
                  ERR_R_INTERNAL_ERROR);
-        return -1;
+        return 0;
     }
     offset = ivlen - SEQ_NUM_SIZE;
     memcpy(iv, staticiv, offset);
@@ -143,7 +142,7 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
     }
     if (loop == 0) {
         /* Sequence has wrapped */
-        return -1;
+        return 0;
     }
 
     /* TODO(size_t): lenu/lenf should be a size_t but EVP doesn't support it */
@@ -151,7 +150,9 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
             || (!sending && EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG,
                                              taglen,
                                              rec->data + rec->length) <= 0)) {
-        return -1;
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_ENC,
+                 ERR_R_INTERNAL_ERROR);
+        return 0;
     }
 
     /* Set up the AAD */
@@ -162,8 +163,10 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
             || !WPACKET_get_total_written(&wpkt, &hdrlen)
             || hdrlen != SSL3_RT_HEADER_LENGTH
             || !WPACKET_finish(&wpkt)) {
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_ENC,
+                 ERR_R_INTERNAL_ERROR);
         WPACKET_cleanup(&wpkt);
-        return -1;
+        return 0;
     }
 
     /*
@@ -179,7 +182,7 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
                                 (unsigned int)rec->length) <= 0
             || EVP_CipherFinal_ex(ctx, rec->data + lenu, &lenf) <= 0
             || (size_t)(lenu + lenf) != rec->length) {
-        return -1;
+        return 0;
     }
     if (sending) {
         /* Add the tag */
@@ -187,7 +190,7 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
                                 rec->data + rec->length) <= 0) {
             SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_ENC,
                      ERR_R_INTERNAL_ERROR);
-            return -1;
+            return 0;
         }
         rec->length += taglen;
     }