Make OpenSSL UpdateServerCert() OS independent.

net/socket/ssl_client_socket_openssl.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.
 
 // OpenSSL binding for SSLClientSocket. The class layout and general principle
 // of operation is derived from SSLClientSocketNSS.
 
 #include "net/socket/ssl_client_socket_openssl.h"
 
 #include <openssl/err.h>
@@ skipping 301 matching lines @@
 
   // This is the index used with SSL_get_ex_data to retrieve the owner
   // SSLClientSocketOpenSSL object from an SSL instance.
   int ssl_socket_data_index_;
 
   crypto::ScopedOpenSSL<SSL_CTX, SSL_CTX_free> ssl_ctx_;
   // |session_cache_| must be destroyed before |ssl_ctx_|.
   SSLSessionCacheOpenSSL session_cache_;
 };
 
+// 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 PeerCertificateChain {
+ public:
+  explicit PeerCertificateChain(SSL* ssl);
+  PeerCertificateChain(const PeerCertificateChain& other);
+  ~PeerCertificateChain();
+  PeerCertificateChain& operator=(const PeerCertificateChain& other);
+
+  void Reset(SSL* ssl);
+  const scoped_refptr<X509Certificate>& AsNativeChain() const {
+    return native_chain_;
+  }
+
+  size_t size() const {
+    if (!openssl_chain_.get())
+      return 0;
+    return sk_X509_num(openssl_chain_.get());
+  }
+
+  // Returns the certificate chain as presented by server.
+  X509* operator[](size_t index) const {
+    DCHECK_LT(index, size());
+    return sk_X509_value(openssl_chain_.get(), index);
+  }
+
+ private:
+  static void FreeX509Stack(STACK_OF(X509) * chain) {
+    sk_X509_pop_free(chain, X509_free);
+  }
+  crypto::ScopedOpenSSL<STACK_OF(X509), FreeX509Stack> openssl_chain_;
+
+  scoped_refptr<X509Certificate> native_chain_;
+};
+
+PeerCertificateChain::PeerCertificateChain(SSL* ssl) { Reset(ssl); }
+PeerCertificateChain::~PeerCertificateChain() {};
+
+PeerCertificateChain::PeerCertificateChain(const PeerCertificateChain& other) {
+  *this = other;
+}
+
+PeerCertificateChain& PeerCertificateChain::operator=(
+    const PeerCertificateChain& other) {
+  if (this == &other)
+    return *this;
+
+  // native_chain is reference counted by scoped_refptr;
+  native_chain_ = other.native_chain_;
+
+  // Must increase the reference count manually for sk_X509_dup
+  openssl_chain_.reset(sk_X509_dup(other.openssl_chain_.get()));
+  for (int i = 0; i < sk_X509_num(openssl_chain_.get()); ++i) {
+    X509* x = sk_X509_value(openssl_chain_.get(), i);
+    CRYPTO_add(&x->references, 1, CRYPTO_LOCK_X509);
+  }
+  return *this;
+}
+
+void PeerCertificateChain::Reset(SSL* ssl) {
+  openssl_chain_.reset(NULL);
+  native_chain_ = NULL;
+
+  if (ssl == NULL)
+    return;
+
+  STACK_OF(X509)* chain = SSL_get_peer_cert_chain(ssl);
+  if (!chain)
+    return;
+
+  // sk_X509_dup does not increase reference count on the certs in the stack.
+  openssl_chain_.reset(sk_X509_dup(chain));
+
+  std::vector<base::StringPiece> der_chain;
+  for (int i = 0; i < sk_X509_num(openssl_chain_.get()); ++i) {
+    X509* x = sk_X509_value(openssl_chain_.get(), i);
+
+    // We increase reference count for the certs in openssl_chain_.
+    CRYPTO_add(&x->references, 1, CRYPTO_LOCK_X509);
+
+    unsigned char* cert_data = NULL;
+    int cert_data_length = i2d_X509(x, &cert_data);
+    if (cert_data_length && cert_data)
+      der_chain.push_back(base::StringPiece(reinterpret_cast<char*>(cert_data),
+                                            cert_data_length));
+  }
+
+  native_chain_ = X509Certificate::CreateFromDERCertChain(der_chain);
+
+  for (size_t i = 0; i < der_chain.size(); ++i) {
+    OPENSSL_free((void*)der_chain[i].data());

[Ryan Sleevi, 2014/02/27 03:07:29]

You shouldn't need to cast to void* here. If you do, C++ style.

[haavardm, 2014/02/27 10:46:28]

chain[i].data() is 'const char*' while OPENSSL_Free takes non-const 'void*' . 

I don't see how this can be done without casting, but I'll can use
const_cast<char*> instead. (something I do so rarely I frankly forgot it
existed)

[wtc, 2014/02/27 17:54:56]

Yes, this requires a cast, and const_cast<char*> is the right cast to use.

[haavardm, 2014/03/11 18:43:21]

Done.

+  }
+
+  if (!native_chain_->os_cert_handle() ||
+      native_chain_->GetIntermediateCertificates().size() + 1 !=
+          static_cast<size_t>(sk_X509_num(openssl_chain_.get()))) {

[Ryan Sleevi, 2014/02/27 03:07:29]

Pathological case: What happens when .size() == numeric_limits<size_t>::max()

Also, what are you trying to test here? The goal with testing der_chain is just
to make sure that all the i2d_X509 succeeded - which should 'never fail'. The
conversion to native certs "may" fail - but thats ok. 

[haavardm, 2014/02/27 10:46:28]

I think we would have a crash or freeze long before that happens. But I don't
see that +1 makes a difference here since the result is size_t anyway. That is
if GetChain().size() existed it would return
GetIntermediateCertificates().size() + 1. 

Will remove anyway, see below
Right.  I thought the point was to test the whole conversion chain
(X509->der->Os).  I forgot that native_chain could 'legally' end up with
different chain size after sorting out the real chain. I followed your example
which used native_chain. I guess that was a typo, and I'll put in the der_chain
instead (which actually has the size() function). That would test that all
i2d_X509 are successful.

[haavardm, 2014/03/11 18:43:21]

Done.

+    openssl_chain_.reset(NULL);
+    native_chain_ = NULL;
+  }
+}
+;
+
 // static
 SSLSessionCacheOpenSSL::Config
     SSLClientSocketOpenSSL::SSLContext::kDefaultSessionCacheConfig = {
         &GetSessionCacheKey,  // key_func
         1024,                 // max_entries
         256,                  // expiration_check_count
         60 * 60,              // timeout_seconds
 };
 
 // static
 void SSLClientSocket::ClearSessionCache() {
   SSLClientSocketOpenSSL::SSLContext* context =
       SSLClientSocketOpenSSL::SSLContext::GetInstance();
   context->session_cache()->Flush();
   OpenSSLClientKeyStore::GetInstance()->Flush();
 }
 
 SSLClientSocketOpenSSL::SSLClientSocketOpenSSL(
     scoped_ptr<ClientSocketHandle> transport_socket,
     const HostPortPair& host_and_port,
     const SSLConfig& ssl_config,
     const SSLClientSocketContext& context)
     : transport_send_busy_(false),
       transport_recv_busy_(false),
       transport_recv_eof_(false),
       weak_factory_(this),
       pending_read_error_(kNoPendingReadResult),
       transport_write_error_(OK),
+      server_cert_chain_(new PeerCertificateChain(NULL)),
+      server_cert_(NULL),
       completed_handshake_(false),
       client_auth_cert_needed_(false),
       cert_verifier_(context.cert_verifier),
       server_bound_cert_service_(context.server_bound_cert_service),
       ssl_(NULL),
       transport_bio_(NULL),
       transport_(transport_socket.Pass()),
       host_and_port_(host_and_port),
       ssl_config_(ssl_config),
       ssl_session_cache_shard_(context.ssl_session_cache_shard),
       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()) {
-}
+      net_log_(transport_->socket()->NetLog()) {}
 
 SSLClientSocketOpenSSL::~SSLClientSocketOpenSSL() {
   Disconnect();
 }
 
 void SSLClientSocketOpenSSL::GetSSLCertRequestInfo(
     SSLCertRequestInfo* cert_request_info) {
   cert_request_info->host_and_port = host_and_port_;
   cert_request_info->cert_authorities = cert_authorities_;
 }
@@ skipping 533 matching lines @@
 
 void SSLClientSocketOpenSSL::DoConnectCallback(int rv) {
   if (!user_connect_callback_.is_null()) {
     CompletionCallback c = user_connect_callback_;
     user_connect_callback_.Reset();
     c.Run(rv > OK ? OK : rv);
   }
 }
 
 X509Certificate* SSLClientSocketOpenSSL::UpdateServerCert() {
-  if (server_cert_.get())
-    return server_cert_.get();
-
-  crypto::ScopedOpenSSL<X509, X509_free> cert(SSL_get_peer_certificate(ssl_));
-  if (!cert.get()) {
-    LOG(WARNING) << "SSL_get_peer_certificate returned NULL";
-    return NULL;
-  }
-
+#ifdef USE_OPENSSL
+  server_cert_ = NULL;
   // Unlike SSL_get_peer_certificate, SSL_get_peer_cert_chain does not
   // increment the reference so sk_X509_free does not need to be called.
   STACK_OF(X509)* chain = SSL_get_peer_cert_chain(ssl_);
   X509Certificate::OSCertHandles intermediates;
   if (chain) {
-    for (int i = 0; i < sk_X509_num(chain); ++i)
+    for (int i = 1; i < sk_X509_num(chain); ++i)
       intermediates.push_back(sk_X509_value(chain, i));
+
+    server_cert_ = X509Certificate::CreateFromHandle(sk_X509_value(chain, 0),
+                                                     intermediates);
   }
-  server_cert_ = X509Certificate::CreateFromHandle(cert.get(), intermediates);
-  DCHECK(server_cert_.get());
+#else
+  server_cert_chain_->Reset(ssl_);
+  server_cert_ = server_cert_chain_->AsNativeChain();
+#endif
 
   return server_cert_.get();
 }
 
 void SSLClientSocketOpenSSL::OnHandshakeIOComplete(int result) {
   int rv = DoHandshakeLoop(result);
   if (rv != ERR_IO_PENDING) {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_SSL_CONNECT, rv);
     DoConnectCallback(rv);
   }
@@ skipping 469 matching lines @@
   }
 
   npn_proto_.assign(reinterpret_cast<const char*>(*out), *outlen);
   server_protos_.assign(reinterpret_cast<const char*>(in), inlen);
   DVLOG(2) << "next protocol: '" << npn_proto_ << "' status: " << npn_status_;
 #endif
   return SSL_TLSEXT_ERR_OK;
 }
 
 }  // namespace net