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..3f34e29cb3904cbc1f50aa8078cc12312c03abb0 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -926,6 +926,7 @@ X509Certificate* SSLClientSocketOpenSSL::UpdateServerCert() {
     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_);
@@ -935,8 +936,39 @@ X509Certificate* SSLClientSocketOpenSSL::UpdateServerCert() {
       intermediates.push_back(sk_X509_value(chain, i));
   }
   server_cert_ = X509Certificate::CreateFromHandle(cert.get(), intermediates);
-  DCHECK(server_cert_.get());
+#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();
 }