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

[Ryan Sleevi, 2014/02/21 22:34:38]

So, this is wrong :)

Compare with the NSS implementation - the server cert can change in a
renegotiation.

Note that for NSS, we track _both_ the 'socket' cert (eg: PeerCertificateChain
"server_cert_chain") and the OS cert (eg: X509Certificate). We should be
adopting the same model here.

[haavardm, 2014/02/24 18:54:31]

Ah, ok. I tried to do this fix without changing the existing behavior.
Do you mean I Should try to add a Openssl version of  PeerCertificateChain() in
this patch or should that be added later?

[Ryan Sleevi, 2014/02/24 19:02:47]

Yeah, but we can fix this while we're having to (somewhat rewrite) this function
Adding it now.

 
   crypto::ScopedOpenSSL<X509, X509_free> cert(SSL_get_peer_certificate(ssl_));

[haavardm, 2014/02/25 16:13:01]

Is this needed? I suddenly realized that cert.get() is the same cert as the
first certificate in the stack returned by SSL_get_peer_cert_chain(ssl_).

I had a look at the OpenSSL documentation, which seems to say that if
SSL_get_peer_cert_chain is used on client side, the first certificate  is the
server cert.

https://www.openssl.org/docs/ssl/SSL_get_peer_cert_chain.html:
"If called on the client side, the stack also contains the peer's certificate"

   if (!cert.get()) {
     LOG(WARNING) << "SSL_get_peer_certificate returned NULL";
     return NULL;
   }
 
+#if defined(USE_OPENSSL)
   // 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)
       intermediates.push_back(sk_X509_value(chain, i));
   }
   server_cert_ = X509Certificate::CreateFromHandle(cert.get(), intermediates);
+#else
+  unsigned char* cert_data = NULL;
+  int cert_data_length = i2d_X509(cert.get(), &cert_data);
+  if (cert_data_length <= 0 || !cert_data) {
+    return NULL;
+  }

[Ryan Sleevi, 2014/02/21 22:34:38]

net/ style: Don't include braces on single-line conditionals (eg: look at line
935-936 or 920-921)

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

Done.

+  std::vector<std::string> cert_chain;
+  cert_chain.push_back(
+      std::string(reinterpret_cast<char*>(cert_data), cert_data_length));
+  OPENSSL_free(cert_data);

[Ryan Sleevi, 2014/02/21 22:34:38]

Rather than copying all of these to strings, then constructing string pieces, we
should do a single allocation (using the OPENSSL memory functions), create
string pieces from them, then free with OPENSSL_free afterwards (using the
scoped_ptr<> or the like to set an appropriate free proc)

[haavardm, 2014/02/24 18:54:31]

Right. Just to clear up one detail: I assume you mean one OPENSSL memory
allocation per certificate der string?

[Ryan Sleevi, 2014/02/24 19:02:47]

I'm not sure I grokked your clarification.

What I mean is, instead of
i2d_X509 (one memory allocation)
std::string ctor (one memory allocation)
OPENSSL_free (one memory deallocation)
base::StringPiece (zero memory allocation)
std::string dtor (one memory deallocation)

(aka 2 allocations / 2 deallocations per cert)

You re-arrange it to

i2d_X509 (one memory allocation)
base::StringPiece
scoped_ptr<> dtor (one memory deallocation / OpenSSL helper)

for 1 allocation/cert

[haavardm, 2014/02/24 20:47:00]

That's how I understood it yes.

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

Done.

+
+  STACK_OF(X509)* chain = SSL_get_peer_cert_chain(ssl_);
+  if (chain) {
+    for (int i = 0; i < sk_X509_num(chain); ++i) {
+      cert_data = NULL;
+      cert_data_length = i2d_X509(sk_X509_value(chain, i), &cert_data);
+      if (cert_data_length <= 0 || !cert_data) {
+        return NULL;
+      }
+      cert_chain.push_back(
+          std::string(reinterpret_cast<char*>(cert_data), cert_data_length));

[wtc, 2014/02/25 01:13:09]

If you directly push a base::StringPiece to chain_ref here:

    chain_ref.push_back(base::StringPiece(reinterpret_cast<char*>(cert_data),
cert_data_length));

then after you are done with chain_ref, you can iterate over it and call
OPENSSL_free:

    for (size_t i = 0; i < chain_ref.size();i++)
        OPENSSL_free(chain_ref[i].data());

You'll also need to do this before the early return on line 956.

I don't know how to free the memory more elegantly using scoped_ptr<>.

[haavardm, 2014/02/25 16:13:01]

For that to work one must actually using C-type cast, which seems a bit ugly: 
OPENSSL_free((unsigned char*)char_ref[i].data());

Another solution is to keep two vectors, one with scoped_ptr<unsigned char,
opensslfree> objects, and one with base::StringPiece to keep track of the string
length. Or perhaps simply create a new class.

On 2014/02/25 01:13:09, wtc wrote:

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

Done.

+      OPENSSL_free(cert_data);
+    }
+  }
+
+  std::vector<base::StringPiece> chain_ref;
+  for (size_t i = 0; i < cert_chain.size();i++) {
+    chain_ref.push_back(base::StringPiece(cert_chain[0]));

[haavardm, 2014/02/24 18:54:31]

Ouch, bad typo right there (cert_chain[0]). Will fix if that code is still there
after the other fixes is done. Is the missing OpenSSL server socket the reason
this was not caught by the unittests?  

[Ryan Sleevi, 2014/02/24 19:02:47]

No. There's no test that covers this. The CertVerifyProcTest.VerifyReturn* tests
only test the CertVerifier, and the
SSLClientSocketTest.VerifyReturnChainProperlyOrdered only does a mapping on the
server cert, not on the additional certs.

We don't have any tests that do 1:1 mapping of the server certs to returned,
since our cert verifier code has to assume it may only get the server cert in
the future.

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

Done.

+  }
+  server_cert_ = X509Certificate::CreateFromDERCertChain(chain_ref);
+#endif  // USE_OPENSSL
+
   DCHECK(server_cert_.get());

[Ryan Sleevi, 2014/02/21 22:34:38]

This DCHECK is going to have to go when adding !OpenSSL support, as server_cert_
may be NULL now (because the underlying OS can't represent it - eg: Windows and
Mac will choke parsing dates too far in the future)

[haavardm, 2014/02/24 18:54:31]

Ok. I'll move it into the USE_OPENSSL code.

[Ryan Sleevi, 2014/02/24 19:02:47]

I was saying to remove it entirely.

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

Done.

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