Consolidate CCS_OK paths in s3_clnt.c.
Introduce a CR_CHANGE state just before entering CR_FINISHED_A. This replaces
the CCS_OK in the CR_FINISHED_A/CR_FINISHED_B case which otherwise would get
applied after partial reads of Finished. The other CCS_OK settings are
redundant with this one.
The copy in tls_secret_session_cb codepath is made unnecessary with
9eaeef81fa2d4fd6246dc02b6203fa936a5eaf67.
The copy in the normal session resumption case is unnecessary with
6444287806d801b9a45baf1f6f02a0e3a16e144c. Before that commit, OpenSSL would
potentially read Finished a state early. Now that we are strict (and get the
book-keeping correct) for expecting the NewSessionTicket message it too is
redundant.
Of particular note is the one after ssl3_send_finished. That was added in
response to upstream's PR#3400. I've reproduced the bug and concluded it was
actually a bug around expecting a NewSessionTicket message. That has been fixed
properly in 6444287806d801b9a45baf1f6f02a0e3a16e144c by resetting
tlsext_expect_ticket on renegotiations.
Change-Id: I6a928386994fcd5efff26a5f0efb12b65bf7f299
Reviewed-on: https://boringssl-review.googlesource.com/1298
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index dda2ef7..52d88c6 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -279,7 +279,7 @@
if (s->hit)
{
- s->state=SSL3_ST_CR_FINISHED_A;
+ s->state=SSL3_ST_CR_CHANGE;
if (s->tlsext_ticket_expected)
{
/* receive renewed session ticket */
@@ -457,7 +457,6 @@
s->method->ssl3_enc->client_finished_label,
s->method->ssl3_enc->client_finished_label_len);
if (ret <= 0) goto end;
- s->s3->flags |= SSL3_FLAGS_CCS_OK;
s->state=SSL3_ST_CW_FLUSH;
/* clear flags */
@@ -493,7 +492,7 @@
if (s->tlsext_ticket_expected)
s->s3->tmp.next_state=SSL3_ST_CR_SESSION_TICKET_A;
else
- s->s3->tmp.next_state=SSL3_ST_CR_FINISHED_A;
+ s->s3->tmp.next_state=SSL3_ST_CR_CHANGE;
}
}
s->init_num=0;
@@ -503,7 +502,7 @@
case SSL3_ST_CR_SESSION_TICKET_B:
ret=ssl3_get_new_session_ticket(s);
if (ret <= 0) goto end;
- s->state=SSL3_ST_CR_FINISHED_A;
+ s->state=SSL3_ST_CR_CHANGE;
s->init_num=0;
break;
@@ -515,10 +514,15 @@
s->init_num=0;
break;
+ case SSL3_ST_CR_CHANGE:
+ /* At this point, the next message must be entirely
+ * behind a ChangeCipherSpec. */
+ s->s3->flags |= SSL3_FLAGS_CCS_OK;
+ s->state = SSL3_ST_CR_FINISHED_A;
+ break;
+
case SSL3_ST_CR_FINISHED_A:
case SSL3_ST_CR_FINISHED_B:
-
- s->s3->flags |= SSL3_FLAGS_CCS_OK;
ret=ssl3_get_finished(s,SSL3_ST_CR_FINISHED_A,
SSL3_ST_CR_FINISHED_B);
if (ret <= 0) goto end;
@@ -943,7 +947,6 @@
s->session->cipher = pref_cipher ?
pref_cipher :
ssl3_get_cipher_by_value(cipher_suite);
- s->s3->flags |= SSL3_FLAGS_CCS_OK;
s->hit = 1;
}
}
@@ -960,7 +963,6 @@
OPENSSL_PUT_ERROR(SSL, ssl3_get_server_hello, SSL_R_ATTEMPT_TO_REUSE_SESSION_IN_DIFFERENT_CONTEXT);
goto f_err;
}
- s->s3->flags |= SSL3_FLAGS_CCS_OK;
s->hit = 1;
}
diff --git a/ssl/ssl_stat.c b/ssl/ssl_stat.c
index 5514277..9a45b45 100644
--- a/ssl/ssl_stat.c
+++ b/ssl/ssl_stat.c
@@ -134,8 +134,7 @@
case SSL3_ST_SW_FINISHED_A: str="SSLv3 write finished A"; break;
case SSL3_ST_CW_FINISHED_B:
case SSL3_ST_SW_FINISHED_B: str="SSLv3 write finished B"; break;
-case SSL3_ST_CR_CHANGE_A: str="SSLv3 read change cipher spec A"; break;
-case SSL3_ST_CR_CHANGE_B: str="SSLv3 read change cipher spec B"; break;
+case SSL3_ST_CR_CHANGE:
case SSL3_ST_SR_CHANGE: str="SSLv3 read change cipher spec"; break;
case SSL3_ST_CR_FINISHED_A:
case SSL3_ST_SR_FINISHED_A: str="SSLv3 read finished A"; break;
@@ -253,8 +252,7 @@
case SSL3_ST_CW_FINISHED_A: str="3WFINA"; break;
case SSL3_ST_SW_FINISHED_B:
case SSL3_ST_CW_FINISHED_B: str="3WFINB"; break;
-case SSL3_ST_CR_CHANGE_A: str="3RCCSA"; break;
-case SSL3_ST_CR_CHANGE_B: str="3RCCSB"; break;
+case SSL3_ST_CR_CHANGE:
case SSL3_ST_SR_CHANGE: str="3RCCS_"; break;
case SSL3_ST_SR_FINISHED_A:
case SSL3_ST_CR_FINISHED_A: str="3RFINA"; break;
