Convert the remaining functions in the record layer to use SSLfatal()

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

diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt
index 932fc466aa4ebaaeec943ee16ed65972a622a347..308abae13ed6cb49c30a80ad113b52853691bdf5 100644 (file)
--- a/crypto/err/openssl.txt
+++ b/crypto/err/openssl.txt
@@ -1043,6 +1043,7 @@ SSL_F_SSL3_CTRL:213:ssl3_ctrl
 SSL_F_SSL3_CTX_CTRL:133:ssl3_ctx_ctrl
 SSL_F_SSL3_DIGEST_CACHED_RECORDS:293:ssl3_digest_cached_records
 SSL_F_SSL3_DO_CHANGE_CIPHER_SPEC:292:ssl3_do_change_cipher_spec
+SSL_F_SSL3_ENC:608:ssl3_enc
 SSL_F_SSL3_FINAL_FINISH_MAC:285:ssl3_final_finish_mac
 SSL_F_SSL3_FINISH_MAC:587:ssl3_finish_mac
 SSL_F_SSL3_GENERATE_KEY_BLOCK:238:ssl3_generate_key_block
@@ -1197,6 +1198,7 @@ SSL_F_STATE_MACHINE:353:state_machine
 SSL_F_TLS12_CHECK_PEER_SIGALG:333:tls12_check_peer_sigalg
 SSL_F_TLS12_COPY_SIGALGS:533:tls12_copy_sigalgs
 SSL_F_TLS13_CHANGE_CIPHER_STATE:440:tls13_change_cipher_state
+SSL_F_TLS13_ENC:609:tls13_enc
 SSL_F_TLS13_FINAL_FINISH_MAC:605:tls13_final_finish_mac
 SSL_F_TLS13_GENERATE_SECRET:591:tls13_generate_secret
 SSL_F_TLS13_HKDF_EXPAND:561:tls13_hkdf_expand

diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h
index ef6b9dd4036ece06e16fe97593fbff82cadcacc0..b54459b0d07b9da2e0b8c9d1e9f40485e77a474d 100644 (file)
--- a/include/openssl/sslerr.h
+++ b/include/openssl/sslerr.h
@@ -97,6 +97,7 @@ int ERR_load_SSL_strings(void);
 # define SSL_F_SSL3_CTX_CTRL                              133
 # define SSL_F_SSL3_DIGEST_CACHED_RECORDS                 293
 # define SSL_F_SSL3_DO_CHANGE_CIPHER_SPEC                 292
+# define SSL_F_SSL3_ENC                                   608
 # define SSL_F_SSL3_FINAL_FINISH_MAC                      285
 # define SSL_F_SSL3_FINISH_MAC                            587
 # define SSL_F_SSL3_GENERATE_KEY_BLOCK                    238
@@ -249,6 +250,7 @@ int ERR_load_SSL_strings(void);
 # define SSL_F_TLS12_CHECK_PEER_SIGALG                    333
 # define SSL_F_TLS12_COPY_SIGALGS                         533
 # define SSL_F_TLS13_CHANGE_CIPHER_STATE                  440
+# define SSL_F_TLS13_ENC                                  609
 # define SSL_F_TLS13_FINAL_FINISH_MAC                     605
 # define SSL_F_TLS13_GENERATE_SECRET                      591
 # define SSL_F_TLS13_HKDF_EXPAND                          561

diff --git a/ssl/record/rec_layer_d1.c b/ssl/record/rec_layer_d1.c
index 71adbe46f16d09c721eadde3d51defcab71f7676..ddb3a6183213b29431e613a504bfaeca5a56cde3 100644 (file)
--- a/ssl/record/rec_layer_d1.c
+++ b/ssl/record/rec_layer_d1.c
@@ -356,7 +356,8 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
     if ((type && (type != SSL3_RT_APPLICATION_DATA) &&
          (type != SSL3_RT_HANDSHAKE)) ||
         (peek && (type != SSL3_RT_APPLICATION_DATA))) {
-        SSLerr(SSL_F_DTLS1_READ_BYTES, ERR_R_INTERNAL_ERROR);
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_DTLS1_READ_BYTES,
+                 ERR_R_INTERNAL_ERROR);
         return -1;
     }
 
@@ -907,8 +908,10 @@ int do_dtls1_write(SSL *s, int type, const unsigned char *buf,
         SSL3_RECORD_add_length(&wr, eivlen);
 
     if (s->method->ssl3_enc->enc(s, &wr, 1, 1) < 1) {
-        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_DO_DTLS1_WRITE,
-                 ERR_R_INTERNAL_ERROR);
+        if (!ossl_statem_in_error(s)) {
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_DO_DTLS1_WRITE,
+                     ERR_R_INTERNAL_ERROR);
+        }
         return -1;
     }
 

diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index 7f35cb897adff90c14f09680d38a17eb3350f786..5f01b04139fa473f6816be521bd71db7b3b66e2d 100644 (file)
--- a/ssl/record/rec_layer_s3.c
+++ b/ssl/record/rec_layer_s3.c
@@ -983,14 +983,18 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
          * send early data - so we need to use the tls13enc function.
          */
         if (tls13_enc(s, wr, numpipes, 1) < 1) {
-            SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_DO_SSL3_WRITE,
-                     ERR_R_INTERNAL_ERROR);
+            if (!ossl_statem_in_error(s)) {
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_DO_SSL3_WRITE,
+                         ERR_R_INTERNAL_ERROR);
+            }
             goto err;
         }
     } else {
         if (s->method->ssl3_enc->enc(s, wr, numpipes, 1) < 1) {
-            SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_DO_SSL3_WRITE,
-                     ERR_R_INTERNAL_ERROR);
+            if (!ossl_statem_in_error(s)) {
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_DO_SSL3_WRITE,
+                         ERR_R_INTERNAL_ERROR);
+            }
             goto err;
         }
     }

diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c
index 049452ba49db104d34a5e391f9dbbff31826bfa2..213f00104d488e8f6e1de7d6f48064532299c3ab 100644 (file)
--- a/ssl/record/ssl3_record.c
+++ b/ssl/record/ssl3_record.c
@@ -488,6 +488,10 @@ int ssl3_get_record(SSL *s)
      *    -1: if the padding is invalid
      */
     if (enc_err == 0) {
+        if (ossl_statem_in_error(s)) {
+            /* SSLfatal() already got called */
+            return -1;
+        }
         if (num_recs == 1 && ossl_statem_skip_early_data(s)) {
             /*
              * Valid early_data that we cannot decrypt might fail here as
@@ -588,6 +592,10 @@ int ssl3_get_record(SSL *s)
     }
 
     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
@@ -776,7 +784,8 @@ int ssl3_do_compress(SSL *ssl, SSL3_RECORD *wr)
 }
 
 /*-
- * ssl3_enc encrypts/decrypts |n_recs| records in |inrecs|
+ * ssl3_enc encrypts/decrypts |n_recs| records in |inrecs|.  Will call
+ * SSLfatal() for internal errors, but not otherwise.
  *
  * Returns:
  *   0: (in non-constant time) if the record is publically invalid (i.e. too
@@ -851,8 +860,11 @@ int ssl3_enc(SSL *s, SSL3_RECORD *inrecs, size_t n_recs, int sending)
         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)
+            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 ((bs != 1) && !sending)
@@ -863,7 +875,8 @@ int ssl3_enc(SSL *s, SSL3_RECORD *inrecs, size_t n_recs, int sending)
 
 #define MAX_PADDING 256
 /*-
- * tls1_enc encrypts/decrypts |n_recs| in |recs|.
+ * tls1_enc encrypts/decrypts |n_recs| in |recs|.  Will call SSLfatal() for
+ * internal errors, but not otherwise.
  *
  * Returns:
  *   0: (in non-constant time) if the record is publically invalid (i.e. too
@@ -883,14 +896,18 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
     int imac_size;
     const EVP_CIPHER *enc;
 
-    if (n_recs == 0)
+    if (n_recs == 0) {
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
+                 ERR_R_INTERNAL_ERROR);
         return 0;
+    }
 
     if (sending) {
         if (EVP_MD_CTX_md(s->write_hash)) {
             int n = EVP_MD_CTX_size(s->write_hash);
             if (!ossl_assert(n >= 0)) {
-                SSLerr(SSL_F_TLS1_ENC, ERR_R_INTERNAL_ERROR);
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
+                         ERR_R_INTERNAL_ERROR);
                 return -1;
             }
         }
@@ -913,10 +930,12 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
                          * we can't write into the input stream: Can this ever
                          * happen?? (steve)
                          */
-                        SSLerr(SSL_F_TLS1_ENC, ERR_R_INTERNAL_ERROR);
+                        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
+                                 ERR_R_INTERNAL_ERROR);
                         return -1;
                     } else if (ssl_randbytes(s, recs[ctr].input, ivlen) <= 0) {
-                        SSLerr(SSL_F_TLS1_ENC, ERR_R_INTERNAL_ERROR);
+                        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
+                                 ERR_R_INTERNAL_ERROR);
                         return -1;
                     }
                 }
@@ -926,7 +945,8 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
         if (EVP_MD_CTX_md(s->read_hash)) {
             int n = EVP_MD_CTX_size(s->read_hash);
             if (!ossl_assert(n >= 0)) {
-                SSLerr(SSL_F_TLS1_ENC, ERR_R_INTERNAL_ERROR);
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
+                         ERR_R_INTERNAL_ERROR);
                 return -1;
             }
         }
@@ -953,7 +973,8 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
                  * We shouldn't have been called with pipeline data if the
                  * cipher doesn't support pipelining
                  */
-                SSLerr(SSL_F_TLS1_ENC, SSL_R_PIPELINE_FAILURE);
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
+                         SSL_R_PIPELINE_FAILURE);
                 return -1;
             }
         }
@@ -991,8 +1012,11 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
                 buf[ctr][12] = (unsigned char)(recs[ctr].length & 0xff);
                 pad = EVP_CIPHER_CTX_ctrl(ds, EVP_CTRL_AEAD_TLS1_AAD,
                                           EVP_AEAD_TLS1_AAD_LEN, buf[ctr]);
-                if (pad <= 0)
+                if (pad <= 0) {
+                    SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
+                             ERR_R_INTERNAL_ERROR);
                     return -1;
+                }
 
                 if (sending) {
                     reclen[ctr] += pad;
@@ -1004,8 +1028,11 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
 
                 /* Add weird padding of upto 256 bytes */
 
-                if (padnum > MAX_PADDING)
+                if (padnum > MAX_PADDING) {
+                    SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
+                             ERR_R_INTERNAL_ERROR);
                     return -1;
+                }
                 /* we need to add 'padnum' padding bytes of value padval */
                 padval = (unsigned char)(padnum - 1);
                 for (loop = reclen[ctr]; loop < reclen[ctr] + padnum; loop++)
@@ -1028,7 +1055,9 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
             }
             if (EVP_CIPHER_CTX_ctrl(ds, EVP_CTRL_SET_PIPELINE_OUTPUT_BUFS,
                                     (int)n_recs, data) <= 0) {
-                SSLerr(SSL_F_TLS1_ENC, SSL_R_PIPELINE_FAILURE);
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
+                         SSL_R_PIPELINE_FAILURE);
+                return -1;
             }
             /* Set the input buffers */
             for (ctr = 0; ctr < n_recs; ctr++) {
@@ -1038,7 +1067,8 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
                                     (int)n_recs, data) <= 0
                 || EVP_CIPHER_CTX_ctrl(ds, EVP_CTRL_SET_PIPELINE_INPUT_LENS,
                                        (int)n_recs, reclen) <= 0) {
-                SSLerr(SSL_F_TLS1_ENC, SSL_R_PIPELINE_FAILURE);
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS1_ENC,
+                         SSL_R_PIPELINE_FAILURE);
                 return -1;
             }
         }
@@ -1051,6 +1081,7 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
             ? (tmpr < 0)
             : (tmpr == 0))
             return -1;          /* AEAD can fail to verify MAC */
+
         if (sending == 0) {
             if (EVP_CIPHER_mode(enc) == EVP_CIPH_GCM_MODE) {
                 for (ctr = 0; ctr < n_recs; ctr++) {
@@ -1070,8 +1101,11 @@ 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)
+            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) {
@@ -1589,6 +1623,10 @@ int dtls1_process_record(SSL *s, DTLS1_BITMAP *bitmap)
      *   -1: if the padding is invalid
      */
     if (enc_err == 0) {
+        if (ossl_statem_in_error(s)) {
+            /* SSLfatal() got called */
+            return 0;
+        }
         /* For DTLS we simply ignore bad packets. */
         rr->length = 0;
         RECORD_LAYER_reset_packet_length(&s->rlayer);

diff --git a/ssl/record/ssl3_record_tls13.c b/ssl/record/ssl3_record_tls13.c
index 696cc37fd3d22b20a7cbba3973785e402c39fc01..f1e1667b9df37355dc83956466121544216e0f54 100644 (file)
--- a/ssl/record/ssl3_record_tls13.c
+++ b/ssl/record/ssl3_record_tls13.c
@@ -12,7 +12,8 @@
 #include "internal/cryptlib.h"
 
 /*-
- * tls13_enc encrypts/decrypts |n_recs| in |recs|.
+ * tls13_enc encrypts/decrypts |n_recs| in |recs|. Will call SSLfatal() for
+ * internal errors, but not otherwise.
  *
  * Returns:
  *    0: (in non-constant time) if the record is publically invalid (i.e. too
@@ -35,6 +36,8 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
     if (n_recs != 1) {
         /* Should not happen */
         /* TODO(TLS1.3): Support pipelining */
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_ENC,
+                 ERR_R_INTERNAL_ERROR);
         return -1;
     }
 
@@ -62,8 +65,11 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
             alg_enc = s->session->cipher->algorithm_enc;
         } else {
             if (!ossl_assert(s->psksession != NULL
-                             && s->psksession->ext.max_early_data > 0))
+                             && s->psksession->ext.max_early_data > 0)) {
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_ENC,
+                         ERR_R_INTERNAL_ERROR);
                 return -1;
+            }
             alg_enc = s->psksession->cipher->algorithm_enc;
         }
     } else {
@@ -71,8 +77,11 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
          * To get here we must have selected a ciphersuite - otherwise ctx would
          * be NULL
          */
-        if (!ossl_assert(s->s3->tmp.new_cipher != NULL))
+        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;
+        }
         alg_enc = s->s3->tmp.new_cipher->algorithm_enc;
     }
 
@@ -82,13 +91,18 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
          else
             taglen = EVP_CCM_TLS_TAG_LEN;
          if (sending && EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, taglen,
-                                         NULL) <= 0)
+                                         NULL) <= 0) {
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_ENC,
+                     ERR_R_INTERNAL_ERROR);
             return -1;
+        }
     } else if (alg_enc & SSL_AESGCM) {
         taglen = EVP_GCM_TLS_TAG_LEN;
     } else if (alg_enc & SSL_CHACHA20) {
         taglen = EVP_CHACHAPOLY_TLS_TAG_LEN;
     } else {
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_ENC,
+                 ERR_R_INTERNAL_ERROR);
         return -1;
     }
 
@@ -105,6 +119,8 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
     /* Set up IV */
     if (ivlen < SEQ_NUM_SIZE) {
         /* Should not happen */
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_ENC,
+                 ERR_R_INTERNAL_ERROR);
         return -1;
     }
     offset = ivlen - SEQ_NUM_SIZE;
@@ -137,8 +153,11 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
     if (sending) {
         /* Add the tag */
         if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_GET_TAG, taglen,
-                                rec->data + rec->length) <= 0)
+                                rec->data + rec->length) <= 0) {
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS13_ENC,
+                     ERR_R_INTERNAL_ERROR);
             return -1;
+        }
         rec->length += taglen;
     }
 

diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index 7710ef0fd872c9e51b5ea46d56c255c750691646..7b201e0260cb084752ff5f466b2dbb6b7e262c6d 100644 (file)
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -128,6 +128,7 @@ static const ERR_STRING_DATA SSL_str_functs[] = {
      "ssl3_digest_cached_records"},
     {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL3_DO_CHANGE_CIPHER_SPEC, 0),
      "ssl3_do_change_cipher_spec"},
+    {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL3_ENC, 0), "ssl3_enc"},
     {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL3_FINAL_FINISH_MAC, 0),
      "ssl3_final_finish_mac"},
     {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL3_FINISH_MAC, 0), "ssl3_finish_mac"},
@@ -362,6 +363,7 @@ static const ERR_STRING_DATA SSL_str_functs[] = {
     {ERR_PACK(ERR_LIB_SSL, SSL_F_TLS12_COPY_SIGALGS, 0), "tls12_copy_sigalgs"},
     {ERR_PACK(ERR_LIB_SSL, SSL_F_TLS13_CHANGE_CIPHER_STATE, 0),
      "tls13_change_cipher_state"},
+    {ERR_PACK(ERR_LIB_SSL, SSL_F_TLS13_ENC, 0), "tls13_enc"},
     {ERR_PACK(ERR_LIB_SSL, SSL_F_TLS13_FINAL_FINISH_MAC, 0),
      "tls13_final_finish_mac"},
     {ERR_PACK(ERR_LIB_SSL, SSL_F_TLS13_GENERATE_SECRET, 0),