From 72620ac79133ca7a4553b70573fd100257e8269d Mon Sep 17 00:00:00 2001 From: Todd Short Date: Mon, 29 Aug 2022 17:00:07 -0400 Subject: [PATCH] Add `for_comp` flag when retrieving certs for compression Reviewed-by: Matt Caswell Reviewed-by: Hugo Landau (Merged from https://github.com/openssl/openssl/pull/18186) --- include/openssl/ssl.h.in | 1 + ssl/ssl_cert_comp.c | 19 +++------------ ssl/ssl_local.h | 2 +- ssl/statem/extensions.c | 12 ++++++--- ssl/statem/extensions_cust.c | 10 +++++--- ssl/statem/statem.c | 10 -------- ssl/statem/statem.h | 1 - ssl/statem/statem_clnt.c | 4 +-- ssl/statem/statem_lib.c | 47 ++++++++++++++++++++++-------------- ssl/statem/statem_srvr.c | 2 +- 10 files changed, 52 insertions(+), 56 deletions(-) diff --git a/include/openssl/ssl.h.in b/include/openssl/ssl.h.in index 4ae96cd1ee1..2224b3269b6 100644 --- a/include/openssl/ssl.h.in +++ b/include/openssl/ssl.h.in @@ -280,6 +280,7 @@ typedef int (*tls_session_secret_cb_fn)(SSL *s, void *secret, int *secret_len, #define SSL_EXT_TLS1_3_CERTIFICATE 0x1000 #define SSL_EXT_TLS1_3_NEW_SESSION_TICKET 0x2000 #define SSL_EXT_TLS1_3_CERTIFICATE_REQUEST 0x4000 +#define SSL_EXT_TLS1_3_CERTIFICATE_COMPRESSION 0x8000 /* Typedefs for handling custom extensions */ diff --git a/ssl/ssl_cert_comp.c b/ssl/ssl_cert_comp.c index a86282279c1..654da2dc03c 100644 --- a/ssl/ssl_cert_comp.c +++ b/ssl/ssl_cert_comp.c @@ -198,7 +198,6 @@ static size_t ssl_get_cert_to_compress(SSL *ssl, CERT_PKEY *cpk, unsigned char * WPACKET tmppkt; BUF_MEM buf = { 0 }; size_t ret = 0; - const SSL_METHOD *method = NULL; if (sc == NULL || cpk == NULL @@ -215,26 +214,14 @@ static size_t ssl_get_cert_to_compress(SSL *ssl, CERT_PKEY *cpk, unsigned char * goto out; /* - * ssl3_output_cert_chain() may generate an SSLfata() error, - * for this case, we want to ignore it + * ssl3_output_cert_chain() may generate an SSLfatal() error, + * for this case, we want to ignore it, argument for_comp = 1 */ - sc->statem.ignore_fatal = 1; - ERR_set_mark(); - /* Must get the certificate as TLSv1.3, restore before returning */ - method = SSL_get_ssl_method(ssl); - if (!SSL_set_ssl_method(ssl, tlsv1_3_server_method())) - goto out; - - if (!ssl3_output_cert_chain(sc, &tmppkt, cpk)) + if (!ssl3_output_cert_chain(sc, &tmppkt, cpk, 1)) goto out; WPACKET_get_total_written(&tmppkt, &ret); out: - /* Don't leave errors in the queue */ - ERR_pop_to_mark(); - sc->statem.ignore_fatal = 0; - if (method != NULL && !SSL_set_ssl_method(ssl, method)) - ret = 0; WPACKET_cleanup(&tmppkt); if (ret != 0 && data != NULL) *data = (unsigned char *)buf.data; diff --git a/ssl/ssl_local.h b/ssl/ssl_local.h index 2580064ebdd..fd21d0be82e 100644 --- a/ssl/ssl_local.h +++ b/ssl/ssl_local.h @@ -2613,7 +2613,7 @@ __owur int ssl3_finish_mac(SSL_CONNECTION *s, const unsigned char *buf, size_t len); void ssl3_free_digest_list(SSL_CONNECTION *s); __owur unsigned long ssl3_output_cert_chain(SSL_CONNECTION *s, WPACKET *pkt, - CERT_PKEY *cpk); + CERT_PKEY *cpk, int for_comp); __owur const SSL_CIPHER *ssl3_choose_cipher(SSL_CONNECTION *s, STACK_OF(SSL_CIPHER) *clnt, STACK_OF(SSL_CIPHER) *srvr); diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index 2cfc9f2b7de..b879050c40a 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -832,6 +832,7 @@ int tls_construct_extensions(SSL_CONNECTION *s, WPACKET *pkt, size_t i; int min_version, max_version = 0, reason; const EXTENSION_DEFINITION *thisexd; + int for_comp = (context & SSL_EXT_TLS1_3_CERTIFICATE_COMPRESSION) != 0; if (!WPACKET_start_sub_packet_u16(pkt) /* @@ -842,15 +843,17 @@ int tls_construct_extensions(SSL_CONNECTION *s, WPACKET *pkt, || ((context & (SSL_EXT_CLIENT_HELLO | SSL_EXT_TLS1_2_SERVER_HELLO)) != 0 && !WPACKET_set_flags(pkt, - WPACKET_FLAGS_ABANDON_ON_ZERO_LENGTH))) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + WPACKET_FLAGS_ABANDON_ON_ZERO_LENGTH))) { + if (!for_comp) + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); return 0; } if ((context & SSL_EXT_CLIENT_HELLO) != 0) { reason = ssl_get_min_max_version(s, &min_version, &max_version, NULL); if (reason != 0) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, reason); + if (!for_comp) + SSLfatal(s, SSL_AD_INTERNAL_ERROR, reason); return 0; } } @@ -894,7 +897,8 @@ int tls_construct_extensions(SSL_CONNECTION *s, WPACKET *pkt, } if (!WPACKET_close(pkt)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + if (!for_comp) + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); return 0; } diff --git a/ssl/statem/extensions_cust.c b/ssl/statem/extensions_cust.c index 57713702f7f..ebfe7d16ee8 100644 --- a/ssl/statem/extensions_cust.c +++ b/ssl/statem/extensions_cust.c @@ -178,6 +178,7 @@ int custom_ext_add(SSL_CONNECTION *s, int context, WPACKET *pkt, X509 *x, custom_ext_method *meth; size_t i; int al; + int for_comp = (context & SSL_EXT_TLS1_3_CERTIFICATE_COMPRESSION) != 0; for (i = 0; i < exts->meths_count; i++) { const unsigned char *out = NULL; @@ -211,7 +212,8 @@ int custom_ext_add(SSL_CONNECTION *s, int context, WPACKET *pkt, X509 *x, meth->add_arg); if (cb_retval < 0) { - SSLfatal(s, al, SSL_R_CALLBACK_FAILED); + if (!for_comp) + SSLfatal(s, al, SSL_R_CALLBACK_FAILED); return 0; /* error */ } if (cb_retval == 0) @@ -222,7 +224,8 @@ int custom_ext_add(SSL_CONNECTION *s, int context, WPACKET *pkt, X509 *x, || !WPACKET_start_sub_packet_u16(pkt) || (outlen > 0 && !WPACKET_memcpy(pkt, out, outlen)) || !WPACKET_close(pkt)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + if (!for_comp) + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); return 0; } if ((context & SSL_EXT_CLIENT_HELLO) != 0) { @@ -230,7 +233,8 @@ int custom_ext_add(SSL_CONNECTION *s, int context, WPACKET *pkt, X509 *x, * We can't send duplicates: code logic should prevent this. */ if (!ossl_assert((meth->ext_flags & SSL_EXT_FLAG_SENT) == 0)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + if (!for_comp) + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); return 0; } /* diff --git a/ssl/statem/statem.c b/ssl/statem/statem.c index ba3681285a3..448d655a17c 100644 --- a/ssl/statem/statem.c +++ b/ssl/statem/statem.c @@ -130,7 +130,6 @@ void ossl_statem_clear(SSL_CONNECTION *s) s->statem.hand_state = TLS_ST_BEFORE; ossl_statem_set_in_init(s, 1); s->statem.no_cert_verify = 0; - s->statem.ignore_fatal = 0; } /* @@ -144,15 +143,6 @@ void ossl_statem_set_renegotiate(SSL_CONNECTION *s) void ossl_statem_send_fatal(SSL_CONNECTION *s, int al) { - /* - * Some public APIs may call internal functions that fatal error, - * which doesn't make sense outside the state machine. Those APIs - * that can handle a failure set this flag to avoid errors sending - * alerts. Example: getting a wire-formatted certificate for - * compression. - */ - if (s->statem.ignore_fatal) - return; /* We shouldn't call SSLfatal() twice. Once is enough */ if (s->statem.in_init && s->statem.state == MSG_FLOW_ERROR) return; diff --git a/ssl/statem/statem.h b/ssl/statem/statem.h index b60103e6e51..2b73eba6f68 100644 --- a/ssl/statem/statem.h +++ b/ssl/statem/statem.h @@ -96,7 +96,6 @@ struct ossl_statem_st { OSSL_HANDSHAKE_STATE request_state; int in_init; int read_state_first_init; - int ignore_fatal; /* true when we are actually in SSL_accept() or SSL_connect() */ int in_handshake; /* diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 303e8772825..b3f4300434d 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -3628,7 +3628,7 @@ CON_FUNC_RETURN tls_construct_client_certificate(SSL_CONNECTION *s, } if (!ssl3_output_cert_chain(s, pkt, (s->s3.tmp.cert_req == 2) ? NULL - : s->cert->key)) { + : s->cert->key, 0)) { /* SSLfatal() already called */ return CON_FUNC_ERROR; } @@ -3676,7 +3676,7 @@ CON_FUNC_RETURN tls_construct_client_compressed_certificate(SSL_CONNECTION *sc, } else if (!WPACKET_sub_memcpy_u8(&tmppkt, sc->pha_context, sc->pha_context_len)) goto err; - if (!ssl3_output_cert_chain(sc, &tmppkt, sc->cert->key)) { + if (!ssl3_output_cert_chain(sc, &tmppkt, sc->cert->key, 0)) { /* SSLfatal() already called */ goto out; } diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index 9712fb37355..b37633e37f0 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -906,25 +906,30 @@ CON_FUNC_RETURN tls_construct_change_cipher_spec(SSL_CONNECTION *s, WPACKET *pkt /* Add a certificate to the WPACKET */ static int ssl_add_cert_to_wpacket(SSL_CONNECTION *s, WPACKET *pkt, - X509 *x, int chain) + X509 *x, int chain, int for_comp) { int len; unsigned char *outbytes; + int context = SSL_EXT_TLS1_3_CERTIFICATE; + + if (for_comp) + context |= SSL_EXT_TLS1_3_CERTIFICATE_COMPRESSION; len = i2d_X509(x, NULL); if (len < 0) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_BUF_LIB); + if (!for_comp) + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_BUF_LIB); return 0; } if (!WPACKET_sub_allocate_bytes_u24(pkt, len, &outbytes) || i2d_X509(x, &outbytes) != len) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + if (!for_comp) + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); return 0; } - if (SSL_CONNECTION_IS_TLS13(s) - && !tls_construct_extensions(s, pkt, SSL_EXT_TLS1_3_CERTIFICATE, x, - chain)) { + if ((SSL_CONNECTION_IS_TLS13(s) || for_comp) + && !tls_construct_extensions(s, pkt, context, x, chain)) { /* SSLfatal() already called */ return 0; } @@ -933,7 +938,7 @@ static int ssl_add_cert_to_wpacket(SSL_CONNECTION *s, WPACKET *pkt, } /* Add certificate chain to provided WPACKET */ -static int ssl_add_cert_chain(SSL_CONNECTION *s, WPACKET *pkt, CERT_PKEY *cpk) +static int ssl_add_cert_chain(SSL_CONNECTION *s, WPACKET *pkt, CERT_PKEY *cpk, int for_comp) { int i, chain_count; X509 *x; @@ -967,12 +972,14 @@ static int ssl_add_cert_chain(SSL_CONNECTION *s, WPACKET *pkt, CERT_PKEY *cpk) sctx->propq); if (xs_ctx == NULL) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_X509_LIB); + if (!for_comp) + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_X509_LIB); return 0; } if (!X509_STORE_CTX_init(xs_ctx, chain_store, x, NULL)) { X509_STORE_CTX_free(xs_ctx); - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_X509_LIB); + if (!for_comp) + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_X509_LIB); return 0; } /* @@ -994,14 +1001,15 @@ static int ssl_add_cert_chain(SSL_CONNECTION *s, WPACKET *pkt, CERT_PKEY *cpk) ERR_raise(ERR_LIB_SSL, SSL_R_CA_MD_TOO_WEAK); #endif X509_STORE_CTX_free(xs_ctx); - SSLfatal(s, SSL_AD_INTERNAL_ERROR, i); + if (!for_comp) + SSLfatal(s, SSL_AD_INTERNAL_ERROR, i); return 0; } chain_count = sk_X509_num(chain); for (i = 0; i < chain_count; i++) { x = sk_X509_value(chain, i); - if (!ssl_add_cert_to_wpacket(s, pkt, x, i)) { + if (!ssl_add_cert_to_wpacket(s, pkt, x, i, for_comp)) { /* SSLfatal() already called */ X509_STORE_CTX_free(xs_ctx); return 0; @@ -1011,16 +1019,17 @@ static int ssl_add_cert_chain(SSL_CONNECTION *s, WPACKET *pkt, CERT_PKEY *cpk) } else { i = ssl_security_cert_chain(s, extra_certs, x, 0); if (i != 1) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, i); + if (!for_comp) + SSLfatal(s, SSL_AD_INTERNAL_ERROR, i); return 0; } - if (!ssl_add_cert_to_wpacket(s, pkt, x, 0)) { + if (!ssl_add_cert_to_wpacket(s, pkt, x, 0, for_comp)) { /* SSLfatal() already called */ return 0; } for (i = 0; i < sk_X509_num(extra_certs); i++) { x = sk_X509_value(extra_certs, i); - if (!ssl_add_cert_to_wpacket(s, pkt, x, i + 1)) { + if (!ssl_add_cert_to_wpacket(s, pkt, x, i + 1, for_comp)) { /* SSLfatal() already called */ return 0; } @@ -1030,18 +1039,20 @@ static int ssl_add_cert_chain(SSL_CONNECTION *s, WPACKET *pkt, CERT_PKEY *cpk) } unsigned long ssl3_output_cert_chain(SSL_CONNECTION *s, WPACKET *pkt, - CERT_PKEY *cpk) + CERT_PKEY *cpk, int for_comp) { if (!WPACKET_start_sub_packet_u24(pkt)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + if (!for_comp) + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); return 0; } - if (!ssl_add_cert_chain(s, pkt, cpk)) + if (!ssl_add_cert_chain(s, pkt, cpk, for_comp)) return 0; if (!WPACKET_close(pkt)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + if (!for_comp) + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); return 0; } diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 0d68e4b1cf9..4f98eb12ad4 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -3733,7 +3733,7 @@ CON_FUNC_RETURN tls_construct_server_certificate(SSL_CONNECTION *s, WPACKET *pkt SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); return CON_FUNC_ERROR; } - if (!ssl3_output_cert_chain(s, pkt, cpk)) { + if (!ssl3_output_cert_chain(s, pkt, cpk, 0)) { /* SSLfatal() already called */ return CON_FUNC_ERROR; } -- 2.47.3