Check for message sequence overflow in DTLS

In DTLS 1.2, message sequence number overflow was impossible. The
counter reset on every handshake, and every handshake had a bounded
number of messages. (The counter reset was actually problematic. The end
of one handshake and the start of the next shared epochs, so sequence
numbers become ambiguous. 1.3 fixes this by removing the reset but, in
hindsight, resetting on each epoch would probably have been better.)

In DTLS 1.3, there is no bound and overflow is possible. Check for
overflow. Somewhat annoyingly, because we tend to store the next
sequence number, we actually need 17 bits per sequence number to
represent this, if we want to accept up to the maximum possible message.
Test this on the read side with KeyUpdate. (To test this on the write
side, we must be able to send KeyUpdate.)

Bug: 42290594
Change-Id: I691855dc72427afb9e82d8de0fb2eeea6818f2ea
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/73507
Reviewed-by: Nick Harper <nharper@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>

ssl/d1_both.cc

diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc
index e5cc7ac..b337b37 100644
--- a/ssl/d1_both.cc
+++ b/ssl/d1_both.cc
@@ -375,7 +375,8 @@
       return false;
     }
 
-    if (msg_hdr.seq < ssl->d1->handshake_read_seq) {
+    if (msg_hdr.seq < ssl->d1->handshake_read_seq ||
+        ssl->d1->handshake_read_overflow) {
       // Ignore fragments from the past. This is a retransmit of data we already
       // received.
       //
@@ -549,6 +550,9 @@
   size_t index = ssl->d1->handshake_read_seq % SSL_MAX_HANDSHAKE_FLIGHT;
   ssl->d1->incoming_messages[index].reset();
   ssl->d1->handshake_read_seq++;
+  if (ssl->d1->handshake_read_seq == 0) {
+    ssl->d1->handshake_read_overflow = true;
+  }
   ssl->s3->has_message = false;
   // If we previously sent a flight, mark it as having a reply, so
   // |on_handshake_complete| can manage post-handshake retransmission.
@@ -674,6 +678,10 @@
   }
 
   if (!is_ccs) {
+    if (ssl->d1->handshake_write_overflow) {
+      OPENSSL_PUT_ERROR(SSL, ERR_R_OVERFLOW);
+      return false;
+    }
     // TODO(svaldez): Move this up a layer to fix abstraction for SSLTranscript
     // on hs.
     if (ssl->s3->hs != NULL && !ssl->s3->hs->transcript.Update(data)) {
@@ -681,6 +689,9 @@
       return false;
     }
     ssl->d1->handshake_write_seq++;
+    if (ssl->d1->handshake_write_seq == 0) {
+      ssl->d1->handshake_write_overflow = true;
+    }
   }
 
   DTLSOutgoingMessage msg;

ssl/d1_lib.cc

diff --git a/ssl/d1_lib.cc b/ssl/d1_lib.cc
index 2c7a1cd..520bde3 100644
--- a/ssl/d1_lib.cc
+++ b/ssl/d1_lib.cc
@@ -81,7 +81,9 @@
 DTLS1_STATE::DTLS1_STATE()
     : has_change_cipher_spec(false),
       outgoing_messages_complete(false),
-      flight_has_reply(false) {}
+      flight_has_reply(false),
+      handshake_write_overflow(false),
+      handshake_read_overflow(false) {}
 
 DTLS1_STATE::~DTLS1_STATE() {}
 

ssl/internal.h

diff --git a/ssl/internal.h b/ssl/internal.h
index a8ceb16..9721cb1 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -3555,6 +3555,11 @@
   // peer sent the final flight.
   bool flight_has_reply : 1;
 
+  // handshake_write_overflow and handshake_read_overflow are true if
+  // handshake_write_seq and handshake_read_seq, respectively have overflowed.
+  bool handshake_write_overflow : 1;
+  bool handshake_read_overflow : 1;
+
   uint16_t handshake_write_seq = 0;
   uint16_t handshake_read_seq = 0;
 

ssl/test/runner/runner.go

diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 5ec1382..fbb8bf9 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -22267,9 +22267,6 @@
 	// Test KeyUpdate overflow conditions. Both the epoch number and the message
 	// number may overflow, in either the read or write direction.
 	//
-	// TODO(crbug.com/42290594): Test the message read number overflowing, once
-	// we fix the lack of checking for it.
-	//
 	// TODO(crbug.com/42290594): Test the epoch write number overflowing, once
 	// we implement sending KeyUpdates.
 	//
@@ -22327,6 +22324,44 @@
 		expectedError:      ":TOO_MANY_KEY_UPDATES:",
 		expectedLocalError: "remote error: unexpected message",
 	})
+
+	// When the runner is a server, the first KeyUpdate is message 7 (SH, EE, C,
+	// CV, Fin, NST, NST) at epoch 3, so the message number overflows first.
+	// Test that the shim, as a client, does not allow the value to wraparound.
+	testCases = append(testCases, testCase{
+		protocol: dtls,
+		name:     "KeyUpdate-ReadMessageOverflow-DTLS",
+		config: Config{
+			MaxVersion: VersionTLS13,
+			Bugs: ProtocolBugs{
+				AllowEpochOverflow: true,
+				WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) {
+					writeFlightKeyUpdate(c, prev, received, next, records)
+					if next[0].Type == typeKeyUpdate && next[0].Sequence == 0xffff {
+						// At this point, the shim has accepted message 0xffff.
+						// Check the shim does not now accept message 0 as the
+						// current message. Test this by sending a garbage
+						// message 0. A shim that overflows and processes the
+						// message will notice the syntax error. A shim that
+						// correctly interprets this as an old message will drop
+						// the record and simply ACK it.
+						//
+						// We do this rather than send a valid KeyUpdate because
+						// the shim will keep the old epoch active and drop
+						// decryption failures. Looking for the lack of an error
+						// is more straightforward.
+						c.WriteFlight([]DTLSMessage{{Epoch: c.OutEpoch(), Sequence: 0, Type: typeKeyUpdate, Data: []byte("INVALID")}})
+						c.ExpectNextTimeout(timeouts[0] / 4)
+						c.AdvanceClock(timeouts[0] / 4)
+						c.ReadACK(c.InEpoch())
+					}
+				},
+			},
+		},
+		sendKeyUpdates:   0xffff - 7 + 1,
+		keyUpdateRequest: keyUpdateNotRequested,
+		flags:            []string{"-async"},
+	})
 }
 
 func worker(dispatcher *shimDispatcher, statusChan chan statusMsg, c chan *testCase, shimPath string, wg *sync.WaitGroup) {