867795 - Fix problems in the new code in libSSL found during code review

Status: RESOLVED FIXED

Description

[Wan-Teh Chang]

Created attachment 744309 [details] [diff] [review]
Also check ss->certStatusArray points to a non-empty SECItemArray.

This bug report fixes the problems Ryan Sleevi and I found
during code review of the new code in libSSL.

The first patch improves the test for the existence of an OCSP
response on the server side. In addition to testing the
ss->certStatusArray pointer, we should also test that the
SECItemArray it points to is non-empty.

Comment 1

[Wan-Teh Chang]

Created attachment 744316 [details] [diff] [review]
Fix bugs in ssl3_HandleHandshakeMessage

ekr: you just need to look at the last change in this patch (the XXX
comment I added).

I looked at ssl3_HandleHandshakeMessage because Ryan complained about
the unbraced-else with braced-switch.  When writing a patch to avoid
that, I discovered and fixed some bugs.

1. If we are in the wait_certificate_status state but the received
message is not certificate_status, we will call ssl3_AuthCertificate
but fail to handle the received message.

2. If we received an unexpected certificate_status, we should also
send a fatal unexpected_message alert.

3. In DTLS, we may fail to increment ss->ssl3.hs.recvMessageSeq if
we received a certificate or certificate_request message and the
ss->authCertificate() or ss->getClientAuthData() user callback
returns SECWouldBlock.

Comment 2

[Wan-Teh Chang]

Created attachment 744318 [details] [diff] [review]
Fix bugs in ssl3_HandleHandshakeMessage (correct patch)

I attached the wrong patch. This is the correct patch.

Comment 3

[Wan-Teh Chang]

Comment on attachment 744318 [details] [diff] [review]
Fix bugs in ssl3_HandleHandshakeMessage (correct patch)

Ryan, this patch ended up being more than fixing the unbraced-else with
braced-switch, so I'm not sure if you want to review it.

Comment 4

[Wan-Teh Chang]

Created attachment 744321 [details] [diff] [review]
Fix variable declaration formatting in SSL_AuthCertificate

Comment 5

[Ryan Sleevi]

Comment on attachment 744318 [details] [diff] [review]
Fix bugs in ssl3_HandleHandshakeMessage (correct patch)

Review of attachment 744318 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl3con.c
@@ +9672,5 @@
>  
> +    /* XXX should this be rv != SECFailure? ssl3_HandleCertificate and
> +     * ssl3_HandleCertificateRequest may call a user callback, which may
> +     * return SECWouldBlock.
> +     */

Seems to be, since we should never block for any other reason (ssl3_HandleHandshakeMessage should only be called with a fully-gathered record)

Comment 6

[Eric Rescorla (:ekr)]

Comment on attachment 744318 [details] [diff] [review]
Fix bugs in ssl3_HandleHandshakeMessage (correct patch)

Review of attachment 744318 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl3con.c
@@ +9671,5 @@
>      }
>  
> +    /* XXX should this be rv != SECFailure? ssl3_HandleCertificate and
> +     * ssl3_HandleCertificateRequest may call a user callback, which may
> +     * return SECWouldBlock.

I'm not sure. What happens when this functions complete? Do we reenter
with the original message or move on to the next one? If the former, I think
it's OK. If the latter, then probably we need to do something along these lines.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 744318 [details] [diff] [review]
Fix bugs in ssl3_HandleHandshakeMessage (correct patch)

Review of attachment 744318 [details] [diff] [review]:
-----------------------------------------------------------------

> If we are in the wait_certificate_status state but the received
> message is not certificate_status, we will call ssl3_AuthCertificate
> but fail to handle the received message.

In other words, we did not correctly handle the case where the server negotiated the certificate status extension, but did not send the CertificateStatus message.

::: lib/ssl/ssl3con.c
@@ +9558,5 @@
>         rv = ssl3_AuthCertificate(ss); /* sets ss->ssl3.hs.ws */
> +
> +       if (ss->ssl3.hs.msg_type == certificate_status) {
> +           return rv;
> +       }

Shouldn't this be if (ss->ssl3.hs.msg_type == certificate_status || rv == SECFailure)?

Otherwise, if ssl3_AuthCertificate fails, then we will ignore the failure because we will replace the value of rv with the return value of a ssl3_Handle*** call. We should never call the ssl3_Handle*** call in the first place if certificate validation failed.

@@ +9672,5 @@
>  
> +    /* XXX should this be rv != SECFailure? ssl3_HandleCertificate and
> +     * ssl3_HandleCertificateRequest may call a user callback, which may
> +     * return SECWouldBlock.
> +     */

Why not just:

    if (IS_DTLS(ss)) {

if rv == SECFailure, then the value of ss->ssl3.hs.recvMessageSeq no longer matters since the connection can no longer be used. This would also match the addition you made above.

Comment 8

[Wan-Teh Chang]

Created attachment 744336 [details] [diff] [review]
Fix problems in SSL_AuthCertificate

In addition to fixing variable declaration formatting, I also fix
two problems.

1. Pass ss->pkcs11PinArg instead of |arg| as the "void *pwArg" argument
to CERT_CacheOCSPResponseFromSideChannel.

2. ss->sec.peerCert is the server's leaf cert, so it matches only
&certStatusArray->items[i] for i = 0. I replaced the for loop with an
if statement.

Comment 9

[Wan-Teh Chang]

Comment on attachment 744309 [details] [diff] [review]
Also check ss->certStatusArray points to a non-empty SECItemArray.

https://hg.mozilla.org/projects/nss/rev/4f4190cc5f9f

Comment 10

[Wan-Teh Chang]

Comment on attachment 744336 [details] [diff] [review]
Fix problems in SSL_AuthCertificate

https://hg.mozilla.org/projects/nss/rev/44a46d334735

Comment 11

[Kai Engert (:kaie:)]

Comment on attachment 744336 [details] [diff] [review]
Fix problems in SSL_AuthCertificate

>diff --git a/lib/ssl/sslauth.c b/lib/ssl/sslauth.c
> 
>-    for (i = 0; i < certStatusArray->len; ++i) {
>+    if (certStatusArray->len) {
>         CERT_CacheOCSPResponseFromSideChannel(handle, ss->sec.peerCert,

Well, ok. The old code somehow played the role of a reminder, but I agree this should be rather done correctly in bug 611836.

Comment 12

[Wan-Teh Chang]

Created attachment 744867 [details] [diff] [review]
Fix code formatting issues reported by Adam Langley

https://hg.mozilla.org/projects/nss/rev/96330ae13a3f

Comment 13

[Wan-Teh Chang]

Created attachment 745307 [details] [diff] [review]
Fix bugs in ssl3_HandleHandshakeMessage, v2

Brian: I made the fix you suggested. I will deal with the
ss->ssl3.hs.recvMessageSeq increment at the end of ssl3_HandleHandshakeMessage
in a separate bug.

Comment 14

[Wan-Teh Chang]

Comment on attachment 745307 [details] [diff] [review]
Fix bugs in ssl3_HandleHandshakeMessage, v2

Review of attachment 745307 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/ssl3con.c
@@ +9558,5 @@
>         rv = ssl3_AuthCertificate(ss); /* sets ss->ssl3.hs.ws */
> +
> +       if (rv == SECFailure || ss->ssl3.hs.msg_type == certificate_status) {
> +           return rv;
> +       }

Brian: this ssl3_AuthCertificate() may return SECWouldBlock. We will proceed
to handle the received handshake message in that case. I assume this is the
correct behavior.

If the ssl3_Handle<Foo> call returns SECSuccess, should ssl3_HandleHandshakeMessage return SECWouldBlock or SECSuccess? With this
patch, ssl3_HandleHandshakeMessage will return SECSuccess.

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Wan-Teh Chang from comment #14)
> ::: lib/ssl/ssl3con.c
> @@ +9558,5 @@
> >         rv = ssl3_AuthCertificate(ss); /* sets ss->ssl3.hs.ws */
> > +
> > +       if (rv == SECFailure || ss->ssl3.hs.msg_type == certificate_status) {
> > +           return rv;
> > +       }
> 
> Brian: this ssl3_AuthCertificate() may return SECWouldBlock. We will proceed
> to handle the received handshake message in that case. I assume this is the
> correct behavior.

ssl3_AuthCertificate will not return SECWouldBlock, AFAICT. If the auth certificate hook returns SECWouldBlock then ssl3_AuthCertificate will return SECSuccess:

        ...

	if (rv == SECWouldBlock) {
	    if (ss->sec.isServer) {
		errCode = SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_SERVERS;
		rv = SECFailure;
		goto loser;
	    }

	    ss->ssl3.hs.authCertificatePending = PR_TRUE;
	    rv = SECSuccess;
            ...
       
> If the ssl3_Handle<Foo> call returns SECSuccess, should
> ssl3_HandleHandshakeMessage return SECWouldBlock or SECSuccess? With this
> patch, ssl3_HandleHandshakeMessage will return SECSuccess.

ssl3_Handle<Foo> will return SECWouldBlock when needed. This will happen in ssl3_HandleServerHelloDone (indirectly via ssl3_SendClientSecondRound) and ssl3_HandleFinished.

The idea is that a function should only return SECWouldBlock when it can't make progress. But, we can still make progress even when we are waiting for the certificate to be authenticated, up until the handshake is over; that is, we complete the handshake while cert authentication is pending but we won't send/receive application data until both the handshake is finished (or false start conditions are met) and the certificate has been successfully authenticated.

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 745307 [details] [diff] [review]
Fix bugs in ssl3_HandleHandshakeMessage, v2

Review of attachment 745307 [details] [diff] [review]:
-----------------------------------------------------------------

Did you find out if there are any ssl tests that test the case where the certificate authentication failed? If not, we should file a follow-up bug for adding some.

::: lib/ssl/ssl3con.c
@@ +9558,5 @@
>         rv = ssl3_AuthCertificate(ss); /* sets ss->ssl3.hs.ws */
> +
> +       if (rv == SECFailure || ss->ssl3.hs.msg_type == certificate_status) {
> +           return rv;
> +       }

See my comment above. (I should have put my comment in splinter).

I suggest that we change this to:

if (rv != SECSuccess || ss->ssl3.hs.msg_type == certificate_status) {
   PORT_Assert(rv == SECFailure); /* Not SECWouldBlock */
   return rv;
}

This should clear up some of the confusion.

Comment 17

[Wan-Teh Chang]

Created attachment 745353 [details] [diff] [review]
Fix bugs in ssl3_HandleHandshakeMessage, v3

Brian, thank you for answering my questions. I made the change you
suggested and checked this in.

I forgot to tell you that NSS test suite passed with my old patch.
So most likely we don't have a test with both an untrusted server
certificate and a missing certificate_status message.

https://hg.mozilla.org/projects/nss/rev/76a4d93de110

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Created attachment 745472 [details] [diff] [review]
Fix assertion of return value of ssl3_AuthCertificate

Wan-Teh, I made an obvious mistake in my suggestion to you:

>         if (rv != SECSuccess || ss->ssl3.hs.msg_type == certificate_status) {
>            PORT_Assert(rv == SECFailure); /* Not SECWouldBlock */

We forgot to account for the case where rv == SECSuccess && ss->ssl3.hs.msg_type == certificate_status.

See the attached patch.

Comment 19

[Wan-Teh Chang]

Comment on attachment 745472 [details] [diff] [review]
Fix assertion of return value of ssl3_AuthCertificate

Review of attachment 745472 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. Could you check this in? Thanks.

Comment 20

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 745472 [details] [diff] [review]
Fix assertion of return value of ssl3_AuthCertificate

https://hg.mozilla.org/projects/nss/rev/4d21439eba16