Don't break on auth if we are about to do client auth....

Don't break on auth if we are about to do client auth.

Don't call SSL_SetSessionOption after the initial handshake is done
because it'll fail with the badReqErr (-909) error.

R=rsleevi
BUG=45576, 52152
TEST=Visit any site that requests SSL client auth over renegotiation.

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

[Ryan Sleevi, 2010-08-26 04:01:06]

Uploaded http://codereview.chromium.org/3120036/show

[wtc, 2010-08-26 23:10:54]

I think we should use a simple fix along the line of this CL
for Chrome 7.0.  Please review.

[Ryan Sleevi, 2010-08-27 00:38:24]

Wan-Teh,

   This looks good, but I have two concerns about this that you may wish to
override. The first relates to how the session cache is handled, and the second
is that the server's certificate will not be re-verified once a client
certificate has been supplied. This also only works on 10.5 if NSS always
performs the initial handshaking/renegotiation. If you're ok with those things,
then we can proceed with this. 

Here's the status of (where things are today, with this CL applied, if BUG is
fixed) to make sure it's ok. The explanations for why each of these is in the
comments.

Server never requests renegotiation:
[Before, Current] 10.5: Works
[Before, Current] 10.6: Works

Server performs renegotiation, but does not request a client certificate:
[Before, Current] 10.5: Does not work. If a client certificate was specified,
returns ERR_SSL_PROTOCOL_ERROR. If one was not, returns
ERR_SSL_CLIENT_AUTH_CERT_NEEDED (even though it is not). See BUG.
[After] 10.5: If BUG is fixed, Does not work. Will return ERR_PROTOCOL_ERROR on
the next attempt for SSLRead.
[Before, Current] 10.6: Does not work. If a client certificate was specified,
returns ERR_SSL_PROTOCOL_ERROR. If one was not, returns
ERR_SSL_CLIENT_AUTH_CERT_NEEDED (even though it is not). See BUG.
[After] 10.5: If BUG is fixed, will work. Server certificate is not re-verified.
Session cache is updated.

Server performs renegotiation, and requests a client certificate, and one has
not been supplied:
[Before, Current] 10.5: Works, but for the wrong reason. Returns
ERR_SSL_CLIENT_AUTH_CERT_NEEDED once it receives the server's Certificate
message, before it has read/parsed the CertificateRequest. See BUG.
[After] 10.5: If BUG is fixed, does not work. Will return ERR_SSL_PROTOCOL_ERROR
on the next attempt for SSLRead.
[Before, Current] 10.6: Works, but for the wrong reason. Returns
ERR_SSL_CLIENT_AUTH_CERT_NEEDED once it receives the server's Certificate
message, before it has read/parsed the CertificateRequest. See BUG.
[After] 10.6: If BUG is fixed, will work. Server certificate is not re-verified.
Returns ERR_SSL_CLIENT_AUTH_CERT_NEEDED. [Session cache is /not/ updated]

Server performs renegotiation, requests a client certificate, and one has been
supplied:
[Before] 10.5: Does not work. Returns ERR_SSL_PROTOCOL_ERROR.
[Current] 10.5: Works. Server certificate is not re-verified. Session cache is
updated.
[Before] 10.6: Does not work. Returns ERR_SSL_PROTOCOL_ERROR.
[Current] 10.6: Works. Server certificate is not re-verified. Session cache is
updated.

http://codereview.chromium.org/3208003/diff/1/2
File net/socket/ssl_client_socket_mac.cc (left):

http://codereview.chromium.org/3208003/diff/1/2#oldcode810
net/socket/ssl_client_socket_mac.cc:810: // to still be able to be resumed.
This comment still applies, though I'm uncertain of it's potential impact.
Without EnableBreakOnAuth set (meaning a client certificate has been supplied,
and we're assuming the server may request renegotiation), you still end up
verifying the server's initial certificate, but the second (and subsequent)
handshakes would not verify the server's certificate, while they will update the
session cache.

Moving this inside the else block should change it - so that the session cache
is only enabled for when we are guaranteed to be able to verify the server's
certificate. However, see the BUG below

http://codereview.chromium.org/3208003/diff/1/2
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/3208003/diff/1/2#newcode795
net/socket/ssl_client_socket_mac.cc:795: //
http://code.google.com/p/chromium/issues/detail?id=38905
This comment can/should be deleted.

http://codereview.chromium.org/3208003/diff/1/2#newcode801
net/socket/ssl_client_socket_mac.cc:801: status = EnableBreakOnAuth(true);
By calling this, 10.5.x clients will be unable to complete any renegotiation,
regardless of certificate request, as errSSLServerAuthCompleted is treated as a
fatal error inside of SSLRead. Line 1140 (original) had been intended to work
around this, except it never worked, as once the socket has been used, you
cannot change the k*Break* options.

As the Certificate message happens before the CertificateRequest, this means
that breaking to verify the server certificate will abort the connection before
a request for client authentication is ever detected.

I'm uncertain if it was your intent to resolve the renegotiation problem in
general, or just as a work-around for the client auth, so I wanted to point it
out.

http://codereview.chromium.org/3208003/diff/1/2#newcode1166
net/socket/ssl_client_socket_mac.cc:1166: return
ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
BUG: This isn't a correct interpretation of errSSLServerAuthCompleted (for the
non-client-cert use of SSLClientSocketMac)

If SSLRead returns errSSLServerAuthCompleted (which will only happen in a
recoverable way on 10.6), it does not necessarily mean that a client certificate
has been requested, just that a renegotiation has been advanced by SSLRead, and
the server's Certificate message has been read and processed.

This is the point where either SSLRead or SSLHandshake should be called, but
only after verifying the server's certificate. If SSLHandshake is called, you'll
have to eventually re-request the SSLRead once SSLHandshake returns noErr.
Otherwise, you should be able to simply burn through SSLRead.

That said, you can delete lines 1156-1163, as they'll never happen anymore when
send_client_cert is set.

[wtc, 2010-09-01 02:14:12]

rsleevi: Thanks for your comments.  I made all of your suggested changes,
except the false assumption that errSSLServerAuthCompleted in renegotiation
implies client cert request.

Please review Patch Set 3.

Again, I am going to a small patch to make client auth work over renegotiation.
Unfortunately, I can't do SSL client auth with two sites
(https://openid4.me/tools/simpleLogin.php and
https://www.startssl.com/logon.ssl).  I tested your CL, and it has the same
problem with those two sites.  I don't think it's worth spending too much
time on it.  I'd rather work on making SSLClientSocketNSS support client auth
on the Mac.

http://codereview.chromium.org/3208003/diff/1/2
File net/socket/ssl_client_socket_mac.cc (left):

http://codereview.chromium.org/3208003/diff/1/2#oldcode810
net/socket/ssl_client_socket_mac.cc:810: // to still be able to be resumed.
On 2010/08/27 00:38:24, rsleevi wrote:
> This comment still applies, though I'm uncertain of it's potential impact.

OK, I reverted this change.  It is not necessary for fixing the client auth over
renegotiation bug.

http://codereview.chromium.org/3208003/diff/1/2
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/3208003/diff/1/2#newcode801
net/socket/ssl_client_socket_mac.cc:801: status = EnableBreakOnAuth(true);
On 2010/08/27 00:38:24, rsleevi wrote:
>
> I'm uncertain if it was your intent to resolve the renegotiation problem in
> general, or just as a work-around for the client auth, so I wanted to point it
> out.

I just want a workaround for the client auth.  Thanks for pointing out the issue
that renegotiation without a client cert request won't work.

[Ryan Sleevi, 2010-09-01 02:33:16]

LGTM then.

[Ryan Sleevi, 2010-09-01 03:50:40]

On 2010/09/01 02:14:12, wtc wrote:
> Again, I am going to a small patch to make client auth work over
renegotiation.
> Unfortunately, I can't do SSL client auth with two sites
> (https://openid4.me/tools/simpleLogin.php and
> https://www.startssl.com/logon.ssl).  I tested your CL, and it has the same
> problem with those two sites.  I don't think it's worth spending too much
> time on it.  I'd rather work on making SSLClientSocketNSS support client auth
> on the Mac.

For what it's worth on investigation, the problem in the case of
startssl/openid4me appears to be that ssl_config_.send_client_cert is set to
true, while ssl_config_.client_cert is NULL. This happens when the server sends
a list of certificate authorities, and the client doesn't have any certificates.
There isn't any prompt, because NSS didn't detect any certificates to
(eventually) display. So it's no surprise that both our CLs fail, as Windows
fails as well.

The call responsible is likely http_network_transaction.cc:214. It sets
send_client_cert to be true, so the Mac SSL socket is used instead of NSS. The
Mac SSL socket has nothing to send during the renegotiation, send_client_cert is
true, so it fails with an error. Switching client_socket_factory.cc:36 to check
send_client_cert AND client_cert causes the NSS socket to be used for the entire
conversation, and it returns the same error as Windows (ERR_SSL_PROTOCOL_ERROR).

I do have an update to my CL that will cause ERR_BAD_SSL_CLIENT_AUTH_CERT to be
returned if a peer handshake alert is sent during a renegotiation handshake
where a certificate was requested, but no actual cert (ssl_config.client_cert)
was sent. However, it's probably a separate bug in NSS (or it's shared in
Win/Mac).

If you wanted to apply a partial fix to it, consider adapting/copying/(moving?)
lines 1105/1106 -1120 to happen around lines 1161. SSLHandshake will return
noErr once the ChangeCipherSpec/Finished is sent, meaning it will not wait
for/check the server's response until the SSLRead call in DoPayloadRead. With
that change, you'll at least be able to return ERR_BAD_SSL_CLIENT_AUTH_CERT,
which is perhaps a more meaningful error message. But like I said, this problem
also seems to exist on Windows as well, so perhaps it's a separate CL.

[wtc, 2010-09-01 04:06:24]

rsleevi: thank you for looking into this.  I think I know
why https://www.startssl.com/logon.ssl and
https://openid4.me/ don't work on the Mac: I think they
require the TLS renegotiation extension for renegotiation.
Both sites support the TLS renegotiation extension.  I
guess they are liberal on the initial handshake but strict
on renegotiation.

If I run chrome with --use-system-ssl, I get errSSLPeerHandshakeFail (-9824)
from SSLRead, even
though I called EnableBreakOnAuth.  This means Secure
Transport doesn't receive the Certificate message during
renegotiation.

I think we should abandon SSLClientSocketMac ASAP.
(SSLClientSocketWin still has values.)

[wtc, 2010-09-01 04:16:04]

On 2010/09/01 03:50:40, rsleevi wrote:
>
> For what it's worth on investigation, the problem in the case of
> startssl/openid4me appears to be that ssl_config_.send_client_cert is set to
> true, while ssl_config_.client_cert is NULL. This happens when the server
sends
> a list of certificate authorities, and the client doesn't have any
certificates.
> There isn't any prompt, because NSS didn't detect any certificates to
> (eventually) display. So it's no surprise that both our CLs fail, as Windows
> fails as well.

I should have told you that I have the client certs for
www.startssl.com and openid4.me, and I am certain
that the client cert for www.startssl.com is correct.

I will read your findings tomorrow.  It's 9:15pm and
I haven't had dinner :-)

[wtc, 2010-09-02 02:44:06]

On 2010/09/01 03:50:40, rsleevi wrote:
>
> Switching client_socket_factory.cc:36 to check
> send_client_cert AND client_cert causes the NSS socket to be used for the
entire
> conversation, ...

This is essentially what the original code does.  The original
code checks client_cert. In r56056, mbelshe changed the check to
send_client_cert.  We can change it back, but I don't think it
matters, unless there is an advantage to let NSS handle the
(send_client_cert=true, client_cert=NULL) case.  What do you
think?

> If you wanted to apply a partial fix to it, consider
adapting/copying/(moving?)
> lines 1105/1106 -1120 to happen around lines 1161. SSLHandshake will return
> noErr once the ChangeCipherSpec/Finished is sent, meaning it will not wait
> for/check the server's response until the SSLRead call in DoPayloadRead. With
> that change, you'll at least be able to return ERR_BAD_SSL_CLIENT_AUTH_CERT,
> which is perhaps a more meaningful error message.

I actually tried this change yesterday afternoon.  Unfortunately
the SSLGetClientCertificateState call returns a client_state of
kSSLClientCertNone.  Secure Transport docs says:
http://developer.apple.com/mac/library/documentation/Security/Reference/secur...

    The value returned reflects the latest change in the state
    of the client certificate exchange. If either peer initiates
    a renegotiation attempt, Secure Transport resets the state
    to kSSLClientCertNone.

Perhaps that's why?  I am on Mac OS X 10.6.4.

[Ryan Sleevi, 2010-09-02 04:22:05]

On 2010/09/02 02:44:06, wtc wrote:
> On 2010/09/01 03:50:40, rsleevi wrote:
> >
> > Switching client_socket_factory.cc:36 to check
> > send_client_cert AND client_cert causes the NSS socket to be used for the
> entire
> > conversation, ...
> 
> This is essentially what the original code does.  The original
> code checks client_cert. In r56056, mbelshe changed the check to
> send_client_cert.  We can change it back, but I don't think it
> matters, unless there is an advantage to let NSS handle the
> (send_client_cert=true, client_cert=NULL) case.  What do you
> think?

I don't know if there is anyone in the wild doing this, but the use case would
be a server that requests renegotiation, requests a client certificate, requires
secure renegotiation, and the user doesn't have a matching client certificate.
In that case, using the NSS socket for the entire conversation should/could (I
don't know if it will today) allow the conversation to continue. That way, the
server's 'native' error message could be displayed - which may include, say, an
alternate form of logon (eg: via form-based credentials with an e-mailed OTP, or
whatever they find sufficient for security)

Again, i don't know if such a server exists in the wild, but given ST's lack of
secure renegotiation, it'd be a possibility. Of course, I wholly agree - using
NSS entirely for the TLS conversation is arguably the best way to solve that
particular problem (and the myriad others that emerge)

> 
> > If you wanted to apply a partial fix to it, consider
> adapting/copying/(moving?)
> > lines 1105/1106 -1120 to happen around lines 1161. SSLHandshake will return
> > noErr once the ChangeCipherSpec/Finished is sent, meaning it will not wait
> > for/check the server's response until the SSLRead call in DoPayloadRead.
With
> > that change, you'll at least be able to return ERR_BAD_SSL_CLIENT_AUTH_CERT,
> > which is perhaps a more meaningful error message.
> 
> I actually tried this change yesterday afternoon.  Unfortunately
> the SSLGetClientCertificateState call returns a client_state of
> kSSLClientCertNone.  Secure Transport docs says:
>
http://developer.apple.com/mac/library/documentation/Security/Reference/secur...
> 
>     The value returned reflects the latest change in the state
>     of the client certificate exchange. If either peer initiates
>     a renegotiation attempt, Secure Transport resets the state
>     to kSSLClientCertNone.
> 
> Perhaps that's why?  I am on Mac OS X 10.6.4.

Hrm. In my packet captures, I was receiving a Certificate Request in the
re-handshake, and SecureTransport was updating with kSSLClientCertRequested when
I called. I'm tempted to re-run the tests with both patches, but I suspect at
this point, there are enough bugs/issues in how the ST state machine behaves
that it's not worth trying to work through all those, as opposed to get the NSS
stuff working.