From c02e8229b38557d6bb41bbf799850408397dd343 Mon Sep 17 00:00:00 2001 From: samuel40791765 Date: Wed, 23 Oct 2024 23:19:08 +0000 Subject: [PATCH] Actually add support for SSL_get_server_tmp_key --- include/openssl/ssl.h | 3 +-- ssl/extensions.cc | 5 ++++- ssl/handshake_client.cc | 9 +++++---- ssl/handshake_server.cc | 3 ++- ssl/internal.h | 12 +++++++++--- ssl/ssl_key_share.cc | 10 ++++++++++ ssl/ssl_lib.cc | 36 +++++++++++++++++++++++++++++++++++- ssl/ssl_test.cc | 33 ++++++++++++++++++++++++++++++++- ssl/tls13_client.cc | 10 ++++------ ssl/tls13_server.cc | 4 +++- 10 files changed, 105 insertions(+), 20 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 54580b7c79d..4e3b89d7411 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -5830,8 +5830,7 @@ DEFINE_STACK_OF(SSL_COMP) // SSL_get_server_tmp_key returns zero. This was deprecated as part of the // removal of |EVP_PKEY_DH|. -OPENSSL_EXPORT OPENSSL_DEPRECATED int SSL_get_server_tmp_key( - SSL *ssl, EVP_PKEY **out_key); +OPENSSL_EXPORT int SSL_get_server_tmp_key(SSL *ssl, EVP_PKEY **out_key); // SSL_CTX_set_tmp_dh returns 1. // diff --git a/ssl/extensions.cc b/ssl/extensions.cc index 70fd8a6cf17..32e67c5677f 100644 --- a/ssl/extensions.cc +++ b/ssl/extensions.cc @@ -2312,6 +2312,7 @@ static bool ext_key_share_add_clienthello(const SSL_HANDSHAKE *hs, CBB *out, bool ssl_ext_key_share_parse_serverhello(SSL_HANDSHAKE *hs, Array *out_secret, + Array *out_peer_key, uint8_t *out_alert, CBS *contents) { CBS peer_key; uint16_t group_id; @@ -2333,7 +2334,9 @@ bool ssl_ext_key_share_parse_serverhello(SSL_HANDSHAKE *hs, key_share = hs->key_shares[1].get(); } - if (!key_share->Finish(out_secret, out_alert, peer_key)) { + if (!key_share->Finish(out_secret, out_alert, peer_key) || + // Save peer's public key for observation with |SSL_get_peer_tmp_key|. + !out_peer_key->CopyFrom(peer_key)) { *out_alert = SSL_AD_INTERNAL_ERROR; return false; } diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index d10576b950a..04639850bcc 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc @@ -1163,7 +1163,7 @@ static enum ssl_hs_wait_t do_read_server_key_exchange(SSL_HANDSHAKE *hs) { // Save the group and peer public key for later. hs->new_session->group_id = group_id; - if (!hs->peer_key.CopyFrom(point)) { + if (!ssl->s3->peer_key.CopyFrom(point)) { return ssl_hs_error; } } else if (!(alg_k & SSL_kPSK)) { @@ -1508,7 +1508,8 @@ static enum ssl_hs_wait_t do_send_client_key_exchange(SSL_HANDSHAKE *hs) { bssl::UniquePtr key_share = SSLKeyShare::Create(hs->new_session->group_id); uint8_t alert = SSL_AD_DECODE_ERROR; - if (!key_share || !key_share->Accept(&child, &pms, &alert, hs->peer_key)) { + if (!key_share || + !key_share->Accept(&child, &pms, &alert, ssl->s3->peer_key)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return ssl_hs_error; } @@ -1516,8 +1517,8 @@ static enum ssl_hs_wait_t do_send_client_key_exchange(SSL_HANDSHAKE *hs) { return ssl_hs_error; } - // The peer key can now be discarded. - hs->peer_key.Reset(); + // The peer key could be discarded, but we preserve it since OpenSSL + // allows the user to observe it with |SSL_get_server_tmp_key|. } else if (alg_k & SSL_kPSK) { // For plain PSK, other_secret is a block of 0s with the same length as // the pre-shared key. diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc index 7dc241aed47..ad9b3ebfd9d 100644 --- a/ssl/handshake_server.cc +++ b/ssl/handshake_server.cc @@ -1532,7 +1532,8 @@ static enum ssl_hs_wait_t do_read_client_key_exchange(SSL_HANDSHAKE *hs) { // Compute the premaster. uint8_t alert = SSL_AD_DECODE_ERROR; - if (!hs->key_shares[0]->Finish(&premaster_secret, &alert, peer_key)) { + if (!hs->key_shares[0]->Finish(&premaster_secret, &alert, peer_key) || + !ssl->s3->peer_key.CopyFrom(peer_key)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return ssl_hs_error; } diff --git a/ssl/internal.h b/ssl/internal.h index 148e4761540..4577116048c 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -1275,6 +1275,11 @@ Span PQGroups(); // false. bool ssl_nid_to_group_id(uint16_t *out_group_id, int nid); +// ssl_nid_to_group_id looks up the group corresponding to |group_id|. On +// success, it sets |*out_nid| to the group's nid and returns true. Otherwise, +// it returns false. +bool ssl_group_id_to_nid(uint16_t *out_nid, int group_id); + // ssl_name_to_group_id looks up the group corresponding to the |name| string of // length |len|. On success, it sets |*out_group_id| to the group ID and returns // true. Otherwise, it returns false. @@ -2061,9 +2066,6 @@ struct SSL_HANDSHAKE { // supports with delegated credentials. Array peer_delegated_credential_sigalgs; - // peer_key is the peer's ECDH key for a TLS 1.2 client. - Array peer_key; - // extension_permutation is the permutation to apply to ClientHello // extensions. It maps indices into the |kExtensions| table into other // indices. @@ -2336,6 +2338,7 @@ bool ssl_setup_key_shares(SSL_HANDSHAKE *hs, uint16_t override_group_id); bool ssl_ext_key_share_parse_serverhello(SSL_HANDSHAKE *hs, Array *out_secret, + Array *out_peer_key, uint8_t *out_alert, CBS *contents); bool ssl_ext_key_share_parse_clienthello(SSL_HANDSHAKE *hs, bool *out_found, Span *out_peer_key, @@ -3033,6 +3036,9 @@ struct SSL3_STATE { // one. UniquePtr hs; + // peer_key is the peer's ECDH key for both TLS 1.2/1.3. + Array peer_key; + uint8_t write_traffic_secret[SSL_MAX_MD_SIZE] = {0}; uint8_t read_traffic_secret[SSL_MAX_MD_SIZE] = {0}; uint8_t exporter_secret[SSL_MAX_MD_SIZE] = {0}; diff --git a/ssl/ssl_key_share.cc b/ssl/ssl_key_share.cc index 6912a2ebf36..872fa805f0c 100644 --- a/ssl/ssl_key_share.cc +++ b/ssl/ssl_key_share.cc @@ -799,6 +799,16 @@ bool ssl_nid_to_group_id(uint16_t *out_group_id, int nid) { return false; } +bool ssl_group_id_to_nid(uint16_t *out_nid, int group_id) { + for (const auto &group : kNamedGroups) { + if (group.group_id == group_id) { + *out_nid = group.nid; + return true; + } + } + return false; +} + bool ssl_name_to_group_id(uint16_t *out_group_id, const char *name, size_t len) { for (const auto &group : kNamedGroups) { if (len == strlen(group.name) && diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 63cb730fb3d..e1f82539fa1 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -2668,7 +2668,41 @@ const COMP_METHOD *SSL_get_current_compression(SSL *ssl) { return NULL; } const COMP_METHOD *SSL_get_current_expansion(SSL *ssl) { return NULL; } -int SSL_get_server_tmp_key(SSL *ssl, EVP_PKEY **out_key) { return 0; } +int SSL_get_server_tmp_key(SSL *ssl, EVP_PKEY **out_key) { + GUARD_PTR(ssl); + GUARD_PTR(ssl->s3); + GUARD_PTR(out_key); + + SSL_SESSION *session = SSL_get_session(ssl); + uint16_t nid; + if (!session || !ssl_group_id_to_nid(&nid, session->group_id)) { + return 0; + } + bssl::UniquePtr ret(EVP_PKEY_new()); + if (!ret) { + return 0; + } + + // Assign key type based on the session's key exchange |nid|. + if (nid == EVP_PKEY_X25519) { + if (!EVP_PKEY_set_type(ret.get(), EVP_PKEY_X25519)) { + return 0; + } + } else { + EC_KEY *key = EC_KEY_new_by_curve_name(nid); + if (!EVP_PKEY_assign_EC_KEY(ret.get(), key)) { + return 0; + } + } + + if (!EVP_PKEY_set1_tls_encodedpoint(ret.get(), ssl->s3->peer_key.data(), + ssl->s3->peer_key.size())) { + return 0; + } + EVP_PKEY_up_ref(ret.get()); + *out_key = ret.get(); + return 1; +} void SSL_CTX_set_quiet_shutdown(SSL_CTX *ctx, int mode) { ctx->quiet_shutdown = (mode != 0); diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 6a65f036eb7..035b6a7e26e 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -6728,7 +6728,6 @@ TEST_P(MultiTransferReadWriteTest, SuiteTransfers) { ClientConfig config; bssl::UniquePtr client, server; - ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), server_ctx.get(), config, true)); @@ -10165,6 +10164,38 @@ TEST_P(SSLVersionTest, TicketSessionIDsMatch) { EXPECT_EQ(Bytes(SessionIDOf(client.get())), Bytes(SessionIDOf(server.get()))); } +TEST_P(SSLVersionTest, PeerTmpKey) { + if (getVersionParam().transfer_ssl) { + // The peer's temporary key is not within the boundary of the SSL transfer + // feature. + return; + } + + // Default should be using X5519 as the key exchange. + ASSERT_TRUE(Connect()); + for (SSL *ssl : {client_.get(), server_.get()}) { + SCOPED_TRACE(SSL_is_server(ssl) ? "server" : "client"); + EVP_PKEY *key = nullptr; + EXPECT_TRUE(SSL_get_server_tmp_key(ssl, &key)); + EXPECT_EQ(EVP_PKEY_id(key), EVP_PKEY_X25519); + bssl::UniquePtr pkey(key); + } + + // Check that EC Groups for the key exchange also work. + ASSERT_TRUE(SSL_CTX_set1_groups_list(server_ctx_.get(), "P-384")); + ASSERT_TRUE(Connect()); + for (SSL *ssl : {client_.get(), server_.get()}) { + SCOPED_TRACE(SSL_is_server(ssl) ? "server" : "client"); + EVP_PKEY *key = nullptr; + EXPECT_TRUE(SSL_get_server_tmp_key(ssl, &key)); + EXPECT_EQ(EVP_PKEY_id(key), EVP_PKEY_EC); + EC_KEY *ec_key = EVP_PKEY_get0_EC_KEY(key); + EXPECT_TRUE(ec_key); + EXPECT_EQ(EC_KEY_get0_group(ec_key), EC_group_p384()); + bssl::UniquePtr pkey(key); + } +} + static void WriteHelloRequest(SSL *server) { // This function assumes TLS 1.2 with ChaCha20-Poly1305. ASSERT_EQ(SSL_version(server), TLS1_2_VERSION); diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc index 71f7496d1e5..aa2a4d40f7c 100644 --- a/ssl/tls13_client.cc +++ b/ssl/tls13_client.cc @@ -420,8 +420,7 @@ static enum ssl_hs_wait_t do_read_server_hello(SSL_HANDSHAKE *hs) { uint16_t version; if (!supported_versions.present || !CBS_get_u16(&supported_versions.data, &version) || - CBS_len(&supported_versions.data) != 0 || - version != ssl->version) { + CBS_len(&supported_versions.data) != 0 || version != ssl->version) { OPENSSL_PUT_ERROR(SSL, SSL_R_SECOND_SERVERHELLO_VERSION_MISMATCH); ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); return ssl_hs_error; @@ -497,15 +496,14 @@ static enum ssl_hs_wait_t do_read_server_hello(SSL_HANDSHAKE *hs) { // Resolve ECDHE and incorporate it into the secret. Array dhe_secret; alert = SSL_AD_DECODE_ERROR; - if (!ssl_ext_key_share_parse_serverhello(hs, &dhe_secret, &alert, - &key_share.data)) { + if (!ssl_ext_key_share_parse_serverhello(hs, &dhe_secret, &ssl->s3->peer_key, + &alert, &key_share.data)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return ssl_hs_error; } if (!tls13_advance_key_schedule(hs, dhe_secret) || - !ssl_hash_message(hs, msg) || - !tls13_derive_handshake_secrets(hs)) { + !ssl_hash_message(hs, msg) || !tls13_derive_handshake_secrets(hs)) { return ssl_hs_error; } diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index 4ed3b914248..fd52f5c9cbe 100644 --- a/ssl/tls13_server.cc +++ b/ssl/tls13_server.cc @@ -78,7 +78,9 @@ static bool resolve_ecdhe_secret(SSL_HANDSHAKE *hs, if (!key_share || // !CBB_init(public_key.get(), 32) || !key_share->Accept(public_key.get(), &secret, &alert, peer_key) || - !CBBFinishArray(public_key.get(), &hs->ecdh_public_key)) { + !CBBFinishArray(public_key.get(), &hs->ecdh_public_key) || + // Save peer's public key for observation with |SSL_get_peer_tmp_key|. + !ssl->s3->peer_key.CopyFrom(peer_key)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return false; }