733521 - SSL3 handshake lock not held when necessary in ssl3_GatherCompleteHandshake

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

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

http://mxr.mozilla.org/security/source/security/nss/lib/ssl/ssl3gthr.c#207

207 	/* Treat an empty msgState like a NULL msgState. (Most of the time
208 	 * when ssl3_HandleHandshake returns SECWouldBlock, it leaves
209 	 * behind a non-NULL but zero-length msgState).
210 	 * Test: async_cert_restart_server_sends_hello_request_first_in_separate_record
211 	 */
212 	if (ss->ssl3.hs.msgState.buf != NULL) {
213 	    if (ss->ssl3.hs.msgState.len == 0) {
214 		ss->ssl3.hs.msgState.buf = NULL;
215 	    }
216 	}
217 
218 	if (ss->ssl3.hs.msgState.buf != NULL) {
219 	    /* ssl3_HandleHandshake previously returned SECWouldBlock and the
220 	     * as-yet-unprocessed plaintext of that previous handshake record.
221 	     * We need to process it now before we overwrite it with the next
222 	     * handshake record.
223 	     */
224 	    rv = ssl3_HandleRecord(ss, NULL, &ss->gs.buf);

It seems like the accessed to ss->ssl3.hs.msgState should be protected by the SSL3 handshake lock.

ssl3_RestartHandshakeAfterCertReq has the same problem.

This is a problem that was identified by Wan-Teh and EKR.

Comment 1

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

Attachment: Hold lock across all uses of ssl3.hs in ssl3_GatherCompleteHandshake

Comment 2

[Wan-Teh Chang]

Comment on attachment 745475 [details] [diff] [review]
Hold lock across all uses of ssl3.hs in ssl3_GatherCompleteHandshake

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

r=wtc.

::: lib/ssl/ssl3gthr.c
@@ +286,1 @@
>  	/* Without this, we may end up wrongly reporting

Do you know what "this" refers to?

It seems to refer to the ss->ssl3.hs.restartTarget == NULL test.
If so, I suggest moving this comment after ssl_GetSSL3HandshakeLock(ss), and also changing "this" to
"this check" or "this test".

@@ +294,3 @@
>  	if (rv != SECSuccess) {
>  	    PORT_SetError(PR_WOULD_BLOCK_ERROR);
> +	    ssl_ReleaseSSL3HandshakeLock(ss);

Call PORT_SetError() after releasing the lock.
We should do as little work as possible while holding a
lock to avoid lock contention.

Comment 3

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

Comment on attachment 745475 [details] [diff] [review]
Hold lock across all uses of ssl3.hs in ssl3_GatherCompleteHandshake

https://hg.mozilla.org/projects/nss/rev/753e9555853d

Thanks for the review. I made the changes you suggested in this changeset.

Comment 4

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

Attachment: Clarify logic in ssl3_GatherCompleteHandshake regarding async. cert verification

Besides the changes you suggested, I found that there are further clarifications that should be made.

Comment 5

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

Attachment: Clarify logic in ssl3_GatherCompleteHandshake regarding async. cert verification

Comment 6

[Wan-Teh Chang]

Comment on attachment 745615 [details] [diff] [review]
Clarify logic in ssl3_GatherCompleteHandshake regarding async. cert verification

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

r=wtc.

::: lib/ssl/ssl3gthr.c
@@ +304,4 @@
>  	    if (ss->ssl3.hs.msgState.len == 0) {
>  		ss->ssl3.hs.msgState.buf = NULL;
> +	    } else {
> +		handleRecordNow = PR_TRUE;

We can also just test ss->ssl3.hs.msgState.len != 0 and
leave a (buf=non-null, len=0) combination unchanged.

Comment 7

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

Comment on attachment 745615 [details] [diff] [review]
Clarify logic in ssl3_GatherCompleteHandshake regarding async. cert verification

(In reply to Wan-Teh Chang from comment #6)
> Comment on attachment 745615 [details] [diff] [review]
> Clarify logic in ssl3_GatherCompleteHandshake regarding async. cert
> verification
> 
> Review of attachment 745615 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=wtc.
> 
> ::: lib/ssl/ssl3gthr.c
> @@ +304,4 @@
> >  	    if (ss->ssl3.hs.msgState.len == 0) {
> >  		ss->ssl3.hs.msgState.buf = NULL;
> > +	    } else {
> > +		handleRecordNow = PR_TRUE;
> 
> We can also just test ss->ssl3.hs.msgState.len != 0 and
> leave a (buf=non-null, len=0) combination unchanged.

Thanks for the review Wan-Teh. I think your suggested change makes sense, but I did not make that change because it would have taken too much time to convince myself that it is definitely OK.

http://hg.mozilla.org/projects/nss/rev/cf3b9b27172e