869080 - ssl3_HandleHandshakeMessage should also increment ssl3.hs.recvMessageSeq if ssl3_Handle<FooBarMessage> returns SECWouldBlock

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Patch

(I originally reported this problem in bug 867795 comment 1 and bug 867795
comment 2.)

At the end of ssl3_HandleHandshakeMessage(), we have a big switch statement
to call the ssl3_Handle<FooBar> function to handle the foo_bar handshake
message, and then increment ssl3.hs.recvMessageSeq:

    switch (ss->ssl3.hs.msg_type) {
    ...
    case <foo_bar>:
        rv = ssl3_Handle<FooBar>(ss, b, length);
        break;
    ...
    default:
        (void)SSL3_SendAlert(ss, alert_fatal, unexpected_message);
        PORT_SetError(SSL_ERROR_RX_UNKNOWN_HANDSHAKE);
        rv = SECFailure;
    }

    if (IS_DTLS(ss) && (rv == SECSuccess)) {
        /* Increment the expected sequence number */
        ss->ssl3.hs.recvMessageSeq++;
    }

    return rv;

At least one ssl3_Handle<FooBar> function, ssl3_HandleCertificateRequest,
may return SECWouldBlock.  (In that case, SECWouldBlock means the user
callback function ss->getClientAuthData() returned SECWouldBlock.)  I
think ssl3_HandleHandshakeMessage() should still increment
ss->ssl3.hs.recvMessageSeq in that case.

Note: I don't know if an ssl3_Handle<FooBar> function will return SECWouldBlock
in any other circumstances. Perhaps ssl3_Finished? Brian is perhaps the best
person to answer this question.

Comment 1

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

Comment on attachment 745957 [details] [diff] [review]
Patch

Basically, except for the client hello message, libssl always works like this:

1. Then wait for more handshake records from the peer.
2. Parse one or more handshake records from the peer.
3. Then, send one or more handshake records to the peer.

The ssl3_Handle<XXX> methods return SECWouldBlock only in the first phase and the third phase. In particular, there is never a need re-parse a handshake record upon a SECWouldBlock, because either (a) we haven't gotten the whole record, so we didn't start parsing it at all, or (b) we parsed the whole thing, and are stuck waiting for information needed to send data to the peer in step 3.

Comment 2

[Wan-Teh Chang]

Comment on attachment 745957 [details] [diff] [review]
Patch

Brian: thank you for the answer. I have a question about your answer.

ssl3_HandleCertificateRequest can also return SECWouldBlock:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.207&mark=6107-6110,6114-6117,6165,6179-6182#6105

That occurs in what you called the second phase (when we parse the
CertificateRequest handshake message), right?

Patch pushed:
https://hg.mozilla.org/projects/nss/rev/3962579505f0

Comment 3

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

(In reply to Wan-Teh Chang from comment #2)
> Comment on attachment 745957 [details] [diff] [review]
> Patch
> 
> Brian: thank you for the answer. I have a question about your answer.
> 
> ssl3_HandleCertificateRequest can also return SECWouldBlock:
> 
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/
> ssl3con.c&rev=1.207&mark=6107-6110,6114-6117,6165,6179-6182#6105
> 
> That occurs in what you called the second phase (when we parse the
> CertificateRequest handshake message), right?

No. AFAICT, we parse the message (second phase) and then we enter the third phase within that same function.

Also, I am not sure that this asynchronous client certificate mechanism works. I know I had to make several changes to get the asynchronous server certificate mechanism to work.

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 745957 [details] [diff] [review]
Patch

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

This seems to have landed.