From 3ec8d113a59162a8ae9020d4f8a501ac2f33d744 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Wed, 22 Nov 2017 17:43:20 +0000 Subject: [PATCH] Convert remaining functions in statem_srvr.c to use SSLfatal() Reviewed-by: Richard Levitte (Merged from https://github.com/openssl/openssl/pull/4778) --- crypto/err/openssl.txt | 8 ++++++ include/openssl/sslerr.h | 5 ++++ ssl/ssl_err.c | 10 +++++++ ssl/statem/extensions_srvr.c | 7 +++-- ssl/statem/statem_srvr.c | 54 ++++++++++++++++++++++++++---------- 5 files changed, 68 insertions(+), 16 deletions(-) diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index f472c06578..d6644835bb 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt @@ -1020,8 +1020,16 @@ SSL_F_OSSL_STATEM_CLIENT_READ_TRANSITION:417:ossl_statem_client_read_transition SSL_F_OSSL_STATEM_CLIENT_WRITE_TRANSITION:599:\ ossl_statem_client_write_transition SSL_F_OSSL_STATEM_SERVER13_READ_TRANSITION:437:* +SSL_F_OSSL_STATEM_SERVER13_WRITE_TRANSITION:600:\ + ossl_statem_server13_write_transition SSL_F_OSSL_STATEM_SERVER_CONSTRUCT_MESSAGE:431:* +SSL_F_OSSL_STATEM_SERVER_POST_PROCESS_MESSAGE:601:\ + ossl_statem_server_post_process_message +SSL_F_OSSL_STATEM_SERVER_POST_WORK:602:ossl_statem_server_post_work +SSL_F_OSSL_STATEM_SERVER_PROCESS_MESSAGE:603:ossl_statem_server_process_message SSL_F_OSSL_STATEM_SERVER_READ_TRANSITION:418:ossl_statem_server_read_transition +SSL_F_OSSL_STATEM_SERVER_WRITE_TRANSITION:604:\ + ossl_statem_server_write_transition SSL_F_PARSE_CA_NAMES:541:parse_ca_names SSL_F_PROCESS_KEY_SHARE_EXT:439:* SSL_F_READ_STATE_MACHINE:352:read_state_machine diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h index ba567bfecf..e4dfc0354a 100644 --- a/include/openssl/sslerr.h +++ b/include/openssl/sslerr.h @@ -77,8 +77,13 @@ int ERR_load_SSL_strings(void); # define SSL_F_OSSL_STATEM_CLIENT_READ_TRANSITION 417 # define SSL_F_OSSL_STATEM_CLIENT_WRITE_TRANSITION 599 # define SSL_F_OSSL_STATEM_SERVER13_READ_TRANSITION 437 +# define SSL_F_OSSL_STATEM_SERVER13_WRITE_TRANSITION 600 # define SSL_F_OSSL_STATEM_SERVER_CONSTRUCT_MESSAGE 431 +# define SSL_F_OSSL_STATEM_SERVER_POST_PROCESS_MESSAGE 601 +# define SSL_F_OSSL_STATEM_SERVER_POST_WORK 602 +# define SSL_F_OSSL_STATEM_SERVER_PROCESS_MESSAGE 603 # define SSL_F_OSSL_STATEM_SERVER_READ_TRANSITION 418 +# define SSL_F_OSSL_STATEM_SERVER_WRITE_TRANSITION 604 # define SSL_F_PARSE_CA_NAMES 541 # define SSL_F_PROCESS_KEY_SHARE_EXT 439 # define SSL_F_READ_STATE_MACHINE 352 diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index 11b9d7b993..62e671a059 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -94,9 +94,19 @@ static const ERR_STRING_DATA SSL_str_functs[] = { {ERR_PACK(ERR_LIB_SSL, SSL_F_OSSL_STATEM_CLIENT_WRITE_TRANSITION, 0), "ossl_statem_client_write_transition"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_OSSL_STATEM_SERVER13_READ_TRANSITION, 0), ""}, + {ERR_PACK(ERR_LIB_SSL, SSL_F_OSSL_STATEM_SERVER13_WRITE_TRANSITION, 0), + "ossl_statem_server13_write_transition"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_OSSL_STATEM_SERVER_CONSTRUCT_MESSAGE, 0), ""}, + {ERR_PACK(ERR_LIB_SSL, SSL_F_OSSL_STATEM_SERVER_POST_PROCESS_MESSAGE, 0), + "ossl_statem_server_post_process_message"}, + {ERR_PACK(ERR_LIB_SSL, SSL_F_OSSL_STATEM_SERVER_POST_WORK, 0), + "ossl_statem_server_post_work"}, + {ERR_PACK(ERR_LIB_SSL, SSL_F_OSSL_STATEM_SERVER_PROCESS_MESSAGE, 0), + "ossl_statem_server_process_message"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_OSSL_STATEM_SERVER_READ_TRANSITION, 0), "ossl_statem_server_read_transition"}, + {ERR_PACK(ERR_LIB_SSL, SSL_F_OSSL_STATEM_SERVER_WRITE_TRANSITION, 0), + "ossl_statem_server_write_transition"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_PARSE_CA_NAMES, 0), "parse_ca_names"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_PROCESS_KEY_SHARE_EXT, 0), ""}, {ERR_PACK(ERR_LIB_SSL, SSL_F_READ_STATE_MACHINE, 0), "read_state_machine"}, diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c index 5ce4df42c2..7adb555e44 100644 --- a/ssl/statem/extensions_srvr.c +++ b/ssl/statem/extensions_srvr.c @@ -1080,8 +1080,11 @@ EXT_RETURN tls_construct_stoc_status_request(SSL *s, WPACKET *pkt, * send back an empty extension, with the certificate status appearing as a * separate message */ - if ((SSL_IS_TLS13(s) && !tls_construct_cert_status_body(s, pkt)) - || !WPACKET_close(pkt)) { + if (SSL_IS_TLS13(s) && !tls_construct_cert_status_body(s, pkt)) { + /* SSLfatal() already called */ + return EXT_RETURN_FAIL; + } + if (!WPACKET_close(pkt)) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_STOC_STATUS_REQUEST, ERR_R_INTERNAL_ERROR); return EXT_RETURN_FAIL; diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 8d63bad9b6..7e6aa94169 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -171,10 +171,9 @@ int ossl_statem_server_read_transition(SSL *s, int mt) * not going to accept it because we require a client * cert. */ - ssl3_send_alert(s, SSL3_AL_FATAL, - SSL3_AD_HANDSHAKE_FAILURE); - SSLerr(SSL_F_OSSL_STATEM_SERVER_READ_TRANSITION, - SSL_R_PEER_DID_NOT_RETURN_A_CERTIFICATE); + SSLfatal(s, SSL_AD_HANDSHAKE_FAILURE, + SSL_F_OSSL_STATEM_SERVER_READ_TRANSITION, + SSL_R_PEER_DID_NOT_RETURN_A_CERTIFICATE); return 0; } st->hand_state = TLS_ST_SR_KEY_EXCH; @@ -379,6 +378,9 @@ static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s) switch (st->hand_state) { default: /* Shouldn't happen */ + SSLfatal(s, SSL_AD_INTERNAL_ERROR, + SSL_F_OSSL_STATEM_SERVER13_WRITE_TRANSITION, + ERR_R_INTERNAL_ERROR); return WRITE_TRAN_ERROR; case TLS_ST_OK: @@ -478,6 +480,9 @@ WRITE_TRAN ossl_statem_server_write_transition(SSL *s) switch (st->hand_state) { default: /* Shouldn't happen */ + SSLfatal(s, SSL_AD_INTERNAL_ERROR, + SSL_F_OSSL_STATEM_SERVER_WRITE_TRANSITION, + ERR_R_INTERNAL_ERROR); return WRITE_TRAN_ERROR; case TLS_ST_OK: @@ -631,8 +636,10 @@ WORK_STATE ossl_statem_server_pre_work(SSL *s, WORK_STATE wst) case TLS_ST_SW_SRVR_DONE: #ifndef OPENSSL_NO_SCTP - if (SSL_IS_DTLS(s) && BIO_dgram_is_sctp(SSL_get_wbio(s))) + if (SSL_IS_DTLS(s) && BIO_dgram_is_sctp(SSL_get_wbio(s))) { + /* Calls SSLfatal() as required */ return dtls_wait_for_dry(s); + } #endif return WORK_FINISHED_CONTINUE; @@ -642,6 +649,8 @@ WORK_STATE ossl_statem_server_pre_work(SSL *s, WORK_STATE wst) * Actually this is the end of the handshake, but we're going * straight into writing the session ticket out. So we finish off * the handshake, but keep the various buffers active. + * + * Calls SSLfatal as required. */ return tls_finish_handshake(s, wst, 0); } if (SSL_IS_DTLS(s)) { @@ -676,6 +685,7 @@ WORK_STATE ossl_statem_server_pre_work(SSL *s, WORK_STATE wst) /* Fall through */ case TLS_ST_OK: + /* Calls SSLfatal() as required */ return tls_finish_handshake(s, wst, 1); } @@ -743,7 +753,9 @@ WORK_STATE ossl_statem_server_post_work(SSL *s, WORK_STATE wst) sizeof(sctpauthkey), labelbuffer, sizeof(labelbuffer), NULL, 0, 0) <= 0) { - ossl_statem_set_error(s); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, + SSL_F_OSSL_STATEM_SERVER_POST_WORK, + ERR_R_INTERNAL_ERROR); return WORK_ERROR; } @@ -760,13 +772,17 @@ WORK_STATE ossl_statem_server_post_work(SSL *s, WORK_STATE wst) if (SSL_IS_TLS13(s)) { if (!s->method->ssl3_enc->setup_key_block(s) || !s->method->ssl3_enc->change_cipher_state(s, - SSL3_CC_HANDSHAKE | SSL3_CHANGE_CIPHER_SERVER_WRITE)) + SSL3_CC_HANDSHAKE | SSL3_CHANGE_CIPHER_SERVER_WRITE)) { + /* SSLfatal() already called */ return WORK_ERROR; + } if (s->ext.early_data != SSL_EARLY_DATA_ACCEPTED && !s->method->ssl3_enc->change_cipher_state(s, - SSL3_CC_HANDSHAKE |SSL3_CHANGE_CIPHER_SERVER_READ)) + SSL3_CC_HANDSHAKE |SSL3_CHANGE_CIPHER_SERVER_READ)) { + /* SSLfatal() already called */ return WORK_ERROR; + } } break; @@ -824,8 +840,10 @@ WORK_STATE ossl_statem_server_post_work(SSL *s, WORK_STATE wst) case TLS_ST_SW_KEY_UPDATE: if (statem_flush(s) != 1) return WORK_MORE_A; - if (!tls13_update_key(s, 1)) + if (!tls13_update_key(s, 1)) { + /* SSLfatal() already called */ return WORK_ERROR; + } break; case TLS_ST_SW_SESSION_TICKET: @@ -1021,6 +1039,9 @@ MSG_PROCESS_RETURN ossl_statem_server_process_message(SSL *s, PACKET *pkt) switch (st->hand_state) { default: /* Shouldn't happen */ + SSLfatal(s, SSL_AD_INTERNAL_ERROR, + SSL_F_OSSL_STATEM_SERVER_PROCESS_MESSAGE, + ERR_R_INTERNAL_ERROR); return MSG_PROCESS_ERROR; case TLS_ST_SR_CLNT_HELLO: @@ -1066,6 +1087,9 @@ WORK_STATE ossl_statem_server_post_process_message(SSL *s, WORK_STATE wst) switch (st->hand_state) { default: /* Shouldn't happen */ + SSLfatal(s, SSL_AD_INTERNAL_ERROR, + SSL_F_OSSL_STATEM_SERVER_POST_PROCESS_MESSAGE, + ERR_R_INTERNAL_ERROR); return WORK_ERROR; case TLS_ST_SR_CLNT_HELLO: @@ -2899,8 +2923,8 @@ static int tls_process_cke_rsa(SSL *s, PACKET *pkt) return ret; #else /* Should never happen */ - *al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_TLS_PROCESS_CKE_RSA, ERR_R_INTERNAL_ERROR); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PROCESS_CKE_RSA, + ERR_R_INTERNAL_ERROR); return 0; #endif } @@ -3707,7 +3731,8 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt) || !WPACKET_allocate_bytes(pkt, hlen, &macdata2) || macdata1 != macdata2 || !WPACKET_close(pkt)) { - SSLerr(SSL_F_TLS_CONSTRUCT_NEW_SESSION_TICKET, ERR_R_INTERNAL_ERROR); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, + SSL_F_TLS_CONSTRUCT_NEW_SESSION_TICKET, ERR_R_INTERNAL_ERROR); goto err; } if (SSL_IS_TLS13(s) @@ -3738,7 +3763,8 @@ int tls_construct_cert_status_body(SSL *s, WPACKET *pkt) if (!WPACKET_put_bytes_u8(pkt, s->ext.status_type) || !WPACKET_sub_memcpy_u24(pkt, s->ext.ocsp.resp, s->ext.ocsp.resp_len)) { - SSLerr(SSL_F_TLS_CONSTRUCT_CERT_STATUS_BODY, ERR_R_INTERNAL_ERROR); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CERT_STATUS_BODY, + ERR_R_INTERNAL_ERROR); return 0; } @@ -3748,7 +3774,7 @@ int tls_construct_cert_status_body(SSL *s, WPACKET *pkt) int tls_construct_cert_status(SSL *s, WPACKET *pkt) { if (!tls_construct_cert_status_body(s, pkt)) { - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR); + /* SSLfatal() already called */ return 0; } -- 2.25.1