Do not allow the server certificate to change in a renegotiation.

Do not allow the server certificate to change in a renegotiation.

R=agl@chromium.org,rsleevi@chromium.org
BUG=306959
TEST=none

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

[wtc, 2013-10-14 23:11:11]

Please take a look at this proof of concept. I took the simple approach of
requiring a server to send the same certificate in a renegotiation. This is not
easy to test in our current test setup. I tested it manually with an Apache
server.

https://codereview.chromium.org/27266002/diff/1/net/socket/ssl_client_socket_...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/27266002/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:1291: // Disallow the server certificate to
change in a renegotiation.

This can also be enforced by NSS.

[agl, 2013-10-15 15:24:42]

LGTM. I think it would be interesting to float this on the dev channel since
it's something that we might want to do.

[Ryan Sleevi, 2013-10-16 01:17:38]

I can live with trying this out.

The only server implementation I'm aware of that lets you switch certificates in
a renegotiation is Apache (because it allows contexts to be re-configured by
vhosts, forcing a renego - which is how it supports per-dir client certs and
such), but this would be a pretty odd case.

The only time I think it would come up is when doing a (EC)DH static/ephemeral
negotiation, where the cert contained the parameters. That seems so far on the
edge case that we "shouldn't" see any breakage.

[Ryan Sleevi, 2013-10-16 01:17:45]

lgtm

[commit-bot: I haz the power, 2013-10-16 19:24:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/27266002/12001

[commit-bot: I haz the power, 2013-10-16 21:54:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/27266002/12001

[commit-bot: I haz the power, 2013-10-17 01:52:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/27266002/12001

[commit-bot: I haz the power, 2013-10-17 02:54:26]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-10-17 04:47:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/27266002/12001

[commit-bot: I haz the power, 2013-10-17 05:59:07]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-10-17 21:04:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/27266002/12001

[commit-bot: I haz the power, 2013-10-18 01:39:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/27266002/12001

[commit-bot: I haz the power, 2013-10-18 21:32:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/27266002/12001

[commit-bot: I haz the power, 2013-10-19 18:35:38]

Change committed as 229611