From f63e42887271c61b1c803586a47ecbfa49243a0a Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Fri, 2 Dec 2016 14:46:54 +0000 Subject: [PATCH] Implement TLSv1.3 style CertificateStatus We remove the separate CertificateStatus message for TLSv1.3, and instead send back the response in the appropriate Certificate message extension. Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/2020) --- include/openssl/ssl.h | 3 ++ ssl/ssl_err.c | 5 +++ ssl/statem/extensions.c | 32 +++++--------- ssl/statem/extensions_clnt.c | 16 +++++-- ssl/statem/extensions_srvr.c | 13 +++++- ssl/statem/statem_clnt.c | 63 +++++++++++++++------------- ssl/statem/statem_lib.c | 4 +- ssl/statem/statem_locl.h | 2 + ssl/statem/statem_srvr.c | 24 +++++++---- test/recipes/70-test_tls13messages.t | 6 +-- 10 files changed, 100 insertions(+), 68 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 68c13d4c97..0974cfe816 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2136,6 +2136,7 @@ int ERR_load_SSL_strings(void); # define SSL_F_SSL3_WRITE_PENDING 159 # define SSL_F_SSL_ADD_CERT_CHAIN 316 # define SSL_F_SSL_ADD_CERT_TO_BUF 319 +# define SSL_F_SSL_ADD_CERT_TO_WPACKET 493 # define SSL_F_SSL_ADD_CLIENTHELLO_RENEGOTIATE_EXT 298 # define SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT 277 # define SSL_F_SSL_ADD_CLIENTHELLO_USE_SRTP_EXT 307 @@ -2261,6 +2262,7 @@ int ERR_load_SSL_strings(void); # define SSL_F_TLS_COLLECT_EXTENSIONS 435 # define SSL_F_TLS_CONSTRUCT_CERTIFICATE_REQUEST 372 # define SSL_F_TLS_CONSTRUCT_CERT_STATUS 429 +# define SSL_F_TLS_CONSTRUCT_CERT_STATUS_BODY 494 # define SSL_F_TLS_CONSTRUCT_CHANGE_CIPHER_SPEC 427 # define SSL_F_TLS_CONSTRUCT_CKE_DHE 404 # define SSL_F_TLS_CONSTRUCT_CKE_ECDHE 405 @@ -2332,6 +2334,7 @@ int ERR_load_SSL_strings(void); # define SSL_F_TLS_PREPARE_CLIENT_CERTIFICATE 360 # define SSL_F_TLS_PROCESS_CERTIFICATE_REQUEST 361 # define SSL_F_TLS_PROCESS_CERT_STATUS 362 +# define SSL_F_TLS_PROCESS_CERT_STATUS_BODY 495 # define SSL_F_TLS_PROCESS_CERT_VERIFY 379 # define SSL_F_TLS_PROCESS_CHANGE_CIPHER_SPEC 363 # define SSL_F_TLS_PROCESS_CKE_DHE 411 diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index 1b3e40907f..5685817846 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -92,6 +92,7 @@ static ERR_STRING_DATA SSL_str_functs[] = { {ERR_FUNC(SSL_F_SSL3_WRITE_PENDING), "ssl3_write_pending"}, {ERR_FUNC(SSL_F_SSL_ADD_CERT_CHAIN), "ssl_add_cert_chain"}, {ERR_FUNC(SSL_F_SSL_ADD_CERT_TO_BUF), "ssl_add_cert_to_buf"}, + {ERR_FUNC(SSL_F_SSL_ADD_CERT_TO_WPACKET), "ssl_add_cert_to_wpacket"}, {ERR_FUNC(SSL_F_SSL_ADD_CLIENTHELLO_RENEGOTIATE_EXT), "ssl_add_clienthello_renegotiate_ext"}, {ERR_FUNC(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT), @@ -259,6 +260,8 @@ static ERR_STRING_DATA SSL_str_functs[] = { {ERR_FUNC(SSL_F_TLS_CONSTRUCT_CERTIFICATE_REQUEST), "tls_construct_certificate_request"}, {ERR_FUNC(SSL_F_TLS_CONSTRUCT_CERT_STATUS), "tls_construct_cert_status"}, + {ERR_FUNC(SSL_F_TLS_CONSTRUCT_CERT_STATUS_BODY), + "tls_construct_cert_status_body"}, {ERR_FUNC(SSL_F_TLS_CONSTRUCT_CHANGE_CIPHER_SPEC), "tls_construct_change_cipher_spec"}, {ERR_FUNC(SSL_F_TLS_CONSTRUCT_CKE_DHE), "tls_construct_cke_dhe"}, @@ -373,6 +376,8 @@ static ERR_STRING_DATA SSL_str_functs[] = { {ERR_FUNC(SSL_F_TLS_PROCESS_CERTIFICATE_REQUEST), "tls_process_certificate_request"}, {ERR_FUNC(SSL_F_TLS_PROCESS_CERT_STATUS), "tls_process_cert_status"}, + {ERR_FUNC(SSL_F_TLS_PROCESS_CERT_STATUS_BODY), + "tls_process_cert_status_body"}, {ERR_FUNC(SSL_F_TLS_PROCESS_CERT_VERIFY), "tls_process_cert_verify"}, {ERR_FUNC(SSL_F_TLS_PROCESS_CHANGE_CIPHER_SPEC), "tls_process_change_cipher_spec"}, diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index 7fe80b52ee..9a76d8b1fa 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -22,8 +22,6 @@ static int final_ec_pt_formats(SSL *s, unsigned int context, int sent, static int init_session_ticket(SSL *s, unsigned int context); #ifndef OPENSSL_NO_OCSP static int init_status_request(SSL *s, unsigned int context); -static int final_status_request(SSL *s, unsigned int context, int sent, - int *al); #endif #ifndef OPENSSL_NO_NEXTPROTONEG static int init_npn(SSL *s, unsigned int context); @@ -162,7 +160,7 @@ static const EXTENSION_DEFINITION ext_defs[] = { | EXT_TLS1_3_CERTIFICATE, init_status_request, tls_parse_ctos_status_request, tls_parse_stoc_status_request, tls_construct_stoc_status_request, - tls_construct_ctos_status_request, final_status_request + tls_construct_ctos_status_request, NULL }, #else INVALID_EXTENSION, @@ -792,25 +790,17 @@ static int init_session_ticket(SSL *s, unsigned int context) #ifndef OPENSSL_NO_OCSP static int init_status_request(SSL *s, unsigned int context) { - if (s->server) + if (s->server) { s->tlsext_status_type = TLSEXT_STATUSTYPE_nothing; - - return 1; -} - -static int final_status_request(SSL *s, unsigned int context, int sent, - int *al) -{ - if (s->server) - return 1; - - /* - * Ensure we get sensible values passed to tlsext_status_cb in the event - * that we don't receive a status message - */ - OPENSSL_free(s->tlsext_ocsp_resp); - s->tlsext_ocsp_resp = NULL; - s->tlsext_ocsp_resplen = 0; + } else { + /* + * Ensure we get sensible values passed to tlsext_status_cb in the event + * that we don't receive a status message + */ + OPENSSL_free(s->tlsext_ocsp_resp); + s->tlsext_ocsp_resp = NULL; + s->tlsext_ocsp_resplen = 0; + } return 1; } diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c index 1fd4d84f73..2a9a182409 100644 --- a/ssl/statem/extensions_clnt.c +++ b/ssl/statem/extensions_clnt.c @@ -776,14 +776,24 @@ int tls_parse_stoc_status_request(SSL *s, PACKET *pkt, X509 *x, size_t chain, int *al) { /* - * MUST be empty and only sent if we've requested a status - * request message. + * MUST only be sent if we've requested a status + * request message. In TLS <= 1.2 it must also be empty. */ if (s->tlsext_status_type == TLSEXT_STATUSTYPE_nothing - || PACKET_remaining(pkt) > 0) { + || (!SSL_IS_TLS13(s) && PACKET_remaining(pkt) > 0)) { *al = SSL_AD_UNSUPPORTED_EXTENSION; return 0; } + + if (SSL_IS_TLS13(s)) { + /* We only know how to handle this if it's for the first Certificate in + * the chain. We ignore any other repsonses. + */ + if (chain != 0) + return 1; + return tls_process_cert_status_body(s, pkt, al); + } + /* Set flag to expect CertificateStatus message */ s->tlsext_status_expected = 1; diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c index 0bdfce8c7d..f2fc979d34 100644 --- a/ssl/statem/extensions_srvr.c +++ b/ssl/statem/extensions_srvr.c @@ -760,7 +760,18 @@ int tls_construct_stoc_status_request(SSL *s, WPACKET *pkt, X509 *x, return 1; if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_status_request) - || !WPACKET_put_bytes_u16(pkt, 0)) { + || !WPACKET_start_sub_packet_u16(pkt)) { + SSLerr(SSL_F_TLS_CONSTRUCT_STOC_STATUS_REQUEST, ERR_R_INTERNAL_ERROR); + return 0; + } + + /* + * In TLSv1.3 we include the certificate status itself. In <= TLSv1.2 we + * 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)) { SSLerr(SSL_F_TLS_CONSTRUCT_STOC_STATUS_REQUEST, ERR_R_INTERNAL_ERROR); return 0; } diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 00062ff5ce..98e19b50ba 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -169,17 +169,6 @@ static int ossl_statem_client13_read_transition(SSL *s, int mt) break; case TLS_ST_CR_CERT: - /* - * The CertificateStatus message is optional even if - * |tlsext_status_expected| is set - */ - if (s->tlsext_status_expected && mt == SSL3_MT_CERTIFICATE_STATUS) { - st->hand_state = TLS_ST_CR_CERT_STATUS; - return 1; - } - /* Fall through */ - - case TLS_ST_CR_CERT_STATUS: if (mt == SSL3_MT_FINISHED) { st->hand_state = TLS_ST_CR_FINISHED; return 1; @@ -2191,41 +2180,57 @@ MSG_PROCESS_RETURN tls_process_new_session_ticket(SSL *s, PACKET *pkt) return MSG_PROCESS_ERROR; } -MSG_PROCESS_RETURN tls_process_cert_status(SSL *s, PACKET *pkt) +/* + * In TLSv1.3 this is called from the extensions code, otherwise it is used to + * parse a separate message. Returns 1 on success or 0 on failure. On failure + * |*al| is populated with a suitable alert code. + */ +int tls_process_cert_status_body(SSL *s, PACKET *pkt, int *al) { - int al; size_t resplen; unsigned int type; if (!PACKET_get_1(pkt, &type) || type != TLSEXT_STATUSTYPE_ocsp) { - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_TLS_PROCESS_CERT_STATUS, SSL_R_UNSUPPORTED_STATUS_TYPE); - goto f_err; + *al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_TLS_PROCESS_CERT_STATUS_BODY, + SSL_R_UNSUPPORTED_STATUS_TYPE); + return 0; } if (!PACKET_get_net_3_len(pkt, &resplen) || PACKET_remaining(pkt) != resplen) { - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_TLS_PROCESS_CERT_STATUS, SSL_R_LENGTH_MISMATCH); - goto f_err; + *al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_TLS_PROCESS_CERT_STATUS_BODY, SSL_R_LENGTH_MISMATCH); + return 0; } s->tlsext_ocsp_resp = OPENSSL_malloc(resplen); if (s->tlsext_ocsp_resp == NULL) { - al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_TLS_PROCESS_CERT_STATUS, ERR_R_MALLOC_FAILURE); - goto f_err; + *al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_TLS_PROCESS_CERT_STATUS_BODY, ERR_R_MALLOC_FAILURE); + return 0; } if (!PACKET_copy_bytes(pkt, s->tlsext_ocsp_resp, resplen)) { - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_TLS_PROCESS_CERT_STATUS, SSL_R_LENGTH_MISMATCH); - goto f_err; + *al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_TLS_PROCESS_CERT_STATUS_BODY, SSL_R_LENGTH_MISMATCH); + return 0; } s->tlsext_ocsp_resplen = resplen; + + return 1; +} + + +MSG_PROCESS_RETURN tls_process_cert_status(SSL *s, PACKET *pkt) +{ + int al; + + if (!tls_process_cert_status_body(s, pkt, &al)) { + ssl3_send_alert(s, SSL3_AL_FATAL, al); + ossl_statem_set_error(s); + return MSG_PROCESS_ERROR; + } + return MSG_PROCESS_CONTINUE_READING; - f_err: - ssl3_send_alert(s, SSL3_AL_FATAL, al); - ossl_statem_set_error(s); - return MSG_PROCESS_ERROR; } /* diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index f41da103a4..cc5ca67b97 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -317,13 +317,13 @@ static int ssl_add_cert_to_wpacket(SSL *s, WPACKET *pkt, X509 *x, int chain, len = i2d_X509(x, NULL); if (len < 0) { - SSLerr(SSL_F_SSL_ADD_CERT_TO_BUF, ERR_R_BUF_LIB); + SSLerr(SSL_F_SSL_ADD_CERT_TO_WPACKET, ERR_R_BUF_LIB); *al = SSL_AD_INTERNAL_ERROR; return 0; } if (!WPACKET_sub_allocate_bytes_u24(pkt, len, &outbytes) || i2d_X509(x, &outbytes) != len) { - SSLerr(SSL_F_SSL_ADD_CERT_TO_BUF, ERR_R_INTERNAL_ERROR); + SSLerr(SSL_F_SSL_ADD_CERT_TO_WPACKET, ERR_R_INTERNAL_ERROR); *al = SSL_AD_INTERNAL_ERROR; return 0; } diff --git a/ssl/statem/statem_locl.h b/ssl/statem/statem_locl.h index 5fa190aada..35115c9e93 100644 --- a/ssl/statem/statem_locl.h +++ b/ssl/statem/statem_locl.h @@ -115,6 +115,7 @@ __owur int tls_construct_client_hello(SSL *s, WPACKET *pkt); __owur MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt); __owur MSG_PROCESS_RETURN tls_process_certificate_request(SSL *s, PACKET *pkt); __owur MSG_PROCESS_RETURN tls_process_new_session_ticket(SSL *s, PACKET *pkt); +__owur int tls_process_cert_status_body(SSL *s, PACKET *pkt, int *al); __owur MSG_PROCESS_RETURN tls_process_cert_status(SSL *s, PACKET *pkt); __owur MSG_PROCESS_RETURN tls_process_server_done(SSL *s, PACKET *pkt); __owur int tls_construct_client_verify(SSL *s, WPACKET *pkt); @@ -123,6 +124,7 @@ __owur int tls_construct_client_certificate(SSL *s, WPACKET *pkt); __owur int ssl_do_client_cert_cb(SSL *s, X509 **px509, EVP_PKEY **ppkey); __owur int tls_construct_client_key_exchange(SSL *s, WPACKET *pkt); __owur int tls_client_key_exchange_post_work(SSL *s); +__owur int tls_construct_cert_status_body(SSL *s, WPACKET *pkt); __owur int tls_construct_cert_status(SSL *s, WPACKET *pkt); __owur MSG_PROCESS_RETURN tls_process_key_exchange(SSL *s, PACKET *pkt); __owur MSG_PROCESS_RETURN tls_process_server_certificate(SSL *s, PACKET *pkt); diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 5e230f086b..8b765a9aae 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -427,12 +427,7 @@ static WRITE_TRAN ossl_statem_server13_write_transition(SSL *s) return WRITE_TRAN_CONTINUE; case TLS_ST_SW_CERT: - st->hand_state = s->tlsext_status_expected ? TLS_ST_SW_CERT_STATUS - : TLS_ST_SW_FINISHED; - return WRITE_TRAN_CONTINUE; - - case TLS_ST_SW_CERT_STATUS: - st->hand_state = TLS_ST_SW_FINISHED; + st->hand_state = TLS_ST_SW_FINISHED; return WRITE_TRAN_CONTINUE; case TLS_ST_SW_FINISHED: @@ -3464,12 +3459,25 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt) return 0; } -int tls_construct_cert_status(SSL *s, WPACKET *pkt) +/* + * In TLSv1.3 this is called from the extensions code, otherwise it is used to + * create a separate message. Returns 1 on success or 0 on failure. + */ +int tls_construct_cert_status_body(SSL *s, WPACKET *pkt) { if (!WPACKET_put_bytes_u8(pkt, s->tlsext_status_type) || !WPACKET_sub_memcpy_u24(pkt, s->tlsext_ocsp_resp, s->tlsext_ocsp_resplen)) { - SSLerr(SSL_F_TLS_CONSTRUCT_CERT_STATUS, ERR_R_INTERNAL_ERROR); + SSLerr(SSL_F_TLS_CONSTRUCT_CERT_STATUS_BODY, ERR_R_INTERNAL_ERROR); + return 0; + } + + return 1; +} + +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); return 0; } diff --git a/test/recipes/70-test_tls13messages.t b/test/recipes/70-test_tls13messages.t index 7286a67b60..8d42058853 100755 --- a/test/recipes/70-test_tls13messages.t +++ b/test/recipes/70-test_tls13messages.t @@ -43,8 +43,6 @@ $ENV{CTLOG_FILE} = srctop_file("test", "ct", "log_list.conf"); checkhandshake::CLIENT_AUTH_HANDSHAKE], [TLSProxy::Message::MT_CERTIFICATE, checkhandshake::ALL_HANDSHAKES & ~checkhandshake::RESUME_HANDSHAKE], - [TLSProxy::Message::MT_CERTIFICATE_STATUS, - checkhandshake::OCSP_HANDSHAKE], [TLSProxy::Message::MT_FINISHED, checkhandshake::ALL_HANDSHAKES], [TLSProxy::Message::MT_CERTIFICATE, @@ -148,7 +146,7 @@ $proxy->clientflags("-status"); $proxy->serverflags("-status_file " .srctop_file("test", "recipes", "ocsp-response.der")); $proxy->start(); -checkhandshake($proxy, checkhandshake::OCSP_HANDSHAKE, +checkhandshake($proxy, checkhandshake::DEFAULT_HANDSHAKE, checkhandshake::DEFAULT_EXTENSIONS | checkhandshake::STATUS_REQUEST_CLI_EXTENSION | checkhandshake::STATUS_REQUEST_SRV_EXTENSION, @@ -232,7 +230,7 @@ $proxy->clientflags("-ct"); $proxy->serverflags("-status_file " .srctop_file("test", "recipes", "ocsp-response.der")); $proxy->start(); -checkhandshake($proxy, checkhandshake::OCSP_HANDSHAKE, +checkhandshake($proxy, checkhandshake::DEFAULT_HANDSHAKE, checkhandshake::DEFAULT_EXTENSIONS | checkhandshake::SCT_CLI_EXTENSION | checkhandshake::STATUS_REQUEST_CLI_EXTENSION -- 2.25.1