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)

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);

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);

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);

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,
+__owur int ssl3_cbc_remove_padding_and_mac(SSL *s,
-                       const SSL3_RECORD *rec, size_t md_size);
+                                           SSL3_RECORD *rec,
-__owur int ssl3_cbc_remove_padding(SSL3_RECORD *rec,
+                                           unsigned char **mac,
-                                   size_t block_size, size_t mac_size);
+                                           int *alloced,
-__owur int tls1_cbc_remove_padding(const SSL *s,
+                                           size_t block_size, size_t mac_size);
-                                   SSL3_RECORD *rec,
+__owur int tls1_cbc_remove_padding_and_mac(const SSL *s,
-                                   size_t block_size, size_t mac_size);
+                                           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);

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
+ * tls13_enc encrypts/decrypts |n_recs| in |recs|. Calls SSLfatal on internal
- * internal errors, but not otherwise.
+ * 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
+ *    0: On failure
- *        short etc).
+ *    1: if the record encryption/decryption was successful.
- *    1: if the record encryption was successful.
- *   -1: if the record's AEAD-authenticator is invalid or, if sending,
- *       an internal error occurred.
  */
-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;
     }

ssl/ssl_lib.c

@@ -34,51 +34,37 @@ DEFINE_STACK_OF(OCSP_RESPID)
 DEFINE_STACK_OF(SRTP_PROTECTION_PROFILE)
 DEFINE_STACK_OF(SCT)
 
-static int ssl_undefined_function_1(SSL *ssl, SSL3_RECORD *r, size_t s, int t)
+static int ssl_undefined_function_1(SSL *ssl, SSL3_RECORD *r, size_t s, int t,
+                                    SSL_MAC_BUF *mac, size_t macsize)
 {
-    (void)r;
-    (void)s;
-    (void)t;
     return ssl_undefined_function(ssl);
 }
 
 static int ssl_undefined_function_2(SSL *ssl, SSL3_RECORD *r, unsigned char *s,
                                     int t)
 {
-    (void)r;
-    (void)s;
-    (void)t;
     return ssl_undefined_function(ssl);
 }
 
 static int ssl_undefined_function_3(SSL *ssl, unsigned char *r,
                                     unsigned char *s, size_t t, size_t *u)
 {
-    (void)r;
-    (void)s;
-    (void)t;
-    (void)u;
     return ssl_undefined_function(ssl);
 }
 
 static int ssl_undefined_function_4(SSL *ssl, int r)
 {
-    (void)r;
     return ssl_undefined_function(ssl);
 }
 
 static size_t ssl_undefined_function_5(SSL *ssl, const char *r, size_t s,
                                        unsigned char *t)
 {
-    (void)r;
-    (void)s;
-    (void)t;
     return ssl_undefined_function(ssl);
 }
 
 static int ssl_undefined_function_6(int r)
 {
-    (void)r;
     return ssl_undefined_function(NULL);
 }
 
@@ -86,13 +72,6 @@ static int ssl_undefined_function_7(SSL *ssl, unsigned char *r, size_t s,
                                     const char *t, size_t u,
                                     const unsigned char *v, size_t w, int x)
 {
-    (void)r;
-    (void)s;
-    (void)t;
-    (void)u;
-    (void)v;
-    (void)w;
-    (void)x;
     return ssl_undefined_function(ssl);
 }
 

ssl/ssl_local.h

@@ -2069,7 +2069,7 @@ typedef struct cert_st {
  * of a mess of functions, but hell, think of it as an opaque structure :-)
  */
 typedef struct ssl3_enc_method {
-    int (*enc) (SSL *, SSL3_RECORD *, size_t, int);
+    int (*enc) (SSL *, SSL3_RECORD *, size_t, int, SSL_MAC_BUF *, size_t);
     int (*mac) (SSL *, SSL3_RECORD *, unsigned char *, int);
     int (*setup_key_block) (SSL *);
     int (*generate_master_secret) (SSL *, unsigned char *, unsigned char *,

test/tls13encryptiontest.c

@@ -368,7 +368,7 @@ static int test_tls13_encryption(void)
         }
 
         /* Encrypt it */
-        if (!TEST_size_t_eq(tls13_enc(s, &rec, 1, 1), 1)) {
+        if (!TEST_size_t_eq(tls13_enc(s, &rec, 1, 1, NULL, 0), 1)) {
             TEST_info("Failed to encrypt record %zu", ctr);
             goto err;
         }
@@ -378,7 +378,7 @@ static int test_tls13_encryption(void)
         }
 
         /* Decrypt it */
-        if (!TEST_int_eq(tls13_enc(s, &rec, 1, 0), 1)) {
+        if (!TEST_int_eq(tls13_enc(s, &rec, 1, 0, NULL, 0), 1)) {
             TEST_info("Failed to decrypt record %zu", ctr);
             goto err;
         }