Bug 1487597 - Improve 0-RTT data delivery, r=ekr

Summary:
This improves the code that delivers 0-RTT.  When the caller provided a read
buffer to small to hold an entire record, the previous code reported errors.
Those errors might cause the connection to be dropped by the caller, but the
socket was still usable.  If the socket was used again, there would be a gap in
the stream.

This fixes that bug and adds a bunch of tests around 0-RTT delivery.  More tests
check the order of operations.

For instance, in TLS, we strictly maintain ordering between 0-RTT data delivery
and handshake completion.  That is not the case for DTLS, where this allows
0-RTT records that arrive before the handshake completes to be read afterwards.
We do drop keys as soon as we see EndOfEarlyData (this is going away for DTLS,
so I assume Certificate/Finished will be the trigger eventually).  The tests
added here confirm that late arrival causes 0-RTT to be dropped.  Another test
confirms that any early arrival that is only read late will be delivered.

Reviewers: ekr

Subscribers: mt, ekr

Tags: #secure-revision, PHID-PROJ-ffhf7tdvqze7zrdn6dh3

Bug #: 1487597

Differential Revision: https://phabricator.services.mozilla.com/D4736

5 files changed, 261 insertions, 16 deletions

diff --git a/gtests/ssl_gtest/ssl_0rtt_unittest.cc b/gtests/ssl_gtest/ssl_0rtt_unittest.cc
index 07eadfbd1..4a47592ca 100644
--- a/gtests/ssl_gtest/ssl_0rtt_unittest.cc
+++ b/gtests/ssl_gtest/ssl_0rtt_unittest.cc
@@ -649,6 +649,227 @@ TEST_P(TlsConnectTls13, ZeroRttOrdering) {
   EXPECT_EQ(2U, step);
 }
 
+// Early data remains available after the handshake completes for TLS.
+TEST_F(TlsConnectStreamTls13, ZeroRttLateReadTls) {
+  SetupForZeroRtt();
+  client_->Set0RttEnabled(true);
+  server_->Set0RttEnabled(true);
+  ExpectResumption(RESUME_TICKET);
+  client_->Handshake();  // ClientHello
+
+  // Write some early data.
+  const uint8_t data[] = {1, 2, 3, 4, 5, 6, 7, 8};
+  PRInt32 rv = PR_Write(client_->ssl_fd(), data, sizeof(data));
+  EXPECT_EQ(static_cast<PRInt32>(sizeof(data)), rv);
+
+  // Consume the ClientHello and generate ServerHello..Finished.
+  server_->Handshake();
+
+  // Read some of the data.
+  std::vector<uint8_t> small_buffer(1 + sizeof(data) / 2);
+  rv = PR_Read(server_->ssl_fd(), small_buffer.data(), small_buffer.size());
+  EXPECT_EQ(static_cast<PRInt32>(small_buffer.size()), rv);
+  EXPECT_EQ(0, memcmp(data, small_buffer.data(), small_buffer.size()));
+
+  Handshake();  // Complete the handshake.
+  ExpectEarlyDataAccepted(true);
+  CheckConnected();
+
+  // After the handshake, it should be possible to read the remainder.
+  uint8_t big_buf[100];
+  rv = PR_Read(server_->ssl_fd(), big_buf, sizeof(big_buf));
+  EXPECT_EQ(static_cast<PRInt32>(sizeof(data) - small_buffer.size()), rv);
+  EXPECT_EQ(0, memcmp(&data[small_buffer.size()], big_buf,
+                      sizeof(data) - small_buffer.size()));
+
+  // And that's all there is to read.
+  rv = PR_Read(server_->ssl_fd(), big_buf, sizeof(big_buf));
+  EXPECT_GT(0, rv);
+  EXPECT_EQ(PR_WOULD_BLOCK_ERROR, PORT_GetError());
+}
+
+// Early data that arrives before the handshake can be read after the handshake
+// is complete.
+TEST_F(TlsConnectDatagram13, ZeroRttLateReadDtls) {
+  SetupForZeroRtt();
+  client_->Set0RttEnabled(true);
+  server_->Set0RttEnabled(true);
+  ExpectResumption(RESUME_TICKET);
+  client_->Handshake();  // ClientHello
+
+  // Write some early data.
+  const uint8_t data[] = {1, 2, 3};
+  PRInt32 written = PR_Write(client_->ssl_fd(), data, sizeof(data));
+  EXPECT_EQ(static_cast<PRInt32>(sizeof(data)), written);
+
+  Handshake();  // Complete the handshake.
+  ExpectEarlyDataAccepted(true);
+  CheckConnected();
+
+  // Reading at the server should return the early data, which was buffered.
+  uint8_t buf[sizeof(data) + 1] = {0};
+  PRInt32 read = PR_Read(server_->ssl_fd(), buf, sizeof(buf));
+  EXPECT_EQ(static_cast<PRInt32>(sizeof(data)), read);
+  EXPECT_EQ(0, memcmp(data, buf, sizeof(data)));
+}
+
+class PacketHolder : public PacketFilter {
+ public:
+  PacketHolder() = default;
+
+  virtual Action Filter(const DataBuffer& input, DataBuffer* output) {
+    packet_ = input;
+    Disable();
+    return DROP;
+  }
+
+  const DataBuffer& packet() const { return packet_; }
+
+ private:
+  DataBuffer packet_;
+};
+
+// Early data that arrives late is discarded for DTLS.
+TEST_F(TlsConnectDatagram13, ZeroRttLateArrivalDtls) {
+  SetupForZeroRtt();
+  client_->Set0RttEnabled(true);
+  server_->Set0RttEnabled(true);
+  ExpectResumption(RESUME_TICKET);
+  client_->Handshake();  // ClientHello
+
+  // Write some early data.  Twice, so that we can read bits of it.
+  const uint8_t data[] = {1, 2, 3};
+  PRInt32 written = PR_Write(client_->ssl_fd(), data, sizeof(data));
+  EXPECT_EQ(static_cast<PRInt32>(sizeof(data)), written);
+
+  // Block and capture the next packet.
+  auto holder = std::make_shared<PacketHolder>();
+  client_->SetFilter(holder);
+  written = PR_Write(client_->ssl_fd(), data, sizeof(data));
+  EXPECT_EQ(static_cast<PRInt32>(sizeof(data)), written);
+  EXPECT_FALSE(holder->enabled()) << "the filter should disable itself";
+
+  // Consume the ClientHello and generate ServerHello..Finished.
+  server_->Handshake();
+
+  // Read some of the data.
+  std::vector<uint8_t> small_buffer(sizeof(data));
+  PRInt32 read =
+      PR_Read(server_->ssl_fd(), small_buffer.data(), small_buffer.size());
+
+  EXPECT_EQ(static_cast<PRInt32>(small_buffer.size()), read);
+  EXPECT_EQ(0, memcmp(data, small_buffer.data(), small_buffer.size()));
+
+  Handshake();  // Complete the handshake.
+  ExpectEarlyDataAccepted(true);
+  CheckConnected();
+
+  server_->SendDirect(holder->packet());
+
+  // Reading now should return nothing, even though a valid packet was
+  // delivered.
+  read = PR_Read(server_->ssl_fd(), small_buffer.data(), small_buffer.size());
+  EXPECT_GT(0, read);
+  EXPECT_EQ(PR_WOULD_BLOCK_ERROR, PORT_GetError());
+}
+
+// Early data reads in TLS should be coalesced.
+TEST_F(TlsConnectStreamTls13, ZeroRttCoalesceReadTls) {
+  SetupForZeroRtt();
+  client_->Set0RttEnabled(true);
+  server_->Set0RttEnabled(true);
+  ExpectResumption(RESUME_TICKET);
+  client_->Handshake();  // ClientHello
+
+  // Write some early data.  In two writes.
+  const uint8_t data[] = {1, 2, 3, 4, 5, 6};
+  PRInt32 written = PR_Write(client_->ssl_fd(), data, 1);
+  EXPECT_EQ(1, written);
+
+  written = PR_Write(client_->ssl_fd(), data + 1, sizeof(data) - 1);
+  EXPECT_EQ(static_cast<PRInt32>(sizeof(data) - 1), written);
+
+  // Consume the ClientHello and generate ServerHello..Finished.
+  server_->Handshake();
+
+  // Read all of the data.
+  std::vector<uint8_t> buffer(sizeof(data));
+  PRInt32 read = PR_Read(server_->ssl_fd(), buffer.data(), buffer.size());
+  EXPECT_EQ(static_cast<PRInt32>(sizeof(data)), read);
+  EXPECT_EQ(0, memcmp(data, buffer.data(), sizeof(data)));
+
+  Handshake();  // Complete the handshake.
+  ExpectEarlyDataAccepted(true);
+  CheckConnected();
+}
+
+// Early data reads in DTLS should not be coalesced.
+TEST_F(TlsConnectDatagram13, ZeroRttNoCoalesceReadDtls) {
+  SetupForZeroRtt();
+  client_->Set0RttEnabled(true);
+  server_->Set0RttEnabled(true);
+  ExpectResumption(RESUME_TICKET);
+  client_->Handshake();  // ClientHello
+
+  // Write some early data.  In two writes.
+  const uint8_t data[] = {1, 2, 3, 4, 5, 6};
+  PRInt32 written = PR_Write(client_->ssl_fd(), data, 1);
+  EXPECT_EQ(1, written);
+
+  written = PR_Write(client_->ssl_fd(), data + 1, sizeof(data) - 1);
+  EXPECT_EQ(static_cast<PRInt32>(sizeof(data) - 1), written);
+
+  // Consume the ClientHello and generate ServerHello..Finished.
+  server_->Handshake();
+
+  // Try to read all of the data.
+  std::vector<uint8_t> buffer(sizeof(data));
+  PRInt32 read = PR_Read(server_->ssl_fd(), buffer.data(), buffer.size());
+  EXPECT_EQ(1, read);
+  EXPECT_EQ(0, memcmp(data, buffer.data(), 1));
+
+  // Read the remainder.
+  read = PR_Read(server_->ssl_fd(), buffer.data(), buffer.size());
+  EXPECT_EQ(static_cast<PRInt32>(sizeof(data) - 1), read);
+  EXPECT_EQ(0, memcmp(data + 1, buffer.data(), sizeof(data) - 1));
+
+  Handshake();  // Complete the handshake.
+  ExpectEarlyDataAccepted(true);
+  CheckConnected();
+}
+
+// Early data reads in DTLS should fail if the buffer is too small.
+TEST_F(TlsConnectDatagram13, ZeroRttShortReadDtls) {
+  SetupForZeroRtt();
+  client_->Set0RttEnabled(true);
+  server_->Set0RttEnabled(true);
+  ExpectResumption(RESUME_TICKET);
+  client_->Handshake();  // ClientHello
+
+  // Write some early data.  In two writes.
+  const uint8_t data[] = {1, 2, 3, 4, 5, 6};
+  PRInt32 written = PR_Write(client_->ssl_fd(), data, sizeof(data));
+  EXPECT_EQ(static_cast<PRInt32>(sizeof(data)), written);
+
+  // Consume the ClientHello and generate ServerHello..Finished.
+  server_->Handshake();
+
+  // Try to read all of the data into a small buffer.
+  std::vector<uint8_t> buffer(sizeof(data));
+  PRInt32 read = PR_Read(server_->ssl_fd(), buffer.data(), 1);
+  EXPECT_GT(0, read);
+  EXPECT_EQ(SSL_ERROR_RX_SHORT_DTLS_READ, PORT_GetError());
+
+  // Read again with more space.
+  read = PR_Read(server_->ssl_fd(), buffer.data(), buffer.size());
+  EXPECT_EQ(static_cast<PRInt32>(sizeof(data)), read);
+  EXPECT_EQ(0, memcmp(data, buffer.data(), sizeof(data)));
+
+  Handshake();  // Complete the handshake.
+  ExpectEarlyDataAccepted(true);
+  CheckConnected();
+}
+
 #ifndef NSS_DISABLE_TLS_1_3
 INSTANTIATE_TEST_CASE_P(Tls13ZeroRttReplayTest, TlsZeroRttReplayTest,
                         TlsConnectTestBase::kTlsVariantsAll);
diff --git a/gtests/ssl_gtest/test_io.h b/gtests/ssl_gtest/test_io.h
index 8efd2305b..cabdd0fd4 100644
--- a/gtests/ssl_gtest/test_io.h
+++ b/gtests/ssl_gtest/test_io.h
@@ -33,9 +33,11 @@ class PacketFilter {
     CHANGE,  // change the packet to a different value
     DROP     // drop the packet
   };
-  PacketFilter(bool enabled = true) : enabled_(enabled) {}
+  explicit PacketFilter(bool enabled = true) : enabled_(enabled) {}
   virtual ~PacketFilter() {}
 
+  bool enabled() const { return enabled_; }
+
   virtual Action Process(const DataBuffer& input, DataBuffer* output) {
     if (!enabled_) {
       return KEEP;
diff --git a/gtests/ssl_gtest/tls_connect.cc b/gtests/ssl_gtest/tls_connect.cc
index 1d1dfd85d..596da6252 100644
--- a/gtests/ssl_gtest/tls_connect.cc
+++ b/gtests/ssl_gtest/tls_connect.cc
@@ -250,6 +250,7 @@ void TlsConnectTestBase::Reset(const std::string& server_name,
     server_->SkipVersionChecks();
   }
 
+  std::cerr << "Reset" << std::endl;
   Init();
 }
 
diff --git a/lib/ssl/sslimpl.h b/lib/ssl/sslimpl.h
index b70ddc955..a9b766290 100644
--- a/lib/ssl/sslimpl.h
+++ b/lib/ssl/sslimpl.h
@@ -576,8 +576,9 @@ struct TLS13KeyShareEntryStr {
 };
 
 typedef struct TLS13EarlyDataStr {
-    PRCList link; /* The linked list link */
-    SECItem data; /* The data */
+    PRCList link;          /* The linked list link */
+    unsigned int consumed; /* How much has been read. */
+    SECItem data;          /* The data */
 } TLS13EarlyData;
 
 typedef enum {
diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c
index 88bc618b3..d590ad70b 100644
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -5544,23 +5544,43 @@ tls13_MaybeDo0RTTHandshake(sslSocket *ss)
 PRInt32
 tls13_Read0RttData(sslSocket *ss, void *buf, PRInt32 len)
 {
-    TLS13EarlyData *msg;
-
     PORT_Assert(!PR_CLIST_IS_EMPTY(&ss->ssl3.hs.bufferedEarlyData));
-    msg = (TLS13EarlyData *)PR_NEXT_LINK(&ss->ssl3.hs.bufferedEarlyData);
+    PRInt32 offset = 0;
+    while (!PR_CLIST_IS_EMPTY(&ss->ssl3.hs.bufferedEarlyData)) {
+        TLS13EarlyData *msg =
+            (TLS13EarlyData *)PR_NEXT_LINK(&ss->ssl3.hs.bufferedEarlyData);
+        unsigned int tocpy = msg->data.len - msg->consumed;
+
+        if (tocpy > (len - offset)) {
+            if (IS_DTLS(ss)) {
+                /* In DTLS, we only return entire records.
+                 * So offset and consumed are always zero. */
+                PORT_Assert(offset == 0);
+                PORT_Assert(msg->consumed == 0);
+                PORT_SetError(SSL_ERROR_RX_SHORT_DTLS_READ);
+                return -1;
+            }
 
-    PR_REMOVE_LINK(&msg->link);
-    if (msg->data.len > len) {
-        PORT_SetError(SSL_ERROR_ILLEGAL_PARAMETER_ALERT);
-        return SECFailure;
-    }
-    len = msg->data.len;
+            tocpy = len - offset;
+        }
+
+        PORT_Memcpy(buf + offset, msg->data.data + msg->consumed, tocpy);
+        offset += tocpy;
+        msg->consumed += tocpy;
 
-    PORT_Memcpy(buf, msg->data.data, msg->data.len);
-    SECITEM_ZfreeItem(&msg->data, PR_FALSE);
-    PORT_ZFree(msg, sizeof(*msg));
+        if (msg->consumed == msg->data.len) {
+            PR_REMOVE_LINK(&msg->link);
+            SECITEM_ZfreeItem(&msg->data, PR_FALSE);
+            PORT_ZFree(msg, sizeof(*msg));
+        }
+
+        /* We are done after one record for DTLS; otherwise, when the buffer fills up. */
+        if (IS_DTLS(ss) || offset == len) {
+            break;
+        }
+    }
 
-    return len;
+    return offset;
 }
 
 static SECStatus