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/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);