Change CCS_OK to EXPECT_CCS.
Now that the flag is set accurately, use it to enforce that the handshake and
CCS synchronization. If EXPECT_CCS is set, enforce that:
(a) No handshake records may be received before ChangeCipherSpec.
(b) There is no pending handshake data at the point EXPECT_CCS is set.
Change-Id: I04b228fe6a7a771cf6600b7d38aa762b2d553f08
Reviewed-on: https://boringssl-review.googlesource.com/1299
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/s3_both.c b/ssl/s3_both.c
index 6c8bc15..8d05201 100644
--- a/ssl/s3_both.c
+++ b/ssl/s3_both.c
@@ -245,7 +245,9 @@
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 52d88c6..72966c7 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -517,7 +517,11 @@
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 97530a3..4191059 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -901,6 +901,22 @@
}
}
+/* 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 @@
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 @@
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 @@
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 36b421e..c9c01fd 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -573,7 +573,11 @@
/* 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 @@
/* 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 @@
/* 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 0097d60..c989c84 100644
--- a/ssl/ssl_error.c
+++ b/ssl/ssl_error.c
@@ -108,6 +108,7 @@
{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 @@
{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 @@
{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 57d63c8..7f63ce6 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -947,6 +947,7 @@
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 7319463..7b8d964 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -375,6 +375,11 @@
// 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 32b90e2..80208dd 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -584,10 +584,7 @@
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 @@
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 @@
} 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 8cdecd7..68ba734 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -598,14 +598,21 @@
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 f4a0891..1ed733c 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -234,7 +234,7 @@
},
},
shouldFail: true,
- expectedError: ":GOT_A_FIN_BEFORE_A_CCS:",
+ expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:",
},
{
testType: serverTest,
@@ -245,7 +245,7 @@
},
},
shouldFail: true,
- expectedError: ":GOT_A_FIN_BEFORE_A_CCS:",
+ expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:",
},
{
testType: serverTest,
@@ -260,7 +260,43 @@
"-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,