Add tests for trailing data in handshake messages.
It's easy to forget to check those. Unfortunately, it's also easy to
forget to check inner structures, which is going to be harder to stress,
but do these to start with. In doing, so fix up and unify some
error-handling, and add a missing check when parsing TLS 1.2
CertificateRequest.
This was also inspired by the recent IETF posting.
Change-Id: I27fe3cd3506258389a75d486036388400f0a33ba
Reviewed-on: https://boringssl-review.googlesource.com/10963
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/handshake_client.c b/ssl/handshake_client.c
index 36a070b..dd3ab04 100644
--- a/ssl/handshake_client.c
+++ b/ssl/handshake_client.c
@@ -985,7 +985,7 @@
if (CBS_len(&server_hello) != 0) {
/* wrong packet length */
al = SSL_AD_DECODE_ERROR;
- OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_PACKET_LENGTH);
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
goto f_err;
}
@@ -1430,6 +1430,13 @@
return -1;
}
+ if (CBS_len(&cbs) != 0) {
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
+ sk_X509_NAME_pop_free(ca_sk, X509_NAME_free);
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+ return -1;
+ }
+
ssl->s3->tmp.cert_request = 1;
sk_X509_NAME_pop_free(ssl->s3->tmp.ca_names, X509_NAME_free);
ssl->s3->tmp.ca_names = ca_sk;
@@ -1446,7 +1453,7 @@
/* ServerHelloDone is empty. */
if (ssl->init_num > 0) {
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
- OPENSSL_PUT_ERROR(SSL, SSL_R_LENGTH_MISMATCH);
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
return -1;
}
diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c
index c48ea6c..421db42 100644
--- a/ssl/handshake_server.c
+++ b/ssl/handshake_server.c
@@ -1730,8 +1730,13 @@
CBS_init(&next_protocol, ssl->init_msg, ssl->init_num);
if (!CBS_get_u8_length_prefixed(&next_protocol, &selected_protocol) ||
!CBS_get_u8_length_prefixed(&next_protocol, &padding) ||
- CBS_len(&next_protocol) != 0 ||
- !CBS_stow(&selected_protocol, &ssl->s3->next_proto_negotiated,
+ CBS_len(&next_protocol) != 0) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
+ return 0;
+ }
+
+ if (!CBS_stow(&selected_protocol, &ssl->s3->next_proto_negotiated,
&ssl->s3->next_proto_negotiated_len)) {
return 0;
}
@@ -1780,7 +1785,8 @@
CBS_len(&encrypted_extensions) != 0 ||
extension_type != TLSEXT_TYPE_channel_id ||
CBS_len(&extension) != TLSEXT_CHANNEL_ID_SIZE) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_MESSAGE);
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
return -1;
}
diff --git a/ssl/s3_both.c b/ssl/s3_both.c
index aa8dea6..4baa839 100644
--- a/ssl/s3_both.c
+++ b/ssl/s3_both.c
@@ -314,18 +314,14 @@
}
size_t finished_len = ssl->s3->tmp.peer_finish_md_len;
- if (finished_len != ssl->init_num) {
- al = SSL_AD_DECODE_ERROR;
- OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_DIGEST_LENGTH);
- goto f_err;
- }
- int finished_ret =
- CRYPTO_memcmp(ssl->init_msg, ssl->s3->tmp.peer_finish_md, finished_len);
+ int finished_ok = ssl->init_num == finished_len &&
+ CRYPTO_memcmp(ssl->init_msg, ssl->s3->tmp.peer_finish_md,
+ finished_len) == 0;
#if defined(BORINGSSL_UNSAFE_FUZZER_MODE)
- finished_ret = 0;
+ finished_ok = 1;
#endif
- if (finished_ret != 0) {
+ if (!finished_ok) {
al = SSL_AD_DECRYPT_ERROR;
OPENSSL_PUT_ERROR(SSL, SSL_R_DIGEST_CHECK_FAILED);
goto f_err;
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index c73d74c..eef1dbe 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -791,6 +791,10 @@
// type to be sent with the wrong value.
SendWrongMessageType byte
+ // SendTrailingMessageData, if non-zero, causes messages of the
+ // specified type to be sent with trailing data.
+ SendTrailingMessageData byte
+
// FragmentMessageTypeMismatch, if true, causes all non-initial
// handshake fragments in DTLS to have the wrong message type.
FragmentMessageTypeMismatch bool
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go
index 77543e6..9969f8b 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -955,8 +955,8 @@
// to the connection and updates the record layer state.
// c.out.Mutex <= L.
func (c *Conn) writeRecord(typ recordType, data []byte) (n int, err error) {
- if wrongType := c.config.Bugs.SendWrongMessageType; wrongType != 0 {
- if typ == recordTypeHandshake && data[0] == wrongType {
+ if msgType := c.config.Bugs.SendWrongMessageType; msgType != 0 {
+ if typ == recordTypeHandshake && data[0] == msgType {
newData := make([]byte, len(data))
copy(newData, data)
newData[0] += 42
@@ -964,6 +964,23 @@
}
}
+ if msgType := c.config.Bugs.SendTrailingMessageData; msgType != 0 {
+ if typ == recordTypeHandshake && data[0] == msgType {
+ newData := make([]byte, len(data))
+ copy(newData, data)
+
+ // Add a 0 to the body.
+ newData = append(newData, 0)
+ // Fix the header.
+ newLen := len(newData) - 4
+ newData[1] = byte(newLen >> 16)
+ newData[2] = byte(newLen >> 8)
+ newData[3] = byte(newLen)
+
+ data = newData
+ }
+ }
+
if c.isDTLS {
return c.dtlsWriteRecord(typ, data)
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index dc2376c..8319690 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -7914,6 +7914,32 @@
}
}
+func addTrailingMessageDataTests() {
+ for _, t := range makePerMessageTests() {
+ t.test.name = "TrailingMessageData-" + t.test.name
+ t.test.config.Bugs.SendTrailingMessageData = t.messageType
+ t.test.shouldFail = true
+ t.test.expectedError = ":DECODE_ERROR:"
+ t.test.expectedLocalError = "remote error: error decoding message"
+
+ if t.test.config.MaxVersion >= VersionTLS13 && t.messageType == typeServerHello {
+ // In TLS 1.3, a bad ServerHello means the client sends
+ // an unencrypted alert while the server expects
+ // encryption, so the alert is not readable by runner.
+ t.test.expectedLocalError = "local error: bad record MAC"
+ }
+
+ if t.messageType == typeFinished {
+ // Bad Finished messages read as the verify data having
+ // the wrong length.
+ t.test.expectedError = ":DIGEST_CHECK_FAILED:"
+ t.test.expectedLocalError = "remote error: error decrypting message"
+ }
+
+ testCases = append(testCases, t.test)
+ }
+}
+
func addTLS13HandshakeTests() {
testCases = append(testCases, testCase{
testType: clientTest,
@@ -8289,6 +8315,7 @@
addAllStateMachineCoverageTests()
addChangeCipherSpecTests()
addWrongMessageTypeTests()
+ addTrailingMessageDataTests()
addTLS13HandshakeTests()
var wg sync.WaitGroup
diff --git a/ssl/tls13_client.c b/ssl/tls13_client.c
index 1bd0f61..cf3f284 100644
--- a/ssl/tls13_client.c
+++ b/ssl/tls13_client.c
@@ -419,6 +419,7 @@
if (!CBS_get_u16_length_prefixed(&cbs, &extensions) ||
CBS_len(&cbs) != 0) {
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
+ sk_X509_NAME_pop_free(ca_sk, X509_NAME_free);
OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
return ssl_hs_error;
}