diff --git a/ssl/ssl_asn1.cc b/ssl/ssl_asn1.cc index 669f776d..3fd7fb6a 100644 --- a/ssl/ssl_asn1.cc +++ b/ssl/ssl_asn1.cc @@ -697,11 +697,6 @@ UniquePtr SSL_SESSION_parse(CBS *cbs, } } - if (!x509_method->session_cache_objects(ret.get())) { - OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_SSL_SESSION); - return nullptr; - } - CBS age_add; int age_add_present; if (!CBS_get_optional_asn1_octet_string(&session, &age_add, &age_add_present, @@ -737,6 +732,11 @@ UniquePtr SSL_SESSION_parse(CBS *cbs, return nullptr; } + if (!x509_method->session_cache_objects(ret.get())) { + OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_SSL_SESSION); + return nullptr; + } + return ret; } diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 705528b3..8d01c03a 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -4521,6 +4521,65 @@ TEST(SSLTest, GetCertificateThreads) { EXPECT_EQ(cert2, cert2_thread); EXPECT_EQ(0, X509_cmp(cert.get(), cert2)); } + +// Functions which access properties on the negotiated session are thread-safe +// where needed. Prior to TLS 1.3, clients resuming sessions and servers +// performing stateful resumption will share an underlying SSL_SESSION object, +// potentially across threads. +TEST_P(SSLVersionTest, SessionPropertiesThreads) { + if (version() == TLS1_3_VERSION) { + // Our TLS 1.3 implementation does not support stateful resumption. + ASSERT_FALSE(CreateClientSession(client_ctx_.get(), server_ctx_.get())); + return; + } + + SSL_CTX_set_options(server_ctx_.get(), SSL_OP_NO_TICKET); + SSL_CTX_set_session_cache_mode(client_ctx_.get(), SSL_SESS_CACHE_BOTH); + SSL_CTX_set_session_cache_mode(server_ctx_.get(), SSL_SESS_CACHE_BOTH); + + ASSERT_TRUE(UseCertAndKey(client_ctx_.get())); + ASSERT_TRUE(UseCertAndKey(server_ctx_.get())); + + // Configure mutual authentication, so we have more session state. + SSL_CTX_set_custom_verify( + client_ctx_.get(), SSL_VERIFY_PEER, + [](SSL *ssl, uint8_t *out_alert) { return ssl_verify_ok; }); + SSL_CTX_set_custom_verify( + server_ctx_.get(), SSL_VERIFY_PEER, + [](SSL *ssl, uint8_t *out_alert) { return ssl_verify_ok; }); + + // Establish a client session to test with. + bssl::UniquePtr session = + CreateClientSession(client_ctx_.get(), server_ctx_.get()); + ASSERT_TRUE(session); + + // Resume with it twice. + UniquePtr ssls[4]; + ClientConfig config; + config.session = session.get(); + ASSERT_TRUE(ConnectClientAndServer(&ssls[0], &ssls[1], client_ctx_.get(), + server_ctx_.get(), config)); + ASSERT_TRUE(ConnectClientAndServer(&ssls[2], &ssls[3], client_ctx_.get(), + server_ctx_.get(), config)); + + // Read properties in parallel. + auto read_properties = [](const SSL *ssl) { + EXPECT_TRUE(SSL_get_peer_cert_chain(ssl)); + bssl::UniquePtr peer(SSL_get_peer_certificate(ssl)); + EXPECT_TRUE(peer); + EXPECT_TRUE(SSL_get_current_cipher(ssl)); + EXPECT_TRUE(SSL_get_curve_id(ssl)); + }; + + std::vector threads; + for (const auto &ssl_ptr : ssls) { + const SSL *ssl = ssl_ptr.get(); + threads.emplace_back([=] { read_properties(ssl); }); + } + for (auto &thread : threads) { + thread.join(); + } +} #endif constexpr size_t kNumQUICLevels = 4; diff --git a/ssl/ssl_x509.cc b/ssl/ssl_x509.cc index ec203b22..eb3a38b7 100644 --- a/ssl/ssl_x509.cc +++ b/ssl/ssl_x509.cc @@ -281,16 +281,25 @@ static void ssl_crypto_x509_cert_dup(CERT *new_cert, const CERT *cert) { } static int ssl_crypto_x509_session_cache_objects(SSL_SESSION *sess) { - bssl::UniquePtr chain; + bssl::UniquePtr chain, chain_without_leaf; if (sk_CRYPTO_BUFFER_num(sess->certs.get()) > 0) { chain.reset(sk_X509_new_null()); if (!chain) { OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); return 0; } + if (sess->is_server) { + // chain_without_leaf is only needed for server sessions. See + // |SSL_get_peer_cert_chain|. + chain_without_leaf.reset(sk_X509_new_null()); + if (!chain_without_leaf) { + OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); + return 0; + } + } } - X509 *leaf = nullptr; + bssl::UniquePtr leaf; for (CRYPTO_BUFFER *cert : sess->certs.get()) { UniquePtr x509(X509_parse_from_buffer(cert)); if (!x509) { @@ -298,7 +307,11 @@ static int ssl_crypto_x509_session_cache_objects(SSL_SESSION *sess) { return 0; } if (leaf == nullptr) { - leaf = x509.get(); + leaf = UpRef(x509); + } else if (chain_without_leaf && + !PushToStack(chain_without_leaf.get(), UpRef(x509))) { + OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); + return 0; } if (!PushToStack(chain.get(), std::move(x509))) { OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); @@ -308,26 +321,28 @@ static int ssl_crypto_x509_session_cache_objects(SSL_SESSION *sess) { sk_X509_pop_free(sess->x509_chain, X509_free); sess->x509_chain = chain.release(); + sk_X509_pop_free(sess->x509_chain_without_leaf, X509_free); - sess->x509_chain_without_leaf = NULL; + sess->x509_chain_without_leaf = chain_without_leaf.release(); X509_free(sess->x509_peer); - if (leaf != NULL) { - X509_up_ref(leaf); - } - sess->x509_peer = leaf; + sess->x509_peer = leaf.release(); return 1; } static int ssl_crypto_x509_session_dup(SSL_SESSION *new_session, const SSL_SESSION *session) { - if (session->x509_peer != NULL) { - X509_up_ref(session->x509_peer); - new_session->x509_peer = session->x509_peer; - } - if (session->x509_chain != NULL) { + new_session->x509_peer = UpRef(session->x509_peer).release(); + if (session->x509_chain != nullptr) { new_session->x509_chain = X509_chain_up_ref(session->x509_chain); - if (new_session->x509_chain == NULL) { + if (new_session->x509_chain == nullptr) { + return 0; + } + } + if (session->x509_chain_without_leaf != nullptr) { + new_session->x509_chain_without_leaf = + X509_chain_up_ref(session->x509_chain_without_leaf); + if (new_session->x509_chain_without_leaf == nullptr) { return 0; } } @@ -525,38 +540,17 @@ X509 *SSL_get_peer_certificate(const SSL *ssl) { STACK_OF(X509) *SSL_get_peer_cert_chain(const SSL *ssl) { check_ssl_x509_method(ssl); - if (ssl == NULL) { - return NULL; + if (ssl == nullptr) { + return nullptr; } SSL_SESSION *session = SSL_get_session(ssl); - if (session == NULL || - session->x509_chain == NULL) { - return NULL; - } - - if (!ssl->server) { - return session->x509_chain; + if (session == nullptr) { + return nullptr; } // OpenSSL historically didn't include the leaf certificate in the returned // certificate chain, but only for servers. - if (session->x509_chain_without_leaf == NULL) { - session->x509_chain_without_leaf = sk_X509_new_null(); - if (session->x509_chain_without_leaf == NULL) { - return NULL; - } - - for (size_t i = 1; i < sk_X509_num(session->x509_chain); i++) { - X509 *cert = sk_X509_value(session->x509_chain, i); - if (!PushToStack(session->x509_chain_without_leaf, UpRef(cert))) { - sk_X509_pop_free(session->x509_chain_without_leaf, X509_free); - session->x509_chain_without_leaf = NULL; - return NULL; - } - } - } - - return session->x509_chain_without_leaf; + return ssl->server ? session->x509_chain_without_leaf : session->x509_chain; } STACK_OF(X509) *SSL_get_peer_full_cert_chain(const SSL *ssl) {