Fix server initiated SSL renegotiation for SSLClientSocketMac

The use of kSSLSessionOptionBreakOnServerAuth/kSSLSessionoptionbreakOnCertRequested is bugged on OS X 10.5.8+, and will prevent server-initiated renegotiation (eg: to request a certificate) from working.

Further, the implementation of SSLClientSocketMac, when used on 10.6+, cause it to abort the connection if, after the initial handshake, a certificate is requested (eg: during a re-handshake).

Finally, if a renegotiation happens after the initial certificate has been validated, we do not update the server certificate with the new value, nor is it revalidated, which is different than what happens on Windows/NSS.

This removes the use of both options, and changes the state machine to detect when a renegotiation/rehandshake is underway, and re-verify the server certificate before continuing with application data.

R=wtc
BUG=45576
TEST=Visit any site that requests SSL client auth over renegotiation.

Review URL: http://codereview.chromium.org/3120036

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

2 files changed, 199 insertions, 245 deletions

diff --git a/net/socket/ssl_client_socket_mac.cc b/net/socket/ssl_client_socket_mac.cc
index 284937a..2df29e7 100644
--- a/net/socket/ssl_client_socket_mac.cc
+++ b/net/socket/ssl_client_socket_mac.cc
@@ -39,7 +39,7 @@
 // it. Therefore, there are different sets of states in our respective state
 // machines, fewer on the Mac because Secure Transport keeps a lot of its own
 // state. The discussion about what each of the states means lives in comments
-// in the DoLoop() function.
+// in the DoHandshakeLoop() function.
 //
 // Secure Transport is designed for use by either blocking or non-blocking
 // network I/O. If, for example, you called SSLRead() to fetch data, Secure
@@ -108,18 +108,6 @@ const unsigned int kWriteSizeResumeLimit = 1 * 1024 * 1024;
 #define SSL_LOG LOG(INFO) << "SSL: "
 
 #if MAC_OS_X_VERSION_MAX_ALLOWED <= MAC_OS_X_VERSION_10_5
-// Declarations needed to call the 10.5.7 and later SSLSetSessionOption()
-// function when building with the 10.5.0 SDK.
-typedef enum {
-  kSSLSessionOptionBreakOnServerAuth,
-  kSSLSessionOptionBreakOnCertRequested,
-} SSLSessionOption;
-
-enum {
-  errSSLServerAuthCompleted = -9841,
-  errSSLClientCertRequested = -9842,
-};
-
 // When compiled against the Mac OS X 10.5 SDK, define symbolic constants for
 // cipher suites added in Mac OS X 10.6.
 enum {
@@ -152,9 +140,6 @@ enum {
 };
 #endif
 
-typedef OSStatus (*SSLSetSessionOptionFuncPtr)(SSLContextRef,
-                                               SSLSessionOption,
-                                               Boolean);
 // For an explanation of the Mac OS X error codes, please refer to:
 // http://developer.apple.com/mac/library/documentation/Security/Reference/secureTransportRef/Reference/reference.html
 int NetErrorFromOSStatus(OSStatus status) {
@@ -515,8 +500,7 @@ SSLClientSocketMac::SSLClientSocketMac(ClientSocketHandle* transport_socket,
       user_read_buf_len_(0),
       user_write_buf_len_(0),
       next_handshake_state_(STATE_NONE),
-      completed_handshake_(false),
-      handshake_interrupted_(false),
+      renegotiating_(false),
       client_cert_requested_(false),
       ssl_context_(NULL),
       pending_send_error_(OK),
@@ -540,7 +524,7 @@ int SSLClientSocketMac::Connect(CompletionCallback* callback) {
     return rv;
   }
 
-  next_handshake_state_ = STATE_HANDSHAKE_START;
+  next_handshake_state_ = STATE_HANDSHAKE;
   rv = DoHandshakeLoop(OK);
   if (rv == ERR_IO_PENDING) {
     user_connect_callback_ = callback;
@@ -551,7 +535,7 @@ int SSLClientSocketMac::Connect(CompletionCallback* callback) {
 }
 
 void SSLClientSocketMac::Disconnect() {
-  completed_handshake_ = false;
+  next_handshake_state_ = STATE_NONE;
 
   if (ssl_context_) {
     SSLClose(ssl_context_);
@@ -572,7 +556,7 @@ bool SSLClientSocketMac::IsConnected() const {
   // layer (HttpNetworkTransaction) needs to handle a persistent connection
   // closed by the server when we send a request anyway, a false positive in
   // exchange for simpler code is a good trade-off.
-  return completed_handshake_ && transport_->socket()->IsConnected();
+  return completed_handshake() && transport_->socket()->IsConnected();
 }
 
 bool SSLClientSocketMac::IsConnectedAndIdle() const {
@@ -584,7 +568,7 @@ bool SSLClientSocketMac::IsConnectedAndIdle() const {
   // bytes to the transport layer below, so
   // transport_->socket()->IsConnectedAndIdle() returns the desired false
   // when we receive close_notify.
-  return completed_handshake_ && transport_->socket()->IsConnectedAndIdle();
+  return completed_handshake() && transport_->socket()->IsConnectedAndIdle();
 }
 
 int SSLClientSocketMac::GetPeerAddress(AddressList* address) const {
@@ -617,7 +601,7 @@ bool SSLClientSocketMac::WasEverUsed() const {
 
 int SSLClientSocketMac::Read(IOBuffer* buf, int buf_len,
                              CompletionCallback* callback) {
-  DCHECK(completed_handshake_);
+  DCHECK(completed_handshake());
   DCHECK(!user_read_callback_);
   DCHECK(!user_read_buf_);
 
@@ -636,7 +620,7 @@ int SSLClientSocketMac::Read(IOBuffer* buf, int buf_len,
 
 int SSLClientSocketMac::Write(IOBuffer* buf, int buf_len,
                               CompletionCallback* callback) {
-  DCHECK(completed_handshake_);
+  DCHECK(completed_handshake());
   DCHECK(!user_write_callback_);
   DCHECK(!user_write_buf_);
 
@@ -728,34 +712,6 @@ SSLClientSocketMac::GetNextProto(std::string* proto) {
   return kNextProtoUnsupported;
 }
 
-OSStatus SSLClientSocketMac::EnableBreakOnAuth(bool enabled) {
-  // SSLSetSessionOption() was introduced in Mac OS X 10.5.7. It allows us
-  // to perform certificate validation during the handshake, which is
-  // required in order to properly enable session resumption.
-  //
-  // With the kSSLSessionOptionBreakOnServerAuth option set, SSLHandshake()
-  // will return errSSLServerAuthCompleted after receiving the server's
-  // Certificate during the handshake. That gives us an opportunity to verify
-  // the server certificate and then re-enter that handshake (assuming the
-  // certificate successfully validated).
-  // If the server also requests a client cert, SSLHandshake() will return
-  // errSSLClientCertRequested in addition to (or in some cases *instead of*)
-  // errSSLServerAuthCompleted.
-  static SSLSetSessionOptionFuncPtr ssl_set_session_options =
-      LookupFunction<SSLSetSessionOptionFuncPtr>(CFSTR("com.apple.security"),
-                                                 CFSTR("SSLSetSessionOption"));
-  if (!ssl_set_session_options)
-    return unimpErr;  // Return this if the API isn't available
-  OSStatus err = ssl_set_session_options(ssl_context_,
-                                         kSSLSessionOptionBreakOnServerAuth,
-                                         enabled);
-  if (err)
-    return err;
-  return ssl_set_session_options(ssl_context_,
-                                 kSSLSessionOptionBreakOnCertRequested,
-                                 enabled);
-}
-
 int SSLClientSocketMac::InitializeSSLContext() {
   SSL_LOG << "----- InitializeSSLContext";
   OSStatus status = noErr;
@@ -810,55 +766,32 @@ int SSLClientSocketMac::InitializeSSLContext() {
   if (status)
     return NetErrorFromOSStatus(status);
 
-  // It is tricky to handle client cert request over renegotiation due to bugs
-  // in Secure Transport.  From Ken McLeod on apple-cdsa:
-  // http://lists.apple.com/archives/apple-cdsa/2010/Feb/msg00058.html
-  //   A possible workaround would be to set the
-  //   kSSLSessionOptionBreakOnCertRequested option initially, then if you get
-  //   that status, ask for a client cert, abort the connection yourself and
-  //   retry it (this time calling SSLSetCertificate before the handshake
-  //   starts, and *not* setting the kSSLSessionOptionBreakOnCertRequested
-  //   option.)
   if (ssl_config_.send_client_cert) {
-    SSL_LOG << "Setting client cert in advance because send_client_cert is set";
     status = SetClientCert();
     if (status)
       return NetErrorFromOSStatus(status);
     return OK;
   }
 
-  status = EnableBreakOnAuth(true);
-  if (status == noErr) {
-    // Only enable session resumption if break-on-auth is available,
-    // because without break-on-auth we are verifying the server's certificate
-    // after the handshake completes (but before any application data is
-    // exchanged). If we were to enable session resumption in this situation,
-    // the session would be cached before we verified the certificate, leaving
-    // the potential for a session in which the certificate failed to validate
-    // to still be able to be resumed.
-
-    // Concatenate the hostname and peer address to use as the peer ID. To
-    // resume a session, we must connect to the same server on the same port
-    // using the same hostname (i.e., localhost and 127.0.0.1 are considered
-    // different peers, which puts us through certificate validation again
-    // and catches hostname/certificate name mismatches.
-    AddressList address;
-    int rv = transport_->socket()->GetPeerAddress(&address);
-    if (rv != OK)
-      return rv;
-    const struct addrinfo* ai = address.head();
-    std::string peer_id(hostname_);
-    peer_id += std::string(reinterpret_cast<char*>(ai->ai_addr),
-                           ai->ai_addrlen);
-
-    // SSLSetPeerID() treats peer_id as a binary blob, and makes its
-    // own copy.
-    status = SSLSetPeerID(ssl_context_, peer_id.data(), peer_id.length());
-    if (status)
-      return NetErrorFromOSStatus(status);
-  } else if (status != unimpErr) {  // it's OK if the API isn't available
+  // Concatenate the hostname and peer address to use as the peer ID. To
+  // resume a session, we must connect to the same server on the same port
+  // using the same hostname (i.e., localhost and 127.0.0.1 are considered
+  // different peers, which puts us through certificate validation again
+  // and catches hostname/certificate name mismatches.
+  AddressList address;
+  int rv = transport_->socket()->GetPeerAddress(&address);
+  if (rv != OK)
+    return rv;
+  const struct addrinfo* ai = address.head();
+  std::string peer_id(hostname_);
+  peer_id += std::string(reinterpret_cast<char*>(ai->ai_addr),
+                         ai->ai_addrlen);
+
+  // SSLSetPeerID() treats peer_id as a binary blob, and makes its
+  // own copy.
+  status = SSLSetPeerID(ssl_context_, peer_id.data(), peer_id.length());
+  if (status)
     return NetErrorFromOSStatus(status);
-  }
 
   return OK;
 }
@@ -866,7 +799,6 @@ int SSLClientSocketMac::InitializeSSLContext() {
 void SSLClientSocketMac::DoConnectCallback(int rv) {
   DCHECK(rv != ERR_IO_PENDING);
   DCHECK(user_connect_callback_);
-  DCHECK(next_handshake_state_ == STATE_NONE);
 
   CompletionCallback* c = user_connect_callback_;
   user_connect_callback_ = NULL;
@@ -900,9 +832,16 @@ void SSLClientSocketMac::DoWriteCallback(int rv) {
 }
 
 void SSLClientSocketMac::OnHandshakeIOComplete(int result) {
-  DCHECK(next_handshake_state_ != STATE_NONE);
   int rv = DoHandshakeLoop(result);
   if (rv != ERR_IO_PENDING) {
+    // If there is no connect callback available to call, we are
+    // renegotiating (which occurs because we are in the middle of a Read
+    // when the renegotiation process starts).  So we complete the Read
+    // here.
+    if (!user_connect_callback_) {
+      DoReadCallback(rv);
+      return;
+    }
     net_log_.EndEvent(NetLog::TYPE_SSL_CONNECT, NULL);
     DoConnectCallback(rv);
   }
@@ -916,14 +855,11 @@ void SSLClientSocketMac::OnTransportReadComplete(int result) {
   }
   read_io_buf_ = NULL;
 
-  if (next_handshake_state_ != STATE_NONE) {
-    int rv = DoHandshakeLoop(result);
-    if (rv != ERR_IO_PENDING) {
-      net_log_.EndEvent(NetLog::TYPE_SSL_CONNECT, NULL);
-      DoConnectCallback(rv);
-    }
+  if (!completed_handshake()) {
+    OnHandshakeIOComplete(result);
     return;
   }
+
   if (user_read_buf_) {
     if (result < 0) {
       DoReadCallback(result);
@@ -948,6 +884,11 @@ void SSLClientSocketMac::OnTransportWriteComplete(int result) {
   if (!send_buffer_.empty())
     SSLWriteCallback(this, NULL, NULL);
 
+  if (!completed_handshake()) {
+    OnHandshakeIOComplete(result);
+    return;
+  }
+
   // If paused because too much data is in flight, try writing again and make
   // the promised callback.
   if (user_write_buf_ && send_buffer_.size() < kWriteSizeResumeLimit) {
@@ -957,8 +898,6 @@ void SSLClientSocketMac::OnTransportWriteComplete(int result) {
   }
 }
 
-// This is the main loop driving the state machine. Most calls coming from the
-// outside just set up a few variables and jump into here.
 int SSLClientSocketMac::DoHandshakeLoop(int last_io_result) {
   DCHECK(next_handshake_state_ != STATE_NONE);
   int rv = last_io_result;
@@ -966,22 +905,28 @@ int SSLClientSocketMac::DoHandshakeLoop(int last_io_result) {
     State state = next_handshake_state_;
     next_handshake_state_ = STATE_NONE;
     switch (state) {
-      case STATE_HANDSHAKE_START:
-        // Do the SSL/TLS handshake, up to the server certificate message.
-        rv = DoHandshakeStart();
+      case STATE_HANDSHAKE:
+        // Do the SSL/TLS handshake.
+        rv = DoHandshake();
         break;
       case STATE_VERIFY_CERT:
         // Kick off server certificate validation.
         rv = DoVerifyCert();
         break;
       case STATE_VERIFY_CERT_COMPLETE:
-        // Check the results of the  server certificate validation.
+        // Check the results of the server certificate validation.
         rv = DoVerifyCertComplete(rv);
         break;
-      case STATE_HANDSHAKE_FINISH:
-        // Do the SSL/TLS handshake, after the server certificate message.
-        rv = DoHandshakeFinish();
+      case STATE_COMPLETED_RENEGOTIATION:
+        // The renegotiation handshake has completed, and the Read() call
+        // that was interrupted by the renegotiation needs to be resumed in
+        // order to to satisfy the original caller's request.
+        rv = DoCompletedRenegotiation(rv);
         break;
+      case STATE_COMPLETED_HANDSHAKE:
+        next_handshake_state_ = STATE_COMPLETED_HANDSHAKE;
+        // This is the end of our state machine, so return.
+        return rv;
       default:
         rv = ERR_UNEXPECTED;
         NOTREACHED() << "unexpected state";
@@ -991,57 +936,49 @@ int SSLClientSocketMac::DoHandshakeLoop(int last_io_result) {
   return rv;
 }
 
-int SSLClientSocketMac::DoHandshakeStart() {
+int SSLClientSocketMac::DoHandshake() {
+  client_cert_requested_ = false;
+
   OSStatus status = SSLHandshake(ssl_context_);
 
-  switch (status) {
-    case errSSLWouldBlock:
-      next_handshake_state_ = STATE_HANDSHAKE_START;
-      break;
+  SSLClientCertificateState client_cert_state;
+  if (SSLGetClientCertificateState(ssl_context_, &client_cert_state) != noErr)
+    client_cert_state = kSSLClientCertNone;
+  if (client_cert_state > kSSLClientCertNone)
+    client_cert_requested_ = true;
 
+  switch (status) {
     case noErr:
-      // TODO(hawk): we verify the certificate chain even on resumed sessions
-      // so that we have the certificate status (valid, expired but overridden
-      // by the user, EV, etc.) available. Eliminate this step once we have
-      // a certificate validation result cache. (Also applies to the
-      // errSSLServerAuthCompleted case below.)
-      SSL_LOG << "Handshake completed (DoHandshakeStart), next verify cert";
-      next_handshake_state_ = STATE_VERIFY_CERT;
-      HandshakeFinished();
-      break;
-
-    case errSSLServerAuthCompleted:
-      // Override errSSLServerAuthCompleted as it's not actually an error,
-      // but rather an indication that we're only half way through the
-      // handshake.
-      SSL_LOG << "Server auth completed (DoHandshakeStart)";
-      next_handshake_state_ = STATE_VERIFY_CERT;
-      handshake_interrupted_ = true;
-      status = noErr;
-      break;
-
-    case errSSLClientCertRequested:
-      SSL_LOG << "Received client cert request in DoHandshakeStart";
-      // If we get this instead of errSSLServerAuthCompleted, the latter is
-      // implicit, and we should begin verification as well.
-      next_handshake_state_ = STATE_VERIFY_CERT;
-      handshake_interrupted_ = true;
-      status = noErr;
-      // We don't want to send a client cert now, because we haven't
-      // verified the server's cert yet. Remember it for later.
-      client_cert_requested_ = true;
+      return DidCompleteHandshake();
+    case errSSLWouldBlock:
+      next_handshake_state_ = STATE_HANDSHAKE;
       break;
-
     case errSSLClosedGraceful:
       // The server unexpectedly closed on us.
       return ERR_SSL_PROTOCOL_ERROR;
+    case errSSLClosedAbort:
+    case errSSLPeerHandshakeFail:
+      if (client_cert_requested_) {
+        // See if the server aborted due to client cert checking.
+        if (!ssl_config_.send_client_cert) {
+          SSL_LOG << "Server requested SSL cert during handshake";
+          return ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
+        }
+        LOG(WARNING) << "Server aborted SSL handshake";
+        return ERR_BAD_SSL_CLIENT_AUTH_CERT;
+      }
+      break;
   }
 
   int net_error = NetErrorFromOSStatus(status);
-  if (status == noErr || IsCertificateError(net_error)) {
-    server_cert_ = GetServerCert(ssl_context_);
-    if (!server_cert_)
-      return ERR_UNEXPECTED;
+  DCHECK(!IsCertificateError(net_error));
+
+  if (!ssl_config_.send_client_cert &&
+     (client_cert_state == kSSLClientCertRejected ||
+      net_error == ERR_BAD_SSL_CLIENT_AUTH_CERT)) {
+    // The server unexpectedly sent a peer certificate error alert when no
+    // certificate had been sent.
+    net_error = ERR_SSL_PROTOCOL_ERROR;
   }
   return net_error;
 }
@@ -1049,8 +986,7 @@ int SSLClientSocketMac::DoHandshakeStart() {
 int SSLClientSocketMac::DoVerifyCert() {
   next_handshake_state_ = STATE_VERIFY_CERT_COMPLETE;
 
-  if (!server_cert_)
-    return ERR_UNEXPECTED;
+  DCHECK(server_cert_);
 
   SSL_LOG << "DoVerifyCert...";
   int flags = 0;
@@ -1072,30 +1008,22 @@ int SSLClientSocketMac::DoVerifyCertComplete(int result) {
   if (IsCertificateError(result) && ssl_config_.IsAllowedBadCert(server_cert_))
     result = OK;
 
-  if (result == OK && client_cert_requested_) {
-    if (!ssl_config_.send_client_cert) {
-      // Caller hasn't specified a client cert, so let it know the server's
-      // asking for one, and abort the connection.
-      return ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
-    }
-    // (We already called SetClientCert during InitializeSSLContext;
-    // no need to do so again.)
+  if (result == OK && client_cert_requested_ &&
+      !ssl_config_.send_client_cert) {
+    // Caller hasn't specified a client cert, so let it know the server is
+    // asking for one, and abort the connection.
+    return ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
   }
+  SSL_LOG << "Handshake finished! (DoVerifyCertComplete)";
 
-  if (handshake_interrupted_) {
-    // With session resumption enabled the full handshake (i.e., the handshake
-    // in a non-resumed session) occurs in two steps. Continue on to the second
-    // step if the certificate is OK.
-    if (result == OK)
-      next_handshake_state_ = STATE_HANDSHAKE_FINISH;
-  } else {
-    // If the session was resumed or session resumption was disabled, we're
-    // done with the handshake.
-    SSL_LOG << "Handshake finished! (DoVerifyCertComplete)";
-    completed_handshake_ = true;
-    DCHECK(next_handshake_state_ == STATE_NONE);
+  if (renegotiating_) {
+    DidCompleteRenegotiation();
+    return result;
   }
 
+  // The initial handshake has completed.
+  next_handshake_state_ = STATE_COMPLETED_HANDSHAKE;
+
   return result;
 }
 
@@ -1112,67 +1040,24 @@ int SSLClientSocketMac::SetClientCert() {
   return result;
 }
 
-int SSLClientSocketMac::DoHandshakeFinish() {
-  OSStatus status = SSLHandshake(ssl_context_);
-
-  switch (status) {
-    case errSSLWouldBlock:
-      next_handshake_state_ = STATE_HANDSHAKE_FINISH;
-      break;
-    case errSSLClientCertRequested:
-      SSL_LOG << "Server requested client cert (DoHandshakeFinish)";
-      DCHECK(!ssl_config_.send_client_cert);
-      return ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
-      break;
-    case errSSLClosedGraceful:
-      return ERR_SSL_PROTOCOL_ERROR;
-    case errSSLClosedAbort:
-    case errSSLPeerHandshakeFail: {
-      // See if the server aborted due to client cert checking.
-      SSLClientCertificateState client_state;
-      if (SSLGetClientCertificateState(ssl_context_, &client_state) == noErr &&
-          client_state > kSSLClientCertNone) {
-        if (client_state == kSSLClientCertRequested &&
-            !ssl_config_.send_client_cert) {
-          SSL_LOG << "Server requested SSL cert during handshake";
-          return ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
-        }
-        LOG(WARNING) << "Server aborted SSL handshake; client_state="
-                     << client_state;
-        return ERR_BAD_SSL_CLIENT_AUTH_CERT;
-      }
-      break;
-    }
-    case noErr:
-      SSL_LOG << "Handshake finished! (DoHandshakeFinish)";
-      HandshakeFinished();
-      completed_handshake_ = true;
-      DCHECK(next_handshake_state_ == STATE_NONE);
-      break;
-    default:
-      break;
-  }
-
-  return NetErrorFromOSStatus(status);
-}
-
-void SSLClientSocketMac::HandshakeFinished() {
-  SSL_LOG << "HandshakeFinished()";
-}
-
 int SSLClientSocketMac::DoPayloadRead() {
   size_t processed = 0;
-  OSStatus status = SSLRead(ssl_context_,
-                            user_read_buf_->data(),
-                            user_read_buf_len_,
-                            &processed);
-
+  OSStatus status = SSLRead(ssl_context_, user_read_buf_->data(),
+                            user_read_buf_len_, &processed);
+  if (status == errSSLWouldBlock && renegotiating_) {
+    CHECK_EQ(static_cast<size_t>(0), processed);
+    next_handshake_state_ = STATE_HANDSHAKE;
+    return DoHandshakeLoop(OK);
+  }
   // There's a subtle difference here in semantics of the "would block" errors.
   // In our code, ERR_IO_PENDING means the whole operation is async, while
   // errSSLWouldBlock means that the stream isn't ending (and is often returned
   // along with partial data). So even though "would block" is returned, if we
-  // have data, let's just return it.
-
+  // have data, let's just return it. This is further complicated by the fact
+  // that errSSLWouldBlock is also used to short-circuit SSLRead()'s
+  // transparent renegotiation, so that we can update our state machine above,
+  // which otherwise would get out of sync with the SSLContextRef's internal
+  // state machine.
   if (processed > 0)
     return processed;
 
@@ -1183,15 +1068,6 @@ int SSLClientSocketMac::DoPayloadRead() {
       // uncleanly, a potential truncation attack.  See http://crbug.com/18586.
       return OK;
 
-    case errSSLServerAuthCompleted:
-    case errSSLClientCertRequested:
-      // Server wants to renegotiate, probably to ask for a client cert,
-      // but SecureTransport doesn't support renegotiation so we have to close.
-      DCHECK(!ssl_config_.send_client_cert);
-      // Tell my caller the server wants a client cert so it can reconnect.
-      SSL_LOG << "Server renegotiating; assuming it wants a client cert...";
-      return ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
-
     default:
       return NetErrorFromOSStatus(status);
   }
@@ -1214,6 +1090,43 @@ int SSLClientSocketMac::DoPayloadWrite() {
   return NetErrorFromOSStatus(status);
 }
 
+int SSLClientSocketMac::DoCompletedRenegotiation(int result) {
+  // The user had a read in progress, which was usurped by the renegotiation.
+  // Restart the read sequence.
+  next_handshake_state_ = STATE_COMPLETED_HANDSHAKE;
+  if (result != OK)
+    return result;
+  return DoPayloadRead();
+}
+
+void SSLClientSocketMac::DidCompleteRenegotiation() {
+  DCHECK(!user_connect_callback_);
+  renegotiating_ = false;
+  next_handshake_state_ = STATE_COMPLETED_RENEGOTIATION;
+}
+
+int SSLClientSocketMac::DidCompleteHandshake() {
+  DCHECK(!server_cert_ || renegotiating_);
+  SSL_LOG << "Handshake completed, next verify cert";
+
+  scoped_refptr<X509Certificate> new_server_cert =
+      GetServerCert(ssl_context_);
+  if (!new_server_cert)
+    return ERR_UNEXPECTED;
+
+  if (renegotiating_ &&
+      X509Certificate::IsSameOSCert(server_cert_->os_cert_handle(),
+                                    new_server_cert->os_cert_handle())) {
+    // We already verified the server certificate.  Either it is good or the
+    // user has accepted the certificate error.
+    DidCompleteRenegotiation();
+  } else {
+    server_cert_ = new_server_cert;
+    next_handshake_state_ = STATE_VERIFY_CERT;
+  }
+  return OK;
+}
+
 // static
 OSStatus SSLClientSocketMac::SSLReadCallback(SSLConnectionRef connection,
                                              void* data,
@@ -1230,6 +1143,32 @@ OSStatus SSLClientSocketMac::SSLReadCallback(SSLConnectionRef connection,
     *data_length = 0;
     return errSSLWouldBlock;
   }
+  if (us->completed_handshake()) {
+    // The state machine for SSLRead, located in libsecurity_ssl's
+    // sslTransport.c, will attempt to fully complete the renegotiation
+    // transparently in SSLRead once it reads the server's HelloRequest
+    // message. In order to make sure that the server certificate is
+    // (re-)verified and that any other parameters are logged (eg:
+    // certificate request state), we try to detect that the
+    // SSLClientSocketMac's state machine is out of sync with the
+    // SSLContext's. When that happens, we break out by faking
+    // errSSLWouldBlock, and set a flag so that DoPayloadRead() knows that
+    // it's not actually blocked. DoPayloadRead() will then restart the
+    // handshake state machine, and finally resume the original Read()
+    // once it successfully completes, similar to the behaviour of
+    // SSLClientSocketWin's DoDecryptPayload() and DoLoop() behave.
+    SSLSessionState state;
+    OSStatus status = SSLGetSessionState(us->ssl_context_, &state);
+    if (status) {
+      *data_length = 0;
+      return status;
+    }
+    if (state == kSSLHandshake) {
+      *data_length = 0;
+      us->renegotiating_ = true;
+      return errSSLWouldBlock;
+    }
+  }
 
   size_t total_read = us->recv_buffer_.size();
 
diff --git a/net/socket/ssl_client_socket_mac.h b/net/socket/ssl_client_socket_mac.h
index 05b9735..00438fc 100644
--- a/net/socket/ssl_client_socket_mac.h
+++ b/net/socket/ssl_client_socket_mac.h
@@ -58,11 +58,12 @@ class SSLClientSocketMac : public SSLClientSocket {
   virtual bool SetSendBufferSize(int32 size);
 
  private:
+  bool completed_handshake() const {
+    return next_handshake_state_ == STATE_COMPLETED_HANDSHAKE;
+  }
   // Initializes the SSLContext.  Returns a net error code.
   int InitializeSSLContext();
 
-  OSStatus EnableBreakOnAuth(bool enabled);
-
   void DoConnectCallback(int result);
   void DoReadCallback(int result);
   void DoWriteCallback(int result);
@@ -74,11 +75,13 @@ class SSLClientSocketMac : public SSLClientSocket {
 
   int DoPayloadRead();
   int DoPayloadWrite();
-  int DoHandshakeStart();
+  int DoHandshake();
   int DoVerifyCert();
   int DoVerifyCertComplete(int result);
-  int DoHandshakeFinish();
-  void HandshakeFinished();
+  int DoCompletedRenegotiation(int result);
+
+  void DidCompleteRenegotiation();
+  int DidCompleteHandshake();
 
   int SetClientCert();
 
@@ -111,10 +114,21 @@ class SSLClientSocketMac : public SSLClientSocket {
 
   enum State {
     STATE_NONE,
-    STATE_HANDSHAKE_START,
+    STATE_HANDSHAKE,
     STATE_VERIFY_CERT,
     STATE_VERIFY_CERT_COMPLETE,
-    STATE_HANDSHAKE_FINISH,
+    STATE_COMPLETED_RENEGOTIATION,
+    STATE_COMPLETED_HANDSHAKE,
+    // After the handshake, the socket remains in the
+    // STATE_COMPLETED_HANDSHAKE state until renegotiation is requested by
+    // the server. When renegotiation is requested, the state machine
+    // restarts at STATE_HANDSHAKE, advances through to
+    // STATE_VERIFY_CERT_COMPLETE, and then continues to
+    // STATE_COMPLETED_RENEGOTIATION. After STATE_COMPLETED_RENEGOTIATION
+    // has been processed, it goes back to STATE_COMPLETED_HANDSHAKE and
+    // will remain there until the server requests renegotiation again.
+    // During the initial handshake, STATE_COMPLETED_RENEGOTIATION is
+    // skipped.
   };
   State next_handshake_state_;
 
@@ -122,8 +136,9 @@ class SSLClientSocketMac : public SSLClientSocket {
   scoped_ptr<CertVerifier> verifier_;
   CertVerifyResult server_cert_verify_result_;
 
-  bool completed_handshake_;
-  bool handshake_interrupted_;
+  // The initial handshake has already completed, and the current handshake
+  // is server-initiated renegotiation.
+  bool renegotiating_;
   bool client_cert_requested_;
   SSLContextRef ssl_context_;