Fix SSLClientSocketOpenSSL error-handling for Channel ID.

The old logic did not propogate asynchronous errors forward and potentially may
trigger two GetOrCreateDomainBoundCert calls in parallel if DoHandshake were
called while a Channel ID request were in flight.

Add a test to ensure this case behaves as expected.

BUG=386276

Review URL: https://codereview.chromium.org/338093012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@278947 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 144 insertions, 65 deletions

diff --git a/net/base/net_error_list.h b/net/base/net_error_list.h
index 65d3a45..e2686bd 100644
--- a/net/base/net_error_list.h
+++ b/net/base/net_error_list.h
@@ -732,6 +732,9 @@ NET_ERROR(SELF_SIGNED_CERT_GENERATION_FAILED, -713)
 // The certificate database changed in some way.
 NET_ERROR(CERT_DATABASE_CHANGED, -714)
 
+// Failure to import Channel ID.
+NET_ERROR(CHANNEL_ID_IMPORT_FAILED, -715)
+
 // DNS error codes.
 
 // DNS resolver received a malformed response.
diff --git a/net/socket/ssl_client_socket_openssl.cc b/net/socket/ssl_client_socket_openssl.cc
index 4ff8d43..68d7e5f 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -127,7 +127,6 @@ class SSLClientSocketOpenSSL::SSLContext {
     session_cache_.Reset(ssl_ctx_.get(), kDefaultSessionCacheConfig);
     SSL_CTX_set_cert_verify_callback(ssl_ctx_.get(), CertVerifyCallback, NULL);
     SSL_CTX_set_client_cert_cb(ssl_ctx_.get(), ClientCertCallback);
-    SSL_CTX_set_channel_id_cb(ssl_ctx_.get(), ChannelIDCallback);
     SSL_CTX_set_verify(ssl_ctx_.get(), SSL_VERIFY_PEER, NULL);
     // TODO(kristianm): Only select this if ssl_config_.next_proto is not empty.
     // It would be better if the callback were not a global setting,
@@ -150,12 +149,6 @@ class SSLClientSocketOpenSSL::SSLContext {
     return socket->ClientCertRequestCallback(ssl, x509, pkey);
   }
 
-  static void ChannelIDCallback(SSL* ssl, EVP_PKEY** pkey) {
-    SSLClientSocketOpenSSL* socket = GetInstance()->GetClientSocketFromSSL(ssl);
-    CHECK(socket);
-    socket->ChannelIDRequestCallback(ssl, pkey);
-  }
-
   static int CertVerifyCallback(X509_STORE_CTX *store_ctx, void *arg) {
     SSL* ssl = reinterpret_cast<SSL*>(X509_STORE_CTX_get_ex_data(
         store_ctx, SSL_get_ex_data_X509_STORE_CTX_idx()));
@@ -358,7 +351,6 @@ SSLClientSocketOpenSSL::SSLClientSocketOpenSSL(
       trying_cached_session_(false),
       next_handshake_state_(STATE_NONE),
       npn_status_(kNextProtoUnsupported),
-      channel_id_request_return_value_(ERR_UNEXPECTED),
       channel_id_xtn_negotiated_(false),
       net_log_(transport_->socket()->NetLog()) {}
 
@@ -476,6 +468,9 @@ void SSLClientSocketOpenSSL::Disconnect() {
   cert_authorities_.clear();
   cert_key_types_.clear();
   client_auth_cert_needed_ = false;
+
+  channel_id_xtn_negotiated_ = false;
+  channel_id_request_handle_.Cancel();
 }
 
 bool SSLClientSocketOpenSSL::IsConnected() const {
@@ -827,13 +822,15 @@ int SSLClientSocketOpenSSL::DoHandshake() {
     int ssl_error = SSL_get_error(ssl_, rv);
 
     if (ssl_error == SSL_ERROR_WANT_CHANNEL_ID_LOOKUP) {
-      // The server supports TLS channel id and the lookup is asynchronous.
-      // Retrieve the error from the call to |server_bound_cert_service_|.
-      net_error = channel_id_request_return_value_;
-    } else {
-      net_error = MapOpenSSLError(ssl_error, err_tracer);
+      // The server supports channel ID. Stop to look one up before returning to
+      // the handshake.
+      channel_id_xtn_negotiated_ = true;
+      GotoState(STATE_CHANNEL_ID_LOOKUP);
+      return OK;
     }
 
+    net_error = MapOpenSSLError(ssl_error, err_tracer);
+
     // If not done, stay in this state
     if (net_error == ERR_IO_PENDING) {
       GotoState(STATE_HANDSHAKE);
@@ -849,6 +846,57 @@ int SSLClientSocketOpenSSL::DoHandshake() {
   return net_error;
 }
 
+int SSLClientSocketOpenSSL::DoChannelIDLookup() {
+  GotoState(STATE_CHANNEL_ID_LOOKUP_COMPLETE);
+  return server_bound_cert_service_->GetOrCreateDomainBoundCert(
+      host_and_port_.host(),
+      &channel_id_private_key_,
+      &channel_id_cert_,
+      base::Bind(&SSLClientSocketOpenSSL::OnHandshakeIOComplete,
+                 base::Unretained(this)),
+      &channel_id_request_handle_);
+}
+
+int SSLClientSocketOpenSSL::DoChannelIDLookupComplete(int result) {
+  if (result < 0)
+    return result;
+
+  DCHECK_LT(0u, channel_id_private_key_.size());
+  // Decode key.
+  std::vector<uint8> encrypted_private_key_info;
+  std::vector<uint8> subject_public_key_info;
+  encrypted_private_key_info.assign(
+      channel_id_private_key_.data(),
+      channel_id_private_key_.data() + channel_id_private_key_.size());
+  subject_public_key_info.assign(
+      channel_id_cert_.data(),
+      channel_id_cert_.data() + channel_id_cert_.size());
+  scoped_ptr<crypto::ECPrivateKey> ec_private_key(
+      crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
+          ServerBoundCertService::kEPKIPassword,
+          encrypted_private_key_info,
+          subject_public_key_info));
+  if (!ec_private_key) {
+    LOG(ERROR) << "Failed to import Channel ID.";
+    return ERR_CHANNEL_ID_IMPORT_FAILED;
+  }
+
+  // Hand the key to OpenSSL. Check for error in case OpenSSL rejects the key
+  // type.
+  crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
+  int rv = SSL_set1_tls_channel_id(ssl_, ec_private_key->key());
+  if (!rv) {
+    LOG(ERROR) << "Failed to set Channel ID.";
+    int err = SSL_get_error(ssl_, rv);
+    return MapOpenSSLError(err, err_tracer);
+  }
+
+  // Return to the handshake.
+  set_channel_id_sent(true);
+  GotoState(STATE_HANDSHAKE);
+  return OK;
+}
+
 int SSLClientSocketOpenSSL::DoVerifyCert(int result) {
   DCHECK(server_cert_.get());
   GotoState(STATE_VERIFY_CERT_COMPLETE);
@@ -993,8 +1041,15 @@ int SSLClientSocketOpenSSL::DoHandshakeLoop(int last_io_result) {
       case STATE_HANDSHAKE:
         rv = DoHandshake();
         break;
+      case STATE_CHANNEL_ID_LOOKUP:
+        DCHECK_EQ(OK, rv);
+        rv = DoChannelIDLookup();
+       break;
+      case STATE_CHANNEL_ID_LOOKUP_COMPLETE:
+        rv = DoChannelIDLookupComplete(rv);
+        break;
       case STATE_VERIFY_CERT:
-        DCHECK(rv == OK);
+        DCHECK_EQ(OK, rv);
         rv = DoVerifyCert(rv);
        break;
       case STATE_VERIFY_CERT_COMPLETE:
@@ -1321,46 +1376,6 @@ int SSLClientSocketOpenSSL::ClientCertRequestCallback(SSL* ssl,
   return 0;
 }
 
-void SSLClientSocketOpenSSL::ChannelIDRequestCallback(SSL* ssl,
-                                                      EVP_PKEY** pkey) {
-  DVLOG(3) << "OpenSSL ChannelIDRequestCallback called";
-  DCHECK_EQ(ssl, ssl_);
-  DCHECK(!*pkey);
-
-  channel_id_xtn_negotiated_ = true;
-  if (!channel_id_private_key_.size()) {
-    channel_id_request_return_value_ =
-        server_bound_cert_service_->GetOrCreateDomainBoundCert(
-            host_and_port_.host(),
-            &channel_id_private_key_,
-            &channel_id_cert_,
-            base::Bind(&SSLClientSocketOpenSSL::OnHandshakeIOComplete,
-                       base::Unretained(this)),
-            &channel_id_request_handle_);
-    if (channel_id_request_return_value_ != OK)
-      return;
-  }
-
-  // Decode key.
-  std::vector<uint8> encrypted_private_key_info;
-  std::vector<uint8> subject_public_key_info;
-  encrypted_private_key_info.assign(
-      channel_id_private_key_.data(),
-      channel_id_private_key_.data() + channel_id_private_key_.size());
-  subject_public_key_info.assign(
-      channel_id_cert_.data(),
-      channel_id_cert_.data() + channel_id_cert_.size());
-  scoped_ptr<crypto::ECPrivateKey> ec_private_key(
-      crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
-          ServerBoundCertService::kEPKIPassword,
-          encrypted_private_key_info,
-          subject_public_key_info));
-  if (!ec_private_key)
-    return;
-  set_channel_id_sent(true);
-  *pkey = EVP_PKEY_dup(ec_private_key->key());
-}
-
 int SSLClientSocketOpenSSL::CertVerifyCallback(X509_STORE_CTX* store_ctx) {
   if (!completed_handshake_) {
     // If the first handshake hasn't completed then we accept any certificates
diff --git a/net/socket/ssl_client_socket_openssl.h b/net/socket/ssl_client_socket_openssl.h
index 5d70c05..ac85483 100644
--- a/net/socket/ssl_client_socket_openssl.h
+++ b/net/socket/ssl_client_socket_openssl.h
@@ -110,6 +110,8 @@ class SSLClientSocketOpenSSL : public SSLClientSocket {
 
   bool DoTransportIO();
   int DoHandshake();
+  int DoChannelIDLookup();
+  int DoChannelIDLookupComplete(int result);
   int DoVerifyCert(int result);
   int DoVerifyCertComplete(int result);
   void DoConnectCallback(int result);
@@ -136,10 +138,6 @@ class SSLClientSocketOpenSSL : public SSLClientSocket {
   // a certificate for this client.
   int ClientCertRequestCallback(SSL* ssl, X509** x509, EVP_PKEY** pkey);
 
-  // Callback from the SSL layer that indicates the remote server supports TLS
-  // Channel IDs.
-  void ChannelIDRequestCallback(SSL* ssl, EVP_PKEY** pkey);
-
   // CertVerifyCallback is called to verify the server's certificates. We do
   // verification after the handshake so this function only enforces that the
   // certificates don't change during renegotiation.
@@ -226,6 +224,8 @@ class SSLClientSocketOpenSSL : public SSLClientSocket {
   enum State {
     STATE_NONE,
     STATE_HANDSHAKE,
+    STATE_CHANNEL_ID_LOOKUP,
+    STATE_CHANNEL_ID_LOOKUP_COMPLETE,
     STATE_VERIFY_CERT,
     STATE_VERIFY_CERT_COMPLETE,
   };
@@ -236,8 +236,6 @@ class SSLClientSocketOpenSSL : public SSLClientSocket {
   // Written by the |server_bound_cert_service_|.
   std::string channel_id_private_key_;
   std::string channel_id_cert_;
-  // The return value of the last call to |server_bound_cert_service_|.
-  int channel_id_request_return_value_;
   // True if channel ID extension was negotiated.
   bool channel_id_xtn_negotiated_;
   // The request handle for |server_bound_cert_service_|.
diff --git a/net/socket/ssl_client_socket_unittest.cc b/net/socket/ssl_client_socket_unittest.cc
index 51c6756..2eaa47e 100644
--- a/net/socket/ssl_client_socket_unittest.cc
+++ b/net/socket/ssl_client_socket_unittest.cc
@@ -620,6 +620,38 @@ class FailingServerBoundCertStore : public ServerBoundCertStore {
   virtual void SetForceKeepSessionState() OVERRIDE {}
 };
 
+// A ServerBoundCertStore that asynchronously returns an error when asked for a
+// certificate.
+class AsyncFailingServerBoundCertStore : public ServerBoundCertStore {
+  virtual int GetServerBoundCert(const std::string& server_identifier,
+                                 base::Time* expiration_time,
+                                 std::string* private_key_result,
+                                 std::string* cert_result,
+                                 const GetCertCallback& callback) OVERRIDE {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(callback, ERR_UNEXPECTED,
+                              server_identifier, base::Time(), "", ""));
+    return ERR_IO_PENDING;
+  }
+  virtual void SetServerBoundCert(const std::string& server_identifier,
+                                  base::Time creation_time,
+                                  base::Time expiration_time,
+                                  const std::string& private_key,
+                                  const std::string& cert) OVERRIDE {}
+  virtual void DeleteServerBoundCert(const std::string& server_identifier,
+                                     const base::Closure& completion_callback)
+      OVERRIDE {}
+  virtual void DeleteAllCreatedBetween(base::Time delete_begin,
+                                       base::Time delete_end,
+                                       const base::Closure& completion_callback)
+      OVERRIDE {}
+  virtual void DeleteAll(const base::Closure& completion_callback) OVERRIDE {}
+  virtual void GetAllServerBoundCerts(const GetCertListCallback& callback)
+      OVERRIDE {}
+  virtual int GetCertCount() OVERRIDE { return 0; }
+  virtual void SetForceKeepSessionState() OVERRIDE {}
+};
+
 class SSLClientSocketTest : public PlatformTest {
  public:
   SSLClientSocketTest()
@@ -884,6 +916,13 @@ class SSLClientSocketChannelIDTest : public SSLClientSocketTest {
     context_.server_bound_cert_service = cert_service_.get();
   }
 
+  void EnableAsyncFailingChannelID() {
+    cert_service_.reset(new ServerBoundCertService(
+        new AsyncFailingServerBoundCertStore(),
+        base::MessageLoopProxy::current()));
+    context_.server_bound_cert_service = cert_service_.get();
+  }
+
  private:
   scoped_ptr<ServerBoundCertService> cert_service_;
 };
@@ -2718,8 +2757,8 @@ TEST_F(SSLClientSocketChannelIDTest, SendChannelID) {
   EXPECT_FALSE(sock_->IsConnected());
 }
 
-// Connect to a server using channel id but without sending a key. It should
-// fail.
+// Connect to a server using Channel ID but failing to look up the Channel
+// ID. It should fail.
 TEST_F(SSLClientSocketChannelIDTest, FailingChannelID) {
   SpawnedTestServer::SSLOptions ssl_options;
 
@@ -2740,4 +2779,22 @@ TEST_F(SSLClientSocketChannelIDTest, FailingChannelID) {
   EXPECT_FALSE(sock_->IsConnected());
 }
 
+// Connect to a server using Channel ID but asynchronously failing to look up
+// the Channel ID. It should fail.
+TEST_F(SSLClientSocketChannelIDTest, FailingChannelIDAsync) {
+  SpawnedTestServer::SSLOptions ssl_options;
+
+  ASSERT_TRUE(ConnectToTestServer(ssl_options));
+
+  EnableAsyncFailingChannelID();
+  SSLConfig ssl_config = kDefaultSSLConfig;
+  ssl_config.channel_id_enabled = true;
+
+  int rv;
+  ASSERT_TRUE(CreateAndConnectSSLClientSocket(ssl_config, &rv));
+
+  EXPECT_EQ(ERR_UNEXPECTED, rv);
+  EXPECT_FALSE(sock_->IsConnected());
+}
+
 }  // namespace net
diff --git a/net/ssl/server_bound_cert_service.cc b/net/ssl/server_bound_cert_service.cc
index 61d610b..b6b67c2 100644
--- a/net/ssl/server_bound_cert_service.cc
+++ b/net/ssl/server_bound_cert_service.cc
@@ -516,9 +516,10 @@ void ServerBoundCertService::GotServerBoundCert(
     HandleResult(OK, server_identifier, key, cert);
     return;
   }
-  // Async lookup did not find a valid cert. If no request asked to create one,
-  // return the error directly.
-  if (!j->second->CreateIfMissing()) {
+  // Async lookup failed or the certificate was missing. Return the error
+  // directly, unless the certificate was missing and a request asked to create
+  // one.
+  if (err != ERR_FILE_NOT_FOUND || !j->second->CreateIfMissing()) {
     HandleResult(err, server_identifier, key, cert);
     return;
   }
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index 919e7c3..41793eb 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -40723,6 +40723,10 @@ Therefore, the affected-histogram name has to have at least one dot in it.
   <int value="358" label="QUIC_HANDSHAKE_FAILED"/>
   <int value="359" label="REQUEST_FOR_SECURE_RESOURCE_OVER_INSECURE_QUIC"/>
   <int value="360" label="SPDY_INADEQUATE_TRANSPORT_SECURITY"/>
+  <int value="361" label="SPDY_FLOW_CONTROL_ERROR"/>
+  <int value="362" label="SPDY_FRAME_SIZE_ERROR"/>
+  <int value="363" label="SPDY_COMPRESSION_ERROR"/>
+  <int value="364" label="PROXY_AUTH_REQUESTED_WITH_NO_CONNECTION"/>
   <int value="400" label="CACHE_MISS"/>
   <int value="401" label="CACHE_READ_FAILURE"/>
   <int value="402" label="CACHE_WRITE_FAILURE"/>
@@ -40756,6 +40760,7 @@ Therefore, the affected-histogram name has to have at least one dot in it.
   <int value="712" label="PRIVATE_KEY_EXPORT_FAILED"/>
   <int value="713" label="SELF_SIGNED_CERT_GENERATION_FAILED"/>
   <int value="714" label="CERT_DATABASE_CHANGED"/>
+  <int value="715" label="CHANNEL_ID_IMPORT_FAILED"/>
   <int value="800" label="DNS_MALFORMED_RESPONSE"/>
   <int value="801" label="DNS_SERVER_REQUIRES_TCP"/>
   <int value="802" label="DNS_SERVER_FAILED"/>