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>

ssl/ssl_asn1.cc

@@ -697,11 +697,6 @@ UniquePtr<SSL_SESSION> 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> 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;
 }
 

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<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;

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<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 @@ 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) {