Add basic ECH support to SSLClientSocket

This is not connected up to the rest of the stack yet, and it also fails
the connection instead of implementing the recovery flow. All that will
be handled by follow-up CLs.

Bug: 1091403
Change-Id: Ic52299752d67d3f435825e159208d14654af8aac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3130761
Reviewed-by: Matt Mueller <mattm@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/main@{#917383}

net/socket/ssl_client_socket_impl.cc

@@ -584,6 +584,7 @@ bool SSLClientSocketImpl::GetSSLInfo(SSLInfo* ssl_info) {
   ssl_info->pkp_bypassed = pkp_bypassed_;
   ssl_info->public_key_hashes = server_cert_verify_result_.public_key_hashes;
   ssl_info->client_cert_sent = send_client_cert_ && client_cert_.get();
+  ssl_info->encrypted_client_hello = SSL_ech_accepted(ssl_.get());
   ssl_info->pinning_failure_log = pinning_failure_log_;
   ssl_info->ocsp_result = server_cert_verify_result_.ocsp_result;
   ssl_info->is_fatal_cert_error = is_fatal_cert_error_;
@@ -911,6 +912,14 @@ int SSLClientSocketImpl::Init() {
         host_and_port_, &client_cert_, &client_private_key_);
   }
 
+  // TODO(https://crbug.com/1091403): Also enable ECH GREASE, gated on SSLConfig
+  // or a base::Feature, for when we don't have an ECHConfig.
+  if (!ssl_config_.ech_config_list.empty() &&
+      !SSL_set1_ech_config_list(ssl_.get(), ssl_config_.ech_config_list.data(),
+                                ssl_config_.ech_config_list.size())) {
+    return ERR_UNEXPECTED;
+  }
+
   return OK;
 }
 
@@ -1101,6 +1110,18 @@ ssl_verify_result_t SSLClientSocketImpl::VerifyCert() {
     return HandleVerifyResult();
   }
 
+  // TODO(crbug.com/1091403): Implement the recovery flow.
+  const char* ech_name_override;
+  size_t ech_name_override_len;
+  SSL_get0_ech_name_override(ssl_.get(), &ech_name_override,
+                             &ech_name_override_len);
+  if (ech_name_override_len > 0) {
+    DCHECK(!ssl_config_.ech_config_list.empty());
+    NOTIMPLEMENTED();
+    OpenSSLPutNetError(FROM_HERE, ERR_FAILED);
+    return ssl_verify_invalid;
+  }
+
   // In this configuration, BoringSSL will perform exactly one certificate
   // verification, so there cannot be state from a previous verification.
   CHECK(!server_cert_);

net/socket/ssl_client_socket_unittest.cc

@@ -23,6 +23,7 @@
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/stringprintf.h"
+#include "base/synchronization/lock.h"
 #include "base/test/bind.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
@@ -96,6 +97,7 @@
 #include "third_party/abseil-cpp/absl/types/optional.h"
 #include "third_party/boringssl/src/include/openssl/bio.h"
 #include "third_party/boringssl/src/include/openssl/evp.h"
+#include "third_party/boringssl/src/include/openssl/hpke.h"
 #include "third_party/boringssl/src/include/openssl/pem.h"
 #include "third_party/boringssl/src/include/openssl/ssl.h"
 #include "url/gurl.h"
@@ -810,6 +812,8 @@ class SSLClientSocketTest : public PlatformTest, public WithTaskEnvironment {
     server->AddDefaultHandlers(base::FilePath());
     server->RegisterRequestHandler(
         base::BindRepeating(&ManySmallRecordsHttpResponse::HandleRequest));
+    server->RegisterRequestHandler(
+        base::BindRepeating(&HandleSSLInfoRequest, base::Unretained(this)));
   }
 
   // Starts the spawned test server with SSL configuration |ssl_options|.
@@ -896,6 +900,15 @@ class SSLClientSocketTest : public PlatformTest, public WithTaskEnvironment {
     cert_verifier_->AddResultForCert(server_cert.get(), verify_result, OK);
   }
 
+  absl::optional<SSLInfo> LastSSLInfoFromServer() {
+    // EmbeddedTestServer callbacks run on another thread, so protect this
+    // with a lock.
+    base::AutoLock lock(server_ssl_info_lock_);
+    auto result = server_ssl_info_;
+    server_ssl_info_ = absl::nullopt;
+    return result;
+  }
+
   RecordingTestNetLog log_;
   ClientSocketFactory* socket_factory_;
   std::unique_ptr<TestSSLConfigService> ssl_config_service_;
@@ -907,8 +920,25 @@ class SSLClientSocketTest : public PlatformTest, public WithTaskEnvironment {
   std::unique_ptr<SSLClientSocket> sock_;
 
  private:
+  static std::unique_ptr<test_server::HttpResponse> HandleSSLInfoRequest(
+      SSLClientSocketTest* test,
+      const test_server::HttpRequest& request) {
+    if (request.relative_url != "/ssl-info") {
+      return nullptr;
+    }
+    {
+      // EmbeddedTestServer callbacks run on another thread, so protect this
+      // with a lock.
+      base::AutoLock lock(test->server_ssl_info_lock_);
+      test->server_ssl_info_ = request.ssl_info;
+    }
+    return std::make_unique<test_server::BasicHttpResponse>();
+  }
+
   std::unique_ptr<SpawnedTestServer> spawned_test_server_;
   std::unique_ptr<EmbeddedTestServer> embedded_test_server_;
+  base::Lock server_ssl_info_lock_;
+  absl::optional<SSLInfo> server_ssl_info_ GUARDED_BY(server_ssl_info_lock_);
   TestCompletionCallback callback_;
   AddressList addr_;
   HostPortPair host_port_pair_;
@@ -1269,8 +1299,8 @@ class SSLClientSocketFalseStartTest : public SSLClientSocketTest {
 
 // Sends an HTTP request on the socket and reads the response. This may be used
 // to ensure some data has been consumed from the server.
-int MakeHTTPRequest(StreamSocket* socket) {
-  base::StringPiece request = "GET / HTTP/1.0\r\n\r\n";
+int MakeHTTPRequest(StreamSocket* socket, const char* path = "/") {
+  std::string request = base::StringPrintf("GET %s HTTP/1.0\r\n\r\n", path);
   TestCompletionCallback callback;
   while (!request.empty()) {
     auto request_buffer =
@@ -1493,6 +1523,41 @@ class HangingCertVerifier : public CertVerifier {
   int num_active_requests_ = 0;
 };
 
+bssl::UniquePtr<SSL_ECH_KEYS> MakeTestECHKeys(
+    const char* public_name,
+    size_t max_name_len,
+    std::vector<uint8_t>* ech_config_list) {
+  bssl::ScopedEVP_HPKE_KEY key;
+  if (!EVP_HPKE_KEY_generate(key.get(), EVP_hpke_x25519_hkdf_sha256())) {
+    return nullptr;
+  }
+
+  uint8_t* ech_config;
+  size_t ech_config_len;
+  if (!SSL_marshal_ech_config(&ech_config, &ech_config_len,
+                              /*config_id=*/1, key.get(), public_name,
+                              max_name_len)) {
+    return nullptr;
+  }
+  bssl::UniquePtr<uint8_t> scoped_ech_config(ech_config);
+
+  uint8_t* ech_config_list_raw;
+  size_t ech_config_list_len;
+  bssl::UniquePtr<SSL_ECH_KEYS> keys(SSL_ECH_KEYS_new());
+  if (!keys ||
+      !SSL_ECH_KEYS_add(keys.get(), /*is_retry_config=*/1, ech_config,
+                        ech_config_len, key.get()) ||
+      !SSL_ECH_KEYS_marshal_retry_configs(keys.get(), &ech_config_list_raw,
+                                          &ech_config_list_len)) {
+    return nullptr;
+  }
+  bssl::UniquePtr<uint8_t> scoped_ech_config_list(ech_config_list_raw);
+
+  ech_config_list->assign(ech_config_list_raw,
+                          ech_config_list_raw + ech_config_list_len);
+  return keys;
+}
+
 }  // namespace
 
 INSTANTIATE_TEST_SUITE_P(TLSVersion,
@@ -5487,6 +5552,87 @@ TEST_F(SSLClientSocketTest, Tag) {
 #endif  // OS_ANDROID
 }
 
+TEST_F(SSLClientSocketTest, ECH) {
+  SSLServerConfig server_config;
+  SSLConfig client_config;
+  server_config.ech_keys = MakeTestECHKeys(
+      "public.example", /*max_name_len=*/64, &client_config.ech_config_list);
+  ASSERT_TRUE(server_config.ech_keys);
+
+  ASSERT_TRUE(
+      StartEmbeddedTestServer(EmbeddedTestServer::CERT_OK, server_config));
+
+  // Connecting with the client should use ECH.
+  int rv;
+  ASSERT_TRUE(CreateAndConnectSSLClientSocket(client_config, &rv));
+  EXPECT_THAT(rv, IsOk());
+  SSLInfo ssl_info;
+  ASSERT_TRUE(sock_->GetSSLInfo(&ssl_info));
+  EXPECT_EQ(SSLInfo::HANDSHAKE_FULL, ssl_info.handshake_type);
+  EXPECT_TRUE(ssl_info.encrypted_client_hello);
+
+  // TLS 1.3 causes the ticket to arrive later. Use the socket to ensure we have
+  // a ticket. This also populates the SSLInfo from the server.
+  EXPECT_THAT(MakeHTTPRequest(sock_.get(), "/ssl-info"), IsOk());
+  absl::optional<SSLInfo> server_ssl_info = LastSSLInfoFromServer();
+  ASSERT_TRUE(server_ssl_info);
+  EXPECT_TRUE(server_ssl_info->encrypted_client_hello);
+
+  // Reconnect. ECH should not interfere with resumption.
+  sock_.reset();
+  ASSERT_TRUE(CreateAndConnectSSLClientSocket(client_config, &rv));
+  EXPECT_THAT(rv, IsOk());
+  ASSERT_TRUE(sock_->GetSSLInfo(&ssl_info));
+  EXPECT_EQ(SSLInfo::HANDSHAKE_RESUME, ssl_info.handshake_type);
+  EXPECT_TRUE(ssl_info.encrypted_client_hello);
+
+  // Check SSLInfo from the server.
+  EXPECT_THAT(MakeHTTPRequest(sock_.get(), "/ssl-info"), IsOk());
+  server_ssl_info = LastSSLInfoFromServer();
+  ASSERT_TRUE(server_ssl_info);
+  EXPECT_TRUE(server_ssl_info->encrypted_client_hello);
+
+  // Connecting without ECH should not report ECH was used.
+  client_config.ech_config_list.clear();
+  sock_.reset();
+  ASSERT_TRUE(CreateAndConnectSSLClientSocket(client_config, &rv));
+  EXPECT_THAT(rv, IsOk());
+  ASSERT_TRUE(sock_->GetSSLInfo(&ssl_info));
+  EXPECT_FALSE(ssl_info.encrypted_client_hello);
+
+  // Check SSLInfo from the server.
+  EXPECT_THAT(MakeHTTPRequest(sock_.get(), "/ssl-info"), IsOk());
+  server_ssl_info = LastSSLInfoFromServer();
+  ASSERT_TRUE(server_ssl_info);
+  EXPECT_FALSE(server_ssl_info->encrypted_client_hello);
+}
+
+TEST_F(SSLClientSocketTest, ECHWrongKeys) {
+  std::vector<uint8_t> ech_config_list1, ech_config_list2;
+  bssl::UniquePtr<SSL_ECH_KEYS> keys1 =
+      MakeTestECHKeys("public.example", /*max_name_len=*/64, &ech_config_list1);
+  ASSERT_TRUE(keys1);
+  bssl::UniquePtr<SSL_ECH_KEYS> keys2 =
+      MakeTestECHKeys("public.example", /*max_name_len=*/64, &ech_config_list2);
+  ASSERT_TRUE(keys2);
+
+  // Configure the client and server with different keys.
+  SSLServerConfig server_config;
+  server_config.ech_keys = std::move(keys1);
+  SSLConfig client_config;
+  client_config.ech_config_list = std::move(ech_config_list2);
+
+  ASSERT_TRUE(
+      StartEmbeddedTestServer(EmbeddedTestServer::CERT_OK, server_config));
+
+  // Connecting with the client should use ECH.
+  int rv;
+  ASSERT_TRUE(CreateAndConnectSSLClientSocket(client_config, &rv));
+  // TODO(crbug.com/1091403): Implement the recovery flow. Test that this both
+  // verifies against the public name and exposes retry configs.
+  EXPECT_THAT(rv, IsError(ERR_FAILED));
+}
+
 class TLS13DowngradeTest
     : public SSLClientSocketTest,
       public ::testing::WithParamInterface<

net/socket/ssl_server_socket_impl.cc

@@ -605,6 +605,7 @@ bool SSLServerContextImpl::SocketImpl::GetSSLInfo(SSLInfo* ssl_info) {
                                 &ssl_info->connection_status);
 
   ssl_info->early_data_received = early_data_received_;
+  ssl_info->encrypted_client_hello = SSL_ech_accepted(ssl_.get());
   ssl_info->handshake_type = SSL_session_reused(ssl_.get())
                                  ? SSLInfo::HANDSHAKE_RESUME
                                  : SSLInfo::HANDSHAKE_FULL;
@@ -1054,6 +1055,11 @@ void SSLServerContextImpl::Init() {
                               ssl_server_config_.ocsp_response.data(),
                               ssl_server_config_.ocsp_response.size());
   }
+
+  if (ssl_server_config_.ech_keys) {
+    CHECK(SSL_CTX_set1_ech_keys(ssl_ctx_.get(),
+                                ssl_server_config_.ech_keys.get()));
+  }
 }
 
 SSLServerContextImpl::~SSLServerContextImpl() = default;

net/ssl/ssl_config.h

@@ -138,6 +138,12 @@ struct NET_EXPORT SSLConfig {
   // session cache is partitioned by this value.
   NetworkIsolationKey network_isolation_key;
 
+  // If non-empty, a serialized ECHConfigList to use to encrypt the ClientHello.
+  //
+  // TODO(crbug.com/1091403): Support is currently incomplete. Implement the
+  // recovery flow and document what this does to the socket behavior.
+  std::vector<uint8_t> ech_config_list;
+
   // An additional boolean to partition the session cache by.
   //
   // TODO(https://crbug.com/775438, https://crbug.com/951205): This should

net/ssl/ssl_info.h

@@ -87,6 +87,12 @@ class NET_EXPORT SSLInfo {
   // set for server sockets.
   bool early_data_received = false;
 
+  // True if the connection negotiated the Encrypted ClientHello extension.
+  //
+  // TODO(crbug.com/1091403): Serialize this field in net_ipc_param_traits.cc
+  // and expose in DevTools Security Panel.
+  bool encrypted_client_hello = false;
+
   HandshakeType handshake_type = HANDSHAKE_UNKNOWN;
 
   // The hashes, in several algorithms, of the SubjectPublicKeyInfos from

net/ssl/ssl_server_config.cc

@@ -6,6 +6,7 @@
 
 #include "net/socket/ssl_client_socket.h"
 #include "net/ssl/ssl_config.h"
+#include "third_party/boringssl/src/include/openssl/ssl.h"
 
 namespace net {
 
@@ -21,4 +22,26 @@ SSLServerConfig::SSLServerConfig(const SSLServerConfig& other) = default;
 
 SSLServerConfig::~SSLServerConfig() = default;
 
+SSLServerConfig::ECHKeysContainer::ECHKeysContainer() = default;
+
+SSLServerConfig::ECHKeysContainer::ECHKeysContainer(
+    bssl::UniquePtr<SSL_ECH_KEYS> keys)
+    : keys_(std::move(keys)) {}
+
+SSLServerConfig::ECHKeysContainer::~ECHKeysContainer() = default;
+
+SSLServerConfig::ECHKeysContainer::ECHKeysContainer(
+    const SSLServerConfig::ECHKeysContainer& other)
+    : keys_(bssl::UpRef(other.keys_)) {}
+
+SSLServerConfig::ECHKeysContainer& SSLServerConfig::ECHKeysContainer::operator=(
+    const SSLServerConfig::ECHKeysContainer& other) {
+  keys_ = bssl::UpRef(other.keys_);
+  return *this;
+}
+
+void SSLServerConfig::ECHKeysContainer::reset(SSL_ECH_KEYS* keys) {
+  keys_.reset(keys);
+}
+
 }  // namespace net

net/ssl/ssl_server_config.h

@@ -7,13 +7,15 @@
 
 #include <stdint.h>
 
+#include <utility>
 #include <vector>
 
 #include "base/containers/flat_map.h"
 #include "net/base/net_export.h"
 #include "net/socket/next_proto.h"
 #include "net/ssl/ssl_config.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
+#include "third_party/boringssl/src/include/openssl/base.h"
 
 namespace net {
 
@@ -111,6 +113,31 @@ struct NET_EXPORT SSLServerConfig {
 
   // If non-empty, the DER-encoded OCSP response to staple.
   std::vector<uint8_t> ocsp_response;
+
+  // This is a workaround for BoringSSL's scopers not being copyable. See
+  // https://crbug.com/boringssl/431.
+  class NET_EXPORT ECHKeysContainer {
+   public:
+    ECHKeysContainer();
+    // Intentionally allow implicit conversion from bssl::UniquePtr.
+    ECHKeysContainer(bssl::UniquePtr<SSL_ECH_KEYS> keys);
+    ~ECHKeysContainer();
+
+    ECHKeysContainer(const ECHKeysContainer& other);
+    ECHKeysContainer& operator=(const ECHKeysContainer& other);
+
+    // Forward APIs from bssl::UniquePtr.
+    SSL_ECH_KEYS* get() const { return keys_.get(); }
+    explicit operator bool() const { return static_cast<bool>(keys_); }
+    // This is defined out-of-line to avoid an ssl.h include.
+    void reset(SSL_ECH_KEYS* keys = nullptr);
+
+   private:
+    bssl::UniquePtr<SSL_ECH_KEYS> keys_;
+  };
+
+  // If not nullptr, an ECH configuration to use on the server.
+  ECHKeysContainer ech_keys;
 };
 
 }  // namespace net