Closed
Bug 935847
Opened 11 years ago
Closed 10 years ago
ss->ssl3.hs.ws assertion in ssl3_AuthCertificateComplete is too strict
Categories
(NSS :: Libraries, defect, P1)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.15.4
People
(Reporter: briansmith, Assigned: briansmith)
References
()
Details
(Keywords: assertion, regression)
Attachments
(1 file, 1 obsolete file)
2.52 KB,
patch
|
wtc
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #934378 +++ The ssl3_AuthCertificateComplete function fails to take into account that the ssl3_AuthCertificateComplete may be called between the time we receive the server's Certificate message and the time we try to send the client second round. This can happen, for example, if the server sends CertificateStatus, ServerKeyExchange, CertificateRequest, and/or ServerHelloDone in a separate packet from the Certificate message. Luckily, the ssl3_WaitingForStartOfServerSecondRound check saves us from doing the bad thing.
Assignee | ||
Comment 1•11 years ago
|
||
This extends the test coverage to include a test for the case where the server sends its certificate in a separate packet from the rest of the server's first round. It is based on the mini test framework created in bug 930874. Note that I had to put the test in sslauth.txt instead of sslstress.txt because tstclnt uses the async cert verification path but strsclnt only uses the synchronous cert verification path.
Attachment #828765 -
Flags: review?(wtc)
Assignee | ||
Comment 2•11 years ago
|
||
This is the patch to fix the bug adding the other cases that have to be considered to the assertion.
Attachment #828766 -
Flags: review?(wtc)
Comment 3•11 years ago
|
||
Comment on attachment 828766 [details] [diff] [review] fix-false-start-assertion.patch Review of attachment 828766 [details] [diff] [review]: ----------------------------------------------------------------- r=wtc. I think the ss->ssl3.hs.ws assertion is still not quite right. ::: lib/ssl/ssl3con.c @@ +10041,5 @@ > > PORT_Assert(!ss->firstHsDone); > PORT_Assert(!ss->sec.isServer); > PORT_Assert(!ss->ssl3.hs.isResuming); > + PORT_Assert(ss->ssl3.hs.ws == wait_certificate_status || Can ss->ssl3.hs.ws be wait_certificate_status? We will not verify the certificate until the CertificateStatus message has been received, right? @@ +10050,2 @@ > ss->ssl3.hs.ws == wait_change_cipher || > ss->ssl3.hs.ws == wait_finished); With so many possible values for ss->ssl3.hs.ws, this assertion doesn't seem that useful. Perhaps we just need to assert ss->ssl3.hs.ws != idle_handshake.
Attachment #828766 -
Flags: review?(wtc) → review+
Comment 4•11 years ago
|
||
Comment on attachment 828766 [details] [diff] [review] fix-false-start-assertion.patch Review of attachment 828766 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/ssl/ssl3con.c @@ +7105,5 @@ > case wait_change_cipher: > result = !ssl3_ExtensionNegotiated(ss, ssl_session_ticket_xtn); > break; > + default: > + result = PR_FALSE; Brian: could you please confirm that |result| should also be PR_FALSE for these states? wait_server_key wait_cert_request wait_hello_done (As I noted yesterday, I believe wait_certificate_status is impossible.) Returning PR_FALSE will cause us to skip the ssl3_CheckFalseStart(ss) call on line 10062. I assume that we will have another chance to call ssl3_CheckFalseStart(ss) later (in ssl3_SendClientSecondRound).
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Wan-Teh Chang from comment #4) > I assume that we will have another chance to call > ssl3_CheckFalseStart(ss) later > (in ssl3_SendClientSecondRound). Yes, that's right.
Assignee | ||
Updated•11 years ago
|
Whiteboard: [testing patch] → [test needs to be reviewed and checked in]
Assignee | ||
Updated•10 years ago
|
Attachment #828765 -
Attachment is obsolete: true
Attachment #828765 -
Flags: review?(wtc)
Assignee | ||
Comment 6•10 years ago
|
||
This was checked in for NSS 3.15.4 but the bug never got resolved: http://hg.mozilla.org/projects/nss/rev/6634b8f10920
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•10 years ago
|
Whiteboard: [test needs to be reviewed and checked in]
You need to log in
before you can comment on or make changes to this bug.
Description
•