From 99dd374055e9179eea082d4c37fd19ed8814fb22 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Mon, 4 Dec 2017 14:47:04 +0000 Subject: [PATCH] Convert ssl3_read_bytes() to use SSLfatal() Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/4841) --- ssl/record/rec_layer_s3.c | 118 +++++++++++++++++++------------------- 1 file changed, 58 insertions(+), 60 deletions(-) diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index 8faf661ddc..99db041830 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -1152,7 +1152,7 @@ int ssl3_write_pending(SSL *s, int type, const unsigned char *buf, size_t len, int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, size_t len, int peek, size_t *readbytes) { - int al, i, j, ret; + int i, j, ret; size_t n, curr_rec, num_recs, totalbytes; SSL3_RECORD *rr; SSL3_BUFFER *rbuf; @@ -1172,7 +1172,8 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, && (type != SSL3_RT_HANDSHAKE)) || (peek && (type != SSL3_RT_APPLICATION_DATA))) { - SSLerr(SSL_F_SSL3_READ_BYTES, ERR_R_INTERNAL_ERROR); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_SSL3_READ_BYTES, + ERR_R_INTERNAL_ERROR); return -1; } @@ -1209,12 +1210,11 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, if (!ossl_statem_get_in_handshake(s) && SSL_in_init(s)) { /* type == SSL3_RT_APPLICATION_DATA */ i = s->handshake_func(s); + /* SSLfatal() already called */ if (i < 0) return i; - if (i == 0) { - SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_SSL_HANDSHAKE_FAILURE); + if (i == 0) return -1; - } } start: s->rwstate = SSL_NOTHING; @@ -1233,14 +1233,16 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, /* get new records if necessary */ if (num_recs == 0) { ret = ssl3_get_record(s); - if (ret <= 0) + if (ret <= 0) { + /* SSLfatal() already called if appropriate */ return ret; + } num_recs = RECORD_LAYER_get_numrpipes(&s->rlayer); if (num_recs == 0) { /* Shouldn't happen */ - al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_SSL3_READ_BYTES, ERR_R_INTERNAL_ERROR); - goto f_err; + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_SSL3_READ_BYTES, + ERR_R_INTERNAL_ERROR); + return -1; } } /* Skip over any records we have already read */ @@ -1268,9 +1270,9 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, if (s->s3->change_cipher_spec /* set when we receive ChangeCipherSpec, * reset by ssl3_get_finished */ && (SSL3_RECORD_get_type(rr) != SSL3_RT_HANDSHAKE)) { - al = SSL_AD_UNEXPECTED_MESSAGE; - SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_DATA_BETWEEN_CCS_AND_FINISHED); - goto f_err; + SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_F_SSL3_READ_BYTES, + SSL_R_DATA_BETWEEN_CCS_AND_FINISHED); + return -1; } /* @@ -1298,17 +1300,17 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, */ if (SSL_in_init(s) && (type == SSL3_RT_APPLICATION_DATA) && (s->enc_read_ctx == NULL)) { - al = SSL_AD_UNEXPECTED_MESSAGE; - SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_APP_DATA_IN_HANDSHAKE); - goto f_err; + SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_F_SSL3_READ_BYTES, + SSL_R_APP_DATA_IN_HANDSHAKE); + return -1; } if (type == SSL3_RT_HANDSHAKE && SSL3_RECORD_get_type(rr) == SSL3_RT_CHANGE_CIPHER_SPEC && s->rlayer.handshake_fragment_len > 0) { - al = SSL_AD_UNEXPECTED_MESSAGE; - SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_CCS_RECEIVED_EARLY); - goto f_err; + SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_F_SSL3_READ_BYTES, + SSL_R_CCS_RECEIVED_EARLY); + return -1; } if (recvd_type != NULL) @@ -1383,9 +1385,9 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, * initial ClientHello. Therefore |type| should always be equal to * |rr->type|. If not then something has gone horribly wrong */ - al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_SSL3_READ_BYTES, ERR_R_INTERNAL_ERROR); - goto f_err; + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_SSL3_READ_BYTES, + ERR_R_INTERNAL_ERROR); + return -1; } if (s->method->version == TLS_ANY_VERSION @@ -1397,9 +1399,9 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, * other than a ClientHello if we are a server. */ s->version = rr->rec_version; - al = SSL_AD_UNEXPECTED_MESSAGE; - SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_UNEXPECTED_MESSAGE); - goto f_err; + SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_F_SSL3_READ_BYTES, + SSL_R_UNEXPECTED_MESSAGE); + return -1; } /* @@ -1472,9 +1474,9 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, || !PACKET_get_1(&alert, &alert_level) || !PACKET_get_1(&alert, &alert_descr) || PACKET_remaining(&alert) != 0) { - al = SSL_AD_UNEXPECTED_MESSAGE; - SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_INVALID_ALERT); - goto f_err; + SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_F_SSL3_READ_BYTES, + SSL_R_INVALID_ALERT); + return -1; } if (s->msg_callback) @@ -1497,9 +1499,9 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, s->rlayer.alert_count++; if (s->rlayer.alert_count == MAX_WARN_ALERT_COUNT) { - al = SSL_AD_UNEXPECTED_MESSAGE; - SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_TOO_MANY_WARN_ALERTS); - goto f_err; + SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_F_SSL3_READ_BYTES, + SSL_R_TOO_MANY_WARN_ALERTS); + return -1; } if (alert_descr == SSL_AD_CLOSE_NOTIFY) { @@ -1511,9 +1513,9 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, * is user_cancelled - which we just ignore. */ if (SSL_IS_TLS13(s) && alert_descr != SSL_AD_USER_CANCELLED) { - al = SSL_AD_ILLEGAL_PARAMETER; - SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_UNKNOWN_ALERT_TYPE); - goto f_err; + SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_F_SSL3_READ_BYTES, + SSL_R_UNKNOWN_ALERT_TYPE); + return -1; } /* * This is a warning but we receive it if we requested @@ -1524,26 +1526,27 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, * the peer refused it where we carry on. */ if (alert_descr == SSL_AD_NO_RENEGOTIATION) { - al = SSL_AD_HANDSHAKE_FAILURE; - SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_NO_RENEGOTIATION); - goto f_err; + SSLfatal(s, SSL_AD_HANDSHAKE_FAILURE, SSL_F_SSL3_READ_BYTES, + SSL_R_NO_RENEGOTIATION); + return -1; } } else if (alert_level == SSL3_AL_FATAL) { char tmp[16]; s->rwstate = SSL_NOTHING; s->s3->fatal_alert = alert_descr; - SSLerr(SSL_F_SSL3_READ_BYTES, SSL_AD_REASON_OFFSET + alert_descr); - BIO_snprintf(tmp, sizeof(tmp), "%d", alert_descr); + SSLfatal(s, SSL_AD_NO_ALERT, SSL_F_SSL3_READ_BYTES, + SSL_AD_REASON_OFFSET + alert_descr); + BIO_snprintf(tmp, sizeof tmp, "%d", alert_descr); ERR_add_error_data(2, "SSL alert number ", tmp); s->shutdown |= SSL_RECEIVED_SHUTDOWN; SSL3_RECORD_set_read(rr); SSL_CTX_remove_session(s->session_ctx, s->session); return 0; } else { - al = SSL_AD_ILLEGAL_PARAMETER; - SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_UNKNOWN_ALERT_TYPE); - goto f_err; + SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_F_SSL3_READ_BYTES, + SSL_R_UNKNOWN_ALERT_TYPE); + return -1; } goto start; @@ -1558,9 +1561,9 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, } if (SSL3_RECORD_get_type(rr) == SSL3_RT_CHANGE_CIPHER_SPEC) { - al = SSL_AD_UNEXPECTED_MESSAGE; - SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_CCS_RECEIVED_EARLY); - goto f_err; + SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_F_SSL3_READ_BYTES, + SSL_R_CCS_RECEIVED_EARLY); + return -1; } /* @@ -1575,10 +1578,10 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, ossl_statem_set_in_init(s, 1); i = s->handshake_func(s); + /* SSLfatal() already called if appropriate */ if (i < 0) return i; if (i == 0) { - SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_SSL_HANDSHAKE_FAILURE); return -1; } @@ -1619,9 +1622,9 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, * no progress is being made and the peer continually sends unrecognised * record types, using up resources processing them. */ - al = SSL_AD_UNEXPECTED_MESSAGE; - SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_UNEXPECTED_RECORD); - goto f_err; + SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_F_SSL3_READ_BYTES, + SSL_R_UNEXPECTED_RECORD); + return -1; case SSL3_RT_CHANGE_CIPHER_SPEC: case SSL3_RT_ALERT: case SSL3_RT_HANDSHAKE: @@ -1630,9 +1633,9 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, * SSL3_RT_HANDSHAKE when ossl_statem_get_in_handshake(s) is true, but * that should not happen when type != rr->type */ - al = SSL_AD_UNEXPECTED_MESSAGE; - SSLerr(SSL_F_SSL3_READ_BYTES, ERR_R_INTERNAL_ERROR); - goto f_err; + SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_F_SSL3_READ_BYTES, + ERR_R_INTERNAL_ERROR); + return -1; case SSL3_RT_APPLICATION_DATA: /* * At this point, we were expecting handshake data, but have @@ -1657,21 +1660,16 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, if (!early_data_count_ok(s, rr->length, EARLY_DATA_CIPHERTEXT_OVERHEAD, 0)) { /* SSLfatal() already called */ - goto f_err; + return -1; } SSL3_RECORD_set_read(rr); goto start; } else { - al = SSL_AD_UNEXPECTED_MESSAGE; - SSLerr(SSL_F_SSL3_READ_BYTES, SSL_R_UNEXPECTED_RECORD); - goto f_err; + SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_F_SSL3_READ_BYTES, + SSL_R_UNEXPECTED_RECORD); + return -1; } } - /* not reached */ - - f_err: - ssl3_send_alert(s, SSL3_AL_FATAL, al); - return -1; } void ssl3_record_sequence_update(unsigned char *seq) -- 2.25.1