When performing an SSL renegotiation and requesting a certificate status, clear any existing certif…

When performing an SSL renegotiation and requesting a certificate status, clear any existing certificate status messages received.

Failure to clear the status results in handshake failures.

BUG=170328
TEST=See bug


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

[Ryan Sleevi, 2013-02-21 03:06:00]

agl: PTAL
wtc: Since you're under the weather, FYI.

I guess this is another example of where I should add renegotiation support to
TLSLite. Without this change, we end up going through the following state cycle
(server messages only):

Socket: 0x22c85c68 {fd=0x228617f8 ops=0x0db3cbd4 opt={...} ...} Message:
server_hello
Socket: 0x22c85c68 {fd=0x228617f8 ops=0x0db3cbd4 opt={...} ...} Message:
certificate
Socket: 0x22c85c68 {fd=0x228617f8 ops=0x0db3cbd4 opt={...} ...} Message:
certificate_status
Socket: 0x22c85c68 {fd=0x228617f8 ops=0x0db3cbd4 opt={...} ...} Message:
server_hello_done
Socket: 0x22c85c68 {fd=0x228617f8 ops=0x0db3cbd4 opt={...} ...} Message:
finished
Socket: 0x22c85c68 {fd=0x228617f8 ops=0x0db3cbd4 opt={...} ...} Message:
hello_request
Socket: 0x22c85c68 {fd=0x228617f8 ops=0x0db3cbd4 opt={...} ...} Message:
server_hello
Socket: 0x22c85c68 {fd=0x228617f8 ops=0x0db3cbd4 opt={...} ...} Message:
certificate
Socket: 0x22c85c68 {fd=0x228617f8 ops=0x0db3cbd4 opt={...} ...} Message:
certificate_status
Socket 0x22c85c68 {fd=0x228617f8 ops=0x0db3cbd4 opt={...} ...} failed
[5708:2868:0220/182240:WARNING:nss_ssl_util.cc(230)] Unknown SSL error -12163
mapped to net::ERR_SSL_PROTOCOL_ERROR

The reason for this is that we don't clear the (previous) cert status data in
ssl3_HandleCertificateStatus after finishing (since we want it to be available
later), so on the renegotiation, we end up hitting the condition on lines
8714-8720 of
http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/ssl/ssl3c...

This goes back to when we first introduced support.

[agl, 2013-02-21 14:57:02]

I wish we could just remove support for renegotiation :(

https://codereview.chromium.org/12327032/diff/1/net/third_party/nss/ssl/ssl3e...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/12327032/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:736: ss->ssl3.hs.may_get_cert_status =
PR_FALSE;
Is ssl3_HandleHelloRequest a better place for this? This fix doesn't really
depend on whether or not we send a StatusRequest extension or not on the
renegotiation.

[Ryan Sleevi, 2013-02-21 18:01:07]

On 2013/02/21 14:57:02, agl wrote:
> I wish we could just remove support for renegotiation :(
> 
>
https://codereview.chromium.org/12327032/diff/1/net/third_party/nss/ssl/ssl3e...
> File net/third_party/nss/ssl/ssl3ext.c (right):
> 
>
https://codereview.chromium.org/12327032/diff/1/net/third_party/nss/ssl/ssl3e...
> net/third_party/nss/ssl/ssl3ext.c:736: ss->ssl3.hs.may_get_cert_status =
> PR_FALSE;
> Is ssl3_HandleHelloRequest a better place for this? This fix doesn't really
> depend on whether or not we send a StatusRequest extension or not on the
> renegotiation.

I'm not sure I follow. This fires whenever we request a new renegotiation and we
send the StatusRequest. If we didn't send a StatusRequest in the new handshake,
then we wouldn't run into this because we wouldn't be expecting a CertStatus
reply. If we did get a CertStatus, it'd fail for that reason - not because we
didn't clear the buffer.

[agl, 2013-02-21 18:10:21]

On Thu, Feb 21, 2013 at 1:01 PM,  <rsleevi@chromium.org> wrote:
> I'm not sure I follow. This fires whenever we request a new renegotiation
> and we
> send the StatusRequest. If we didn't send a StatusRequest in the new
> handshake,
> then we wouldn't run into this because we wouldn't be expecting a CertStatus
> reply. If we did get a CertStatus, it'd fail for that reason - not because
> we
> didn't clear the buffer.

It's just that clearing these flags in a function called
ssl3_ClientSendStatusRequestXtn seems a little off. Not enough to be
worth bothering much with although, if you do want it in there, can
you move it above the if (!ss->opt.enableOCSPStapling)?


Cheers

AGL

[Ryan Sleevi, 2013-02-21 19:38:23]

On Thu, Feb 21, 2013 at 10:10 AM, Adam Langley <agl@chromium.org> wrote:
> On Thu, Feb 21, 2013 at 1:01 PM,  <rsleevi@chromium.org> wrote:
>> I'm not sure I follow. This fires whenever we request a new renegotiation
>> and we
>> send the StatusRequest. If we didn't send a StatusRequest in the new
>> handshake,
>> then we wouldn't run into this because we wouldn't be expecting a CertStatus
>> reply. If we did get a CertStatus, it'd fail for that reason - not because
>> we
>> didn't clear the buffer.
>
> It's just that clearing these flags in a function called
> ssl3_ClientSendStatusRequestXtn seems a little off. Not enough to be
> worth bothering much with although, if you do want it in there, can
> you move it above the if (!ss->opt.enableOCSPStapling)?
>
>
> Cheers
>
> AGL

Fair enough, and point well taken.

The original concern was wanting to avoid nuking the data in the event
that we're doing session resumption, but in a renegotiation, that
shouldn't happen. Upstream, this has all been rewritten by Kai, and
the response is (IIRC) stored as part of the session cache, so even
there it wouldn't be an issue.

Since it's trivial to move, I moved it to where we nuke the other TLS
extension data. Once we roll past NSS 3.14.x, we'll be good.

[agl, 2013-02-21 19:48:09]

lgtm

[Ryan Sleevi, 2013-02-23 00:46:56]

Ok, after looking at tlslite in more detail, I think the renego support there is
going to be a bit more of a pain, so I'm going to go and push this through with
local testing and then chat with Trevor more about next steps.

[commit-bot: I haz the power, 2013-02-23 00:49:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/12327032/7001

[commit-bot: I haz the power, 2013-02-23 02:56:35]

Change committed as 184270

[wtc, 2013-02-25 23:59:21]

Patch set 2 LGTM. Thanks.

https://codereview.chromium.org/12327032/diff/7001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/12327032/diff/7001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:4190: }

This is a good place to clear these two members.

NOTE: This bug seems similar to a bug you fixed before:

----------------------------
revision 1.150
date: 2011/01/22 02:25:06;  author: wtc%google.com;  state: Exp;  lines: +20 -22
Bug 616757: in ssl3_SendCertificateVerify, we must destroy
ss->ssl3.clientPrivateKey for all key exchange algorithms, otherwise we
will send a Certificate message in renegotiation even if the renegotiation
doesn't request client auth.  Move the cleanup of clientCertChain and
clientPrivateKey from ssl3_HandleCertificateRequest to
ssl3_HandleServerHello as a second defense.  The patch is contributed by
Ryan Sleevi <ryan.sleevi@gmail.com>.  r=wtc.
----------------------------

In that bug, we clear the relevant members in ssl3_HandleServerHello. I don't
know why we didn't add
that code to ssl3_SendClientHello. Do you remember?

It would be nice to consolidate the code all in this
function.

[Ryan Sleevi, 2013-02-26 00:07:17]

On 2013/02/25 23:59:21, wtc wrote:
> Patch set 2 LGTM. Thanks.
> 
>
https://codereview.chromium.org/12327032/diff/7001/net/third_party/nss/ssl/ss...
> File net/third_party/nss/ssl/ssl3con.c (right):
> 
>
https://codereview.chromium.org/12327032/diff/7001/net/third_party/nss/ssl/ss...
> net/third_party/nss/ssl/ssl3con.c:4190: }
> 
> This is a good place to clear these two members.
> 
> NOTE: This bug seems similar to a bug you fixed before:
> 
> ----------------------------
> revision 1.150
> date: 2011/01/22 02:25:06;  author: wtc%google.com;  state: Exp;  lines: +20
-22
> Bug 616757: in ssl3_SendCertificateVerify, we must destroy
> ss->ssl3.clientPrivateKey for all key exchange algorithms, otherwise we
> will send a Certificate message in renegotiation even if the renegotiation
> doesn't request client auth.  Move the cleanup of clientCertChain and
> clientPrivateKey from ssl3_HandleCertificateRequest to
> ssl3_HandleServerHello as a second defense.  The patch is contributed by
> Ryan Sleevi <mailto:ryan.sleevi@gmail.com>.  r=wtc.
> ----------------------------
> 
> In that bug, we clear the relevant members in ssl3_HandleServerHello. I don't
> know why we didn't add
> that code to ssl3_SendClientHello. Do you remember?

Nope.

> 
> It would be nice to consolidate the code all in this
> function.

Agreed. That said, this code is part of our 'private fork' - Kai's upstream has
substantially changed this. Once we integrate NSS 3.14.3, we can revisit this
(esp. if Kai's stuff is affected)

I agree that cleanup should happen when sending the client hello, since we want
to make sure our state machine is prepared for any potentially bad/malicious
data.