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

5 files changed, 336 insertions, 28 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
diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index 0375cf42c..1ac0a7ea5 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -13381,9 +13381,13 @@ ssl3_HandleRecord(sslSocket *ss, SSL3Ciphertext *cText)
         rv = Null_Cipher(NULL, plaintext->buf, &plaintext->len,
                          plaintext->space, cText->buf->buf, cText->buf->len);
 #else
-        /* IMPORTANT: Unprotect functions MUST NOT send alerts
+        /* IMPORTANT:
+         * Unprotect functions MUST NOT send alerts
          * because we still hold the spec read lock. Instead, if they
-         * return SECFailure, they set *alert to the alert to be sent. */
+         * return SECFailure, they set *alert to the alert to be sent.
+         * Additionaly, this is used to silently drop DTLS encryption/record
+         * errors/alerts using the error handling below as suggested in the
+         * DTLS specification [RFC6347, Section 4.1.2.7]. */
         if (spec->version < SSL_LIBRARY_VERSION_TLS_1_3 ||
             spec->epoch == 0) {
             rv = ssl3_UnprotectRecord(ss, spec, cText, plaintext, &alert);
@@ -13394,6 +13398,7 @@ ssl3_HandleRecord(sslSocket *ss, SSL3Ciphertext *cText)
 #endif
     }
 
+    /* Error/Alert handling for ssl3/tls13_UnprotectRecord */
     if (rv != SECSuccess) {
         ssl_ReleaseSpecReadLock(ss); /***************************/
 
@@ -13421,10 +13426,19 @@ ssl3_HandleRecord(sslSocket *ss, SSL3Ciphertext *cText)
             }
         }
 
+        /* All errors/alerts that might occur during unprotection are related
+         * to invalid records (e.g. invalid formatting, length, MAC, ...).
+         * Following the DTLS specification such errors/alerts SHOULD be
+         * dropped silently [RFC6347, Section 4.1.2.7].
+         * This is done below. */
         if ((IS_DTLS(ss) && !dtls13_AeadLimitReached(spec)) ||
             (!IS_DTLS(ss) && ss->sec.isServer &&
              ss->ssl3.hs.zeroRttIgnore == ssl_0rtt_ignore_trial)) {
-            /* Silently drop the packet unless we sent a fatal alert. */
+            /* Silently drop the packet unless we set ss->ssl3.fatalAlertSent.
+             * (Manually or by using functions like
+             * SSL3_SendAlert(.., alert_fatal,..))
+             * This is not currently used in the unprotection functions since
+             * all TLS and DTLS errors are propagated to this handler. */
             if (ss->ssl3.fatalAlertSent) {
                 return SECFailure;
             }
diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c
index 6a1f352bc..8c54555d3 100644
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -5817,7 +5817,7 @@ tls13_UnprotectRecord(sslSocket *ss,
                 SSL_GETPID(), ss->fd, spec, spec->epoch, spec->phase,
                 cText->seqNum, cText->buf->len));
 
-    /* Verify that the content type is right, even though we overwrite it.
+    /* Verify that the content type is right.
      * Also allow the DTLS short header in TLS 1.3. */
     if (!(cText->hdr[0] == ssl_ct_application_data ||
           (IS_DTLS(ss) &&
@@ -5927,6 +5927,29 @@ tls13_UnprotectRecord(sslSocket *ss,
     *innerType = (SSLContentType)plaintext->buf[plaintext->len - 1];
     --plaintext->len;
 
+    /* Check for zero-length encrypted Alert and Handshake fragments
+     * (zero-length + inner content type byte).
+     *
+     * 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]. */
+    if (!plaintext->len && ((!IS_DTLS(ss) && cText->hdr[0] == ssl_ct_application_data) ||
+                            (IS_DTLS(ss) && dtls_IsDtls13Ciphertext(spec->version, cText->hdr[0])))) {
+        switch (*innerType) {
+            case ssl_ct_alert:
+                *alert = unexpected_message;
+                PORT_SetError(SSL_ERROR_RX_MALFORMED_ALERT);
+                return SECFailure;
+            case ssl_ct_handshake:
+                *alert = unexpected_message;
+                PORT_SetError(SSL_ERROR_RX_MALFORMED_HANDSHAKE);
+                return SECFailure;
+            default:
+                break;
+        }
+    }
+
     /* Check that we haven't received too much 0-RTT data. */
     if (spec->epoch == TrafficKeyEarlyApplicationData &&
         *innerType == ssl_ct_application_data) {
diff --git a/tests/tlsfuzzer/config.json.in b/tests/tlsfuzzer/config.json.in
index 28a7dc015..d24195d21 100644
--- a/tests/tlsfuzzer/config.json.in
+++ b/tests/tlsfuzzer/config.json.in
@@ -32,14 +32,6 @@
                 ]
             },
             {
-                "name" : "test-tls13-empty-alert.py",
-                "arguments": [
-                    "-p", "@PORT@"
-                ],
-                "comment": "https://bugzilla.mozilla.org/show_bug.cgi?id=1471656",
-                "exp_pass": false
-            },
-            {
                 "name" : "test-tls13-ffdhe-sanity.py",
                 "arguments": [
                     "-p", "@PORT@"