Mac: Ignoring optional client-cert requests from server

net/socket/ssl_client_socket_mac.cc

 // Copyright (c) 2008-2009 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_mac.h"
 
 #include <CoreServices/CoreServices.h>
 #include <netdb.h>
 #include <sys/socket.h>
 #include <sys/types.h>
@@ skipping 80 matching lines @@
 // value indicates an error, a zero return value indicates end-of-stream (for
 // reads), and a positive return value indicates the number of bytes read or
 // written. Thus, many functions start off with |if (result < 0) return
 // result;|. That gets the error condition out of the way, and from that point
 // forward the result can be treated as a length.
 
 namespace net {
 
 namespace {
 
+// You can change this to LOG(WARNING) during development.
+#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,
@@ skipping 431 matching lines @@
   return rv;
 }
 
 void SSLClientSocketMac::Disconnect() {
   completed_handshake_ = false;
 
   if (ssl_context_) {
     SSLClose(ssl_context_);
     SSLDisposeContext(ssl_context_);
     ssl_context_ = NULL;
+    SSL_LOG << "----- Disposed SSLContext";
   }
 
   // Shut down anything that may call us back.
   verifier_.reset();
   transport_->Disconnect();
 }
 
 bool SSLClientSocketMac::IsConnected() const {
   // Ideally, we should also check if we have received the close_notify alert
   // message from the server, and return false in that case.  We're not doing
@@ skipping 81 matching lines @@
   // security info
   SSLCipherSuite suite;
   OSStatus status = SSLGetNegotiatedCipher(ssl_context_, &suite);
   if (!status)
     ssl_info->security_bits = KeySizeOfCipherSuite(suite);
 }
 
 void SSLClientSocketMac::GetSSLCertRequestInfo(
     SSLCertRequestInfo* cert_request_info) {
   // I'm being asked for available client certs (identities).
+
+  CFArrayRef allowed_issuer_names = NULL;
+  if (SSLCopyDistinguishedNames(ssl_context_, &allowed_issuer_names) == noErr &&
+      allowed_issuer_names != NULL) {
+    SSL_LOG << "Server has " << CFArrayGetCount(allowed_issuer_names)
+        << " allowed issuer names";

[wtc, 2010/03/16 00:42:48]

Nit: please align "<<" with the first "<<" on the previous
line.  Please make the same change to lines 672 and 1057
below.

+    CFRelease(allowed_issuer_names);
+    // TODO(snej): Filter GetSSLClientCertificates using this array.
+  }
+
   cert_request_info->host_and_port = hostname_;
   cert_request_info->client_certs.clear();
   X509Certificate::GetSSLClientCertificates(hostname_,
                                             &cert_request_info->client_certs);
+  SSL_LOG << "Asking user to choose between "
+      << cert_request_info->client_certs.size() << " client certs...";
 }
 
 SSLClientSocket::NextProtoStatus
 SSLClientSocketMac::GetNextProto(std::string* proto) {
   proto->clear();
   return kNextProtoUnsupported;
 }
 
 int SSLClientSocketMac::InitializeSSLContext() {
+  SSL_LOG << "----- InitializeSSLContext";
   OSStatus status = noErr;
 
   status = SSLNewContext(false, &ssl_context_);
   if (status)
     return NetErrorFromOSStatus(status);
 
   status = SSLSetProtocolVersionEnabled(ssl_context_,
                                         kSSLProtocol2,
                                         ssl_config_.ssl2_enabled);
   if (status)
@@ skipping 56 matching lines @@
   // 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.
   static SSLSetSessionOptionFuncPtr ssl_set_session_options =
       LookupFunction<SSLSetSessionOptionFuncPtr>(CFSTR("com.apple.security"),
                                                  CFSTR("SSLSetSessionOption"));
   if (ssl_set_session_options && !ssl_config_.send_client_cert) {
     status = ssl_set_session_options(ssl_context_,
                                      kSSLSessionOptionBreakOnServerAuth,
                                      true);
-    if (!status)
+    if (!status) {
+      SSL_LOG << "Setting kSSLSessionOptionBreakOnCertRequested";
       status = ssl_set_session_options(ssl_context_,
                                        kSSLSessionOptionBreakOnCertRequested,
                                        true);
+    }
     if (status)
       return NetErrorFromOSStatus(status);
 
     // 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_->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 {
+  } else if (ssl_config_.send_client_cert) {
     // If I have a cert, set it up-front, otherwise the server may try to get
     // it later by renegotiating, which SecureTransport doesn't support well.
+    SSL_LOG << "Setting client cert in advance because send_client_cert is set";
     status = SetClientCert();
     if (status)
       return NetErrorFromOSStatus(status);
   }
 
   return OK;
 }
 
 void SSLClientSocketMac::DoConnectCallback(int rv) {
   DCHECK(rv != ERR_IO_PENDING);
@@ skipping 115 matching lines @@
         rv = ERR_UNEXPECTED;
         NOTREACHED() << "unexpected state";
         break;
     }
   } while (rv != ERR_IO_PENDING && next_handshake_state_ != STATE_NONE);
   return rv;
 }
 
 int SSLClientSocketMac::DoHandshakeStart() {
   OSStatus status = SSLHandshake(ssl_context_);
-  if (status == errSSLWouldBlock)
-    next_handshake_state_ = STATE_HANDSHAKE_START;
 
-  if (status == noErr || status == errSSLServerAuthCompleted) {
-    // 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.
-    next_handshake_state_ = STATE_VERIFY_CERT;
-    if (status == errSSLServerAuthCompleted) {
+  switch (status) {
+    case errSSLWouldBlock:
+      next_handshake_state_ = STATE_HANDSHAKE_START;
+      break;
+
+    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.)
+      next_handshake_state_ = STATE_VERIFY_CERT;
+      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;
 
-  if (status == errSSLClosedGraceful) {
-    // The server unexpectedly closed on us.
-    return ERR_SSL_PROTOCOL_ERROR;
+    case errSSLClientCertRequested:

[wtc, 2010/03/16 00:42:48]

IMPORTANT:

1. Please document that if we get errSSLClientCertRequested,
we won't get errSSLServerAuthCompleted.  This is not obvious
at all.

2. This means we still end up sending the client certificate
before verifying the server certificate.  Can you verify
this?

The only solution I have is:
- Enable breakOnServerAuth and breakOnCertRequested for
  the initial handshake, but disable them after the initial
  handshake is done, so that renegotiation, if any, won't
  be interrupted.
- During initial handshake, when we get
  errSSLClientCertRequested, verify the server certificate
  first.

If you want to try this solution, please check in this CL
first, so that this code review won't drag on for too long.

[Jens Alfke, 2010/03/16 18:50:10]

No, because we always abort the connection in this case (by returning
ERR_SSL_CLIENT_AUTH_CERT_NEEDED.) The unreachable code you had me delete in the
previous round of review took care of the case where we would proceed with this
connection; it deferred sending the client cert till the verification succeeded.
But as things are now we won't ever proceed.

+      SSL_LOG << "Received client cert request in DoHandshakeStart";
+      // If we knew the client cert, we would have set it already in
+      // InitializeSSLContext, instead of asking for this callback.
+      // So tell the caller that we need a client cert.
+      DCHECK(!ssl_config_.send_client_cert);
+      return ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
+
+    case errSSLClosedGraceful:
+      // The server unexpectedly closed on us.
+      return ERR_SSL_PROTOCOL_ERROR;
   }
 
   int net_error = NetErrorFromOSStatus(status);
   if (status == noErr || IsCertificateError(net_error)) {
     server_cert_ = GetServerCert(ssl_context_);
     if (!server_cert_)
       return ERR_UNEXPECTED;
   }
   return net_error;
 }
 
 int SSLClientSocketMac::DoVerifyCert() {
   next_handshake_state_ = STATE_VERIFY_CERT_COMPLETE;
 
   if (!server_cert_)
     return ERR_UNEXPECTED;
 
+  SSL_LOG << "DoVerifyCert...";
   int flags = 0;
   if (ssl_config_.rev_checking_enabled)
     flags |= X509Certificate::VERIFY_REV_CHECKING_ENABLED;
   if (ssl_config_.verify_ev_cert)
     flags |= X509Certificate::VERIFY_EV_CERT;
   verifier_.reset(new CertVerifier);
   return verifier_->Verify(server_cert_, hostname_, flags,
                            &server_cert_verify_result_,
                            &handshake_io_callback_);
 }
 
 int SSLClientSocketMac::DoVerifyCertComplete(int result) {
   DCHECK(verifier_.get());
   verifier_.reset();
 
+  SSL_LOG << "...DoVerifyCertComplete (result=" << result << ")";
   if (IsCertificateError(result) && ssl_config_.IsAllowedBadCert(server_cert_))
     result = OK;
 
   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);
   }
 
   return result;
 }
 
 int SSLClientSocketMac::SetClientCert() {
   if (!ssl_config_.send_client_cert || !ssl_config_.client_cert)
     return noErr;
 
   scoped_cftyperef<CFArrayRef> cert_refs(
       ssl_config_.client_cert->CreateClientCertificateChain());
+  SSL_LOG << "SSLSetCertificate(" << CFArrayGetCount(cert_refs) << " certs)";
   OSStatus result = SSLSetCertificate(ssl_context_, cert_refs);
   if (result)
     LOG(ERROR) << "SSLSetCertificate returned OSStatus " << result;
   return result;
 }
 
 int SSLClientSocketMac::DoHandshakeFinish() {
   OSStatus status = SSLHandshake(ssl_context_);
 
   switch (status) {
     case errSSLWouldBlock:
       next_handshake_state_ = STATE_HANDSHAKE_FINISH;
       break;
     case errSSLClientCertRequested:
-      status = SetClientCert();
-      next_handshake_state_ = STATE_HANDSHAKE_FINISH;
-      break;
+      SSL_LOG << "Server requested client cert (DoHandshakeFinish)";
+      // If we knew the client cert, we would have set it already in
+      // InitializeSSLContext, instead of asking for this callback.
+      // So tell the caller that we need a client cert.
+      DCHECK(!ssl_config_.send_client_cert);
+      return ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
     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_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;
+        }
+        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)";
       completed_handshake_ = true;
       DCHECK(next_handshake_state_ == STATE_NONE);
       break;
     default:
       break;
   }
 
   return NetErrorFromOSStatus(status);
 }
 
@@ skipping 18 matching lines @@
       // TODO(wtc): Unless we have received the close_notify alert, we need to
       // return an error code indicating that the SSL connection ended
       // 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.
       // Tell my caller the server wants a client cert so it can reconnect.
-      LOG(INFO) << "Server renegotiating for client cert; restarting...";
+      SSL_LOG << "Server renegotiating; assuming it wants a client cert...";
       return ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
 
     default:
       return NetErrorFromOSStatus(status);
   }
 }
 
 int SSLClientSocketMac::DoPayloadWrite() {
   size_t processed = 0;
   OSStatus status = SSLWrite(ssl_context_,
@@ skipping 104 matching lines @@
   if (rv < 0 && rv != ERR_IO_PENDING) {
     us->write_io_buf_ = NULL;
     return OSStatusFromNetError(rv);
   }
 
   // always lie to our caller
   return noErr;
 }
 
 }  // namespace net