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..77bad4ba29636112f9a5eed95c263c9272510561 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -319,6 +319,108 @@ 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_;

[wtc, 2014/03/10 21:45:34]

Nit: it's not clear what "native" means here.

I would think OpenSSL is more "native" than Chromium's X509Certificate is.

Update: I now understand what you meant by "native". Please put the entire
PeerCertificateChain class inside #if !defined(USE_OPENSSL). A comment that
point out that in this case (USE_OPENSSL is not defined) X509Certificate
contains the native OS certificate handles would still be helpful.

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

For consistency and possible future use of this function, it might be best to
leave this as is but rename it to OSCertChain(). See comment at line 1037.

On 2014/03/10 21:45:34, wtc wrote:

+  }
+
+  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.

[wtc, 2014/03/10 21:45:34]

This comment seems wrong; it doesn't seem to describe the [] operator.

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

Done.

+  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) {

[wtc, 2014/03/10 21:45:34]

Nit: delete the space before '*'.

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

Done.

+    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(const_cast<char*>(der_chain[i].data()));
+  }
+
+  if (der_chain.size() !=
+      static_cast<size_t>(sk_X509_num(openssl_chain_.get()))) {

[wtc, 2014/03/10 21:45:34]

Just to check my understanding: this can only happen if the check on line 406
inside the first for loop fails, right? Otherwise, |der_chain| and
|openssl_chain_| should be of the same size.

[Ryan Sleevi, 2014/03/11 00:15:15]

Correct

+    openssl_chain_.reset(NULL);
+    native_chain_ = NULL;
+  }
+}
+
 // static
 SSLSessionCacheOpenSSL::Config
     SSLClientSocketOpenSSL::SSLContext::kDefaultSessionCacheConfig = {
@@ -347,6 +449,8 @@ SSLClientSocketOpenSSL::SSLClientSocketOpenSSL(
       weak_factory_(this),
       pending_read_error_(kNoPendingReadResult),
       transport_write_error_(OK),
+      server_cert_chain_(new PeerCertificateChain(NULL)),

[wtc, 2014/03/10 21:45:34]

Is it necessary to create an empty PeerCertificateChain object?

If not, this initializer can be omitted.

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

That was just to avoid having to check for NULL and create it when used at line
1037.

+      server_cert_(NULL),

[wtc, 2014/03/10 21:45:34]

Nit: we usually don't initialize a scoped_refptr to NULL like this. The
scoped_refptr constructor that takes no argument initializes the internal
pointer to NULL automatically.

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

Done.

       completed_handshake_(false),
       client_auth_cert_needed_(false),
       cert_verifier_(context.cert_verifier),
@@ -362,8 +466,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 +1020,23 @@ void SSLClientSocketOpenSSL::DoConnectCallback(int rv) {
 }
 
 X509Certificate* SSLClientSocketOpenSSL::UpdateServerCert() {
-  if (server_cert_.get())
-    return server_cert_.get();

[wtc, 2014/03/10 21:45:34]

Please confirm that this "one-time initialization" check is not needed.

-
-  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

[wtc, 2014/03/10 21:45:34]

NitL: the Chromium coding style recommends always using the more verbose #if
defined(FOO). The reason is that it can be more easily extended with other ||
defined(BAR) if necessary.

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

Done.

+  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.

[wtc, 2014/03/10 21:45:34]

Nit: I think the "Unlike SSL_get_peer_certificate, " part can be removed from
this comment.

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

Done.

   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();

[wtc, 2014/03/10 21:45:34]

IMPORTANT: comparing the two implementations of this method, I see one
difference: the first implementation sets server_cert_ but does not set
server_cert_chain_. Is this a bug?

If this is not a bug, it seems to imply that in the second implementation,
server_cert_chain_ just needs to be a local variable rather than a class member.
If so, this will also solve the problem of the forward declaration of
PeerCertificateChain in ssl_client_socket_openssl.h.

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

The difference is that the first implementation does not compile when
OSCertHandle is anything else than OpenSSL's X509 structure. This is not
portable to mac and windows. The only reason for keeping it is to avoid
converting back and forth two times between X509 structure and der when
USE_OPENSSL is defined.

However, the ifdef was added before adding the server_cert_chain_ logic. You
have a good point now that the implementation has evolved.

I've removed the first implementation, and instead created a special Reset(SSL*
ssl) for USE_OPENSSL to keep this consistent.


On 2014/03/10 21:45:34, wtc wrote:

+#endif
 
   return server_cert_.get();
 }
@@ -1424,4 +1525,9 @@ int SSLClientSocketOpenSSL::SelectNextProtoCallback(unsigned char** out,
   return SSL_TLSEXT_ERR_OK;
 }
 
+const scoped_refptr<X509Certificate>
+SSLClientSocketOpenSSL::GetUnverifiedServerCertificate() const {
+  return server_cert_;
+}
+
 }  // namespace net