Remove redundant 3-SHAKE mitigation.

net/socket/ssl_client_socket_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/socket/ssl_client_socket_impl.h"
 
 #include <errno.h>
 #include <string.h>
 
 #include <algorithm>
@@ skipping 219 matching lines @@
     "SSLLegacyECDSACiphers", base::FEATURE_DISABLED_BY_DEFAULT};
 
 bool AreLegacyECDSACiphersEnabled() {
   return base::FeatureList::IsEnabled(kLegacyECDSACiphersFeature);
 }
 #endif
 
 const base::Feature kShortRecordHeaderFeature{
     "SSLShortRecordHeader", base::FEATURE_DISABLED_BY_DEFAULT};
 
+scoped_refptr<X509Certificate> OSChainFromOpenSSL(STACK_OF(X509) *
+                                                  openssl_chain) {
+  if (sk_X509_num(openssl_chain) == 0) {
+    NOTREACHED();
+    return nullptr;
+  }
+
+#if defined(USE_OPENSSL_CERTS)
+  // When OSCertHandle is typedef'ed to X509, this implementation does a short
+  // cut to avoid converting back and forth between DER and the X509 struct.
+  X509Certificate::OSCertHandles intermediates;
+  for (size_t i = 1; i < sk_X509_num(openssl_chain); ++i) {
+    X509* cert = sk_X509_value(openssl_chain, i);
+    DCHECK(cert->buf);
+    intermediates.push_back(cert);
+  }
+
+  X509* leaf = sk_X509_value(openssl_chain, 0);
+  DCHECK(leaf->buf);
+  return X509Certificate::CreateFromHandle(leaf, intermediates);
+#else
+  // Convert the certificate chains to a platform certificate handle.
+  std::vector<base::StringPiece> der_chain;
+  der_chain.reserve(sk_X509_num(openssl_chain));
+  for (size_t i = 0; i < sk_X509_num(openssl_chain); ++i) {
+    X509* cert = sk_X509_value(openssl_chain, i);
+    DCHECK(cert->buf);
+    base::StringPiece der;
+    if (!x509_util::GetDER(cert, &der))
+      return nullptr;
+    der_chain.push_back(der);
+  }
+  return X509Certificate::CreateFromDERCertChain(der_chain);
+#endif
+}
+
 }  // namespace
 
 class SSLClientSocketImpl::SSLContext {
  public:
   static SSLContext* GetInstance() {
     return base::Singleton<SSLContext,
                            base::LeakySingletonTraits<SSLContext>>::get();
   }
   SSL_CTX* ssl_ctx() { return ssl_ctx_.get(); }
   SSLClientSessionCache* session_cache() { return &session_cache_; }
@@ skipping 171 matching lines @@
 const SSL_PRIVATE_KEY_METHOD
     SSLClientSocketImpl::SSLContext::kPrivateKeyMethod = {
         &SSLClientSocketImpl::SSLContext::PrivateKeyTypeCallback,
         &SSLClientSocketImpl::SSLContext::PrivateKeyMaxSignatureLenCallback,
         nullptr /* sign */,
         &SSLClientSocketImpl::SSLContext::PrivateKeySignDigestCallback,
         nullptr /* decrypt */,
         &SSLClientSocketImpl::SSLContext::PrivateKeyCompleteCallback,
 };
 
-// PeerCertificateChain is a helper object which extracts the certificate
-// chain, as given by the server, from an OpenSSL socket and performs the needed
-// resource management. The first element of the chain is the leaf certificate
-// and the other elements are in the order given by the server.
-class SSLClientSocketImpl::PeerCertificateChain {
- public:
-  explicit PeerCertificateChain(STACK_OF(X509) * chain) { Reset(chain); }
-  PeerCertificateChain(const PeerCertificateChain& other) { *this = other; }
-  ~PeerCertificateChain() {}
-  PeerCertificateChain& operator=(const PeerCertificateChain& other);
-
-  // Resets the PeerCertificateChain to the set of certificates in|chain|,
-  // which may be NULL, indicating to empty the store certificates.
-  // Note: If an error occurs, such as being unable to parse the certificates,
-  // this will behave as if Reset(NULL) was called.
-  void Reset(STACK_OF(X509) * chain);
-
-  // Note that when USE_OPENSSL_CERTS is defined, OSCertHandle is X509*
-  scoped_refptr<X509Certificate> AsOSChain() const;
-
-  size_t size() const {
-    if (!openssl_chain_.get())
-      return 0;
-    return sk_X509_num(openssl_chain_.get());
-  }
-
-  bool empty() const { return size() == 0; }
-
-  X509* Get(size_t index) const {
-    DCHECK_LT(index, size());
-    return sk_X509_value(openssl_chain_.get(), index);
-  }
-
- private:
-  bssl::UniquePtr<STACK_OF(X509)> openssl_chain_;
-};
-
-SSLClientSocketImpl::PeerCertificateChain&
-SSLClientSocketImpl::PeerCertificateChain::operator=(
-    const PeerCertificateChain& other) {
-  if (this == &other)
-    return *this;
-
-  openssl_chain_.reset(X509_chain_up_ref(other.openssl_chain_.get()));
-  return *this;
-}
-
-void SSLClientSocketImpl::PeerCertificateChain::Reset(STACK_OF(X509) * chain) {
-  openssl_chain_.reset(chain ? X509_chain_up_ref(chain) : NULL);
-}
-
-scoped_refptr<X509Certificate>
-SSLClientSocketImpl::PeerCertificateChain::AsOSChain() const {
-#if defined(USE_OPENSSL_CERTS)
-  // When OSCertHandle is typedef'ed to X509, this implementation does a short
-  // cut to avoid converting back and forth between DER and the X509 struct.
-  X509Certificate::OSCertHandles intermediates;
-  for (size_t i = 1; i < sk_X509_num(openssl_chain_.get()); ++i) {
-    X509* cert = sk_X509_value(openssl_chain_.get(), i);
-    DCHECK(cert->buf);
-    intermediates.push_back(cert);
-  }
-
-  X509* leaf = sk_X509_value(openssl_chain_.get(), 0);
-  DCHECK(leaf->buf);
-  return X509Certificate::CreateFromHandle(leaf, intermediates);
-#else
-  // Convert the certificate chains to a platform certificate handle.
-  std::vector<base::StringPiece> der_chain;
-  der_chain.reserve(sk_X509_num(openssl_chain_.get()));
-  for (size_t i = 0; i < sk_X509_num(openssl_chain_.get()); ++i) {
-    X509* cert = sk_X509_value(openssl_chain_.get(), i);
-    DCHECK(cert->buf);
-    base::StringPiece der;
-    if (!x509_util::GetDER(cert, &der))
-      return nullptr;
-    der_chain.push_back(der);
-  }
-  return X509Certificate::CreateFromDERCertChain(der_chain);
-#endif
-}
-
 // static
 void SSLClientSocket::ClearSessionCache() {
   SSLClientSocketImpl::SSLContext* context =
       SSLClientSocketImpl::SSLContext::GetInstance();
   context->session_cache()->Flush();
 }
 
 SSLClientSocketImpl::SSLClientSocketImpl(
     std::unique_ptr<ClientSocketHandle> transport_socket,
     const HostPortPair& host_and_port,
     const SSLConfig& ssl_config,
     const SSLClientSocketContext& context)
     : pending_read_error_(kNoPendingResult),
       pending_read_ssl_error_(SSL_ERROR_NONE),
-      server_cert_chain_(new PeerCertificateChain(NULL)),
       completed_connect_(false),
       was_ever_used_(false),
       cert_verifier_(context.cert_verifier),
       cert_transparency_verifier_(context.cert_transparency_verifier),
       channel_id_service_(context.channel_id_service),
       tb_was_negotiated_(false),
       tb_negotiated_param_(TB_PARAM_ECDSAP256),
       tb_signature_map_(10),
       transport_(std::move(transport_socket)),
       host_and_port_(host_and_port),
@@ skipping 247 matching lines @@
 bool SSLClientSocketImpl::WasAlpnNegotiated() const {
   return negotiated_protocol_ != kProtoUnknown;
 }
 
 NextProto SSLClientSocketImpl::GetNegotiatedProtocol() const {
   return negotiated_protocol_;
 }
 
 bool SSLClientSocketImpl::GetSSLInfo(SSLInfo* ssl_info) {
   ssl_info->Reset();
-  if (server_cert_chain_->empty())
+  if (!server_cert_)
     return false;
 
   ssl_info->cert = server_cert_verify_result_.verified_cert;
   ssl_info->unverified_cert = server_cert_;
   ssl_info->cert_status = server_cert_verify_result_.cert_status;
   ssl_info->is_issued_by_known_root =
       server_cert_verify_result_.is_issued_by_known_root;
   ssl_info->pkp_bypassed = pkp_bypassed_;
   ssl_info->public_key_hashes = server_cert_verify_result_.public_key_hashes;
   ssl_info->client_cert_sent =
@@ skipping 29 matching lines @@
   out->clear();
 }
 
 int64_t SSLClientSocketImpl::GetTotalReceivedBytes() const {
   return transport_->socket()->GetTotalReceivedBytes();
 }
 
 void SSLClientSocketImpl::DumpMemoryStats(SocketMemoryStats* stats) const {
   if (transport_adapter_)
     stats->buffer_size = transport_adapter_->GetAllocationSize();
-  if (server_cert_chain_) {
-    for (size_t i = 0; i < server_cert_chain_->size(); ++i) {
-      X509* cert = server_cert_chain_->Get(i);
+  STACK_OF(X509)* server_cert_chain = SSL_get_peer_cert_chain(ssl_.get());
+  if (server_cert_chain) {
+    for (size_t i = 0; i < sk_X509_num(server_cert_chain); ++i) {
+      X509* cert = sk_X509_value(server_cert_chain, i);
       // Estimate the size of the certificate before deduplication.
       // The multiplier (4) is added to account for the difference between the
       // serialized cert size and the actual cert allocation.
       // TODO(xunjieli): Update this after crbug.com/671420 is done.
       stats->cert_size += 4 * i2d_X509(cert, nullptr);
     }
-    stats->cert_count = server_cert_chain_->size();
+    stats->cert_count = sk_X509_num(server_cert_chain);
   }
   stats->total_size = stats->buffer_size + stats->cert_size;
 }
 
 // static
 void SSLClientSocketImpl::DumpSSLClientSessionMemoryStats(
     base::trace_event::ProcessMemoryDump* pmd) {
   SSLContext::GetInstance()->session_cache()->DumpMemoryStats(pmd);
 }
 
@@ skipping 329 matching lines @@
   if (!IsRenegotiationAllowed())
     SSL_set_renegotiate_mode(ssl_.get(), ssl_renegotiate_never);
 
   uint16_t signature_algorithm = SSL_get_peer_signature_algorithm(ssl_.get());
   if (signature_algorithm != 0) {
     UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SSLSignatureAlgorithm",
                                 signature_algorithm);
   }
 
   // Verify the certificate.
-  UpdateServerCert();
   next_handshake_state_ = STATE_VERIFY_CERT;
   return OK;
 }
 
 int SSLClientSocketImpl::DoChannelIDLookup() {
   NetLogParametersCallback callback = base::Bind(
       &NetLogChannelIDLookupCallback, base::Unretained(channel_id_service_));
   net_log_.BeginEvent(NetLogEventType::SSL_GET_CHANNEL_ID, callback);
   next_handshake_state_ = STATE_CHANNEL_ID_LOOKUP_COMPLETE;
   return channel_id_service_->GetOrCreateChannelID(
@@ skipping 19 matching lines @@
     return ERR_FAILED;
   }
 
   // Return to the handshake.
   channel_id_sent_ = true;
   next_handshake_state_ = STATE_HANDSHAKE;
   return OK;
 }
 
 int SSLClientSocketImpl::DoVerifyCert(int result) {
-  DCHECK(!server_cert_chain_->empty());
   DCHECK(start_cert_verification_time_.is_null());
 
-  next_handshake_state_ = STATE_VERIFY_CERT_COMPLETE;
+  server_cert_ = OSChainFromOpenSSL(SSL_get_peer_cert_chain(ssl_.get()));
 
   // OpenSSL decoded the certificate, but the platform certificate
   // implementation could not. This is treated as a fatal SSL-level protocol
   // error rather than a certificate error. See https://crbug.com/91341.
   if (!server_cert_)
     return ERR_SSL_SERVER_CERT_BAD_FORMAT;
 
+  net_log_.AddEvent(NetLogEventType::SSL_CERTIFICATES_RECEIVED,
+                    base::Bind(&NetLogX509CertificateCallback,
+                               base::Unretained(server_cert_.get())));
+
+  next_handshake_state_ = STATE_VERIFY_CERT_COMPLETE;
+
   // If the certificate is bad and has been previously accepted, use
   // the previous status and bypass the error.
   CertStatus cert_status;
   if (ssl_config_.IsAllowedBadCert(server_cert_.get(), &cert_status)) {
     server_cert_verify_result_.Reset();
     server_cert_verify_result_.cert_status = cert_status;
     server_cert_verify_result_.verified_cert = server_cert_;
     return OK;
   }
 
@@ skipping 84 matching lines @@
 }
 
 void SSLClientSocketImpl::DoConnectCallback(int rv) {
   if (!user_connect_callback_.is_null()) {
     CompletionCallback c = user_connect_callback_;
     user_connect_callback_.Reset();
     c.Run(rv > OK ? OK : rv);
   }
 }
 
-void SSLClientSocketImpl::UpdateServerCert() {
-  server_cert_chain_->Reset(SSL_get_peer_cert_chain(ssl_.get()));
-  server_cert_ = server_cert_chain_->AsOSChain();
-  if (server_cert_.get()) {
-    net_log_.AddEvent(NetLogEventType::SSL_CERTIFICATES_RECEIVED,
-                      base::Bind(&NetLogX509CertificateCallback,
-                                 base::Unretained(server_cert_.get())));
-  }
-}
-
 void SSLClientSocketImpl::OnHandshakeIOComplete(int result) {
   int rv = DoHandshakeLoop(result);
   if (rv != ERR_IO_PENDING) {
     LogConnectEndEvent(rv);
     DoConnectCallback(rv);
   }
 }
 
 int SSLClientSocketImpl::DoHandshakeLoop(int last_io_result) {
   TRACE_EVENT0(kNetTracingCategory, "SSLClientSocketImpl::DoHandshakeLoop");
@@ skipping 347 matching lines @@
     return 1;
   }
 #endif  // defined(OS_IOS)
 
   // Send no client certificate.
   net_log_.AddEvent(NetLogEventType::SSL_CLIENT_CERT_PROVIDED,
                     NetLog::IntCallback("cert_count", 0));
   return 1;
 }
 
 int SSLClientSocketImpl::CertVerifyCallback(X509_STORE_CTX* store_ctx) {

[svaldez, 2017/03/07 15:05:04]

Can we just remove the CertVerifyCallback entirely?

[davidben, 2017/03/07 17:25:25]

This is what stops us from running OpenSSL's verifier. :-)

-  if (!completed_connect_) {
-    // If the first handshake hasn't completed then we accept any certificates
-    // because we verify after the handshake.
-    return 1;
-  }
-
-  // Disallow the server certificate to change in a renegotiation.
-  if (server_cert_chain_->empty()) {
-    LOG(ERROR) << "Received invalid certificate chain between handshakes";
-    return 0;
-  }
-  base::StringPiece old_der, new_der;
-  if (store_ctx->cert == NULL ||
-      !x509_util::GetDER(server_cert_chain_->Get(0), &old_der) ||
-      !x509_util::GetDER(store_ctx->cert, &new_der)) {
-    LOG(ERROR) << "Failed to encode certificates";
-    return 0;
-  }
-  if (old_der != new_der) {
-    LOG(ERROR) << "Server certificate changed between handshakes";
-    return 0;
-  }
-
+  // The server certificate is verified later in DoVerifyCert.
   return 1;
 }
 
 void SSLClientSocketImpl::MaybeCacheSession() {
   // Only cache the session once both a new session has been established and the
   // certificate has been verified. Due to False Start, these events may happen
   // in either order.
   if (!pending_session_ || !certificate_verified_)
     return;
 
@@ skipping 281 matching lines @@
     if (ERR_GET_REASON(info->error_code) == SSL_R_TLSV1_ALERT_ACCESS_DENIED &&
         !certificate_requested_) {
       net_error = ERR_SSL_PROTOCOL_ERROR;
     }
   }
 
   return net_error;
 }
 
 }  // namespace net