From de3a27964b222c4be979b46c1662d48f67059711 Mon Sep 17 00:00:00 2001 From: jsing Date: Sun, 9 Jan 2022 13:17:33 +0000 Subject: [PATCH] Clean up pkey handling in ssl3_get_server_key_exchange() With TLSv1.2 and earlier, the authentication algorithm used to sign the ServerKeyExchange message is dependent on the cipher suite in use and has nothing to do with the key exchange algorithm. As such, check the authentication algorithm based on the cipher suite in ssl3_get_server_key_exchange() and handle things accordingly. ok inoguchi@ tb@ --- lib/libssl/ssl_clnt.c | 64 +++++++++++++++---------------------------- 1 file changed, 22 insertions(+), 42 deletions(-) diff --git a/lib/libssl/ssl_clnt.c b/lib/libssl/ssl_clnt.c index 70b6fff6bf0..1d1918b956a 100644 --- a/lib/libssl/ssl_clnt.c +++ b/lib/libssl/ssl_clnt.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_clnt.c,v 1.128 2022/01/08 12:59:58 jsing Exp $ */ +/* $OpenBSD: ssl_clnt.c,v 1.129 2022/01/09 13:17:33 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -1212,13 +1212,10 @@ ssl3_get_server_certificate(SSL *s) } static int -ssl3_get_server_kex_dhe(SSL *s, EVP_PKEY **pkey, CBS *cbs) +ssl3_get_server_kex_dhe(SSL *s, CBS *cbs) { - int nid = NID_dhKeyAgreement; int invalid_params, invalid_key; - long alg_a; - - alg_a = S3I(s)->hs.cipher->algorithm_auth; + int nid = NID_dhKeyAgreement; tls_key_share_free(S3I(s)->hs.key_share); if ((S3I(s)->hs.key_share = tls_key_share_new_nid(nid)) == NULL) @@ -1242,12 +1239,6 @@ ssl3_get_server_kex_dhe(SSL *s, EVP_PKEY **pkey, CBS *cbs) goto err; } - if (alg_a & SSL_aRSA) - *pkey = X509_get_pubkey(s->session->peer_pkeys[SSL_PKEY_RSA].x509); - else - /* XXX - Anonymous DH, so no certificate or pkey. */ - *pkey = NULL; - return 1; decode_err: @@ -1259,14 +1250,11 @@ ssl3_get_server_kex_dhe(SSL *s, EVP_PKEY **pkey, CBS *cbs) } static int -ssl3_get_server_kex_ecdhe(SSL *s, EVP_PKEY **pkey, CBS *cbs) +ssl3_get_server_kex_ecdhe(SSL *s, CBS *cbs) { - CBS public; uint8_t curve_type; uint16_t curve_id; - long alg_a; - - alg_a = S3I(s)->hs.cipher->algorithm_auth; + CBS public; if (!CBS_get_u8(cbs, &curve_type)) goto decode_err; @@ -1300,19 +1288,6 @@ ssl3_get_server_kex_ecdhe(SSL *s, EVP_PKEY **pkey, CBS *cbs) if (!tls_key_share_peer_public(S3I(s)->hs.key_share, &public, NULL)) goto err; - /* - * The ECC/TLS specification does not mention the use of DSA to sign - * ECParameters in the server key exchange message. We do support RSA - * and ECDSA. - */ - if (alg_a & SSL_aRSA) - *pkey = X509_get_pubkey(s->session->peer_pkeys[SSL_PKEY_RSA].x509); - else if (alg_a & SSL_aECDSA) - *pkey = X509_get_pubkey(s->session->peer_pkeys[SSL_PKEY_ECC].x509); - else - /* XXX - Anonymous ECDH, so no certificate or pkey. */ - *pkey = NULL; - return 1; decode_err: @@ -1326,7 +1301,6 @@ int ssl3_get_server_key_exchange(SSL *s) { CBS cbs, signature; - EVP_PKEY *pkey = NULL; EVP_MD_CTX *md_ctx; const unsigned char *param; size_t param_len; @@ -1372,10 +1346,10 @@ ssl3_get_server_key_exchange(SSL *s) param_len = CBS_len(&cbs); if (alg_k & SSL_kDHE) { - if (!ssl3_get_server_kex_dhe(s, &pkey, &cbs)) + if (!ssl3_get_server_kex_dhe(s, &cbs)) goto err; } else if (alg_k & SSL_kECDHE) { - if (!ssl3_get_server_kex_ecdhe(s, &pkey, &cbs)) + if (!ssl3_get_server_kex_ecdhe(s, &cbs)) goto err; } else if (alg_k != 0) { al = SSL_AD_UNEXPECTED_MESSAGE; @@ -1386,10 +1360,24 @@ ssl3_get_server_key_exchange(SSL *s) param_len -= CBS_len(&cbs); /* if it was signed, check the signature */ - if (pkey != NULL) { + if ((alg_a & SSL_aNULL) == 0) { uint16_t sigalg_value = SIGALG_NONE; const struct ssl_sigalg *sigalg; EVP_PKEY_CTX *pctx; + EVP_PKEY *pkey = NULL; + + if ((alg_a & SSL_aRSA) != 0) { + pkey = X509_get0_pubkey( + s->session->peer_pkeys[SSL_PKEY_RSA].x509); + } else if ((alg_a & SSL_aECDSA) != 0) { + pkey = X509_get0_pubkey( + s->session->peer_pkeys[SSL_PKEY_ECC].x509); + } + if (pkey == NULL) { + al = SSL_AD_ILLEGAL_PARAMETER; + SSLerror(s, SSL_R_UNKNOWN_CERTIFICATE_TYPE); + goto err; + } if (SSL_USE_SIGALGS(s)) { if (!CBS_get_u16(&cbs, &sigalg_value)) @@ -1432,12 +1420,6 @@ ssl3_get_server_key_exchange(SSL *s) SSLerror(s, SSL_R_BAD_SIGNATURE); goto fatal_err; } - } else { - /* aNULL does not need public keys. */ - if (!(alg_a & SSL_aNULL)) { - SSLerror(s, ERR_R_INTERNAL_ERROR); - goto err; - } } if (CBS_len(&cbs) != 0) { @@ -1446,7 +1428,6 @@ ssl3_get_server_key_exchange(SSL *s) goto fatal_err; } - EVP_PKEY_free(pkey); EVP_MD_CTX_free(md_ctx); return (1); @@ -1459,7 +1440,6 @@ ssl3_get_server_key_exchange(SSL *s) ssl3_send_alert(s, SSL3_AL_FATAL, al); err: - EVP_PKEY_free(pkey); EVP_MD_CTX_free(md_ctx); return (-1); -- 2.20.1