From 921d84a0ad2e70ad91b6e1b06573e97162387f8a Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Mon, 4 Dec 2017 16:54:59 +0000 Subject: [PATCH] Convert the remaining functions in the record layer to use SSLfatal() Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/4841) --- crypto/err/openssl.txt | 2 ++ include/openssl/sslerr.h | 2 ++ ssl/record/rec_layer_d1.c | 9 +++-- ssl/record/rec_layer_s3.c | 12 ++++--- ssl/record/ssl3_record.c | 66 ++++++++++++++++++++++++++-------- ssl/record/ssl3_record_tls13.c | 29 ++++++++++++--- ssl/ssl_err.c | 2 ++ 7 files changed, 96 insertions(+), 26 deletions(-) diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index 932fc466aa..308abae13e 100644 --- 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 ef6b9dd403..b54459b0d0 100644 --- 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 71adbe46f1..ddb3a61832 100644 --- 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 7f35cb897a..5f01b04139 100644 --- 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 049452ba49..213f00104d 100644 --- 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 696cc37fd3..f1e1667b9d 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 @@ 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 7710ef0fd8..7b201e0260 100644 --- 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), -- 2.34.1