From faff9856acc3c72ac4f5ccd4a1ab6ccd2b5c8f1e Mon Sep 17 00:00:00 2001 From: "davidben@chromium.org" Date: Sat, 21 Jun 2014 06:13:46 +0000 Subject: 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 --- net/base/net_error_list.h | 3 + net/socket/ssl_client_socket_openssl.cc | 123 +++++++++++++++++-------------- net/socket/ssl_client_socket_openssl.h | 10 +-- net/socket/ssl_client_socket_unittest.cc | 61 ++++++++++++++- net/ssl/server_bound_cert_service.cc | 7 +- tools/metrics/histograms/histograms.xml | 5 ++ 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(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 encrypted_private_key_info; + std::vector 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 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 encrypted_private_key_info; - std::vector 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 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 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. + + + + @@ -40756,6 +40760,7 @@ Therefore, the affected-histogram name has to have at least one dot in it. + -- cgit v1.1