diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 63307b45..35055813 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2522,6 +2522,7 @@ void ERR_load_SSL_strings(void); #define SSL_F_tls1_change_cipher_state_aead 279 #define SSL_F_tls1_aead_ctx_init 280 #define SSL_F_tls1_check_duplicate_extensions 281 +#define SSL_F_ssl3_expect_change_cipher_spec 282 #define SSL_R_UNABLE_TO_FIND_ECDH_PARAMETERS 100 #define SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC 101 #define SSL_R_INVALID_NULL_CMD_NAME 102 @@ -2832,6 +2833,8 @@ void ERR_load_SSL_strings(void); #define SSL_R_CLIENTHELLO_PARSE_FAILED 437 #define SSL_R_CONNECTION_REJECTED 438 #define SSL_R_DECODE_ERROR 439 +#define SSL_R_UNPROCESSED_HANDSHAKE_DATA 440 +#define SSL_R_HANDSHAKE_RECORD_BEFORE_CCS 441 #define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010 #define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020 #define SSL_R_TLSV1_ALERT_DECRYPTION_FAILED 1021 diff --git a/include/openssl/ssl3.h b/include/openssl/ssl3.h index db9c381b..4edb6dfb 100644 --- a/include/openssl/ssl3.h +++ b/include/openssl/ssl3.h @@ -343,7 +343,9 @@ typedef struct ssl3_buffer_st #define TLS1_FLAGS_TLS_PADDING_BUG 0x0008 #define TLS1_FLAGS_SKIP_CERT_VERIFY 0x0010 #define TLS1_FLAGS_KEEP_HANDSHAKE 0x0020 -#define SSL3_FLAGS_CCS_OK 0x0080 +/* 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 /* SSL3_FLAGS_SGC_RESTART_DONE is set when we * restart a handshake because of MS SGC and so prevents us diff --git a/ssl/s3_both.c b/ssl/s3_both.c index 6c8bc155..8d052018 100644 --- a/ssl/s3_both.c +++ b/ssl/s3_both.c @@ -245,7 +245,9 @@ int ssl3_get_finished(SSL *s, int a, int b) if (!ok) return((int)n); - /* If this occurs, we have missed a message */ + /* 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; diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c index 52d88c69..72966c76 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c @@ -517,7 +517,11 @@ int ssl3_connect(SSL *s) case SSL3_ST_CR_CHANGE: /* At this point, the next message must be entirely * behind a ChangeCipherSpec. */ - s->s3->flags |= SSL3_FLAGS_CCS_OK; + if (!ssl3_expect_change_cipher_spec(s)) + { + ret = -1; + goto end; + } s->state = SSL3_ST_CR_FINISHED_A; break; diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c index 97530a3a..4191059b 100644 --- a/ssl/s3_pkt.c +++ b/ssl/s3_pkt.c @@ -901,6 +901,22 @@ int ssl3_write_pending(SSL *s, int type, const unsigned char *buf, } } +/* 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. Moreover, if + * there are unprocessed handshake bytes, the handshake will also fail and the + * function returns zero. Otherwise, the function returns one. */ +int ssl3_expect_change_cipher_spec(SSL *s) + { + if (s->s3->handshake_fragment_len > 0 || s->s3->tmp.reuse_message) + { + OPENSSL_PUT_ERROR(SSL, ssl3_expect_change_cipher_spec, SSL_R_UNPROCESSED_HANDSHAKE_DATA); + return 0; + } + s->s3->flags |= SSL3_FLAGS_EXPECT_CCS; + return 1; + } + /* Return up to 'len' payload bytes received in 'type' records. * 'type' is one of the following: * @@ -1007,6 +1023,15 @@ start: 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, ssl3_read_bytes, 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) @@ -1016,7 +1041,6 @@ start: return(0); } - if (type == rr->type) /* SSL3_RT_APPLICATION_DATA or SSL3_RT_HANDSHAKE */ { /* make sure that we are not getting application data when we @@ -1274,14 +1298,14 @@ start: goto f_err; } - if (!(s->s3->flags & SSL3_FLAGS_CCS_OK)) + if (!(s->s3->flags & SSL3_FLAGS_EXPECT_CCS)) { al=SSL_AD_UNEXPECTED_MESSAGE; OPENSSL_PUT_ERROR(SSL, ssl3_read_bytes, SSL_R_CCS_RECEIVED_EARLY); goto f_err; } - s->s3->flags &= ~SSL3_FLAGS_CCS_OK; + s->s3->flags &= ~SSL3_FLAGS_EXPECT_CCS; rr->length=0; diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index 36b421e2..c9c01fde 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -573,7 +573,11 @@ int ssl3_accept(SSL *s) /* At this point, the next message must be entirely * behind a ChangeCipherSpec. */ - s->s3->flags |= SSL3_FLAGS_CCS_OK; + if (!ssl3_expect_change_cipher_spec(s)) + { + ret = -1; + goto end; + } if (next_proto_neg) s->state = SSL3_ST_SR_NEXT_PROTO_A; else if (channel_id) @@ -3023,7 +3027,9 @@ int ssl3_get_next_proto(SSL *s) /* 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). */ + * 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, ssl3_get_next_proto, SSL_R_GOT_NEXT_PROTO_BEFORE_A_CCS); @@ -3096,7 +3102,9 @@ int ssl3_get_channel_id(SSL *s) /* 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). */ + * 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, ssl3_get_channel_id, SSL_R_GOT_CHANNEL_ID_BEFORE_A_CCS); diff --git a/ssl/ssl_error.c b/ssl/ssl_error.c index 0097d605..c989c84c 100644 --- a/ssl/ssl_error.c +++ b/ssl/ssl_error.c @@ -108,6 +108,7 @@ const ERR_STRING_DATA SSL_error_string_data[] = { {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_ctx_ctrl, 0), "ssl3_ctx_ctrl"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_digest_cached_records, 0), "ssl3_digest_cached_records"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_do_change_cipher_spec, 0), "ssl3_do_change_cipher_spec"}, + {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_expect_change_cipher_spec, 0), "ssl3_expect_change_cipher_spec"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_generate_key_block, 0), "ssl3_generate_key_block"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_get_cert_status, 0), "ssl3_get_cert_status"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_get_cert_verify, 0), "ssl3_get_cert_verify"}, @@ -298,6 +299,7 @@ const ERR_STRING_DATA SSL_error_string_data[] = { {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_GOT_CHANNEL_ID_BEFORE_A_CCS), "GOT_CHANNEL_ID_BEFORE_A_CCS"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_GOT_NEXT_PROTO_BEFORE_A_CCS), "GOT_NEXT_PROTO_BEFORE_A_CCS"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_GOT_NEXT_PROTO_WITHOUT_EXTENSION), "GOT_NEXT_PROTO_WITHOUT_EXTENSION"}, + {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_HANDSHAKE_RECORD_BEFORE_CCS), "HANDSHAKE_RECORD_BEFORE_CCS"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_HTTPS_PROXY_REQUEST), "HTTPS_PROXY_REQUEST"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_HTTP_REQUEST), "HTTP_REQUEST"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_ILLEGAL_PADDING), "ILLEGAL_PADDING"}, @@ -513,6 +515,7 @@ const ERR_STRING_DATA SSL_error_string_data[] = { {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_UNKNOWN_SSL_VERSION), "UNKNOWN_SSL_VERSION"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_UNKNOWN_STATE), "UNKNOWN_STATE"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_UNKNOWN_SUPPLEMENTAL_DATA_TYPE), "UNKNOWN_SUPPLEMENTAL_DATA_TYPE"}, + {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_UNPROCESSED_HANDSHAKE_DATA), "UNPROCESSED_HANDSHAKE_DATA"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED), "UNSAFE_LEGACY_RENEGOTIATION_DISABLED"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_UNSUPPORTED_CIPHER), "UNSUPPORTED_CIPHER"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_UNSUPPORTED_COMPRESSION_ALGORITHM), "UNSUPPORTED_COMPRESSION_ALGORITHM"}, diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 57d63c8c..7f63ce66 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -947,6 +947,7 @@ const SSL_CIPHER *ssl3_get_cipher(unsigned int u); int ssl3_renegotiate(SSL *ssl); int ssl3_renegotiate_check(SSL *ssl); int ssl3_dispatch_alert(SSL *s); +int ssl3_expect_change_cipher_spec(SSL *s); int ssl3_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek); int ssl3_write_bytes(SSL *s, int type, const void *buf, int len); int ssl3_final_finish_mac(SSL *s, const char *sender, int slen,unsigned char *p); diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index 73194637..7b8d9646 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go @@ -375,6 +375,11 @@ type ProtocolBugs struct { // and 1.0.1 modes, respectively. EarlyChangeCipherSpec int + // FragmentAcrossChangeCipherSpec causes the implementation to fragment + // the Finished (or NextProto) message around the ChangeCipherSpec + // messages. + FragmentAcrossChangeCipherSpec bool + // SkipNewSessionTicket causes the server to skip sending the // NewSessionTicket message despite promising to in ServerHello. SkipNewSessionTicket bool diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index 32b90e2b..80208dd1 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go @@ -584,10 +584,7 @@ func (hs *clientHandshakeState) readSessionTicket() error { func (hs *clientHandshakeState) sendFinished() error { c := hs.c - if !c.config.Bugs.SkipChangeCipherSpec && - c.config.Bugs.EarlyChangeCipherSpec == 0 { - c.writeRecord(recordTypeChangeCipherSpec, []byte{1}) - } + var postCCSBytes []byte if hs.serverHello.nextProtoNeg { nextProto := new(nextProtoMsg) proto, fallback := mutualProtocol(c.config.NextProtos, hs.serverHello.nextProtos) @@ -595,8 +592,9 @@ func (hs *clientHandshakeState) sendFinished() error { c.clientProtocol = proto c.clientProtocolFallback = fallback - hs.finishedHash.Write(nextProto.marshal()) - c.writeRecord(recordTypeHandshake, nextProto.marshal()) + nextProtoBytes := nextProto.marshal() + hs.finishedHash.Write(nextProtoBytes) + postCCSBytes = append(postCCSBytes, nextProtoBytes...) } finished := new(finishedMsg) @@ -605,8 +603,21 @@ func (hs *clientHandshakeState) sendFinished() error { } else { finished.verifyData = hs.finishedHash.clientSum(hs.masterSecret) } - hs.finishedHash.Write(finished.marshal()) - c.writeRecord(recordTypeHandshake, finished.marshal()) + finishedBytes := finished.marshal() + hs.finishedHash.Write(finishedBytes) + postCCSBytes = append(postCCSBytes, finishedBytes...) + + if c.config.Bugs.FragmentAcrossChangeCipherSpec { + c.writeRecord(recordTypeHandshake, postCCSBytes[:5]) + postCCSBytes = postCCSBytes[5:] + } + + if !c.config.Bugs.SkipChangeCipherSpec && + c.config.Bugs.EarlyChangeCipherSpec == 0 { + c.writeRecord(recordTypeChangeCipherSpec, []byte{1}) + } + + c.writeRecord(recordTypeHandshake, postCCSBytes) return nil } diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index 8cdecd7c..68ba734a 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go @@ -598,14 +598,21 @@ func (hs *serverHandshakeState) sendSessionTicket() error { func (hs *serverHandshakeState) sendFinished() error { c := hs.c + finished := new(finishedMsg) + finished.verifyData = hs.finishedHash.serverSum(hs.masterSecret) + postCCSBytes := finished.marshal() + hs.finishedHash.Write(postCCSBytes) + + if c.config.Bugs.FragmentAcrossChangeCipherSpec { + c.writeRecord(recordTypeHandshake, postCCSBytes[:5]) + postCCSBytes = postCCSBytes[5:] + } + if !c.config.Bugs.SkipChangeCipherSpec { c.writeRecord(recordTypeChangeCipherSpec, []byte{1}) } - finished := new(finishedMsg) - finished.verifyData = hs.finishedHash.serverSum(hs.masterSecret) - hs.finishedHash.Write(finished.marshal()) - c.writeRecord(recordTypeHandshake, finished.marshal()) + c.writeRecord(recordTypeHandshake, postCCSBytes) c.cipherSuite = hs.suite.id diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index f4a08916..1ed733c9 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -234,7 +234,7 @@ var testCases = []testCase{ }, }, shouldFail: true, - expectedError: ":GOT_A_FIN_BEFORE_A_CCS:", + expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:", }, { testType: serverTest, @@ -245,7 +245,7 @@ var testCases = []testCase{ }, }, shouldFail: true, - expectedError: ":GOT_A_FIN_BEFORE_A_CCS:", + expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:", }, { testType: serverTest, @@ -260,7 +260,43 @@ var testCases = []testCase{ "-advertise-npn", "\x03foo\x03bar\x03baz", }, shouldFail: true, - expectedError: ":GOT_NEXT_PROTO_BEFORE_A_CCS:", + expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:", + }, + { + name: "FragmentAcrossChangeCipherSpec-Client", + config: Config{ + Bugs: ProtocolBugs{ + FragmentAcrossChangeCipherSpec: true, + }, + }, + shouldFail: true, + expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:", + }, + { + testType: serverTest, + name: "FragmentAcrossChangeCipherSpec-Server", + config: Config{ + Bugs: ProtocolBugs{ + FragmentAcrossChangeCipherSpec: true, + }, + }, + shouldFail: true, + expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:", + }, + { + testType: serverTest, + name: "FragmentAcrossChangeCipherSpec-Server-NPN", + config: Config{ + NextProtos: []string{"bar"}, + Bugs: ProtocolBugs{ + FragmentAcrossChangeCipherSpec: true, + }, + }, + flags: []string{ + "-advertise-npn", "\x03foo\x03bar\x03baz", + }, + shouldFail: true, + expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:", }, { testType: serverTest,