From 02edcd009800c9d5354c1bb7080d0ec4dd4f23eb Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 27 Jul 2016 17:40:37 -0400 Subject: [PATCH] Reject stray post-Finished messages in DTLS. This is in preparation for switching finish_handshake to a release_current_message hook. finish_handshake in DTLS is also responsible for releasing any memory associated with extra messages in the handshake. Except that's not right and we need to make it an error anyway. Given that the rest of the DTLS dispatch layer already strongly assumes there is only one message in epoch one, putting the check in the fragment processing works fine enough. Add tests for this. This will certainly need revising when DTLS 1.3 happens (perhaps just a version check, perhaps bringing finish_handshake back as a function that can fail... which means we need a state just before SSL_ST_OK), but DTLS 1.3 post-handshake messages haven't really been written down, so let's do the easy thing for now and add a test for when it gets more interesting. This removes the sequence number reset in the DTLS code. That reset never did anything becase we don't and never will renego. We should make sure DTLS 1.3 does not bring the reset back for post-handshake stuff. (It was wrong in 1.2 too. Penultimate-flight retransmits and renego requests are ambiguous in DTLS.) BUG=83 Change-Id: I33d645a8550f73e74606030b9815fdac0c9fb682 Reviewed-on: https://boringssl-review.googlesource.com/8988 Reviewed-by: Adam Langley --- ssl/d1_both.c | 7 +++++++ ssl/d1_pkt.c | 3 ++- ssl/dtls_method.c | 2 -- ssl/test/runner/common.go | 4 ++++ ssl/test/runner/handshake_client.go | 9 +++++++++ ssl/test/runner/handshake_server.go | 8 ++++++++ ssl/test/runner/runner.go | 24 ++++++++++++++++++++++++ 7 files changed, 54 insertions(+), 3 deletions(-) diff --git a/ssl/d1_both.c b/ssl/d1_both.c index 331703ad..1d9cd75c 100644 --- a/ssl/d1_both.c +++ b/ssl/d1_both.c @@ -368,6 +368,13 @@ start: return -1; } + /* The encrypted epoch in DTLS has only one handshake message. */ + if (ssl->d1->r_epoch == 1 && msg_hdr.seq != ssl->d1->handshake_read_seq) { + OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD); + ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE); + return -1; + } + if (msg_hdr.seq < ssl->d1->handshake_read_seq || msg_hdr.seq > (unsigned)ssl->d1->handshake_read_seq + SSL_MAX_HANDSHAKE_FLIGHT) { diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c index 2376e968..ceb6e179 100644 --- a/ssl/d1_pkt.c +++ b/ssl/d1_pkt.c @@ -223,7 +223,8 @@ again: return -1; } - if (msg_hdr.type == SSL3_MT_FINISHED) { + if (msg_hdr.type == SSL3_MT_FINISHED && + msg_hdr.seq == ssl->d1->handshake_read_seq) { if (msg_hdr.frag_off == 0) { /* Retransmit our last flight of messages. If the peer sends the second * Finished, they may not have received ours. Only do this for the diff --git a/ssl/dtls_method.c b/ssl/dtls_method.c index 15d90f61..09bd4550 100644 --- a/ssl/dtls_method.c +++ b/ssl/dtls_method.c @@ -93,8 +93,6 @@ static uint16_t dtls1_version_to_wire(uint16_t version) { } static void dtls1_finish_handshake(SSL *ssl) { - ssl->d1->handshake_read_seq = 0; - ssl->d1->handshake_write_seq = 0; dtls_clear_incoming_messages(ssl); ssl->init_msg = NULL; ssl->init_num = 0; diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index 1014dea5..141f160d 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go @@ -1009,6 +1009,10 @@ type ProtocolBugs struct { // PackHelloRequestWithFinished, if true, causes the TLS server to send // HelloRequest in the same record as Finished. PackHelloRequestWithFinished bool + + // SendExtraFinished, if true, causes an extra Finished message to be + // sent. + SendExtraFinished bool } func (c *Config) serverInit() { diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index b32be0e8..36bd7e43 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go @@ -493,6 +493,7 @@ NextCipherSuite: // leg of the handshake is retransmited upon re-receiving a // Finished. if err := c.simulatePacketLoss(func() { + c.sendHandshakeSeq-- c.writeRecord(recordTypeHandshake, hs.finishedBytes) c.flushHandshake() }); err != nil { @@ -764,6 +765,9 @@ func (hs *clientHandshakeState) doTLS13Handshake() error { } else { c.writeRecord(recordTypeHandshake, finished.marshal()) } + if c.config.Bugs.SendExtraFinished { + c.writeRecord(recordTypeHandshake, finished.marshal()) + } c.flushHandshake() // Switch to application data keys. @@ -1345,6 +1349,11 @@ func (hs *clientHandshakeState) sendFinished(out []byte, isResume bool) error { for _, msg := range postCCSMsgs { c.writeRecord(recordTypeHandshake, msg) } + + if c.config.Bugs.SendExtraFinished { + c.writeRecord(recordTypeHandshake, finished.marshal()) + } + c.flushHandshake() } return nil diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index 012c8364..ff8005bd 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go @@ -84,6 +84,7 @@ func (c *Conn) serverHandshake() error { // leg of the handshake is retransmited upon re-receiving a // Finished. if err := c.simulatePacketLoss(func() { + c.sendHandshakeSeq-- c.writeRecord(recordTypeHandshake, hs.finishedBytes) c.flushHandshake() }); err != nil { @@ -582,6 +583,9 @@ Curves: } hs.writeServerHash(finished.marshal()) c.writeRecord(recordTypeHandshake, finished.marshal()) + if c.config.Bugs.SendExtraFinished { + c.writeRecord(recordTypeHandshake, finished.marshal()) + } c.flushHandshake() // The various secrets do not incorporate the client's final leg, so @@ -1399,6 +1403,10 @@ func (hs *serverHandshakeState) sendFinished(out []byte) error { if !c.config.Bugs.SkipFinished && len(postCCSBytes) > 0 { c.writeRecord(recordTypeHandshake, postCCSBytes) + if c.config.Bugs.SendExtraFinished { + c.writeRecord(recordTypeHandshake, finished.marshal()) + } + if !c.config.Bugs.PackHelloRequestWithFinished { // Defer flushing until renegotiation. c.flushHandshake() diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index d3cd77ff..cde55926 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -2086,6 +2086,30 @@ func addBasicTests() { }, resumeSession: true, }, + { + protocol: dtls, + name: "DTLS-SendExtraFinished", + config: Config{ + Bugs: ProtocolBugs{ + SendExtraFinished: true, + }, + }, + shouldFail: true, + expectedError: ":UNEXPECTED_RECORD:", + }, + { + protocol: dtls, + name: "DTLS-SendExtraFinished-Reordered", + config: Config{ + Bugs: ProtocolBugs{ + MaxHandshakeRecordLength: 2, + ReorderHandshakeFragments: true, + SendExtraFinished: true, + }, + }, + shouldFail: true, + expectedError: ":UNEXPECTED_RECORD:", + }, } testCases = append(testCases, basicTests...) }