From ad61876728b30189ae0c1e5139bcb91061b10673 Mon Sep 17 00:00:00 2001 From: jsing Date: Wed, 17 Aug 2022 07:39:19 +0000 Subject: [PATCH] Deduplicate peer certificate chain processing code. Rather than reimplement this in each TLS client and server, deduplicate it into a single function. Furthermore, rather than dealing with the API hazard that is SSL_get_peer_cert_chain() in this code, simply produce two chains - one that has the leaf and one that does not. SSL_get_peer_cert_chain() can then return the appropriate one. This also moves the peer cert chain from the SSL_SESSION to the SSL_HANDSHAKE, which makes more sense since it is not available on resumption. ok tb@ --- lib/libssl/Makefile | 5 +-- lib/libssl/s3_lib.c | 15 ++++++--- lib/libssl/ssl_clnt.c | 36 +++------------------ lib/libssl/ssl_lib.c | 13 +++++--- lib/libssl/ssl_locl.h | 10 ++++-- lib/libssl/ssl_sess.c | 6 ++-- lib/libssl/ssl_srvr.c | 23 ++++++------- lib/libssl/tls13_client.c | 26 +++------------ lib/libssl/tls13_server.c | 28 ++-------------- lib/libssl/tls_lib.c | 68 +++++++++++++++++++++++++++++++++++++++ 10 files changed, 121 insertions(+), 109 deletions(-) create mode 100644 lib/libssl/tls_lib.c diff --git a/lib/libssl/Makefile b/lib/libssl/Makefile index d0d7bc4e02b..1788cd75a3a 100644 --- a/lib/libssl/Makefile +++ b/lib/libssl/Makefile @@ -1,4 +1,4 @@ -# $OpenBSD: Makefile,v 1.76 2022/07/24 14:28:16 jsing Exp $ +# $OpenBSD: Makefile,v 1.77 2022/08/17 07:39:19 jsing Exp $ .include .ifndef NOMAN @@ -85,7 +85,8 @@ SRCS= \ tls13_server.c \ tls_buffer.c \ tls_content.c \ - tls_key_share.c + tls_key_share.c \ + tls_lib.c HDRS= dtls1.h srtp.h ssl.h ssl2.h ssl23.h ssl3.h tls1.h diff --git a/lib/libssl/s3_lib.c b/lib/libssl/s3_lib.c index b6a2c269389..27267443576 100644 --- a/lib/libssl/s3_lib.c +++ b/lib/libssl/s3_lib.c @@ -1,4 +1,4 @@ -/* $OpenBSD: s3_lib.c,v 1.235 2022/07/02 16:31:04 tb Exp $ */ +/* $OpenBSD: s3_lib.c,v 1.236 2022/08/17 07:39:19 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -1559,8 +1559,10 @@ ssl3_free(SSL *s) tls1_cleanup_key_block(s); ssl3_release_read_buffer(s); ssl3_release_write_buffer(s); - freezero(s->s3->hs.sigalgs, s->s3->hs.sigalgs_len); + freezero(s->s3->hs.sigalgs, s->s3->hs.sigalgs_len); + sk_X509_pop_free(s->s3->hs.peer_certs, X509_free); + sk_X509_pop_free(s->s3->hs.peer_certs_no_leaf, X509_free); tls_key_share_free(s->s3->hs.key_share); tls13_secrets_destroy(s->s3->hs.tls13.secrets); @@ -1586,8 +1588,8 @@ ssl3_free(SSL *s) void ssl3_clear(SSL *s) { - unsigned char *rp, *wp; - size_t rlen, wlen; + unsigned char *rp, *wp; + size_t rlen, wlen; tls1_cleanup_key_block(s); sk_X509_NAME_pop_free(s->s3->hs.tls12.ca_names, X509_NAME_free); @@ -1598,6 +1600,11 @@ ssl3_clear(SSL *s) s->s3->hs.sigalgs = NULL; s->s3->hs.sigalgs_len = 0; + sk_X509_pop_free(s->s3->hs.peer_certs, X509_free); + s->s3->hs.peer_certs = NULL; + sk_X509_pop_free(s->s3->hs.peer_certs_no_leaf, X509_free); + s->s3->hs.peer_certs_no_leaf = NULL; + tls_key_share_free(s->s3->hs.key_share); s->s3->hs.key_share = NULL; diff --git a/lib/libssl/ssl_clnt.c b/lib/libssl/ssl_clnt.c index 13196848681..0e50285898e 100644 --- a/lib/libssl/ssl_clnt.c +++ b/lib/libssl/ssl_clnt.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_clnt.c,v 1.152 2022/08/15 10:45:25 tb Exp $ */ +/* $OpenBSD: ssl_clnt.c,v 1.153 2022/08/17 07:39:19 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -1090,8 +1090,6 @@ ssl3_get_server_certificate(SSL *s) STACK_OF(X509) *certs = NULL; X509 *cert = NULL; const uint8_t *p; - EVP_PKEY *pkey; - int cert_type; int al, ret; if ((ret = ssl3_get_message(s, SSL3_ST_CR_CERT_A, @@ -1156,37 +1154,11 @@ ssl3_get_server_certificate(SSL *s) SSLerror(s, SSL_R_CERTIFICATE_VERIFY_FAILED); goto fatal_err; } - ERR_clear_error(); /* but we keep s->verify_result */ - - /* - * Inconsistency alert: cert_chain does include the peer's - * certificate, which we don't include in s3_srvr.c - */ - cert = sk_X509_value(certs, 0); - X509_up_ref(cert); - - if ((pkey = X509_get0_pubkey(cert)) == NULL || - EVP_PKEY_missing_parameters(pkey)) { - al = SSL3_AL_FATAL; - SSLerror(s, SSL_R_UNABLE_TO_FIND_PUBLIC_KEY_PARAMETERS); - goto fatal_err; - } - if ((cert_type = ssl_cert_type(pkey)) < 0) { - al = SSL3_AL_FATAL; - SSLerror(s, SSL_R_UNKNOWN_CERTIFICATE_TYPE); - goto fatal_err; - } - - X509_free(s->session->peer_cert); - X509_up_ref(cert); - s->session->peer_cert = cert; - s->session->peer_cert_type = cert_type; - s->session->verify_result = s->verify_result; + ERR_clear_error(); - sk_X509_pop_free(s->session->cert_chain, X509_free); - s->session->cert_chain = certs; - certs = NULL; + if (!tls_process_peer_certs(s, certs)) + goto err; ret = 1; diff --git a/lib/libssl/ssl_lib.c b/lib/libssl/ssl_lib.c index e346e3cf7f1..9af1934dd63 100644 --- a/lib/libssl/ssl_lib.c +++ b/lib/libssl/ssl_lib.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_lib.c,v 1.300 2022/07/24 15:05:16 jsing Exp $ */ +/* $OpenBSD: ssl_lib.c,v 1.301 2022/08/17 07:39:19 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -880,14 +880,17 @@ SSL_get_peer_certificate(const SSL *s) STACK_OF(X509) * SSL_get_peer_cert_chain(const SSL *s) { - if (s == NULL || s->session == NULL) + if (s == NULL) return NULL; /* - * If we are a client, cert_chain includes the peer's own - * certificate; if we are a server, it does not. + * Achtung! Due to API inconsistency, a client includes the peer's leaf + * certificate in the peer certificate chain, while a server does not. */ - return s->session->cert_chain; + if (!s->server) + return s->s3->hs.peer_certs; + + return s->s3->hs.peer_certs_no_leaf; } STACK_OF(X509) * diff --git a/lib/libssl/ssl_locl.h b/lib/libssl/ssl_locl.h index 18daf791f05..1bfeeb97407 100644 --- a/lib/libssl/ssl_locl.h +++ b/lib/libssl/ssl_locl.h @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_locl.h,v 1.417 2022/07/24 14:28:16 jsing Exp $ */ +/* $OpenBSD: ssl_locl.h,v 1.418 2022/08/17 07:39:19 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -518,8 +518,6 @@ struct ssl_session_st { * not_resumable_session_cb to disable session caching and tickets. */ int not_resumable; - STACK_OF(X509) *cert_chain; /* as received from peer */ - size_t tlsext_ecpointformatlist_length; uint8_t *tlsext_ecpointformatlist; /* peer's list */ size_t tlsext_supportedgroups_length; @@ -645,6 +643,10 @@ typedef struct ssl_handshake_st { uint8_t peer_finished[EVP_MAX_MD_SIZE]; size_t peer_finished_len; + /* List of certificates received from our peer. */ + STACK_OF(X509) *peer_certs; + STACK_OF(X509) *peer_certs_no_leaf; + SSL_HANDSHAKE_TLS12 tls12; SSL_HANDSHAKE_TLS13 tls13; } SSL_HANDSHAKE; @@ -1566,6 +1568,8 @@ int srtp_find_profile_by_num(unsigned int profile_num, #endif /* OPENSSL_NO_SRTP */ +int tls_process_peer_certs(SSL *s, STACK_OF(X509) *peer_certs); + __END_HIDDEN_DECLS #endif diff --git a/lib/libssl/ssl_sess.c b/lib/libssl/ssl_sess.c index fcb259f6a29..7cf36f8984f 100644 --- a/lib/libssl/ssl_sess.c +++ b/lib/libssl/ssl_sess.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_sess.c,v 1.116 2022/06/07 17:49:22 tb Exp $ */ +/* $OpenBSD: ssl_sess.c,v 1.117 2022/08/17 07:39:19 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -230,6 +230,8 @@ SSL_SESSION_new(void) ss->next = NULL; ss->tlsext_hostname = NULL; + ss->peer_cert_type = -1; + ss->tlsext_ecpointformatlist_length = 0; ss->tlsext_ecpointformatlist = NULL; ss->tlsext_supportedgroups_length = 0; @@ -761,8 +763,6 @@ SSL_SESSION_free(SSL_SESSION *ss) explicit_bzero(ss->master_key, sizeof ss->master_key); explicit_bzero(ss->session_id, sizeof ss->session_id); - sk_X509_pop_free(ss->cert_chain, X509_free); - X509_free(ss->peer_cert); sk_SSL_CIPHER_free(ss->ciphers); diff --git a/lib/libssl/ssl_srvr.c b/lib/libssl/ssl_srvr.c index d665a568d1d..acdcb15398b 100644 --- a/lib/libssl/ssl_srvr.c +++ b/lib/libssl/ssl_srvr.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_srvr.c,v 1.148 2022/07/03 14:58:00 jsing Exp $ */ +/* $OpenBSD: ssl_srvr.c,v 1.149 2022/08/17 07:39:19 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -2175,6 +2175,11 @@ ssl3_get_client_certificate(SSL *s) al = SSL_AD_HANDSHAKE_FAILURE; goto fatal_err; } + + /* + * If we asked for a client certificate and the client has none, + * it must respond with a certificate list of length zero. + */ if (s->s3->hs.tls12.cert_request != 0) { SSLerror(s, SSL_R_TLS_PEER_DID_NOT_RESPOND_WITH_CERTIFICATE_LIST); al = SSL_AD_UNEXPECTED_MESSAGE; @@ -2244,19 +2249,11 @@ ssl3_get_client_certificate(SSL *s) SSLerror(s, SSL_R_NO_CERTIFICATE_RETURNED); goto fatal_err; } - - X509_free(s->session->peer_cert); - s->session->peer_cert = sk_X509_shift(certs); - - /* - * Inconsistency alert: cert_chain does *not* include the - * peer's own certificate, while we do include it in s3_clnt.c - */ - sk_X509_pop_free(s->session->cert_chain, X509_free); - s->session->cert_chain = certs; - certs = NULL; - s->session->verify_result = s->verify_result; + ERR_clear_error(); + + if (!tls_process_peer_certs(s, certs)) + goto err; done: ret = 1; diff --git a/lib/libssl/tls13_client.c b/lib/libssl/tls13_client.c index b1efafdfdde..87759632f9b 100644 --- a/lib/libssl/tls13_client.c +++ b/lib/libssl/tls13_client.c @@ -1,4 +1,4 @@ -/* $OpenBSD: tls13_client.c,v 1.97 2022/07/24 14:16:29 jsing Exp $ */ +/* $OpenBSD: tls13_client.c,v 1.98 2022/08/17 07:39:19 jsing Exp $ */ /* * Copyright (c) 2018, 2019 Joel Sing * @@ -553,9 +553,8 @@ tls13_server_certificate_recv(struct tls13_ctx *ctx, CBS *cbs) struct stack_st_X509 *certs = NULL; SSL *s = ctx->ssl; X509 *cert = NULL; - EVP_PKEY *pkey; const uint8_t *p; - int alert_desc, cert_type; + int alert_desc; int ret = 0; if ((certs = sk_X509_new_null()) == NULL) @@ -610,28 +609,11 @@ tls13_server_certificate_recv(struct tls13_ctx *ctx, CBS *cbs) "failed to verify peer certificate", NULL); goto err; } + s->session->verify_result = s->verify_result; ERR_clear_error(); - cert = sk_X509_value(certs, 0); - X509_up_ref(cert); - - if ((pkey = X509_get0_pubkey(cert)) == NULL) - goto err; - if (EVP_PKEY_missing_parameters(pkey)) + if (!tls_process_peer_certs(s, certs)) goto err; - if ((cert_type = ssl_cert_type(pkey)) < 0) - goto err; - - X509_up_ref(cert); - X509_free(s->session->peer_cert); - s->session->peer_cert = cert; - s->session->peer_cert_type = cert_type; - - s->session->verify_result = s->verify_result; - - sk_X509_pop_free(s->session->cert_chain, X509_free); - s->session->cert_chain = certs; - certs = NULL; if (ctx->ocsp_status_recv_cb != NULL && !ctx->ocsp_status_recv_cb(ctx)) diff --git a/lib/libssl/tls13_server.c b/lib/libssl/tls13_server.c index 5aee5f1a939..8f225433f08 100644 --- a/lib/libssl/tls13_server.c +++ b/lib/libssl/tls13_server.c @@ -1,4 +1,4 @@ -/* $OpenBSD: tls13_server.c,v 1.100 2022/07/24 14:16:29 jsing Exp $ */ +/* $OpenBSD: tls13_server.c,v 1.101 2022/08/17 07:39:19 jsing Exp $ */ /* * Copyright (c) 2019, 2020 Joel Sing * Copyright (c) 2020 Bob Beck @@ -860,9 +860,7 @@ tls13_client_certificate_recv(struct tls13_ctx *ctx, CBS *cbs) struct stack_st_X509 *certs = NULL; SSL *s = ctx->ssl; X509 *cert = NULL; - EVP_PKEY *pkey; const uint8_t *p; - int cert_type; int ret = 0; if (!CBS_get_u8_length_prefixed(cbs, &cert_request_context)) @@ -911,31 +909,11 @@ tls13_client_certificate_recv(struct tls13_ctx *ctx, CBS *cbs) "failed to verify peer certificate", NULL); goto err; } + s->session->verify_result = s->verify_result; ERR_clear_error(); - /* - * Achtung! Due to API inconsistency, a client includes the peer's leaf - * certificate in the stored certificate chain, while a server does not. - */ - cert = sk_X509_shift(certs); - - if ((pkey = X509_get0_pubkey(cert)) == NULL) + if (!tls_process_peer_certs(s, certs)) goto err; - if (EVP_PKEY_missing_parameters(pkey)) - goto err; - if ((cert_type = ssl_cert_type(pkey)) < 0) - goto err; - - X509_up_ref(cert); - X509_free(s->session->peer_cert); - s->session->peer_cert = cert; - s->session->peer_cert_type = cert_type; - - s->session->verify_result = s->verify_result; - - sk_X509_pop_free(s->session->cert_chain, X509_free); - s->session->cert_chain = certs; - certs = NULL; ctx->handshake_stage.hs_type |= WITH_CCV; ret = 1; diff --git a/lib/libssl/tls_lib.c b/lib/libssl/tls_lib.c new file mode 100644 index 00000000000..48db071803c --- /dev/null +++ b/lib/libssl/tls_lib.c @@ -0,0 +1,68 @@ +/* $OpenBSD: tls_lib.c,v 1.1 2022/08/17 07:39:19 jsing Exp $ */ +/* + * Copyright (c) 2019, 2021 Joel Sing + * + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +#include "ssl_locl.h" + +int +tls_process_peer_certs(SSL *s, STACK_OF(X509) *peer_certs) +{ + STACK_OF(X509) *peer_certs_no_leaf; + X509 *peer_cert = NULL; + EVP_PKEY *pkey; + int cert_type; + int ret = 0; + + if (sk_X509_num(peer_certs) < 1) + goto err; + peer_cert = sk_X509_value(peer_certs, 0); + X509_up_ref(peer_cert); + + if ((pkey = X509_get0_pubkey(peer_cert)) == NULL) { + SSLerror(s, SSL_R_NO_PUBLICKEY); + goto err; + } + if (EVP_PKEY_missing_parameters(pkey)) { + SSLerror(s, SSL_R_UNABLE_TO_FIND_PUBLIC_KEY_PARAMETERS); + goto err; + } + if ((cert_type = ssl_cert_type(pkey)) < 0) { + SSLerror(s, SSL_R_UNKNOWN_CERTIFICATE_TYPE); + goto err; + } + + s->session->peer_cert_type = cert_type; + + X509_free(s->session->peer_cert); + s->session->peer_cert = peer_cert; + peer_cert = NULL; + + sk_X509_pop_free(s->s3->hs.peer_certs, X509_free); + if ((s->s3->hs.peer_certs = X509_chain_up_ref(peer_certs)) == NULL) + goto err; + + if ((peer_certs_no_leaf = X509_chain_up_ref(peer_certs)) == NULL) + goto err; + X509_free(sk_X509_shift(peer_certs_no_leaf)); + sk_X509_pop_free(s->s3->hs.peer_certs_no_leaf, X509_free); + s->s3->hs.peer_certs_no_leaf = peer_certs_no_leaf; + + ret = 1; + err: + X509_free(peer_cert); + + return ret; +} -- 2.20.1