935847 - ss->ssl3.hs.ws assertion in ssl3_AuthCertificateComplete is too strict

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

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

+++ 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.

Comment 1

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

Attachment: test-false-start-assertion.patch

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.

Comment 2

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

Attachment: fix-false-start-assertion.patch

This is the patch to fix the bug adding the other cases that have to be considered to the assertion.

Comment 3

[Wan-Teh Chang]

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.

Comment 4

[Wan-Teh Chang]

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).

Comment 5

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

(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.

Comment 6

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

This was checked in for NSS 3.15.4 but the bug never got resolved:
http://hg.mozilla.org/projects/nss/rev/6634b8f10920