Bug 1294978 - Reworked overlong record size checks and added TLS1.3 specific boundaries. r=djackson

Old overlong record check flow:
1.) There is a check for the default maximally allowed record size in ssl3gthr.c/ssl3_GatherData after reception of TLS records. In the same file the DTLS reception buffers are set to the maximum possible record size in dtls_GatherData.
2.) Next the ssl3_HandleRecord handler checks TLS and DTLS records sizes, considering possibly set size limits by the record-size-limit-extension and the maximally approximated cipher expansion possible in NSS.
3.) Until this patch there was a less strict redundant size check in ssl3con.c/ssl3_UnprotectRecord. In tls13con.c/tls13_UnprotectRecord and ssl3con.c/ssl3_UnprotectRecord the plaintext size is checked for validity after unprotecting (plaintext checks were not changed in this patch).
4.) DTLS errors regarding record size and unprotecting are inconsistently sometimes propagated to the peer (alerts) and sometimes silently dropped.

Changes:
1.) In ssl3gthr.c TLS 1.3 specific cases for overlong record checks and DTLS buffer allocation have been added.
2.) The ssl3_HandleRecord handler checks for RFC compliant records sizes (all TLS versions), considering limits set by record_size_limit_extension. This is less strict for TLS <= 1.2, stricter checks have been moved to the unprotection functions to create a similar 'check flow/levels' for all TLS versions.
3.)
- TLS <= 1.2: Moved strict check for maximum allowed plaintext + approximated maximum cipher expansion to ssl3con.c/ssl3_UnprotectRecord.
- TLS 1.3: Added strict check for maximum allowed plaintext + actually used cipher expansion to tls13con.c/tls13_UnprotectRecord.
(Maximum allowed plaintext considers limits set by record_size_limit_extension)
4.) Following RFC6347, Section 4.1.2.7 DTLS errors regarding records and unprotecting and now consistently dropped silently.

Added Tests:
- Positive tests (All (D)TLS versions): Test that largest valid plainext + encryption expansion are successfully sent and handled.
- Negative tests (All (D)TLS versions): Test that all added/updated boundaries lead to the expected alerts. Tested with smallest illegal record size for each of the mentioned checks.

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

2 files changed, 233 insertions, 5 deletions

diff --git a/gtests/ssl_gtest/ssl_recordsize_unittest.cc b/gtests/ssl_gtest/ssl_recordsize_unittest.cc
index a18510440..8a84db574 100644
--- a/gtests/ssl_gtest/ssl_recordsize_unittest.cc
+++ b/gtests/ssl_gtest/ssl_recordsize_unittest.cc
@@ -223,7 +223,7 @@ class TlsRecordExpander : public TlsRecordFilter {
 };
 
 // Tweak the plaintext of server records so that they exceed the client's limit.
-TEST_P(TlsConnectTls13, RecordSizePlaintextExceed) {
+TEST_F(TlsConnectStreamTls13, RecordSizePlaintextExceed) {
   EnsureTlsSetup();
   auto server_expand = MakeTlsFilter<TlsRecordExpander>(server_, 1);
   server_expand->EnableDecryption();
@@ -247,7 +247,7 @@ TEST_P(TlsConnectTls13, RecordSizePlaintextExceed) {
 // This requires a much larger expansion than for plaintext to trigger the
 // guard, which runs before decryption (current allowance is 320 octets,
 // see MAX_EXPANSION in ssl3con.c).
-TEST_P(TlsConnectTls13, RecordSizeCiphertextExceed) {
+TEST_F(TlsConnectStreamTls13, RecordSizeCiphertextExceed) {
   EnsureTlsSetup();
 
   client_->SetOption(SSL_RECORD_SIZE_LIMIT, 64);
@@ -329,7 +329,7 @@ class TlsRecordPadder : public TlsRecordFilter {
   size_t padding_;
 };
 
-TEST_P(TlsConnectTls13, RecordSizeExceedPad) {
+TEST_F(TlsConnectStreamTls13, RecordSizeExceedPad) {
   EnsureTlsSetup();
   auto server_max = std::make_shared<TlsRecordMaximum>(server_);
   auto server_expand = std::make_shared<TlsRecordPadder>(server_, 1);
@@ -494,4 +494,233 @@ TEST_F(RecordSizeDefaultsTest, RecordSizeGetValue) {
   EXPECT_EQ(3000, v);
 }
 
-}  // namespace nss_test
+class TlsCtextResizer : public TlsRecordFilter {
+ public:
+  TlsCtextResizer(const std::shared_ptr<TlsAgent>& a, size_t size)
+      : TlsRecordFilter(a), size_(size) {}
+
+ protected:
+  virtual PacketFilter::Action FilterRecord(const TlsRecordHeader& header,
+                                            const DataBuffer& data,
+                                            DataBuffer* changed) {
+    // allocate and initialise buffer
+    changed->Allocate(size_);
+
+    // copy record data (partially)
+    changed->Write(0, data.data(),
+                   ((data.len() >= size_) ? size_ : data.len()));
+
+    return CHANGE;
+  }
+
+ private:
+  size_t size_;
+};
+
+/* (D)TLS overlong record test for maximum default record size of
+ * 2^14 + (256 (TLS 1.3) OR 2048 (TLS <= 1.2)
+ * [RFC8446, Section 5.2; RFC5246 , Section 6.2.3].
+ * This should fail the first size check in ssl3gthr.c/ssl3_GatherData().
+ * DTLS Record errors are dropped silently. [RFC6347, Section 4.1.2.7]. */
+TEST_P(TlsConnectGeneric, RecordGatherOverlong) {
+  EnsureTlsSetup();
+
+  size_t max_ctext = MAX_FRAGMENT_LENGTH;
+  if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
+    max_ctext += TLS_1_3_MAX_EXPANSION;
+  } else {
+    max_ctext += TLS_1_2_MAX_EXPANSION;
+  }
+
+  Connect();
+
+  MakeTlsFilter<TlsCtextResizer>(server_, max_ctext + 1);
+  // Dummy record will be overwritten
+  server_->SendData(0xf0);
+
+  /* Drop DTLS Record Errors silently [RFC6347, Section 4.1.2.7]. */
+  if (variant_ == ssl_variant_datagram) {
+    size_t received = client_->received_bytes();
+    client_->ReadBytes(max_ctext + 1);
+    ASSERT_EQ(received, client_->received_bytes());
+  } else {
+    client_->ExpectSendAlert(kTlsAlertRecordOverflow);
+    client_->ReadBytes(max_ctext + 1);
+    server_->ExpectReceiveAlert(kTlsAlertRecordOverflow);
+    server_->Handshake();
+  }
+}
+
+/* (D)TLS overlong record test with recordSizeLimit Extension and plus RFC
+ * specified maximum Expansion: 2^14 + (256 (TLS 1.3) OR 2048 (TLS <= 1.2)
+ * [RFC8446, Section 5.2; RFC5246 , Section 6.2.3].
+ * DTLS Record errors are dropped silently. [RFC6347, Section 4.1.2.7]. */
+TEST_P(TlsConnectGeneric, RecordSizeExtensionOverlong) {
+  EnsureTlsSetup();
+
+  // Set some boundary
+  size_t max_ctext = 1000;
+
+  client_->SetOption(SSL_RECORD_SIZE_LIMIT, max_ctext);
+
+  if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
+    // The record size limit includes the inner content type byte
+    max_ctext += TLS_1_3_MAX_EXPANSION - 1;
+  } else {
+    max_ctext += TLS_1_2_MAX_EXPANSION;
+  }
+
+  Connect();
+
+  MakeTlsFilter<TlsCtextResizer>(server_, max_ctext + 1);
+  // Dummy record will be overwritten
+  server_->SendData(0xf);
+
+  /* Drop DTLS Record Errors silently [RFC6347, Section 4.1.2.7].
+   * For DTLS 1.0 and 1.2 the package is dropped before the size check because
+   * of the modification. This just tests that no error is thrown as required.
+   */
+  if (variant_ == ssl_variant_datagram) {
+    size_t received = client_->received_bytes();
+    client_->ReadBytes(max_ctext + 1);
+    ASSERT_EQ(received, client_->received_bytes());
+  } else {
+    client_->ExpectSendAlert(kTlsAlertRecordOverflow);
+    client_->ReadBytes(max_ctext + 1);
+    server_->ExpectReceiveAlert(kTlsAlertRecordOverflow);
+    server_->Handshake();
+  }
+}
+
+/* For TLS <= 1.2:
+ * MAX_EXPANSION is the amount by which a record might plausibly be expanded
+ * when protected.  It's the worst case estimate, so the sum of block cipher
+ * padding (up to 256 octets), HMAC (48 octets for SHA-384), and IV (16
+ * octets for AES). */
+#define MAX_EXPANSION (256 + 48 + 16)
+
+/* (D)TLS overlong record test for specific ciphersuite expansion.
+ * Testing the smallest illegal record.
+ * This check is performed in ssl3con.c/ssl3_UnprotectRecord() OR
+ * tls13con.c/tls13_UnprotectRecord() and enforces stricter size limitations,
+ * dependent on the implemented cipher suites, than the RFC.
+ * DTLS Record errors are dropped silently. [RFC6347, Section 4.1.2.7]. */
+TEST_P(TlsConnectGeneric, RecordExpansionOverlong) {
+  EnsureTlsSetup();
+
+  // Set some boundary
+  size_t max_ctext = 1000;
+
+  client_->SetOption(SSL_RECORD_SIZE_LIMIT, max_ctext);
+
+  if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
+    // For TLS1.3 all ciphers expand the cipherext by 16B
+    // The inner content type byte is included in the record size limit
+    max_ctext += 16;
+  } else {
+    // For TLS<=1.2 the max possible expansion in the NSS implementation is 320
+    max_ctext += MAX_EXPANSION;
+  }
+
+  Connect();
+
+  MakeTlsFilter<TlsCtextResizer>(server_, max_ctext + 1);
+  // Dummy record will be overwritten
+  server_->SendData(0xf);
+
+  /* Drop DTLS Record Errors silently [RFC6347, Section 4.1.2.7].
+   * For DTLS 1.0 and 1.2 the package is dropped before the size check because
+   * of the modification. This just tests that no error is thrown as required/
+   * no bytes are received. */
+  if (variant_ == ssl_variant_datagram) {
+    size_t received = client_->received_bytes();
+    client_->ReadBytes(max_ctext + 1);
+    ASSERT_EQ(received, client_->received_bytes());
+  } else {
+    client_->ExpectSendAlert(kTlsAlertRecordOverflow);
+    client_->ReadBytes(max_ctext + 1);
+    server_->ExpectReceiveAlert(kTlsAlertRecordOverflow);
+    server_->Handshake();
+  }
+}
+
+/* (D)TLS longest allowed record default size test. */
+TEST_P(TlsConnectGeneric, RecordSizeDefaultLong) {
+  EnsureTlsSetup();
+  Connect();
+
+  // Maximum allowed plaintext size
+  size_t max = MAX_FRAGMENT_LENGTH;
+
+  /* For TLS 1.0 the first byte of application data is sent in a single record
+   * as explained in the documentation of SSL_CBC_RANDOM_IV in ssl.h.
+   * Because of that we use TlsCTextResizer to send a record of max size.
+   * A bad record mac alert is expected since we modify the record. */
+  if (version_ == SSL_LIBRARY_VERSION_TLS_1_0 &&
+      variant_ == ssl_variant_stream) {
+    // Set size to maxi plaintext + max allowed expansion
+    MakeTlsFilter<TlsCtextResizer>(server_, max + MAX_EXPANSION);
+    // Dummy record will be overwritten
+    server_->SendData(0xF);
+    // Expect alert
+    client_->ExpectSendAlert(kTlsAlertBadRecordMac);
+    // Receive record
+    client_->ReadBytes(max);
+    // Handle alert on server side
+    server_->ExpectReceiveAlert(kTlsAlertBadRecordMac);
+    server_->Handshake();
+  } else {  // Everything but TLS 1.0
+    // Send largest legal plaintext as single record
+    // by setting SendData() block size to max.
+    server_->SendData(max, max);
+    // Receive record
+    client_->ReadBytes(max);
+    // Assert that data was received successfully
+    ASSERT_EQ(client_->received_bytes(), max);
+  }
+}
+
+/* (D)TLS longest allowed record size limit extension test. */
+TEST_P(TlsConnectGeneric, RecordSizeLimitLong) {
+  EnsureTlsSetup();
+
+  // Set some boundary
+  size_t max = 1000;
+  client_->SetOption(SSL_RECORD_SIZE_LIMIT, max);
+
+  Connect();
+
+  // For TLS 1.3 the InnerContentType byte is included in the record size limit
+  if (version_ == SSL_LIBRARY_VERSION_TLS_1_3) {
+    max--;
+  }
+
+  /* For TLS 1.0 the first byte of application data is sent in a single record
+   * as explained in the documentation of SSL_CBC_RANDOM_IV in ssl.h.
+   * Because of that we use TlsCTextResizer to send a record of max size.
+   * A bad record mac alert is expected since we modify the record. */
+  if (version_ == SSL_LIBRARY_VERSION_TLS_1_0 &&
+      variant_ == ssl_variant_stream) {
+    // Set size to maxi plaintext + max allowed expansion
+    MakeTlsFilter<TlsCtextResizer>(server_, max + MAX_EXPANSION);
+    // Dummy record will be overwritten
+    server_->SendData(0xF);
+    // Expect alert
+    client_->ExpectSendAlert(kTlsAlertBadRecordMac);
+    // Receive record
+    client_->ReadBytes(max);
+    // Handle alert on server side
+    server_->ExpectReceiveAlert(kTlsAlertBadRecordMac);
+    server_->Handshake();
+  } else {  // Everything but TLS 1.0
+    // Send largest legal plaintext as single record
+    // by setting SendData() block size to max.
+    server_->SendData(max, max);
+    // Receive record
+    client_->ReadBytes(max);
+    // Assert that data was received successfully
+    ASSERT_EQ(client_->received_bytes(), max);
+  }
+}
+
+}  // namespace nss_test
\ No newline at end of file
diff --git a/gtests/ssl_gtest/test_io.cc b/gtests/ssl_gtest/test_io.cc
index 4a7f91459..e4651a235 100644
--- a/gtests/ssl_gtest/test_io.cc
+++ b/gtests/ssl_gtest/test_io.cc
@@ -110,7 +110,6 @@ int32_t DummyPrSocket::Recv(PRFileDesc *f, void *buf, int32_t buflen,
 
   auto &front = input_.front();
   if (static_cast<size_t>(buflen) < front.len()) {
-    PR_ASSERT(false);
     PR_SetError(PR_BUFFER_OVERFLOW_ERROR, 0);
     return -1;
   }