Pull ChangeCipherSpec into the handshake state machine.
This uses ssl3_read_bytes for now. We still need to dismantle that
function and then invert the handshake state machine, but this gets
things closer to the right shape as an intermediate step and is a large
chunk in itself. It simplifies a lot of the CCS/handshake
synchronization as a lot of the invariants much more clearly follow from
the handshake itself.
Tests need to be adjusted since this changes some error codes. Now all
the CCS/Handshake checks fall through to the usual
SSL_R_UNEXPECTED_RECORD codepath. Most of what used to be a special-case
falls out naturally. (If half of Finished was in the same record as the
pre-CCS message, that part of the handshake record would have been left
unconsumed, so read_change_cipher_spec would have noticed, just like
read_app_data would have noticed.)
Change-Id: I15c7501afe523d5062f0e24a3b65f053008d87be
Reviewed-on: https://boringssl-review.googlesource.com/6642
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index f4b9535..2dbd9e4 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -3914,13 +3914,7 @@
uint16_t cap;
} SSL3_BUFFER;
-/* TODO(davidben): This flag can probably be merged into s3->change_cipher_spec
- * to something tri-state. (Normal / Expect CCS / Between CCS and Finished). */
-#define SSL3_FLAGS_EXPECT_CCS 0x0080
-
typedef struct ssl3_state_st {
- long flags;
-
uint8_t read_sequence[8];
int read_mac_secret_size;
uint8_t read_mac_secret[EVP_MAX_MD_SIZE];
@@ -3969,10 +3963,6 @@
* the handshake hash for TLS 1.1 and below. */
EVP_MD_CTX handshake_md5;
- /* this is set whenerver we see a change_cipher_spec message come in when we
- * are not looking for one */
- int change_cipher_spec;
-
int warn_alert;
int fatal_alert;
/* we allow one fatal and one warning alert to be outstanding, send close
diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c
index 0ed1aea..5448632 100644
--- a/ssl/d1_clnt.c
+++ b/ssl/d1_clnt.c
@@ -235,7 +235,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 */
s->state = SSL3_ST_CR_SESSION_TICKET_A;
@@ -337,7 +337,6 @@
s->state = SSL3_ST_CW_CERT_VRFY_A;
} else {
s->state = SSL3_ST_CW_CHANGE_A;
- s->s3->change_cipher_spec = 0;
}
s->init_num = 0;
@@ -353,7 +352,6 @@
}
s->state = SSL3_ST_CW_CHANGE_A;
s->init_num = 0;
- s->s3->change_cipher_spec = 0;
break;
case SSL3_ST_CW_CHANGE_A:
@@ -401,7 +399,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;
@@ -413,7 +411,7 @@
if (ret <= 0) {
goto end;
}
- s->state = SSL3_ST_CR_FINISHED_A;
+ s->state = SSL3_ST_CR_CHANGE;
s->init_num = 0;
break;
@@ -427,9 +425,21 @@
s->init_num = 0;
break;
+ case SSL3_ST_CR_CHANGE:
+ ret = s->method->ssl_read_change_cipher_spec(s);
+ if (ret <= 0) {
+ goto end;
+ }
+
+ if (!ssl3_do_change_cipher_spec(s)) {
+ ret = -1;
+ goto end;
+ }
+ s->state = SSL3_ST_CR_FINISHED_A;
+ break;
+
case SSL3_ST_CR_FINISHED_A:
case SSL3_ST_CR_FINISHED_B:
- s->d1->change_cipher_spec_ok = 1;
ret =
ssl3_get_finished(s, SSL3_ST_CR_FINISHED_A, SSL3_ST_CR_FINISHED_B);
if (ret <= 0) {
diff --git a/ssl/d1_meth.c b/ssl/d1_meth.c
index 4dc3404..49a2595 100644
--- a/ssl/d1_meth.c
+++ b/ssl/d1_meth.c
@@ -67,6 +67,7 @@
dtls1_connect,
dtls1_get_message,
dtls1_read_app_data,
+ dtls1_read_change_cipher_spec,
dtls1_read_close_notify,
dtls1_write_app_data,
dtls1_dispatch_alert,
diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c
index 12cdeac..c8ed9d8 100644
--- a/ssl/d1_pkt.c
+++ b/ssl/d1_pkt.c
@@ -190,6 +190,29 @@
return dtls1_read_bytes(ssl, SSL3_RT_APPLICATION_DATA, buf, len, peek);
}
+int dtls1_read_change_cipher_spec(SSL *ssl) {
+ uint8_t byte;
+ int ret = dtls1_read_bytes(ssl, SSL3_RT_CHANGE_CIPHER_SPEC, &byte,
+ 1 /* len */, 0 /* no peek */);
+ if (ret <= 0) {
+ return ret;
+ }
+ assert(ret == 1);
+
+ if (ssl->s3->rrec.length != 0 || byte != SSL3_MT_CCS) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_CHANGE_CIPHER_SPEC);
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
+ return -1;
+ }
+
+ if (ssl->msg_callback != NULL) {
+ ssl->msg_callback(0, ssl->version, SSL3_RT_CHANGE_CIPHER_SPEC, &byte, 1,
+ ssl, ssl->msg_callback_arg);
+ }
+
+ return 1;
+}
+
void dtls1_read_close_notify(SSL *ssl) {
/* Bidirectional shutdown doesn't make sense for an unordered transport. DTLS
* alerts also aren't delivered reliably, so we may even time out because the
@@ -201,36 +224,23 @@
/* Return up to 'len' payload bytes received in 'type' records.
* 'type' is one of the following:
*
- * - SSL3_RT_HANDSHAKE (when ssl3_get_message calls us)
- * - SSL3_RT_APPLICATION_DATA (when ssl3_read calls us)
+ * - SSL3_RT_HANDSHAKE (when dtls1_get_message calls us)
+ * - SSL3_RT_CHANGE_CIPHER_SPEC (when dtls1_read_change_cipher_spec calls us)
+ * - SSL3_RT_APPLICATION_DATA (when dtls1_read_app_data calls us)
*
- * If we don't have stored data to work from, read a SSL/TLS record first
- * (possibly multiple records if we still don't have anything to return).
+ * If we don't have stored data to work from, read a DTLS record first (possibly
+ * multiple records if we still don't have anything to return).
*
* This function must handle any surprises the peer may have for us, such as
- * Alert records (e.g. close_notify), ChangeCipherSpec records (not really
- * a surprise, but handled as if it were), or renegotiation requests.
- * Also if record payloads contain fragments too small to process, we store
- * them until there is enough for the respective protocol (the record protocol
- * may use arbitrary fragmentation and even interleaving):
- * Change cipher spec protocol
- * just 1 byte needed, no need for keeping anything stored
- * Alert protocol
- * 2 bytes needed (AlertLevel, AlertDescription)
- * Handshake protocol
- * 4 bytes needed (HandshakeType, uint24 length) -- we just have
- * to detect unexpected Client Hello and Hello Request messages
- * here, anything else is handled by higher layers
- * Application data protocol
- * none of our business
- */
+ * Alert records (e.g. close_notify) and out of records. */
int dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek) {
int al, i, ret;
unsigned int n;
SSL3_RECORD *rr;
void (*cb)(const SSL *ssl, int type, int value) = NULL;
- if ((type != SSL3_RT_APPLICATION_DATA && type != SSL3_RT_HANDSHAKE) ||
+ if ((type != SSL3_RT_APPLICATION_DATA && type != SSL3_RT_HANDSHAKE &&
+ type != SSL3_RT_CHANGE_CIPHER_SPEC) ||
(peek && type != SSL3_RT_APPLICATION_DATA)) {
OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
return -1;
@@ -278,17 +288,6 @@
/* we now have a packet which can be read and processed */
- /* |change_cipher_spec is set when we receive a ChangeCipherSpec and reset by
- * ssl3_get_finished. */
- if (s->s3->change_cipher_spec && rr->type != SSL3_RT_HANDSHAKE &&
- rr->type != SSL3_RT_ALERT) {
- /* We now have an unexpected record between CCS and Finished. Most likely
- * the packets were reordered on their way. DTLS is unreliable, so drop the
- * packet and expect the peer to retransmit. */
- rr->length = 0;
- goto start;
- }
-
/* If the other end has shut down, throw anything we read away (even in
* 'peek' mode) */
if (s->shutdown & SSL_RECEIVED_SHUTDOWN) {
@@ -298,9 +297,9 @@
}
- if (type == rr->type) { /* SSL3_RT_APPLICATION_DATA or SSL3_RT_HANDSHAKE */
- /* make sure that we are not getting application data when we
- * are doing a handshake for the first time */
+ if (type == rr->type) {
+ /* Make sure that we are not getting application data when we
+ * are doing a handshake for the first time. */
if (SSL_in_init(s) && (type == SSL3_RT_APPLICATION_DATA) &&
(s->aead_read_ctx == NULL)) {
/* TODO(davidben): Is this check redundant with the handshake_func
@@ -396,43 +395,33 @@
goto start;
}
- if (rr->type == SSL3_RT_CHANGE_CIPHER_SPEC) {
- /* 'Change Cipher Spec' is just a single byte, so we know exactly what the
- * record payload has to look like */
- if (rr->length != 1 || rr->off != 0 || rr->data[0] != SSL3_MT_CCS) {
- al = SSL_AD_ILLEGAL_PARAMETER;
- OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_CHANGE_CIPHER_SPEC);
- goto f_err;
- }
-
+ /* Cross-epoch records are discarded, but we may receive out-of-order
+ * application data between ChangeCipherSpec and Finished or a ChangeCipherSpec
+ * before the appropriate point in the handshake. Those must be silently
+ * discarded.
+ *
+ * However, only allow the out-of-order records in the correct epoch.
+ * Application data must come in the encrypted epoch, and ChangeCipherSpec in
+ * the unencrypted epoch (we never renegotiate). Other cases fall through and
+ * fail with a fatal error. */
+ if ((rr->type == SSL3_RT_APPLICATION_DATA && s->aead_read_ctx != NULL) ||
+ (rr->type == SSL3_RT_CHANGE_CIPHER_SPEC && s->aead_read_ctx == NULL)) {
rr->length = 0;
-
- if (s->msg_callback) {
- s->msg_callback(0, s->version, SSL3_RT_CHANGE_CIPHER_SPEC, rr->data, 1, s,
- s->msg_callback_arg);
- }
-
- /* We can't process a CCS now, because previous handshake
- * messages are still missing, so just drop it.
- */
- if (!s->d1->change_cipher_spec_ok) {
- goto start;
- }
-
- s->d1->change_cipher_spec_ok = 0;
-
- s->s3->change_cipher_spec = 1;
- if (!ssl3_do_change_cipher_spec(s)) {
- goto err;
- }
-
goto start;
}
- /* Unexpected handshake message. It may be a retransmitted Finished (the only
- * post-CCS message). Otherwise, it's a pre-CCS handshake message from an
- * unsupported renegotiation attempt. */
- if (rr->type == SSL3_RT_HANDSHAKE && !s->in_handshake) {
+ if (rr->type == SSL3_RT_HANDSHAKE) {
+ if (type != SSL3_RT_APPLICATION_DATA) {
+ /* Out-of-order handshake record while looking for ChangeCipherSpec. Drop
+ * it silently. */
+ assert(type == SSL3_RT_CHANGE_CIPHER_SPEC);
+ rr->length = 0;
+ goto start;
+ }
+
+ /* Parse the first fragment header to determine if this is a pre-CCS or
+ * post-CCS handshake record. DTLS resets handshake message numbers on each
+ * handshake, so renegotiations and retransmissions are ambiguous. */
if (rr->length < DTLS1_HM_HEADER_LENGTH) {
al = SSL_AD_DECODE_ERROR;
OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_HANDSHAKE_RECORD);
@@ -441,7 +430,6 @@
struct hm_header_st msg_hdr;
dtls1_get_message_header(&rr->data[rr->off], &msg_hdr);
- /* Ignore a stray Finished from the previous handshake. */
if (msg_hdr.type == SSL3_MT_FINISHED) {
if (msg_hdr.frag_off == 0) {
/* Retransmit our last flight of messages. If the peer sends the second
@@ -457,17 +445,16 @@
rr->length = 0;
goto start;
}
- }
- /* We already handled these. */
- assert(rr->type != SSL3_RT_CHANGE_CIPHER_SPEC && rr->type != SSL3_RT_ALERT);
+ /* Otherwise, this is a pre-CCS handshake message from an unsupported
+ * renegotiation attempt. Fall through to the error path. */
+ }
al = SSL_AD_UNEXPECTED_MESSAGE;
OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
f_err:
ssl3_send_alert(s, SSL3_AL_FATAL, al);
-err:
return -1;
}
diff --git a/ssl/d1_srvr.c b/ssl/d1_srvr.c
index 79f762b..9eb1020 100644
--- a/ssl/d1_srvr.c
+++ b/ssl/d1_srvr.c
@@ -345,13 +345,26 @@
if (ret <= 0) {
goto end;
}
- s->state = SSL3_ST_SR_FINISHED_A;
+ s->state = SSL3_ST_SR_CHANGE;
s->init_num = 0;
break;
+ case SSL3_ST_SR_CHANGE:
+ ret = s->method->ssl_read_change_cipher_spec(s);
+ if (ret <= 0) {
+ goto end;
+ }
+
+ if (!ssl3_do_change_cipher_spec(s)) {
+ ret = -1;
+ goto end;
+ }
+
+ s->state = SSL3_ST_SR_FINISHED_A;
+ break;
+
case SSL3_ST_SR_FINISHED_A:
case SSL3_ST_SR_FINISHED_B:
- s->d1->change_cipher_spec_ok = 1;
ret =
ssl3_get_finished(s, SSL3_ST_SR_FINISHED_A, SSL3_ST_SR_FINISHED_B);
if (ret <= 0) {
@@ -414,7 +427,7 @@
}
s->state = SSL3_ST_SW_FLUSH;
if (s->hit) {
- s->s3->tmp.next_state = SSL3_ST_SR_FINISHED_A;
+ s->s3->tmp.next_state = SSL3_ST_SR_CHANGE;
} else {
s->s3->tmp.next_state = SSL_ST_OK;
}
diff --git a/ssl/internal.h b/ssl/internal.h
index b5c98d6..5832076 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -763,26 +763,27 @@
struct ssl_protocol_method_st {
/* is_dtls is one if the protocol is DTLS and zero otherwise. */
char is_dtls;
- int (*ssl_new)(SSL *s);
- void (*ssl_free)(SSL *s);
- int (*ssl_accept)(SSL *s);
- int (*ssl_connect)(SSL *s);
- long (*ssl_get_message)(SSL *s, int header_state, int body_state,
+ int (*ssl_new)(SSL *ssl);
+ void (*ssl_free)(SSL *ssl);
+ int (*ssl_accept)(SSL *ssl);
+ int (*ssl_connect)(SSL *ssl);
+ long (*ssl_get_message)(SSL *ssl, int header_state, int body_state,
int msg_type, long max,
enum ssl_hash_message_t hash_message, int *ok);
- int (*ssl_read_app_data)(SSL *s, uint8_t *buf, int len, int peek);
- void (*ssl_read_close_notify)(SSL *s);
- int (*ssl_write_app_data)(SSL *s, const void *buf_, int len);
- int (*ssl_dispatch_alert)(SSL *s);
+ int (*ssl_read_app_data)(SSL *ssl, uint8_t *buf, int len, int peek);
+ int (*ssl_read_change_cipher_spec)(SSL *ssl);
+ void (*ssl_read_close_notify)(SSL *ssl);
+ int (*ssl_write_app_data)(SSL *ssl, const void *buf_, int len);
+ int (*ssl_dispatch_alert)(SSL *ssl);
/* supports_cipher returns one if |cipher| is supported by this protocol and
* zero otherwise. */
int (*supports_cipher)(const SSL_CIPHER *cipher);
/* Handshake header length */
unsigned int hhlen;
/* Set the handshake header */
- int (*set_handshake_header)(SSL *s, int type, unsigned long len);
+ int (*set_handshake_header)(SSL *ssl, int type, unsigned long len);
/* Write out handshake message */
- int (*do_write)(SSL *s);
+ int (*do_write)(SSL *ssl);
};
/* This is for the SSLv3/TLSv1.0 differences in crypto/hash stuff It is a bit
@@ -911,8 +912,6 @@
/* Timeout duration */
unsigned short timeout_duration;
-
- unsigned int change_cipher_spec_ok;
} DTLS1_STATE;
extern const SSL3_ENC_METHOD TLSv1_enc_data;
@@ -1007,8 +1006,8 @@
int ssl3_send_finished(SSL *s, int a, int b, const char *sender, int slen);
int ssl3_supports_cipher(const SSL_CIPHER *cipher);
int ssl3_dispatch_alert(SSL *s);
-int ssl3_expect_change_cipher_spec(SSL *s);
int ssl3_read_app_data(SSL *ssl, uint8_t *buf, int len, int peek);
+int ssl3_read_change_cipher_spec(SSL *ssl);
void ssl3_read_close_notify(SSL *ssl);
int ssl3_read_bytes(SSL *s, int type, uint8_t *buf, int len, int peek);
int ssl3_write_app_data(SSL *ssl, const void *buf, int len);
@@ -1036,6 +1035,7 @@
int dtls1_do_handshake_write(SSL *s, enum dtls1_use_epoch_t use_epoch);
int dtls1_read_app_data(SSL *ssl, uint8_t *buf, int len, int peek);
+int dtls1_read_change_cipher_spec(SSL *ssl);
void dtls1_read_close_notify(SSL *ssl);
int dtls1_read_bytes(SSL *s, int type, uint8_t *buf, int len, int peek);
void dtls1_set_message_header(SSL *s, uint8_t mt, unsigned long len,
diff --git a/ssl/s3_both.c b/ssl/s3_both.c
index 1416bb3..577e59b 100644
--- a/ssl/s3_both.c
+++ b/ssl/s3_both.c
@@ -240,15 +240,6 @@
goto err;
}
- /* If this occurs, we have missed a message.
- * TODO(davidben): Is this check now redundant with SSL3_FLAGS_EXPECT_CCS? */
- if (!s->s3->change_cipher_spec) {
- al = SSL_AD_UNEXPECTED_MESSAGE;
- OPENSSL_PUT_ERROR(SSL, SSL_R_GOT_A_FIN_BEFORE_A_CCS);
- goto f_err;
- }
- s->s3->change_cipher_spec = 0;
-
p = s->init_msg;
finished_len = s->s3->tmp.peer_finish_md_len;
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index f5af366..f9d7693 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -355,7 +355,6 @@
s->state = SSL3_ST_CW_CERT_VRFY_A;
} else {
s->state = SSL3_ST_CW_CHANGE_A;
- s->s3->change_cipher_spec = 0;
}
s->init_num = 0;
@@ -370,7 +369,6 @@
}
s->state = SSL3_ST_CW_CHANGE_A;
s->init_num = 0;
- s->s3->change_cipher_spec = 0;
break;
case SSL3_ST_CW_CHANGE_A:
@@ -484,9 +482,12 @@
break;
case SSL3_ST_CR_CHANGE:
- /* At this point, the next message must be entirely behind a
- * ChangeCipherSpec. */
- if (!ssl3_expect_change_cipher_spec(s)) {
+ ret = s->method->ssl_read_change_cipher_spec(s);
+ if (ret <= 0) {
+ goto end;
+ }
+
+ if (!ssl3_do_change_cipher_spec(s)) {
ret = -1;
goto end;
}
diff --git a/ssl/s3_meth.c b/ssl/s3_meth.c
index 01c1101..b60b5f2 100644
--- a/ssl/s3_meth.c
+++ b/ssl/s3_meth.c
@@ -67,6 +67,7 @@
ssl3_connect,
ssl3_get_message,
ssl3_read_app_data,
+ ssl3_read_change_cipher_spec,
ssl3_read_close_notify,
ssl3_write_app_data,
ssl3_dispatch_alert,
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index 722041e..6ca8a84 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -315,24 +315,33 @@
return ssl3_write_pending(s, type, buf, len);
}
-/* ssl3_expect_change_cipher_spec informs the record layer that a
- * ChangeCipherSpec record is required at this point. If a Handshake record is
- * received before ChangeCipherSpec, the connection will fail. If there is an
- * unprocessed handshake message, it returns zero. Otherwise, it returns one. */
-int ssl3_expect_change_cipher_spec(SSL *s) {
- if (s->s3->tmp.reuse_message) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_UNPROCESSED_HANDSHAKE_DATA);
- return 0;
- }
-
- s->s3->flags |= SSL3_FLAGS_EXPECT_CCS;
- return 1;
-}
-
int ssl3_read_app_data(SSL *ssl, uint8_t *buf, int len, int peek) {
return ssl3_read_bytes(ssl, SSL3_RT_APPLICATION_DATA, buf, len, peek);
}
+int ssl3_read_change_cipher_spec(SSL *ssl) {
+ uint8_t byte;
+ int ret = ssl3_read_bytes(ssl, SSL3_RT_CHANGE_CIPHER_SPEC, &byte, 1 /* len */,
+ 0 /* no peek */);
+ if (ret <= 0) {
+ return ret;
+ }
+ assert(ret == 1);
+
+ if (ssl->s3->rrec.length != 0 || byte != SSL3_MT_CCS) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_CHANGE_CIPHER_SPEC);
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
+ return -1;
+ }
+
+ if (ssl->msg_callback != NULL) {
+ ssl->msg_callback(0, ssl->version, SSL3_RT_CHANGE_CIPHER_SPEC, &byte, 1,
+ ssl, ssl->msg_callback_arg);
+ }
+
+ return 1;
+}
+
void ssl3_read_close_notify(SSL *ssl) {
ssl3_read_bytes(ssl, 0, NULL, 0, 0);
}
@@ -357,36 +366,23 @@
* 'type' is one of the following:
*
* - SSL3_RT_HANDSHAKE (when ssl3_get_message calls us)
- * - SSL3_RT_APPLICATION_DATA (when ssl3_read calls us)
+ * - SSL3_RT_CHANGE_CIPHER_SPEC (when ssl3_read_change_cipher_spec calls us)
+ * - SSL3_RT_APPLICATION_DATA (when ssl3_read_app_data calls us)
* - 0 (during a shutdown, no data has to be returned)
*
* If we don't have stored data to work from, read a SSL/TLS record first
* (possibly multiple records if we still don't have anything to return).
*
* This function must handle any surprises the peer may have for us, such as
- * Alert records (e.g. close_notify), ChangeCipherSpec records (not really
- * a surprise, but handled as if it were), or renegotiation requests.
- * Also if record payloads contain fragments too small to process, we store
- * them until there is enough for the respective protocol (the record protocol
- * may use arbitrary fragmentation and even interleaving):
- * Change cipher spec protocol
- * just 1 byte needed, no need for keeping anything stored
- * Alert protocol
- * 2 bytes needed (AlertLevel, AlertDescription)
- * Handshake protocol
- * 4 bytes needed (HandshakeType, uint24 length) -- we just have
- * to detect unexpected Client Hello and Hello Request messages
- * here, anything else is handled by higher layers
- * Application data protocol
- * none of our business
- */
+ * Alert records (e.g. close_notify) or renegotiation requests. */
int ssl3_read_bytes(SSL *s, int type, uint8_t *buf, int len, int peek) {
int al, i, ret;
unsigned int n;
SSL3_RECORD *rr;
void (*cb)(const SSL *ssl, int type, int value) = NULL;
- if ((type && type != SSL3_RT_APPLICATION_DATA && type != SSL3_RT_HANDSHAKE) ||
+ if ((type && type != SSL3_RT_APPLICATION_DATA && type != SSL3_RT_HANDSHAKE &&
+ type != SSL3_RT_CHANGE_CIPHER_SPEC) ||
(peek && type != SSL3_RT_APPLICATION_DATA)) {
OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
return -1;
@@ -428,23 +424,6 @@
/* we now have a packet which can be read and processed */
- /* |change_cipher_spec is set when we receive a ChangeCipherSpec and reset by
- * ssl3_get_finished. */
- if (s->s3->change_cipher_spec && rr->type != SSL3_RT_HANDSHAKE &&
- rr->type != SSL3_RT_ALERT) {
- al = SSL_AD_UNEXPECTED_MESSAGE;
- OPENSSL_PUT_ERROR(SSL, SSL_R_DATA_BETWEEN_CCS_AND_FINISHED);
- goto f_err;
- }
-
- /* If we are expecting a ChangeCipherSpec, it is illegal to receive a
- * Handshake record. */
- if (rr->type == SSL3_RT_HANDSHAKE && (s->s3->flags & SSL3_FLAGS_EXPECT_CCS)) {
- al = SSL_AD_UNEXPECTED_MESSAGE;
- OPENSSL_PUT_ERROR(SSL, SSL_R_HANDSHAKE_RECORD_BEFORE_CCS);
- goto f_err;
- }
-
/* If the other end has shut down, throw anything we read away (even in
* 'peek' mode) */
if (s->shutdown & SSL_RECEIVED_SHUTDOWN) {
@@ -456,9 +435,8 @@
if (type != 0 && type == rr->type) {
s->s3->warning_alert_count = 0;
- /* SSL3_RT_APPLICATION_DATA or SSL3_RT_HANDSHAKE */
- /* make sure that we are not getting application data when we are doing a
- * handshake for the first time */
+ /* Make sure that we are not getting application data when we are doing a
+ * handshake for the first time. */
if (SSL_in_init(s) && type == SSL3_RT_APPLICATION_DATA &&
s->aead_read_ctx == NULL) {
/* TODO(davidben): Is this check redundant with the handshake_func
@@ -499,7 +477,7 @@
/* Process unexpected records. */
- if (rr->type == SSL3_RT_HANDSHAKE) {
+ if (type == SSL3_RT_APPLICATION_DATA && rr->type == SSL3_RT_HANDSHAKE) {
/* If peer renegotiations are disabled, all out-of-order handshake records
* are fatal. Renegotiations as a server are never supported. */
if (s->server || !ssl3_can_renegotiate(s)) {
@@ -651,49 +629,6 @@
goto start;
}
- if (rr->type == SSL3_RT_CHANGE_CIPHER_SPEC) {
- /* 'Change Cipher Spec' is just a single byte, so we know exactly what the
- * record payload has to look like */
- if (rr->length != 1 || rr->off != 0 || rr->data[0] != SSL3_MT_CCS) {
- al = SSL_AD_ILLEGAL_PARAMETER;
- OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_CHANGE_CIPHER_SPEC);
- goto f_err;
- }
-
- /* Check we have a cipher to change to */
- if (s->s3->tmp.new_cipher == NULL) {
- al = SSL_AD_UNEXPECTED_MESSAGE;
- OPENSSL_PUT_ERROR(SSL, SSL_R_CCS_RECEIVED_EARLY);
- goto f_err;
- }
-
- if (!(s->s3->flags & SSL3_FLAGS_EXPECT_CCS)) {
- al = SSL_AD_UNEXPECTED_MESSAGE;
- OPENSSL_PUT_ERROR(SSL, SSL_R_CCS_RECEIVED_EARLY);
- goto f_err;
- }
-
- s->s3->flags &= ~SSL3_FLAGS_EXPECT_CCS;
-
- rr->length = 0;
-
- if (s->msg_callback) {
- s->msg_callback(0, s->version, SSL3_RT_CHANGE_CIPHER_SPEC, rr->data, 1, s,
- s->msg_callback_arg);
- }
-
- s->s3->change_cipher_spec = 1;
- if (!ssl3_do_change_cipher_spec(s)) {
- goto err;
- } else {
- goto start;
- }
- }
-
- /* We already handled these. */
- assert(rr->type != SSL3_RT_CHANGE_CIPHER_SPEC && rr->type != SSL3_RT_ALERT &&
- rr->type != SSL3_RT_HANDSHAKE);
-
al = SSL_AD_UNEXPECTED_MESSAGE;
OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 13e1d35..a3130f9 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -419,27 +419,25 @@
s->init_num = 0;
break;
- case SSL3_ST_SR_CHANGE: {
- char next_proto_neg = 0;
- char channel_id = 0;
- next_proto_neg = s->s3->next_proto_neg_seen;
- channel_id = s->s3->tlsext_channel_id_valid;
+ case SSL3_ST_SR_CHANGE:
+ ret = s->method->ssl_read_change_cipher_spec(s);
+ if (ret <= 0) {
+ goto end;
+ }
- /* At this point, the next message must be entirely behind a
- * ChangeCipherSpec. */
- if (!ssl3_expect_change_cipher_spec(s)) {
+ if (!ssl3_do_change_cipher_spec(s)) {
ret = -1;
goto end;
}
- if (next_proto_neg) {
+
+ if (s->s3->next_proto_neg_seen) {
s->state = SSL3_ST_SR_NEXT_PROTO_A;
- } else if (channel_id) {
+ } else if (s->s3->tlsext_channel_id_valid) {
s->state = SSL3_ST_SR_CHANNEL_ID_A;
} else {
s->state = SSL3_ST_SR_FINISHED_A;
}
break;
- }
case SSL3_ST_SR_NEXT_PROTO_A:
case SSL3_ST_SR_NEXT_PROTO_B:
@@ -2406,17 +2404,6 @@
return n;
}
- /* s->state doesn't reflect whether ChangeCipherSpec has been received in
- * this handshake, but s->s3->change_cipher_spec does (will be reset by
- * ssl3_get_finished).
- *
- * TODO(davidben): Is this check now redundant with
- * SSL3_FLAGS_EXPECT_CCS? */
- if (!s->s3->change_cipher_spec) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_GOT_NEXT_PROTO_BEFORE_A_CCS);
- return -1;
- }
-
CBS_init(&next_protocol, s->init_msg, n);
/* The payload looks like:
@@ -2470,16 +2457,6 @@
return -1;
}
- /* s->state doesn't reflect whether ChangeCipherSpec has been received in
- * this handshake, but s->s3->change_cipher_spec does (will be reset by
- * ssl3_get_finished).
- *
- * TODO(davidben): Is this check now redundant with SSL3_FLAGS_EXPECT_CCS? */
- if (!s->s3->change_cipher_spec) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_GOT_CHANNEL_ID_BEFORE_A_CCS);
- return -1;
- }
-
CBS_init(&encrypted_extensions, s->init_msg, n);
/* EncryptedExtensions could include multiple extensions, but the only
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 5af93df..4748dd5 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -994,7 +994,7 @@
},
},
shouldFail: true,
- expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:",
+ expectedError: ":UNEXPECTED_RECORD:",
},
{
testType: serverTest,
@@ -1005,7 +1005,7 @@
},
},
shouldFail: true,
- expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:",
+ expectedError: ":UNEXPECTED_RECORD:",
},
{
testType: serverTest,
@@ -1020,7 +1020,7 @@
"-advertise-npn", "\x03foo\x03bar\x03baz",
},
shouldFail: true,
- expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:",
+ expectedError: ":UNEXPECTED_RECORD:",
},
{
name: "FragmentAcrossChangeCipherSpec-Client",
@@ -1030,7 +1030,7 @@
},
},
shouldFail: true,
- expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:",
+ expectedError: ":UNEXPECTED_RECORD:",
},
{
testType: serverTest,
@@ -1041,7 +1041,7 @@
},
},
shouldFail: true,
- expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:",
+ expectedError: ":UNEXPECTED_RECORD:",
},
{
testType: serverTest,
@@ -1056,7 +1056,7 @@
"-advertise-npn", "\x03foo\x03bar\x03baz",
},
shouldFail: true,
- expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:",
+ expectedError: ":UNEXPECTED_RECORD:",
},
{
testType: serverTest,
@@ -1115,7 +1115,7 @@
},
},
shouldFail: true,
- expectedError: ":CCS_RECEIVED_EARLY:",
+ expectedError: ":UNEXPECTED_RECORD:",
},
{
testType: serverTest,
@@ -1126,7 +1126,7 @@
},
},
shouldFail: true,
- expectedError: ":CCS_RECEIVED_EARLY:",
+ expectedError: ":UNEXPECTED_RECORD:",
},
{
name: "SkipNewSessionTicket",
@@ -1136,7 +1136,7 @@
},
},
shouldFail: true,
- expectedError: ":CCS_RECEIVED_EARLY:",
+ expectedError: ":UNEXPECTED_RECORD:",
},
{
testType: serverTest,
@@ -1413,7 +1413,7 @@
},
},
shouldFail: true,
- expectedError: ":DATA_BETWEEN_CCS_AND_FINISHED:",
+ expectedError: ":UNEXPECTED_RECORD:",
},
{
name: "AppDataAfterChangeCipherSpec-Empty",
@@ -1423,7 +1423,7 @@
},
},
shouldFail: true,
- expectedError: ":DATA_BETWEEN_CCS_AND_FINISHED:",
+ expectedError: ":UNEXPECTED_RECORD:",
},
{
protocol: dtls,
