Do not assume OSCertHandle is OpenSSL's X509 structure.

Do not assume OSCertHandle is OpenSSL's X509 structure.

Instead create a X509Certificate instance from the OpenSSL 
chain via PeerCertificateChain, and compare with the existing
X509Certificate.

BUG=none

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

[haavardm, 2014-03-18 10:51:47]

Some more OpenSSL portable stuff that went under my radar when doing cl
173853014.

[Ryan Sleevi, 2014-03-18 20:47:06]

https://codereview.chromium.org/203323004/diff/1/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/203323004/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1527: // The certificates match so the
renegotiation can continue.
Why not convert the untrusted cert to an X509Certificate/PeerCertificateChain,
and compare against that? Like NSS?

[haavardm, 2014-03-18 21:36:40]

https://codereview.chromium.org/203323004/diff/1/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/203323004/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1527: // The certificates match so the
renegotiation can continue.
On 2014/03/18 20:47:07, Ryan Sleevi wrote:
> Why not convert the untrusted cert to an X509Certificate/PeerCertificateChain,
> and compare against that? Like NSS?

Not sure I understand. If I get this correctly, NSS is actually comparing the
der strings from two CERTCertificate retrieved from PeerCertificateChain and the
socket. As most of the code here is to turn X509 into der I'm not sure it would
simplify this much. 

But I could certainly extend PeerCertificateChain with a function to take in
store_ctx->chain (which would also be called by Reset(ssl) and then compare new
and old X509Certificate objects.

[Ryan Sleevi, 2014-03-18 21:47:51]

On 2014/03/18 21:36:40, Håvard Molland wrote:
>
https://codereview.chromium.org/203323004/diff/1/net/socket/ssl_client_socket...
> File net/socket/ssl_client_socket_openssl.cc (right):
> 
>
https://codereview.chromium.org/203323004/diff/1/net/socket/ssl_client_socket...
> net/socket/ssl_client_socket_openssl.cc:1527: // The certificates match so the
> renegotiation can continue.
> On 2014/03/18 20:47:07, Ryan Sleevi wrote:
> > Why not convert the untrusted cert to an
X509Certificate/PeerCertificateChain,
> > and compare against that? Like NSS?
> 
> Not sure I understand. If I get this correctly, NSS is actually comparing the
> der strings from two CERTCertificate retrieved from PeerCertificateChain and
the
> socket. As most of the code here is to turn X509 into der I'm not sure it
would
> simplify this much. 
> 
> But I could certainly extend PeerCertificateChain with a function to take in
> store_ctx->chain (which would also be called by Reset(ssl) and then compare
new
> and old X509Certificate objects.

Yeah. My feeling is now that we PeerCertChain, take the incoming X509_STORE and
create a stack-allocated PeerCertificateChain, then check if
old_chain.Equals(new_chain_), and if not, bounce the cert.

The related bug is about ensuring certs don't change during verifications.

[wtc, 2014-03-19 00:29:24]

Review comments on patch set 1:

This is the same as patch set 3 in Adam's CL:
    https://codereview.chromium.org/177143004/
so I am fine with this. Ryan should be able to offer better suggestions.

[haavardm, 2014-03-19 10:23:29]

On 2014/03/19 00:29:24, wtc wrote:
> Review comments on patch set 1:
> 
> This is the same as patch set 3 in Adam's CL:
>     https://codereview.chromium.org/177143004/
> so I am fine with this. Ryan should be able to offer better suggestions.

Yes, I grabbed the patch from there as I knew you already approved it.

Followed Ryan's last suggestion though

[wtc, 2014-03-19 18:49:35]

Patch set 2 LGTM.

https://codereview.chromium.org/203323004/diff/2/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/203323004/diff/2/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:335: explicit
PeerCertificateChain(STACK_OF(X509) * chain) { Reset(chain); }

Nit: remove the space before '*'.

https://codereview.chromium.org/203323004/diff/2/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:341: // |chain|, which may be NULL,
indicating to empty the store

Nit: change "supplied by |chain|" to "in |chain|".

https://codereview.chromium.org/203323004/diff/2/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1062: DVLOG(1) << "UpdateServerCert
received invalid certificate from peer";

Nit: add "chain" to the error message?

https://codereview.chromium.org/203323004/diff/2/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1512: LOG(ERROR) << "Server certificate
changed between handshakes";

Nit: consider emitting a different error message if chain.IsValid() is false.

[wtc, 2014-03-19 18:51:19]

Please update the CL's description before you commit this. It contains some
background info that doesn't seem useful in the future.

[haavardm, 2014-03-20 13:56:29]

https://codereview.chromium.org/203323004/diff/2/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/203323004/diff/2/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:335: explicit
PeerCertificateChain(STACK_OF(X509) * chain) { Reset(chain); }
On 2014/03/19 18:49:35, wtc wrote:
> 
> Nit: remove the space before '*'.

Done.

https://codereview.chromium.org/203323004/diff/2/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:341: // |chain|, which may be NULL,
indicating to empty the store
On 2014/03/19 18:49:35, wtc wrote:
> 
> Nit: change "supplied by |chain|" to "in |chain|".

Done.

https://codereview.chromium.org/203323004/diff/2/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1062: DVLOG(1) << "UpdateServerCert
received invalid certificate from peer";
On 2014/03/19 18:49:35, wtc wrote:
> 
> Nit: add "chain" to the error message?

Done.

https://codereview.chromium.org/203323004/diff/2/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1512: LOG(ERROR) << "Server certificate
changed between handshakes";
On 2014/03/19 18:49:35, wtc wrote:
> 
> Nit: consider emitting a different error message if chain.IsValid() is false.

Done.

[haavardm, 2014-03-20 13:56:44]

The CQ bit was checked by haavardm@opera.com

[commit-bot: I haz the power, 2014-03-20 13:57:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haavardm@opera.com/203323004/30001

[commit-bot: I haz the power, 2014-03-20 13:59:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 13:59:12]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[haavardm, 2014-03-20 14:01:51]

The CQ bit was checked by haavardm@opera.com

[commit-bot: I haz the power, 2014-03-20 14:02:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haavardm@opera.com/203323004/30001

[Ryan Sleevi, 2014-03-20 17:11:59]

lgtm

[haavardm, 2014-03-20 17:14:20]

On 2014/03/20 17:11:59, Ryan Sleevi wrote:
> lgtm

Should I have waited for this second lgtm?

[Ryan Sleevi, 2014-03-20 17:26:10]

On 2014/03/20 17:14:20, Håvard Molland wrote:
> On 2014/03/20 17:11:59, Ryan Sleevi wrote:
> > lgtm
> 
> Should I have waited for this second lgtm?

Generally no - but if I initially reviewed a change, I like to make sure I ack
it out of my queue (see "Changes with multiple reviewers" -
http://dev.chromium.org/developers/committers-responsibility )

[commit-bot: I haz the power, 2014-03-20 17:54:20]

Change committed as 258337