Bug 1764788 - Correct invalid record inner and outter content type alerts. r=djackson
authorLeander Schwarz <lschwarz@mozilla.com>
Tue, 17 May 2022 10:44:16 +0000
changeset 16212 7f4b0af3a526e801a977831ef23f82e4e66d6b8a
parent 16211 bc7bfba47e0aba6aab5849c58038c881610dab7f
child 16213 99e32fcca1c78920f6c0d9dd84eb0ca291dc69b6
push id4139
push userdjackson@mozilla.com
push dateTue, 17 May 2022 10:46:24 +0000
reviewersdjackson
bugs1764788
Bug 1764788 - Correct invalid record inner and outter content type alerts. r=djackson Added test cases for alerts during and pre handshake as well as TLS 1.3 only after handshake (application data) cases due to unsupported de- and encryption of lower TLS version records in gtest. Adjusted some test cases that expect failed connections to the updated alerts. Differential Revision: https://phabricator.services.mozilla.com/D144029
gtests/ssl_gtest/ssl_record_unittest.cc
gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc
lib/ssl/ssl3con.c
lib/ssl/ssl3gthr.c
lib/ssl/tls13con.c
--- a/gtests/ssl_gtest/ssl_record_unittest.cc
+++ b/gtests/ssl_gtest/ssl_record_unittest.cc
@@ -305,32 +305,26 @@ const static size_t kContentSizesArr[] =
 
 auto kContentSizes = ::testing::ValuesIn(kContentSizesArr);
 const static bool kTrueFalseArr[] = {true, false};
 auto kTrueFalse = ::testing::ValuesIn(kTrueFalseArr);
 
 INSTANTIATE_TEST_SUITE_P(TlsPadding, TlsPaddingTest,
                          ::testing::Combine(kContentSizes, kTrueFalse));
 
-/* 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 {
+/* Filter to modify record header and content */
+class Tls13RecordModifier : 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) {}
+  Tls13RecordModifier(const std::shared_ptr<TlsAgent>& a,
+                      uint8_t contentType = ssl_ct_handshake, size_t size = 0,
+                      size_t padding = 0)
+      : TlsRecordFilter(a),
+        contentType_(contentType),
+        size_(size),
+        padding_(padding) {}
 
  protected:
   PacketFilter::Action FilterRecord(const TlsRecordHeader& header,
                                     const DataBuffer& record, size_t* offset,
                                     DataBuffer* output) override {
     if (!header.is_protected()) {
       return KEEP;
     }
@@ -345,28 +339,29 @@ class ZeroLengthInnerPlaintextCreatorFil
     }
 
     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_);
+                      DataBuffer(size_), &ciphertext, &out_header, padding_);
     EXPECT_TRUE(ok);
     if (!ok) {
       return KEEP;
     }
 
     *offset = out_header.Write(output, *offset, ciphertext);
     return CHANGE;
   }
 
  private:
-  SSLContentType contentType_;
+  uint8_t contentType_;
+  size_t size_;
   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
@@ -384,23 +379,28 @@ class ZeroLengthInnerPlaintextSetupTls13
         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. */
+ * zero-length inner plaintext content length with and without padding.
+ *
+ * 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]. */
 TEST_P(ZeroLengthInnerPlaintextSetupTls13, ZeroLengthInnerPlaintextRun) {
   EnsureTlsSetup();
 
   // Filter modifies record to be zero-length
-  auto filter = MakeTlsFilter<ZeroLengthInnerPlaintextCreatorFilter>(
-      client_, contentType_, padding_);
+  auto filter =
+      MakeTlsFilter<Tls13RecordModifier>(client_, contentType_, 0, padding_);
   filter->EnableDecryption();
   filter->Disable();
 
   Connect();
 
   filter->Enable();
 
   // Record will be overwritten
@@ -598,9 +598,207 @@ INSTANTIATE_TEST_SUITE_P(
           contentType = "ChangeCipherSpec";
           break;
         default:
           contentType = "InvalidParameter";
       }
       return variant + "ZeroLength" + contentType + "Test";
     });
 
+/* Test correct handling of records with invalid content types.
+ *
+ * TLS:
+ * If a TLS implementation receives an unexpected record type, it MUST
+ * terminate the connection with an "unexpected_message" alert
+ * [RFC8446, Section 5].
+ *
+ * DTLS:
+ * In general, invalid records SHOULD be silently discarded...
+ * [RFC6347, Section 4.1.2.7]. */
+class UndefinedContentTypeSetup : public TlsConnectGeneric {
+ public:
+  UndefinedContentTypeSetup() : TlsConnectGeneric() { StartConnect(); };
+
+  void createUndefinedContentTypeRecord(DataBuffer& buffer, unsigned epoch = 0,
+                                        unsigned seqn = 0) {
+    // dummy data
+    uint8_t data[] = {0xAA, 0xBB, 0xCC, 0xDD, 0xEE};
+
+    size_t idx = 0;
+    // Set undefined content type
+    idx = buffer.Write(idx, 0xFF, 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/during handshake)
+      idx = buffer.Write(idx, epoch, 2);
+      // Set 6B sequence number (0 if send as first message)
+      idx = buffer.Write(idx, 0U, 2);
+      idx = buffer.Write(idx, seqn, 4);
+    }
+    // Set fragment length
+    idx = buffer.Write(idx, 5U, 2);
+    // Add data to record
+    (void)buffer.Write(idx, data, 5);
+  }
+
+  void checkUndefinedContentTypeHandling(std::shared_ptr<TlsAgent> sender,
+                                         std::shared_ptr<TlsAgent> receiver) {
+    if (variant_ == ssl_variant_stream) {
+      // Handle record and expect alert to be sent
+      receiver->ExpectSendAlert(kTlsAlertUnexpectedMessage);
+      receiver->ReadBytes();
+      /* Digest and assert that the correct alert was received at peer
+       *
+       * The 1.3 server expects all messages other than the ClientHello to be
+       * encrypted and responds with an unexpected message alert to alerts. */
+      if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3 && sender == server_) {
+        sender->ExpectSendAlert(kTlsAlertUnexpectedMessage);
+      } else {
+        sender->ExpectReceiveAlert(kTlsAlertUnexpectedMessage);
+      }
+      sender->ReadBytes();
+    } else {  // DTLS drops invalid records silently
+      size_t received = receiver->received_bytes();
+      receiver->ReadBytes();
+      // Ensure no bytes were received/record was dropped
+      ASSERT_EQ(received, receiver->received_bytes());
+    }
+  }
+
+ protected:
+  DataBuffer buffer_;
+};
+
+INSTANTIATE_TEST_SUITE_P(
+    UndefinedContentTypePreHandshakeStream, UndefinedContentTypeSetup,
+    ::testing::Combine(TlsConnectTestBase::kTlsVariantsStream,
+                       TlsConnectTestBase::kTlsVAll));
+
+INSTANTIATE_TEST_SUITE_P(
+    UndefinedContentTypePreHandshakeDatagram, UndefinedContentTypeSetup,
+    ::testing::Combine(TlsConnectTestBase::kTlsVariantsDatagram,
+                       TlsConnectTestBase::kTlsV11Plus));
+
+TEST_P(UndefinedContentTypeSetup,
+       ServerReceiveUndefinedContentTypePreClientHello) {
+  createUndefinedContentTypeRecord(buffer_);
+
+  // Send undefined content type record
+  client_->SendDirect(buffer_);
+
+  checkUndefinedContentTypeHandling(client_, server_);
+}
+
+TEST_P(UndefinedContentTypeSetup,
+       ServerReceiveUndefinedContentTypePostClientHello) {
+  // Set epoch to 0 (handshake), and sequence number to 1 since hello is sent
+  createUndefinedContentTypeRecord(buffer_, 0, 1);
+
+  // Send ClientHello
+  client_->Handshake();
+  // Send undefined content type record
+  client_->SendDirect(buffer_);
+
+  checkUndefinedContentTypeHandling(client_, server_);
+}
+
+TEST_P(UndefinedContentTypeSetup,
+       ClientReceiveUndefinedContentTypePreClientHello) {
+  createUndefinedContentTypeRecord(buffer_);
+
+  // Send undefined content type record
+  server_->SendDirect(buffer_);
+
+  checkUndefinedContentTypeHandling(server_, client_);
+}
+
+TEST_P(UndefinedContentTypeSetup,
+       ClientReceiveUndefinedContentTypePostClientHello) {
+  // Set epoch to 0 (handshake), and sequence number to 1 since hello is sent
+  createUndefinedContentTypeRecord(buffer_, 0, 1);
+
+  // Send ClientHello
+  client_->Handshake();
+  // Send undefined content type record
+  server_->SendDirect(buffer_);
+
+  checkUndefinedContentTypeHandling(server_, client_);
+}
+
+class RecordOuterContentTypeSetter : public TlsRecordFilter {
+ public:
+  RecordOuterContentTypeSetter(const std::shared_ptr<TlsAgent>& a,
+                               uint8_t contentType = ssl_ct_handshake)
+      : TlsRecordFilter(a), contentType_(contentType) {}
+
+ protected:
+  PacketFilter::Action FilterRecord(const TlsRecordHeader& header,
+                                    const DataBuffer& record, size_t* offset,
+                                    DataBuffer* output) override {
+    TlsRecordHeader hdr(header.variant(), header.version(), contentType_,
+                        header.sequence_number());
+
+    *offset = hdr.Write(output, *offset, record);
+    return CHANGE;
+  }
+
+ private:
+  uint8_t contentType_;
+};
+
+/* Test correct handling of invalid inner and outer record content type.
+ * This is only possible for TLS 1.3, since only for this version decryption
+ * and encryption of manipulated records is supported by the test suite. */
+TEST_P(TlsConnectTls13, UndefinedOuterContentType13) {
+  EnsureTlsSetup();
+  Connect();
+
+  // Manipulate record: set invalid content type 0xff
+  MakeTlsFilter<RecordOuterContentTypeSetter>(client_, 0xff);
+  client_->SendData(50);
+
+  if (variant_ == ssl_variant_stream) {
+    // Handle invalid record
+    server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
+    server_->ReadBytes();
+    // Handle alert at peer
+    client_->ExpectReceiveAlert(kTlsAlertUnexpectedMessage);
+    client_->ReadBytes();
+  } else {
+    // Make sure DTLS drops invalid record silently
+    size_t received = server_->received_bytes();
+    server_->ReadBytes();
+    ASSERT_EQ(received, server_->received_bytes());
+  }
+}
+
+TEST_P(TlsConnectTls13, UndefinedInnerContentType13) {
+  EnsureTlsSetup();
+
+  // Manipulate record: set invalid content type 0xff and length to 50.
+  auto filter = MakeTlsFilter<Tls13RecordModifier>(client_, 0xff, 50, 0);
+  filter->EnableDecryption();
+  filter->Disable();
+
+  Connect();
+
+  filter->Enable();
+  // Send manipulate record with invalid content type
+  client_->SendData(50);
+
+  if (variant_ == ssl_variant_stream) {
+    // Handle invalid record
+    server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
+    server_->ReadBytes();
+    // Handle alert at peer
+    client_->ExpectReceiveAlert(kTlsAlertUnexpectedMessage);
+    client_->ReadBytes();
+  } else {
+    // Make sure DTLS drops invalid record silently
+    size_t received = server_->received_bytes();
+    server_->ReadBytes();
+    ASSERT_EQ(received, server_->received_bytes());
+  }
+}
+
 }  // namespace nss_test
\ No newline at end of file
--- a/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc
+++ b/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc
@@ -209,17 +209,17 @@ TEST_P(SSLv2ClientHelloTest, ConnectDisa
   // ClientHello puts a version number.  So the version number (0x0301+) appears
   // to be a length and server blocks waiting for that much data.
   EXPECT_EQ(PR_WOULD_BLOCK_ERROR, PORT_GetError());
 
   // This is usually what happens with v2-compatible: the server hangs.
   // But to be certain, feed in more data to see if an error comes out.
   uint8_t zeros[SSL_LIBRARY_VERSION_TLS_1_2] = {0};
   client_->SendDirect(DataBuffer(zeros, sizeof(zeros)));
-  ExpectAlert(server_, kTlsAlertIllegalParameter);
+  ExpectAlert(server_, kTlsAlertUnexpectedMessage);
   server_->Handshake();
   client_->Handshake();
 }
 
 // Sending a v2 ClientHello after a no-op v3 record must fail.
 TEST_P(SSLv2ClientHelloTest, ConnectAfterEmptyV3Record) {
   DataBuffer buffer;
 
@@ -232,18 +232,18 @@ TEST_P(SSLv2ClientHelloTest, ConnectAfte
   EnsureTlsSetup();
   client_->SendDirect(buffer);
 
   // Need padding so the connection doesn't just time out. With a v2
   // ClientHello parsed as a v3 record we will use the record version
   // as the record length.
   SetPadding(255);
 
-  ConnectExpectAlert(server_, kTlsAlertIllegalParameter);
-  EXPECT_EQ(SSL_ERROR_BAD_CLIENT, server_->error_code());
+  ConnectExpectAlert(server_, kTlsAlertUnexpectedMessage);
+  EXPECT_EQ(SSL_ERROR_RX_UNKNOWN_RECORD_TYPE, server_->error_code());
 }
 
 // Test negotiating TLS 1.3.
 TEST_F(SSLv2ClientHelloTestF, Connect13) {
   EnsureTlsSetup();
   SetExpectedVersion(SSL_LIBRARY_VERSION_TLS_1_3);
   ConfigureVersion(SSL_LIBRARY_VERSION_TLS_1_3);
 
@@ -272,17 +272,17 @@ TEST_P(SSLv2ClientHelloTest, SendSecurit
   SetAvailableCipherSuite(TLS_DHE_RSA_WITH_AES_128_CBC_SHA);
 
   // Send a security escape.
   SetSendEscape(true);
 
   // Set a big padding so that the server fails instead of timing out.
   SetPadding(255);
 
-  ConnectExpectAlert(server_, kTlsAlertIllegalParameter);
+  ConnectExpectAlert(server_, kTlsAlertUnexpectedMessage);
 }
 
 // Invalid SSLv2 client hello padding must fail the handshake.
 TEST_P(SSLv2ClientHelloTest, AddErroneousPadding) {
   SetAvailableCipherSuite(TLS_DHE_RSA_WITH_AES_128_CBC_SHA);
 
   // Append 5 bytes of padding but say it's only 4.
   SetPadding(5, 4);
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -13192,20 +13192,35 @@ ssl3_HandleNonApplicationData(sslSocket 
             break;
         case ssl_ct_ack:
             if (IS_DTLS(ss) && tls13_MaybeTls13(ss)) {
                 rv = dtls13_HandleAck(ss, databuf);
                 break;
             }
         /* Fall through. */
         default:
+            /* If a TLS implementation receives an unexpected record type,
+             * it MUST terminate the connection with an "unexpected_message"
+             * alert [RFC8446, Section 5].
+             *
+             * For TLS 1.3 the outer content type is checked before in
+             * tls13con.c/tls13_UnprotectRecord(),
+             * For DTLS 1.3 the outer content type is checked before in
+             * ssl3gthr.c/dtls_GatherData.
+             * The inner content types will be checked here.
+             *
+             * In DTLS generally invalid records SHOULD be silently discarded,
+             * no alert is sent [RFC6347, Section 4.1.2.7].
+             */
+            if (!IS_DTLS(ss)) {
+                SSL3_SendAlert(ss, alert_fatal, unexpected_message);
+            }
+            PORT_SetError(SSL_ERROR_RX_UNKNOWN_RECORD_TYPE);
             SSL_DBG(("%d: SSL3[%d]: bogus content type=%d",
                      SSL_GETPID(), ss->fd, rType));
-            PORT_SetError(SSL_ERROR_RX_UNKNOWN_RECORD_TYPE);
-            ssl3_DecodeError(ss);
             rv = SECFailure;
             break;
     }
 
     ssl_ReleaseSSL3HandshakeLock(ss);
     return rv;
 }
 
--- a/lib/ssl/ssl3gthr.c
+++ b/lib/ssl/ssl3gthr.c
@@ -343,17 +343,25 @@ dtls_GatherData(sslSocket *ss, sslGather
     }
 
     contentType = gs->dtlsPacket.buf[gs->dtlsPacketOffset];
     if (dtls_IsLongHeader(ss->version, contentType)) {
         headerLen = 13;
     } else if (contentType == ssl_ct_application_data) {
         headerLen = 7;
     } else if (dtls_IsDtls13Ciphertext(ss->version, contentType)) {
-        /* We don't support CIDs. */
+        /* We don't support CIDs.
+         *
+         * This condition is met on all invalid outer content types.
+         * For lower DTLS versions as well as the inner content types,
+         * this is checked in ssl3con.c/ssl3_HandleNonApplicationData().
+         *
+         * In DTLS generally invalid records SHOULD be silently discarded,
+         * no alert is sent [RFC6347, Section 4.1.2.7].
+         */
         if (contentType & 0x10) {
             PORT_Assert(PR_FALSE);
             PORT_SetError(SSL_ERROR_RX_UNKNOWN_RECORD_TYPE);
             gs->dtlsPacketOffset = 0;
             gs->dtlsPacket.len = 0;
             return -1;
         }
 
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -5812,17 +5812,24 @@ tls13_UnprotectRecord(sslSocket *ss,
 
     *alert = bad_record_mac; /* Default alert for most issues. */
 
     PORT_Assert(spec->direction == ssl_secret_read);
     SSL_TRC(3, ("%d: TLS13[%d]: spec=%d epoch=%d (%s) unprotect 0x%0llx len=%u",
                 SSL_GETPID(), ss->fd, spec, spec->epoch, spec->phase,
                 cText->seqNum, cText->buf->len));
 
-    /* Verify that the content type is right.
+    /* Verify that the outer content type is right.
+     *
+     * For the inner content type as well as lower TLS versions this is checked
+     * in ssl3con.c/ssl3_HandleNonApllicationData().
+     *
+     * For DTLS 1.3 this is checked in ssl3gthr.c/dtls_GatherData(). DTLS drops
+     * invalid records silently [RFC6347, Section 4.1.2.7].
+     *
      * Also allow the DTLS short header in TLS 1.3. */
     if (!(cText->hdr[0] == ssl_ct_application_data ||
           (IS_DTLS(ss) &&
            ss->version >= SSL_LIBRARY_VERSION_TLS_1_3 &&
            (cText->hdr[0] & 0xe0) == 0x20))) {
         SSL_TRC(3,
                 ("%d: TLS13[%d]: record has invalid exterior type=%2.2x",
                  SSL_GETPID(), ss->fd, cText->hdr[0]));