Closed
Bug 869080
Opened 11 years ago
Closed 11 years ago
ssl3_HandleHandshakeMessage should also increment ssl3.hs.recvMessageSeq if ssl3_Handle<FooBarMessage> returns SECWouldBlock
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.15
People
(Reporter: wtc, Assigned: wtc)
Details
Attachments
(1 file)
660 bytes,
patch
|
briansmith
:
superreview+
wtc
:
checked-in+
|
Details | Diff | Splinter 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 1•11 years ago
|
||
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+
Assignee | ||
Comment 2•11 years ago
|
||
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+
Assignee | ||
Updated•11 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 3•11 years ago
|
||
(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•10 years ago
|
||
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.
Description
•