Workaround a bug in NSS when using DHE+client authentication.

When performing a SSL renegotiation handshake, do not send Certificate/CertificateVerify messages unless the peer sends a CertificateRequest, requesting client auth. 

This would happen if the following conditions were true:
 - In the initial/previous handshake, the peer requests client authentication.
 - The client chooses a certificate, versus declining to provide one.
 - A (EC-)DHE cipher suite is negotiated.
 - The peer requests (secure) renegotiation.
 - The peer does NOT request a client certificate during the renegotiated handshake.

R=wtc
BUG=62027
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=68829

[Ryan Sleevi, 2010-12-05 04:28:51]

wtc: Same fix I proposed for upstream NSS. Given the lack of fixed-DH support in
NSS, this seems the most appropriate and expedient way to resolve this problem.
This is also the least-invasive of the solutions I could think of, which I
believe makes it a better candidate-CL to be merged into M9.

Alternatively, I can wait until it's fixed in upstream NSS now that the problem
has been identified.

[wtc, 2010-12-07 02:47:13]

LGTM.  Excellent work!

Let's patch our copy of NSS's libSSL first.

I suggested two potential improvements to your fix in
the upstream NSS bug report.

http://codereview.chromium.org/5611005/diff/1/net/third_party/nss/ssl/ssl3con.c
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/5611005/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:4865: /* Fixed DH is not supported, so the
private key is no longer needed. */
This comment should explain the consequences of not destroying
the client private key here.

Please fix the indentation of the next two lines (lines
4866-4867).

[Ryan Sleevi, 2010-12-07 03:12:15]

Thanks for the feedback Wan-Teh. Per your comments upstream, I moved it into the
ssl3_HandleHelloRequest, as this is what I used primarily to test the issue. My
proposal for the limited modification was primarily driven by unfamiliarity with
the NSS code, so I wanted to play it safe.

I've since gone and looked through for any access to clientPrivateKey,
clientCertificate, and clientCertChain, and it appears that nothing should be
negatively affected.

[wtc, 2010-12-08 03:03:12]

On 2010/12/07 03:12:15, Ryan Sleevi wrote:
> Per your comments upstream, I moved it into the
> ssl3_HandleHelloRequest, as this is what I used primarily to test the issue.

Just to clarify: you (correctly) moved it into ssl3_HandleServerHello,
as opposed to ssl3_HandleHelloRequest.

[wtc, 2010-12-08 03:04:13]

LGTM.

Could you generate a patch and document it in README.chromium.
We should also generate a patch for the platform client auth
change.

http://codereview.chromium.org/5611005/diff/1/net/third_party/nss/ssl/ssl3con.c
File net/third_party/nss/ssl/ssl3con.c (left):

http://codereview.chromium.org/5611005/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:4867: /* If we're doing RSA key exchange,
we're all done with the private key
I think we should still do something here.

It seems that even for fixed DH, we would be done with
the client private key (and client certificate and client
cert chain) here.  We would do the DH key derivation in
ssl3_SendCertificate, when we send the client's fixed DH
certificate. Agreed?

http://codereview.chromium.org/5611005/diff/5001/net/third_party/nss/ssl/ssl3...
net/third_party/nss/ssl/ssl3con.c:5523: /* clean up anything left from previous
handshake. */
You can replace these with PORT_Assert assertions
that all the relevant fields are NULL.

[Ryan Sleevi, 2010-12-08 07:19:34]

Thanks for the review. I've added a patch against NSS_3_12_BRANCH (at least, I
hope I did - this is my first use of CVS) and updated the README.chromium
appropriately.

Regarding ssl3_HandleHelloRequest, yes, that was a typo, it is part of the
ssl3_HandleServerHello, as shown in the patch.

I'll look to land in the next day or two, unless you have any concerns with the
comments, and then attach the patch upstream.

http://codereview.chromium.org/5611005/diff/1/net/third_party/nss/ssl/ssl3con.c
File net/third_party/nss/ssl/ssl3con.c (left):

http://codereview.chromium.org/5611005/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:4867: /* If we're doing RSA key exchange,
we're all done with the private key
On 2010/12/08 03:04:14, wtc wrote:
> I think we should still do something here.
> 
> It seems that even for fixed DH, we would be done with
> the client private key (and client certificate and client
> cert chain) here.  We would do the DH key derivation in
> ssl3_SendCertificate, when we send the client's fixed DH
> certificate. Agreed?

Er, yes, agreed. I'm assuming you meant ssl3_SendClientKeyExchange, rather than
ssl3_SendCertificate, since an empty client key exchange is still sent for fixed
DH. But yes, by the time this code is reached, the premaster secret should
already have been derived (whether from a fixed key or ephemeral), so there's no
point in holding onto the key.

[wtc, 2010-12-08 19:24:41]

LGTM!

http://codereview.chromium.org/5611005/diff/1/net/third_party/nss/ssl/ssl3con.c
File net/third_party/nss/ssl/ssl3con.c (left):

http://codereview.chromium.org/5611005/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:4867: /* If we're doing RSA key exchange,
we're all done with the private key
On 2010/12/08 07:19:35, Ryan Sleevi wrote:
>> Er, yes, agreed. I'm assuming you meant ssl3_SendClientKeyExchange, rather
than
> ssl3_SendCertificate, since an empty client key exchange is still sent for
fixed
> DH.

I didn't know that.  Yes, in that case, we can derive the
premaster secret for fixed DH in ssl3_SendClientKeyExchange.

http://codereview.chromium.org/5611005/diff/16001/net/third_party/nss/ssl/ssl...
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/5611005/diff/16001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl3con.c:5539: PORT_Assert(ss->ssl3.platformClientKey
== NULL);
We may need a (PlatformKey) cast for NULL here.