Make OpenSSL UpdateServerCert() OS independent.

net/socket/ssl_client_socket_openssl.cc

Index: net/socket/ssl_client_socket_openssl.cc
diff --git a/net/socket/ssl_client_socket_openssl.cc b/net/socket/ssl_client_socket_openssl.cc
index b253dfeae50884859e2dceb3657363d2a6834b29..71465b8165783d71fb847ea32a51a64b1366e62a 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -319,6 +319,110 @@ class SSLClientSocketOpenSSL::SSLContext {
   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 = {
@@ -347,6 +451,8 @@ SSLClientSocketOpenSSL::SSLClientSocketOpenSSL(
       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),
@@ -362,8 +468,7 @@ SSLClientSocketOpenSSL::SSLClientSocketOpenSSL(
       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();
@@ -917,25 +1022,23 @@ void SSLClientSocketOpenSSL::DoConnectCallback(int 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();
 }