Bug 1386475 - Ensure early data is delivered before the handshake cal…

…lback fires, r=ekr

--HG--
extra : source : eef3bbd90a81c32ba1b48afa7b3ad4dc70c905fb
extra : amend_source : 43adb30b7f0ac5ffcfa68db9a1b8ddd31c8ba50d

gtests/ssl_gtest/ssl_0rtt_unittest.cc

@@ -400,4 +400,87 @@ TEST_P(TlsConnectTls13, ReceiveTooMuchEarlyData) {
   }
 }
 
+class PacketCoalesceFilter : public PacketFilter {
+ public:
+  PacketCoalesceFilter() : packet_data_() {}
+
+  void SendCoalesced(std::shared_ptr<TlsAgent> agent) {
+    agent->SendDirect(packet_data_);
+  }
+
+ protected:
+  PacketFilter::Action Filter(const DataBuffer& input,
+                              DataBuffer* output) override {
+    packet_data_.Write(packet_data_.len(), input);
+    return DROP;
+  }
+
+ private:
+  DataBuffer packet_data_;
+};
+
+TEST_P(TlsConnectTls13, ZeroRttOrdering) {
+  SetupForZeroRtt();
+  client_->Set0RttEnabled(true);
+  server_->Set0RttEnabled(true);
+  ExpectResumption(RESUME_TICKET);
+
+  // Send out the ClientHello.
+  client_->Handshake();
+
+  // Now, coalesce the next three things from the client: early data, second
+  // flight and 1-RTT data.
+  auto coalesce = std::make_shared<PacketCoalesceFilter>();
+  client_->SetPacketFilter(coalesce);
+
+  // Send (and hold) early data.
+  static const std::vector<uint8_t> early_data = {3, 2, 1};
+  EXPECT_EQ(static_cast<PRInt32>(early_data.size()),
+            PR_Write(client_->ssl_fd(), early_data.data(), early_data.size()));
+
+  // Send (and hold) the second client handshake flight.
+  // The client sends EndOfEarlyData after seeing the server Finished.
+  server_->Handshake();
+  client_->Handshake();
+
+  // Send (and hold) 1-RTT data.
+  static const std::vector<uint8_t> late_data = {7, 8, 9, 10};
+  EXPECT_EQ(static_cast<PRInt32>(late_data.size()),
+            PR_Write(client_->ssl_fd(), late_data.data(), late_data.size()));
+
+  // Now release them all at once.
+  coalesce->SendCoalesced(client_);
+
+  // Now ensure that the three steps are exposed in the right order on the
+  // server: delivery of early data, handshake callback, delivery of 1-RTT.
+  size_t step = 0;
+  server_->SetHandshakeCallback([&step](TlsAgent*) {
+    EXPECT_EQ(1U, step);
+    ++step;
+  });
+
+  std::vector<uint8_t> buf(10);
+  // The first read here blocks because there isn't any 0-RTT to deliver so it
+  // processes everything.  When the EndOfEarlyData message is read, it stalls
+  // out of the loop.  The second read should return the early data.
+  PRInt32 read = PR_Read(server_->ssl_fd(), buf.data(), buf.size());
+  EXPECT_GT(0, read);
+  EXPECT_EQ(PR_WOULD_BLOCK_ERROR, PORT_GetError());
+  read = PR_Read(server_->ssl_fd(), buf.data(), buf.size());
+  ASSERT_EQ(static_cast<PRInt32>(early_data.size()), read);
+  buf.resize(read);
+  EXPECT_EQ(early_data, buf);
+  EXPECT_EQ(0U, step);
+  ++step;
+
+  // The third read should be after the handshake callback and should return the
+  // data that was sent after the handshake completed.
+  buf.resize(10);
+  read = PR_Read(server_->ssl_fd(), buf.data(), buf.size());
+  ASSERT_EQ(static_cast<PRInt32>(late_data.size()), read);
+  buf.resize(read);
+  EXPECT_EQ(late_data, buf);
+  EXPECT_EQ(2U, step);
+}
+
 }  // namespace nss_test

lib/ssl/ssl.h

@@ -231,10 +231,17 @@ SSL_IMPORT PRFileDesc *DTLS_ImportFD(PRFileDesc *model, PRFileDesc *fd);
  * parameters.
  *
  * The transition between the 0-RTT and 1-RTT modes is marked by the
- * handshake callback.
+ * handshake callback.  However, it is possible to force the completion
+ * of the handshake (and cause the handshake callback to be called)
+ * prior to reading all 0-RTT data using SSL_ForceHandshake().  To
+ * ensure that all early data is read before the handshake callback, any
+ * time that SSL_ForceHandshake() returns a PR_WOULD_BLOCK_ERROR, use
+ * PR_Read() to read all available data.  If PR_Read() is called
+ * multiple times, this will result in the handshake completing, but the
+ * handshake callback will occur after early data has all been read.
  *
  * WARNING: 0-RTT data has different anti-replay and PFS properties than
- * the rest of the TLS data. See [draft-ietf-tls-tls13; Section 6.2.3]
+ * the rest of the TLS data. See [draft-ietf-tls-tls13; Section 8]
  * for more details.
  */
 #define SSL_ENABLE_0RTT_DATA 33

lib/ssl/ssl3gthr.c

@@ -1,3 +1,4 @@
+/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
 /*
  * Gather (Read) entire SSL3 records from socket into buffer.
  *
@@ -405,9 +406,12 @@ ssl3_GatherCompleteHandshake(sslSocket *ss, int flags)
 
     do {
         PRBool handleRecordNow = PR_FALSE;
+        PRBool processingEarlyData;
 
         ssl_GetSSL3HandshakeLock(ss);
 
+        processingEarlyData = ss->ssl3.hs.zeroRttState == ssl_0rtt_accepted;
+
         /* Without this, we may end up wrongly reporting
          * SSL_ERROR_RX_UNEXPECTED_* errors if we receive any records from the
          * peer while we are waiting to be restarted.
@@ -555,6 +559,15 @@ ssl3_GatherCompleteHandshake(sslSocket *ss, int flags)
             } else {
                 ss->ssl3.hs.canFalseStart = PR_FALSE;
             }
+        } else if (processingEarlyData &&
+                   ss->ssl3.hs.zeroRttState == ssl_0rtt_done &&
+                   !PR_CLIST_IS_EMPTY(&ss->ssl3.hs.bufferedEarlyData)) {
+            /* If we were processing early data and we are no longer, then force
+             * the handshake to block.  This ensures that early data is
+             * delivered to the application before the handshake completes. */
+            ssl_ReleaseSSL3HandshakeLock(ss);
+            PORT_SetError(PR_WOULD_BLOCK_ERROR);
+            return SECWouldBlock;
         }
         ssl_ReleaseSSL3HandshakeLock(ss);
     } while (keepGoing);