kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

DTLS fragments may not be split across two records.

Browse Source 
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>
This commit is contained in:
David Benjamin 2015-06-16 11:40:24 -04:00 committed by Adam Langley
parent 95d3182576
commit 11fc66a04c
4 changed files with 31 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
ssl/d1_both.c
View File

@ -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);

15
ssl/test/runner/common.go
View File

@ -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.

11
ssl/test/runner/dtls.go
View File

@ -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)
}

21
ssl/test/runner/runner.go
View File

@ -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,