DTLS fragments may not be split across two records.
See also upstream's 9dcab127e14467733523ff7626da8906e67eedd6. The root problem
is dtls1_read_bytes is wrong, but we can get the right behavior now and add a
regression test for it before cleaning it up.
Change-Id: I4e5c39ab254a872d9f64242c9b77b020bdded6e6
Reviewed-on: https://boringssl-review.googlesource.com/5123
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/d1_both.c b/ssl/d1_both.c
index ac35a66..f8724ba 100644
--- a/ssl/d1_both.c
+++ b/ssl/d1_both.c
@@ -473,11 +473,7 @@
/* dtls1_process_fragment reads a handshake fragment and processes it. It
* returns one if a fragment was successfully processed and 0 or -1 on error. */
static int dtls1_process_fragment(SSL *s) {
- /* Read handshake message header.
- *
- * TODO(davidben): ssl_read_bytes allows splitting the fragment header and
- * body across two records. Change this interface to consume the fragment in
- * one pass. */
+ /* Read handshake message header. */
uint8_t header[DTLS1_HM_HEADER_LENGTH];
int ret = dtls1_read_bytes(s, SSL3_RT_HANDSHAKE, header,
DTLS1_HM_HEADER_LENGTH, 0);
@@ -494,12 +490,15 @@
struct hm_header_st msg_hdr;
dtls1_get_message_header(header, &msg_hdr);
+ /* TODO(davidben): dtls1_read_bytes is the wrong abstraction for DTLS. There
+ * should be no need to reach into |s->s3->rrec.length|. */
const size_t frag_off = msg_hdr.frag_off;
const size_t frag_len = msg_hdr.frag_len;
const size_t msg_len = msg_hdr.msg_len;
if (frag_off > msg_len || frag_off + frag_len < frag_off ||
frag_off + frag_len > msg_len ||
- msg_len > dtls1_max_handshake_message_len(s)) {
+ msg_len > dtls1_max_handshake_message_len(s) ||
+ frag_len > s->s3->rrec.length) {
OPENSSL_PUT_ERROR(SSL, dtls1_process_fragment,
SSL_R_EXCESSIVE_MESSAGE_SIZE);
ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
@@ -535,8 +534,8 @@
ret = dtls1_read_bytes(s, SSL3_RT_HANDSHAKE, frag->fragment + frag_off,
frag_len, 0);
if (ret != frag_len) {
- OPENSSL_PUT_ERROR(SSL, dtls1_process_fragment, SSL_R_UNEXPECTED_MESSAGE);
- ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+ OPENSSL_PUT_ERROR(SSL, dtls1_process_fragment, ERR_R_INTERNAL_ERROR);
+ ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
return -1;
}
dtls1_hm_fragment_mark(frag, frag_off, frag_off + frag_len);
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 207de0c..928c2b2 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -668,17 +668,10 @@
// handshake fragments in DTLS to have the wrong message length.
FragmentMessageLengthMismatch bool
- // SplitFragmentHeader, if true, causes the handshake fragments in DTLS
- // to be split across two records.
- SplitFragmentHeader bool
-
- // SplitFragmentBody, if true, causes the handshake bodies in DTLS to be
- // split across two records.
- //
- // TODO(davidben): There's one final split to test: when the header and
- // body are split across two records. But those are (incorrectly)
- // accepted right now.
- SplitFragmentBody bool
+ // SplitFragments, if non-zero, causes the handshake fragments in DTLS
+ // to be split across two records. The value of |SplitFragments| is the
+ // number of bytes in the first fragment.
+ SplitFragments int
// SendEmptyFragments, if true, causes handshakes to include empty
// fragments in DTLS.
diff --git a/ssl/test/runner/dtls.go b/ssl/test/runner/dtls.go
index 50f7786..538bf51 100644
--- a/ssl/test/runner/dtls.go
+++ b/ssl/test/runner/dtls.go
@@ -216,13 +216,10 @@
// Pack handshake fragments into records.
var records [][]byte
for _, fragment := range fragments {
- if c.config.Bugs.SplitFragmentHeader {
- records = append(records, fragment[:2])
- records = append(records, fragment[2:])
- } else if c.config.Bugs.SplitFragmentBody {
- if len(fragment) > 12 {
- records = append(records, fragment[:13])
- records = append(records, fragment[13:])
+ if n := c.config.Bugs.SplitFragments; n > 0 {
+ if len(fragment) > n {
+ records = append(records, fragment[:n])
+ records = append(records, fragment[n:])
} else {
records = append(records, fragment)
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 524b3d0..f02a221 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -913,10 +913,10 @@
},
{
protocol: dtls,
- name: "SplitFragmentHeader-DTLS",
+ name: "SplitFragments-Header-DTLS",
config: Config{
Bugs: ProtocolBugs{
- SplitFragmentHeader: true,
+ SplitFragments: 2,
},
},
shouldFail: true,
@@ -924,14 +924,25 @@
},
{
protocol: dtls,
- name: "SplitFragmentBody-DTLS",
+ name: "SplitFragments-Boundary-DTLS",
config: Config{
Bugs: ProtocolBugs{
- SplitFragmentBody: true,
+ SplitFragments: dtlsRecordHeaderLen,
},
},
shouldFail: true,
- expectedError: ":UNEXPECTED_MESSAGE:",
+ expectedError: ":EXCESSIVE_MESSAGE_SIZE:",
+ },
+ {
+ protocol: dtls,
+ name: "SplitFragments-Body-DTLS",
+ config: Config{
+ Bugs: ProtocolBugs{
+ SplitFragments: dtlsRecordHeaderLen + 1,
+ },
+ },
+ shouldFail: true,
+ expectedError: ":EXCESSIVE_MESSAGE_SIZE:",
},
{
protocol: dtls,
