Fix thread-safety bug in SSL_get_peer_cert_chain.
https://boringssl-review.googlesource.com/12704 pushed it just too far
to the edge. Once we have an established SSL_SESSION, any modifications
need to either be locked or done ahead of time. Do it ahead of time.
session->is_server gives a suitable place to check and X509s are
ref-counted so this should be cheap.
Add a regression test via TSan. Confirmed that TSan indeed catches this.
Change-Id: I30ce7b757d3a44465b318af3c98961ff3667483e
Reviewed-on: https://boringssl-review.googlesource.com/c/33606
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/ssl_asn1.cc b/ssl/ssl_asn1.cc
index 669f776..3fd7fb6 100644
--- a/ssl/ssl_asn1.cc
+++ b/ssl/ssl_asn1.cc
@@ -697,11 +697,6 @@
}
}
- 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 @@
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 705528b..8d01c03 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -4521,6 +4521,65 @@
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<SSL_SESSION> session =
+ CreateClientSession(client_ctx_.get(), server_ctx_.get());
+ ASSERT_TRUE(session);
+
+ // Resume with it twice.
+ UniquePtr<SSL> 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<X509> 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<std::thread> 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 ec203b2..eb3a38b 100644
--- a/ssl/ssl_x509.cc
+++ b/ssl/ssl_x509.cc
@@ -281,16 +281,25 @@
}
static int ssl_crypto_x509_session_cache_objects(SSL_SESSION *sess) {
- bssl::UniquePtr<STACK_OF(X509)> chain;
+ bssl::UniquePtr<STACK_OF(X509)> 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<X509> leaf;
for (CRYPTO_BUFFER *cert : sess->certs.get()) {
UniquePtr<X509> x509(X509_parse_from_buffer(cert));
if (!x509) {
@@ -298,7 +307,11 @@
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 @@
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 @@
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) {
