Bug 1786018 - Add explicit handling of zero length records. r=mt

This is based on the patch developed by Leander in D157183, but is a
little more explicit.

Co-Authored-By: Leander Schwarz

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

2 files changed, 40 insertions, 17 deletions

diff --git a/gtests/mozpkix_gtest/pkixder_input_tests.cpp b/gtests/mozpkix_gtest/pkixder_input_tests.cpp
index c66f06e6a..2667e32b5 100644
--- a/gtests/mozpkix_gtest/pkixder_input_tests.cpp
+++ b/gtests/mozpkix_gtest/pkixder_input_tests.cpp
@@ -246,14 +246,13 @@ TEST_F(pkixder_input_tests, ReadWordWithInsufficentData)
   ASSERT_NE(0x1122, readWord1);
 }
 
-TEST_F(pkixder_input_tests, ReadWordWrapAroundPointer)
+static void UNSANITIZED_ReadWordWrapAroundPointer()
+#if defined(__clang__)
+    /* Use "undefined" instead of more specific "pointer-overflow" for
+     * clang 4.0.0 backward compatability. */
+    __attribute__((no_sanitize("undefined")))
+#endif
 {
-  // The original implementation of our buffer read overflow checks was
-  // susceptible to integer overflows which could make the checks ineffective.
-  // This attempts to verify that we've fixed that. Unfortunately, decrementing
-  // a null pointer is undefined behavior according to the C++ language spec.,
-  // but this should catch the problem on at least some compilers, if not all of
-  // them.
   const uint8_t* der = nullptr;
   --der;
   Input buf;
@@ -263,6 +262,16 @@ TEST_F(pkixder_input_tests, ReadWordWrapAroundPointer)
   ASSERT_EQ(Result::ERROR_BAD_DER, input.Read(b));
 }
 
+TEST_F(pkixder_input_tests, ReadWordWrapAroundPointer) {
+  // The original implementation of our buffer read overflow checks was
+  // susceptible to integer overflows which could make the checks ineffective.
+  // This attempts to verify that we've fixed that. Unfortunately, decrementing
+  // a null pointer is undefined behavior according to the C++ language spec.,
+  // but this should catch the problem on at least some compilers, if not all of
+  // them.
+  UNSANITIZED_ReadWordWrapAroundPointer();
+}
+
 TEST_F(pkixder_input_tests, Skip)
 {
   const uint8_t der[] = { 0x11, 0x22, 0x33, 0x44 };
@@ -352,14 +361,13 @@ TEST_F(pkixder_input_tests, Skip_ToInput)
   ASSERT_TRUE(InputsAreEqual(expected, item));
 }
 
-TEST_F(pkixder_input_tests, Skip_WrapAroundPointer)
+static void UNSANITIZED_Skip_WrapAroundPointer()
+#if defined(__clang__)
+    /* Use "undefined" instead of more specific "pointer-overflow" for
+     * clang 4.0.0 backward compatability. */
+    __attribute__((no_sanitize("undefined")))
+#endif
 {
-  // The original implementation of our buffer read overflow checks was
-  // susceptible to integer overflows which could make the checks ineffective.
-  // This attempts to verify that we've fixed that. Unfortunately, decrementing
-  // a null pointer is undefined behavior according to the C++ language spec.,
-  // but this should catch the problem on at least some compilers, if not all of
-  // them.
   const uint8_t* der = nullptr;
   // coverity[FORWARD_NULL]
   --der;
@@ -369,6 +377,16 @@ TEST_F(pkixder_input_tests, Skip_WrapAroundPointer)
   ASSERT_EQ(Result::ERROR_BAD_DER, input.Skip(1));
 }
 
+TEST_F(pkixder_input_tests, Skip_WrapAroundPointer) {
+  // The original implementation of our buffer read overflow checks was
+  // susceptible to integer overflows which could make the checks ineffective.
+  // This attempts to verify that we've fixed that. Unfortunately, decrementing
+  // a null pointer is undefined behavior according to the C++ language spec.,
+  // but this should catch the problem on at least some compilers, if not all of
+  // them.
+  UNSANITIZED_Skip_WrapAroundPointer();
+}
+
 TEST_F(pkixder_input_tests, Skip_ToInputPastEnd)
 {
   const uint8_t der[] = { 0x11, 0x22, 0x33, 0x44 };
diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index ef883b725..84246954a 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -13429,7 +13429,7 @@ ssl3_GetCipherSpec(sslSocket *ss, SSL3Ciphertext *cText)
 SECStatus
 ssl3_HandleRecord(sslSocket *ss, SSL3Ciphertext *cText)
 {
-    SECStatus rv;
+    SECStatus rv = SECFailure;
     PRBool isTLS, isTLS13;
     DTLSEpoch epoch;
     ssl3CipherSpec *spec = NULL;
@@ -13555,8 +13555,13 @@ ssl3_HandleRecord(sslSocket *ss, SSL3Ciphertext *cText)
          * 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) {
+        if (spec->cipherDef->cipher == cipher_null && cText->buf->len == 0) {
+            /* Handle a zero-length unprotected record
+             * In this case, we treat it as a no-op and let later functions decide
+             * whether to ignore or alert accordingly. */
+            PR_ASSERT(plaintext->len == 0);
+            rv = SECSuccess;
+        } else if (spec->version < SSL_LIBRARY_VERSION_TLS_1_3 || spec->epoch == 0) {
             rv = ssl3_UnprotectRecord(ss, spec, cText, plaintext, &alert);
         } else {
             rv = tls13_UnprotectRecord(ss, spec, cText, plaintext, &rType,