Bug 1755264 - Added TLS 1.3 zero-length inner plaintext checks and tests, zero-length record/fragment handling tests. Enabled tls fuzzer empty alert test. r=djacksonNSS_3_78_BETA1

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

2 files changed, 295 insertions, 16 deletions

diff --git a/gtests/ssl_gtest/ssl_gather_unittest.cc b/gtests/ssl_gtest/ssl_gather_unittest.cc
index 745432951..cdcdf5626 100644
--- a/gtests/ssl_gtest/ssl_gather_unittest.cc
+++ b/gtests/ssl_gtest/ssl_gather_unittest.cc
@@ -25,21 +25,6 @@ class GatherV2ClientHelloTest : public TlsConnectTestBase {
   }
 };
 
-// Gather a 5-byte v3 record, with a zero fragment length. The empty handshake
-// message should be ignored, and the connection will succeed afterwards.
-TEST_F(TlsConnectTest, GatherEmptyV3Record) {
-  DataBuffer buffer;
-
-  size_t idx = 0;
-  idx = buffer.Write(idx, 0x16, 1);    // handshake
-  idx = buffer.Write(idx, 0x0301, 2);  // record_version
-  (void)buffer.Write(idx, 0U, 2);      // length=0
-
-  EnsureTlsSetup();
-  client_->SendDirect(buffer);
-  Connect();
-}
-
 // Gather a 5-byte v3 record, with a fragment length exceeding the maximum.
 TEST_F(TlsConnectTest, GatherExcessiveV3Record) {
   DataBuffer buffer;
diff --git a/gtests/ssl_gtest/ssl_record_unittest.cc b/gtests/ssl_gtest/ssl_record_unittest.cc
index a63e6adc6..6e06657be 100644
--- a/gtests/ssl_gtest/ssl_record_unittest.cc
+++ b/gtests/ssl_gtest/ssl_record_unittest.cc
@@ -309,4 +309,298 @@ auto kTrueFalse = ::testing::ValuesIn(kTrueFalseArr);
 
 INSTANTIATE_TEST_SUITE_P(TlsPadding, TlsPaddingTest,
                          ::testing::Combine(kContentSizes, kTrueFalse));
-}  // namespace nss_test
+
+/* This filter modifies any application data record, changes its inner content
+ * type to the specified one (default handshake) and optionally add padding,
+ * to create records/fragments invalid in TLS 1.3.
+ *
+ * Implementations MUST NOT send Handshake and Alert records that have a
+ * zero-length TLSInnerPlaintext.content; if such a message is received,
+ * the receiving implementation MUST terminate the connection with an
+ * "unexpected_message" alert [RFC8446, Section 5.4].
+ *
+ * !!! TLS 1.3 ONLY !!! */
+class ZeroLengthInnerPlaintextCreatorFilter : public TlsRecordFilter {
+ public:
+  ZeroLengthInnerPlaintextCreatorFilter(
+      const std::shared_ptr<TlsAgent>& a,
+      SSLContentType contentType = ssl_ct_handshake, size_t padding = 0)
+      : TlsRecordFilter(a), contentType_(contentType), padding_(padding) {}
+
+ protected:
+  PacketFilter::Action FilterRecord(const TlsRecordHeader& header,
+                                    const DataBuffer& record, size_t* offset,
+                                    DataBuffer* output) override {
+    if (!header.is_protected()) {
+      return KEEP;
+    }
+
+    uint16_t protection_epoch;
+    uint8_t inner_content_type;
+    DataBuffer plaintext;
+    TlsRecordHeader out_header;
+    if (!Unprotect(header, record, &protection_epoch, &inner_content_type,
+                   &plaintext, &out_header)) {
+      return KEEP;
+    }
+
+    if (decrypting() && inner_content_type != ssl_ct_application_data) {
+      return KEEP;
+    }
+
+    DataBuffer ciphertext;
+    bool ok = Protect(spec(protection_epoch), out_header, contentType_,
+                      DataBuffer(0), &ciphertext, &out_header, padding_);
+    EXPECT_TRUE(ok);
+    if (!ok) {
+      return KEEP;
+    }
+
+    *offset = out_header.Write(output, *offset, ciphertext);
+    return CHANGE;
+  }
+
+ private:
+  SSLContentType contentType_;
+  size_t padding_;
+};
+
+/* Zero-length InnerPlaintext test class
+ *
+ * Parameter = Tuple of:
+ * - TLS variant (datagram/stream)
+ * - Content type to be set in zero-length inner plaintext record
+ * - Padding of record plaintext
+ */
+class ZeroLengthInnerPlaintextSetupTls13
+    : public TlsConnectTestBase,
+      public testing::WithParamInterface<
+          std::tuple<SSLProtocolVariant, SSLContentType, size_t>> {
+ public:
+  ZeroLengthInnerPlaintextSetupTls13()
+      : TlsConnectTestBase(std::get<0>(GetParam()),
+                           SSL_LIBRARY_VERSION_TLS_1_3),
+        contentType_(std::get<1>(GetParam())),
+        padding_(std::get<2>(GetParam())){};
+
+ protected:
+  SSLContentType contentType_;
+  size_t padding_;
+};
+
+/* Test correct rejection of TLS 1.3 encrypted handshake/alert records with
+ * zero-length inner plaintext content length with and without padding. */
+TEST_P(ZeroLengthInnerPlaintextSetupTls13, ZeroLengthInnerPlaintextRun) {
+  EnsureTlsSetup();
+
+  // Filter modifies record to be zero-length
+  auto filter = MakeTlsFilter<ZeroLengthInnerPlaintextCreatorFilter>(
+      client_, contentType_, padding_);
+  filter->EnableDecryption();
+  filter->Disable();
+
+  Connect();
+
+  filter->Enable();
+
+  // Record will be overwritten
+  client_->SendData(0xf);
+
+  // Receive corrupt record
+  if (variant_ == ssl_variant_stream) {
+    server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
+    // 22B = 16B MAC + 1B innerContentType + 5B Header
+    server_->ReadBytes(22);
+    // Process alert at peer
+    client_->ExpectReceiveAlert(kTlsAlertUnexpectedMessage);
+    client_->Handshake();
+  } else { /* DTLS */
+    size_t received = server_->received_bytes();
+    // 22B = 16B MAC + 1B innerContentType + 5B Header
+    server_->ReadBytes(22);
+    // Check that no bytes were received => packet was dropped
+    ASSERT_EQ(received, server_->received_bytes());
+    // Check that we are still connected / not in error state
+    EXPECT_EQ(TlsAgent::STATE_CONNECTED, client_->state());
+    EXPECT_EQ(TlsAgent::STATE_CONNECTED, server_->state());
+  }
+}
+
+// Test for TLS and DTLS
+const SSLProtocolVariant kZeroLengthInnerPlaintextVariants[] = {
+    ssl_variant_stream, ssl_variant_datagram};
+// Test for handshake and alert fragments
+const SSLContentType kZeroLengthInnerPlaintextContentTypes[] = {
+    ssl_ct_handshake, ssl_ct_alert};
+// Test with 0,1 and 100 octets of padding
+const size_t kZeroLengthInnerPlaintextPadding[] = {0, 1, 100};
+
+INSTANTIATE_TEST_SUITE_P(
+    ZeroLengthInnerPlaintextTest, ZeroLengthInnerPlaintextSetupTls13,
+    testing::Combine(testing::ValuesIn(kZeroLengthInnerPlaintextVariants),
+                     testing::ValuesIn(kZeroLengthInnerPlaintextContentTypes),
+                     testing::ValuesIn(kZeroLengthInnerPlaintextPadding)),
+    [](const testing::TestParamInfo<
+        ZeroLengthInnerPlaintextSetupTls13::ParamType>& inf) {
+      return std::string(std::get<0>(inf.param) == ssl_variant_stream
+                             ? "Tls"
+                             : "Dtls") +
+             "ZeroLengthInnerPlaintext" +
+             (std::get<1>(inf.param) == ssl_ct_handshake ? "Handshake"
+                                                         : "Alert") +
+             (std::get<2>(inf.param)
+                  ? "Padding" + std::to_string(std::get<2>(inf.param)) + "B"
+                  : "") +
+             "Test";
+    });
+
+/* Zero-length record test class
+ *
+ * Parameter = Tuple of:
+ * - TLS variant (datagram/stream)
+ * - Content type to be set in zero-length record
+ */
+class ZeroLengthRecordSetup
+    : public TlsConnectTestBase,
+      public testing::WithParamInterface<
+          std::tuple<SSLProtocolVariant, SSLContentType>> {
+ public:
+  ZeroLengthRecordSetup()
+      : TlsConnectTestBase(std::get<0>(GetParam()), 0),
+        variant_(std::get<0>(GetParam())),
+        contentType_(std::get<1>(GetParam())){};
+
+  void createZeroLengthRecord(DataBuffer& buffer, unsigned epoch = 0,
+                              unsigned seqn = 0) {
+    size_t idx = 0;
+    // Set header content type
+    idx = buffer.Write(idx, contentType_, 1);
+    // The record version is not checked during record layer handling
+    idx = buffer.Write(idx, 0xDEAD, 2);
+    // DTLS (version always < TLS 1.3)
+    if (variant_ == ssl_variant_datagram) {
+      // Set epoch (Should be 0 before handshake)
+      idx = buffer.Write(idx, 0U, 2);
+      // Set 6B sequence number (0 if send as first message)
+      idx = buffer.Write(idx, 0U, 2);
+      idx = buffer.Write(idx, 0U, 4);
+    }
+    // Set fragment to be of zero-length
+    (void)buffer.Write(idx, 0U, 2);
+  }
+
+ protected:
+  SSLProtocolVariant variant_;
+  SSLContentType contentType_;
+};
+
+/* Test handling of zero-length (ciphertext/fragment) records before handshake.
+ *
+ * This is only tested before the first handshake, since after it all of these
+ * messages are expected to be encrypted which is impossible for a content
+ * length of zero, always leading to a bad record mac. For TLS 1.3 only
+ * records of application data content type is legal after the handshake.
+ *
+ * Handshake records of length zero will be ignored in the record layer since
+ * the RFC does only specify that such records MUST NOT be sent but it does not
+ * state that an alert should be sent or the connection be terminated
+ * [RFC8446, Section 5.1].
+ *
+ * Even though only handshake messages are handled (ignored) in the record
+ * layer handling, this test covers zero-length records of all content types
+ * for complete coverage of cases.
+ *
+ * !!! Expected TLS (Stream) behavior !!!
+ * - Handshake records of zero length are ignored.
+ * - Alert and ChangeCipherSpec records of zero-length lead to illegal
+ * parameter alerts due to the malformed record content.
+ * - ApplicationData before the handshake leads to an unexpected message alert.
+ *
+ * !!! Expected DTLS (Datagram) behavior !!!
+ * - Handshake message of zero length are ignored.
+ * - Alert messages lead to an illegal parameter alert due to malformed record
+ * content.
+ * - ChangeCipherSpec records before the first handshake are not expected and
+ * ignored (see ssl3con.c, line 3276).
+ * - ApplicationData before the handshake is ignored since it could be a packet
+ * received in incorrect order (see ssl3con.c, line 13353).
+ */
+TEST_P(ZeroLengthRecordSetup, ZeroLengthRecordRun) {
+  DataBuffer buffer;
+  createZeroLengthRecord(buffer);
+
+  EnsureTlsSetup();
+  // Send zero-length record
+  client_->SendDirect(buffer);
+  // This must be set, otherwise handshake completness assertions might fail
+  server_->StartConnect();
+
+  // DTLS ignores all cases but malformed alert
+  if (contentType_ != ssl_ct_alert && variant_ == ssl_variant_datagram) {
+    contentType_ = ssl_ct_handshake;
+  }
+
+  SSLAlertDescription alert;
+  switch (contentType_) {
+    case ssl_ct_alert:
+    case ssl_ct_change_cipher_spec:
+      alert = illegal_parameter;
+      break;
+    case ssl_ct_application_data:
+      alert = unexpected_message;
+      break;
+    default:  // ssl_ct_handshake
+      // Receive zero-length record
+      server_->Handshake();
+      // Connect successfully since empty record was ignored
+      Connect();
+      return;
+  }
+
+  // Assert alert is send for TLS and DTLS alert records
+  server_->ExpectSendAlert(alert);
+  server_->Handshake();
+
+  // Consume alert at peer, expect alert for TLS and DTLS alert records
+  client_->StartConnect();
+  client_->ExpectReceiveAlert(alert);
+  client_->Handshake();
+}
+
+// Test for TLS and DTLS
+const SSLProtocolVariant kZeroLengthRecordVariants[] = {ssl_variant_datagram,
+                                                        ssl_variant_stream};
+
+// Test for handshake, alert, change_cipher_spec and application data fragments
+const SSLContentType kZeroLengthRecordContentTypes[] = {
+    ssl_ct_handshake, ssl_ct_alert, ssl_ct_change_cipher_spec,
+    ssl_ct_application_data};
+
+INSTANTIATE_TEST_SUITE_P(
+    ZeroLengthRecordTest, ZeroLengthRecordSetup,
+    testing::Combine(testing::ValuesIn(kZeroLengthRecordVariants),
+                     testing::ValuesIn(kZeroLengthRecordContentTypes)),
+    [](const testing::TestParamInfo<ZeroLengthRecordSetup::ParamType>& inf) {
+      std::string variant =
+          (std::get<0>(inf.param) == ssl_variant_stream) ? "Tls" : "Dtls";
+      std::string contentType;
+      switch (std::get<1>(inf.param)) {
+        case ssl_ct_handshake:
+          contentType = "Handshake";
+          break;
+        case ssl_ct_alert:
+          contentType = "Alert";
+          break;
+        case ssl_ct_application_data:
+          contentType = "ApplicationData";
+          break;
+        case ssl_ct_change_cipher_spec:
+          contentType = "ChangeCipherSpec";
+          break;
+        default:
+          contentType = "InvalidParameter";
+      }
+      return variant + "ZeroLength" + contentType + "Test";
+    });
+
+}  // namespace nss_test
\ No newline at end of file