Closed Bug 869080 Opened 8 years ago Closed 8 years ago

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

Categories

(NSS :: Libraries, defect, P2)

3.14
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wtc, Assigned: wtc)

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
(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.
Attachment #745957 - Flags: superreview?(bsmith)
Attachment #745957 - Flags: review?(ekr)
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.
Attachment #745957 - Flags: superreview?(bsmith) → superreview+
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
Attachment #745957 - Flags: checked-in+
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
(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 on attachment 745957 [details] [diff] [review]
Patch

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

This seems to have landed.
Attachment #745957 - Flags: review?(ekr)
You need to log in before you can comment on or make changes to this bug.