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 932fc46..308abae 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt
@@ -1043,6 +1043,7 @@ 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_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 ef6b9dd..b54459b 100644 --- a/include/openssl/sslerr.h +++ b/include/openssl/sslerr.h
@@ -97,6 +97,7 @@ # 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 @@ # 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 71adbe4..ddb3a61 100644 --- a/ssl/record/rec_layer_d1.c +++ b/ssl/record/rec_layer_d1.c
@@ -356,7 +356,8 @@ 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 @@ 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 7f35cb8..5f01b04 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c
@@ -983,14 +983,18 @@ * 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 049452b..213f001 100644 --- a/ssl/record/ssl3_record.c +++ b/ssl/record/ssl3_record.c
@@ -488,6 +488,10 @@ * -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 @@ } 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 @@ } /*- - * 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 @@ 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 @@ #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 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 @@ * 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 @@ 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 @@ * 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 @@ 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 @@ /* 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 @@ } 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)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 @@ ? (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 @@ 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 @@ * -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 696cc37..f1e1667 100644 --- 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 @@ 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 @@ 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 @@ * 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 @@ 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 @@ /* 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 @@ 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 7710ef0..7b201e0 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c
@@ -128,6 +128,7 @@ "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 @@ {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),