From 11fc66a04c100af403efb075fe3f7f6aafa87602 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 16 Jun 2015 11:40:24 -0400 Subject: [PATCH] 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 --- ssl/d1_both.c | 15 +++++++-------- ssl/test/runner/common.go | 15 ++++----------- ssl/test/runner/dtls.go | 11 ++++------- ssl/test/runner/runner.go | 21 ++++++++++++++++----- 4 files changed, 31 insertions(+), 31 deletions(-) diff --git a/ssl/d1_both.c b/ssl/d1_both.c index ac35a660..f8724bac 100644 --- a/ssl/d1_both.c +++ b/ssl/d1_both.c @@ -473,11 +473,7 @@ static size_t dtls1_max_handshake_message_len(const SSL *s) { /* 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 @@ static int dtls1_process_fragment(SSL *s) { 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 @@ static int dtls1_process_fragment(SSL *s) { 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 207de0cb..928c2b26 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go @@ -668,17 +668,10 @@ type ProtocolBugs struct { // 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 50f7786a..538bf511 100644 --- a/ssl/test/runner/dtls.go +++ b/ssl/test/runner/dtls.go @@ -216,13 +216,10 @@ func (c *Conn) dtlsFlushHandshake() error { // 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 524b3d0c..f02a2218 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -913,10 +913,10 @@ var testCases = []testCase{ }, { protocol: dtls, - name: "SplitFragmentHeader-DTLS", + name: "SplitFragments-Header-DTLS", config: Config{ Bugs: ProtocolBugs{ - SplitFragmentHeader: true, + SplitFragments: 2, }, }, shouldFail: true, @@ -924,14 +924,25 @@ var testCases = []testCase{ }, { 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,