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

[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 = {
         &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)),

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

[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();
 }
 
 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 468 matching lines @@
     *outlen = ssl_config_.next_protos[0].size();
   }
 
   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;
 }
 
+const scoped_refptr<X509Certificate>
+SSLClientSocketOpenSSL::GetUnverifiedServerCertificate() const {
+  return server_cert_;
+}
+
 }  // namespace net