From 7776a36cfa5853175a858fa32983f22f36513171 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Tue, 15 Nov 2016 10:13:09 +0000 Subject: [PATCH] Ensure the end of first server flight processing is done There is a set of miscellaneous processing for OCSP, CT etc at the end of the ServerDone processing. In TLS1.3 we don't have a ServerDone, so this needs to move elsewhere. Reviewed-by: Rich Salz --- include/openssl/ssl.h | 1 + ssl/ssl_err.c | 2 + ssl/statem/statem_clnt.c | 83 +++++++++++++++++++++++++--------------- ssl/statem/statem_lib.c | 12 +++--- ssl/statem/statem_locl.h | 1 + 5 files changed, 63 insertions(+), 36 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 2fd0e9fb42..8769f46ba2 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2294,6 +2294,7 @@ int ERR_load_SSL_strings(void); # define SSL_F_TLS_PROCESS_CLIENT_HELLO 381 # define SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE 382 # define SSL_F_TLS_PROCESS_FINISHED 364 +# define SSL_F_TLS_PROCESS_INITIAL_SERVER_FLIGHT 442 # define SSL_F_TLS_PROCESS_KEY_EXCHANGE 365 # define SSL_F_TLS_PROCESS_NEW_SESSION_TICKET 366 # define SSL_F_TLS_PROCESS_NEXT_PROTO 383 diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index 825c5638b8..49a9d44b33 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -313,6 +313,8 @@ static ERR_STRING_DATA SSL_str_functs[] = { {ERR_FUNC(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE), "tls_process_client_key_exchange"}, {ERR_FUNC(SSL_F_TLS_PROCESS_FINISHED), "tls_process_finished"}, + {ERR_FUNC(SSL_F_TLS_PROCESS_INITIAL_SERVER_FLIGHT), + "tls_process_initial_server_flight"}, {ERR_FUNC(SSL_F_TLS_PROCESS_KEY_EXCHANGE), "tls_process_key_exchange"}, {ERR_FUNC(SSL_F_TLS_PROCESS_NEW_SESSION_TICKET), "tls_process_new_session_ticket"}, diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index be3148df0f..9745850387 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -2186,34 +2186,22 @@ MSG_PROCESS_RETURN tls_process_cert_status(SSL *s, PACKET *pkt) return MSG_PROCESS_ERROR; } -MSG_PROCESS_RETURN tls_process_server_done(SSL *s, PACKET *pkt) +/* + * Perform miscellaneous checks and processing after we have received the + * server's initial flight. In TLS1.3 this is after the Server Finished message. + * In <=TLS1.2 this is after the ServerDone message. + * + * Returns 1 on success or 0 on failure. + */ +int tls_process_initial_server_flight(SSL *s, int *al) { - if (PACKET_remaining(pkt) > 0) { - /* should contain no data */ - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); - SSLerr(SSL_F_TLS_PROCESS_SERVER_DONE, SSL_R_LENGTH_MISMATCH); - ossl_statem_set_error(s); - return MSG_PROCESS_ERROR; - } -#ifndef OPENSSL_NO_SRP - if (s->s3->tmp.new_cipher->algorithm_mkey & SSL_kSRP) { - if (SRP_Calc_A_param(s) <= 0) { - SSLerr(SSL_F_TLS_PROCESS_SERVER_DONE, SSL_R_SRP_A_CALC); - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR); - ossl_statem_set_error(s); - return MSG_PROCESS_ERROR; - } - } -#endif - /* * at this point we check that we have the required stuff from * the server */ if (!ssl3_check_cert_and_algorithm(s)) { - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE); - ossl_statem_set_error(s); - return MSG_PROCESS_ERROR; + *al = SSL_AD_HANDSHAKE_FAILURE; + return 0; } /* @@ -2225,28 +2213,56 @@ MSG_PROCESS_RETURN tls_process_server_done(SSL *s, PACKET *pkt) int ret; ret = s->ctx->tlsext_status_cb(s, s->ctx->tlsext_status_arg); if (ret == 0) { - ssl3_send_alert(s, SSL3_AL_FATAL, - SSL_AD_BAD_CERTIFICATE_STATUS_RESPONSE); - SSLerr(SSL_F_TLS_PROCESS_SERVER_DONE, + *al = SSL_AD_BAD_CERTIFICATE_STATUS_RESPONSE; + SSLerr(SSL_F_TLS_PROCESS_INITIAL_SERVER_FLIGHT, SSL_R_INVALID_STATUS_RESPONSE); - return MSG_PROCESS_ERROR; + return 0; } if (ret < 0) { - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR); - SSLerr(SSL_F_TLS_PROCESS_SERVER_DONE, ERR_R_MALLOC_FAILURE); - return MSG_PROCESS_ERROR; + *al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_TLS_PROCESS_INITIAL_SERVER_FLIGHT, + ERR_R_MALLOC_FAILURE); + return 0; } } #ifndef OPENSSL_NO_CT if (s->ct_validation_callback != NULL) { /* Note we validate the SCTs whether or not we abort on error */ if (!ssl_validate_ct(s) && (s->verify_mode & SSL_VERIFY_PEER)) { - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE); - return MSG_PROCESS_ERROR; + *al = SSL_AD_HANDSHAKE_FAILURE; + return 0; + } + } +#endif + + return 1; +} + +MSG_PROCESS_RETURN tls_process_server_done(SSL *s, PACKET *pkt) +{ + int al = SSL_AD_INTERNAL_ERROR; + + if (PACKET_remaining(pkt) > 0) { + /* should contain no data */ + al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_TLS_PROCESS_SERVER_DONE, SSL_R_LENGTH_MISMATCH); + goto err; + } +#ifndef OPENSSL_NO_SRP + if (s->s3->tmp.new_cipher->algorithm_mkey & SSL_kSRP) { + if (SRP_Calc_A_param(s) <= 0) { + SSLerr(SSL_F_TLS_PROCESS_SERVER_DONE, SSL_R_SRP_A_CALC); + goto err; } } #endif + /* + * Error queue messages are generated directly by this function + */ + if (!tls_process_initial_server_flight(s, &al)) + goto err; + #ifndef OPENSSL_NO_SCTP /* Only applies to renegotiation */ if (SSL_IS_DTLS(s) && BIO_dgram_is_sctp(SSL_get_wbio(s)) @@ -2255,6 +2271,11 @@ MSG_PROCESS_RETURN tls_process_server_done(SSL *s, PACKET *pkt) else #endif return MSG_PROCESS_FINISHED_READING; + + err: + ssl3_send_alert(s, SSL3_AL_FATAL, al); + ossl_statem_set_error(s); + return MSG_PROCESS_ERROR; } static int tls_construct_cke_psk_preamble(SSL *s, WPACKET *pkt, int *al) diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index 6d32666e43..a971c51631 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -326,7 +326,7 @@ MSG_PROCESS_RETURN tls_process_change_cipher_spec(SSL *s, PACKET *pkt) MSG_PROCESS_RETURN tls_process_finished(SSL *s, PACKET *pkt) { - int al; + int al = SSL_AD_INTERNAL_ERROR; size_t md_len; /* If this occurs, we have missed a message */ @@ -367,12 +367,14 @@ MSG_PROCESS_RETURN tls_process_finished(SSL *s, PACKET *pkt) s->s3->previous_server_finished_len = md_len; } - /* In TLS1.3 we also have to change cipher state */ + /* + * In TLS1.3 we also have to change cipher state and do any final processing + * of the initial server flight (if we are a client) + */ if (SSL_IS_TLS13(s)) { if (s->server) { if (!s->method->ssl3_enc->change_cipher_state(s, SSL3_CC_APPLICATION | SSL3_CHANGE_CIPHER_SERVER_READ)) { - al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_TLS_PROCESS_FINISHED, SSL_R_CANNOT_CHANGE_CIPHER); goto f_err; } @@ -380,16 +382,16 @@ MSG_PROCESS_RETURN tls_process_finished(SSL *s, PACKET *pkt) if (!s->method->ssl3_enc->generate_master_secret(s, s->session->master_key, s->handshake_secret, 0, &s->session->master_key_length)) { - al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_TLS_PROCESS_FINISHED, SSL_R_CANNOT_CHANGE_CIPHER); goto f_err; } if (!s->method->ssl3_enc->change_cipher_state(s, SSL3_CC_APPLICATION | SSL3_CHANGE_CIPHER_CLIENT_READ)) { - al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_TLS_PROCESS_FINISHED, SSL_R_CANNOT_CHANGE_CIPHER); goto f_err; } + if (!tls_process_initial_server_flight(s, &al)) + goto f_err; } } diff --git a/ssl/statem/statem_locl.h b/ssl/statem/statem_locl.h index 740595bba0..f6c76ab3a2 100644 --- a/ssl/statem/statem_locl.h +++ b/ssl/statem/statem_locl.h @@ -77,6 +77,7 @@ __owur int tls_get_message_body(SSL *s, size_t *len); __owur int dtls_get_message(SSL *s, int *mt, size_t *len); /* Message construction and processing functions */ +__owur int tls_process_initial_server_flight(SSL *s, int *al); __owur MSG_PROCESS_RETURN tls_process_change_cipher_spec(SSL *s, PACKET *pkt); __owur MSG_PROCESS_RETURN tls_process_finished(SSL *s, PACKET *pkt); __owur int tls_construct_change_cipher_spec(SSL *s, WPACKET *pkt); -- 2.25.1