OpenSSL: don't allow the server certificate to change during renegotiation.

OpenSSL: don't allow the server certificate to change during renegotiation.

This mirrors r229611, but for OpenSSL.

BUG=306959
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254022

[wtc, 2014-02-27 22:12:07]

Patch set 2 LGTM. Please wait for Ryan's approval because I'm not that familiar
with some OpenSSL functions you used.

https://codereview.chromium.org/177143004/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/177143004/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:303: static int
CertificateCallback(X509_STORE_CTX *store_ctx, void *arg) {

Nit: also rename this function "CertVerifyCallback".

https://codereview.chromium.org/177143004/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1406: i2d_X509(leaf_cert, &outp);

Nit: should we also check the return value of the second i2d_X509 call, as
defensive programming? (I think it is unlikely to return different value
though.)

One way to do this is (starting from line 1397:

  int len = i2d_X509(leaf_cert, NULL);
  if (len < 0) {
    ...
  }
  scoped_ptr<uint8[]> der_leaf_cert(new uint8[len]);
  uint8 *outp = der_leaf_cert.get();
  len = i2d_X509(leaf_cert, &outp);
  if (static_cast<size_t>(len) == der_current_cert.size() &&
      memcmp(der_leaf_cert.get(),
             ...
             der_current_cert.size()) == 0) {
    // The certificates match so the renegotiation can continue.
    return 1;
  }

  LOG(ERROR) << "Server certificate changed between handshakes";
  return 0;

https://codereview.chromium.org/177143004/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/177143004/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.h:139: int
CertificateCallback(X509_STORE_CTX *store_ctx);

Nit: rename this function "VerifyCertCallback".

[agl, 2014-02-27 22:31:23]

https://codereview.chromium.org/177143004/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/177143004/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:303: static int
CertificateCallback(X509_STORE_CTX *store_ctx, void *arg) {
On 2014/02/27 22:12:07, wtc wrote:
> 
> Nit: also rename this function "CertVerifyCallback".

Done.

https://codereview.chromium.org/177143004/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1406: i2d_X509(leaf_cert, &outp);
On 2014/02/27 22:12:07, wtc wrote:
> Nit: should we also check the return value of the second i2d_X509 call, as
> defensive programming? (I think it is unlikely to return different value
> though.)

That's a fair point. I was trying to avoid running i2d_X509 twice, but since
it's only happening on renegotiations it's not worth worrying about.

Done.

https://codereview.chromium.org/177143004/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/177143004/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.h:139: int
CertificateCallback(X509_STORE_CTX *store_ctx);
On 2014/02/27 22:12:07, wtc wrote:
> 
> Nit: rename this function "VerifyCertCallback".

Done.

[agl, 2014-02-27 22:31:30]

The CQ bit was checked by agl@chromium.org

[commit-bot: I haz the power, 2014-02-27 22:33:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/177143004/30001

[Ryan Sleevi, 2014-02-27 22:43:40]

I still don't fully understand why this is needed - continuing on the email
thread.

The functions used here are fine though.

https://codereview.chromium.org/177143004/diff/30001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/177143004/diff/30001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1413: }
Why do this? Why not simply use

if (!completed_handshake_)
  return 1;

if (X509Certificate::IsSameOSCert(server_cert_->os_cert_handle(),
sk_X509_value(store_ctx->chain, 0)))
  return 1;

return 0;

[agl, 2014-02-27 22:46:06]

The CQ bit was unchecked by agl@chromium.org

[agl, 2014-02-27 22:57:10]

https://codereview.chromium.org/177143004/diff/30001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/177143004/diff/30001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1413: }
On 2014/02/27 22:43:40, Ryan Sleevi wrote:
> if (X509Certificate::IsSameOSCert(server_cert_->os_cert_handle(),
> sk_X509_value(store_ctx->chain, 0)))
>   return 1;

Yep, that works. Thanks!

[agl, 2014-02-27 22:57:13]

The CQ bit was checked by agl@chromium.org

[commit-bot: I haz the power, 2014-02-27 23:04:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/177143004/50001

[commit-bot: I haz the power, 2014-02-28 01:11:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/177143004/50001

[commit-bot: I haz the power, 2014-02-28 02:20:22]

Change committed as 254022