Bug 1564499 - land NSS NSS_3_46_BETA1 UPGRADE_NSS_RELEASE, r=kjacobs
authorJ.C. Jones <jc@mozilla.com>
Fri, 23 Aug 2019 22:45:47 +0000
changeset 489665 2cced2d9a28c8d84f8f61be9897b9c94006dc06b
parent 489664 4f33a872155557891911dd0da47ea268aa88886a
child 489666 ffda592c113d2fc619438e11a12fef412f6c0ffb
push id36479
push useraciure@mozilla.com
push dateSat, 24 Aug 2019 09:49:46 +0000
treeherdermozilla-central@ea3eebc73ccb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskjacobs
bugs1564499, 1560593, 1575968, 1573942, 1564284, 1528666, 1568803
milestone70.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1564499 - land NSS NSS_3_46_BETA1 UPGRADE_NSS_RELEASE, r=kjacobs 2019-08-23 Kevin Jacobs <kjacobs@mozilla.com> * tests/common/cleanup.sh: Bug 1560593 - Check that BUILD_OPT is defined before testing its value. r=jcj [44aa330de2aa] [NSS_3_46_BETA1] * cmd/strsclnt/strsclnt.c: Bug 1575968 - Add strsclnt option to enforce the use of either IPv4 or IPv6 r=jcj [da284d8993ea] 2019-08-23 Marcus Burghardt <mburghardt@mozilla.com> * gtests/softoken_gtest/softoken_gtest.cc: Bug 1573942 - Gtest for pkcs11.txt with different breaking line formats. r=kjacobs [d07a07eb0e40] 2019-08-21 Kevin Jacobs <kjacobs@mozilla.com> * lib/util/utilmod.c: Bug 1564284: Added check for CR + LF, r=marcusburghardt,kjacobs Looks good and it was already tested locally with this gtest patch: [d1d2e1e320cd] 2019-08-22 Martin Thomson <mt@lowentropy.net> * lib/ssl/ssl3con.c: Bug 1528666 - Formatting, a=bustage [60eeac76c8ec] 2019-08-20 Martin Thomson <martin.thomson@gmail.com> * gtests/ssl_gtest/ssl_0rtt_unittest.cc, gtests/ssl_gtest/ssl_resumption_unittest.cc, lib/ssl/ssl3con.c: Bug 1528666 - Correct resumption validation checks, r=jcj We allowed cross-suite resumption before, but it didn't work. This enables that for clients. As a secondary minor tweak, clients will no longer validate the availability of a cipher suite based on their configured version range when attempting resumption. Instead, they will check whether the suite works for the version in the session that they are attempting to resume. In theory, this doesn't change anything because the previous session should not have selected an incompatible combination of version and cipher suite, but it's worth being extra precise. [cab2c8905214] 2019-08-22 Martin Thomson <mt@lowentropy.net> * gtests/ssl_gtest/ssl_auth_unittest.cc, gtests/ssl_gtest/ssl_resumption_unittest.cc, lib/ssl/ssl3con.c: Bug 1568803 - More tests for client certificate authentication, r=kjacobs These were previously disabled because of difficulties (at the time) in writing these tests for TLS 1.3. The framework, and my understanding of it, has since improved, so these tests can be restored and expanded. This exposed a minor correctness issue that is also corrected. [95f97d31c313] Differential Revision: https://phabricator.services.mozilla.com/D43308
security/nss/TAG-INFO
security/nss/cmd/strsclnt/strsclnt.c
security/nss/coreconf/coreconf.dep
security/nss/gtests/softoken_gtest/softoken_gtest.cc
security/nss/gtests/ssl_gtest/ssl_0rtt_unittest.cc
security/nss/gtests/ssl_gtest/ssl_auth_unittest.cc
security/nss/gtests/ssl_gtest/ssl_resumption_unittest.cc
security/nss/lib/ssl/ssl3con.c
security/nss/lib/util/utilmod.c
security/nss/tests/common/cleanup.sh
--- a/security/nss/TAG-INFO
+++ b/security/nss/TAG-INFO
@@ -1,1 +1,1 @@
-eeb9a6715a93
+NSS_3_46_BETA1
--- a/security/nss/cmd/strsclnt/strsclnt.c
+++ b/security/nss/cmd/strsclnt/strsclnt.c
@@ -161,16 +161,19 @@ Usage(void)
             "          All versions are enabled by default.\n"
             "          Possible values for min/max: ssl3 tls1.0 tls1.1 tls1.2\n"
             "          Example: \"-V ssl3:\" enables SSL 3 and newer.\n"
             "       -U means enable throttling up threads\n"
             "       -T enable the cert_status extension (OCSP stapling)\n"
             "       -u enable TLS Session Ticket extension\n"
             "       -z enable compression\n"
             "       -g enable false start\n"
+            "       -4  Enforce using an IPv4 destination address\n"
+            "       -6  Enforce using an IPv6 destination address\n"
+            "           Note: Default behavior is both IPv4 and IPv6 enabled\n"
             "       -J enable signature schemes\n"
             "          This takes a comma separated list of signature schemes in preference\n"
             "          order.\n"
             "          Possible values are:\n"
             "          rsa_pkcs1_sha1, rsa_pkcs1_sha256, rsa_pkcs1_sha384, rsa_pkcs1_sha512,\n"
             "          ecdsa_sha1, ecdsa_secp256r1_sha256, ecdsa_secp384r1_sha384,\n"
             "          ecdsa_secp521r1_sha512,\n"
             "          rsa_pss_rsae_sha256, rsa_pss_rsae_sha384, rsa_pss_rsae_sha512,\n"
@@ -1056,17 +1059,19 @@ hexchar_to_int(int c)
 }
 
 void
 client_main(
     unsigned short port,
     int connections,
     cert_and_key *Cert_And_Key,
     const char *hostName,
-    const char *sniHostName)
+    const char *sniHostName,
+    PRBool allowIPv4,
+    PRBool allowIPv6)
 {
     PRFileDesc *model_sock = NULL;
     int i;
     int rv;
     PRStatus status;
     PRNetAddr addr;
 
     status = PR_StringToNetAddr(hostName, &addr);
@@ -1078,21 +1083,25 @@ client_main(
         void *enumPtr = NULL;
 
         addrInfo = PR_GetAddrInfoByName(hostName, PR_AF_UNSPEC,
                                         PR_AI_ADDRCONFIG | PR_AI_NOCANONNAME);
         if (!addrInfo) {
             SECU_PrintError(progName, "error looking up host");
             return;
         }
-        do {
+        for (;;) {
             enumPtr = PR_EnumerateAddrInfo(enumPtr, addrInfo, port, &addr);
-        } while (enumPtr != NULL &&
-                 addr.raw.family != PR_AF_INET &&
-                 addr.raw.family != PR_AF_INET6);
+            if (enumPtr == NULL)
+                break;
+            if (addr.raw.family == PR_AF_INET && allowIPv4)
+                break;
+            if (addr.raw.family == PR_AF_INET6 && allowIPv6)
+                break;
+        }
         PR_FreeAddrInfo(addrInfo);
         if (enumPtr == NULL) {
             SECU_PrintError(progName, "error looking up host address");
             return;
         }
     }
 
     /* all suites except RSA_NULL_MD5 are enabled by Domestic Policy */
@@ -1314,16 +1323,18 @@ main(int argc, char **argv)
     const char *dir = ".";
     const char *fileName = NULL;
     char *hostName = NULL;
     char *nickName = NULL;
     char *tmp = NULL;
     int connections = 1;
     int exitVal;
     int tmpInt;
+    PRBool allowIPv4 = PR_TRUE;
+    PRBool allowIPv6 = PR_TRUE;
     unsigned short port = 443;
     SECStatus rv;
     PLOptState *optstate;
     PLOptStatus status;
     cert_and_key Cert_And_Key;
     char *sniHostName = NULL;
 
     /* Call the NSPR initialization routines */
@@ -1333,19 +1344,35 @@ main(int argc, char **argv)
     tmp = strrchr(argv[0], '/');
     tmp = tmp ? tmp + 1 : argv[0];
     progName = strrchr(tmp, '\\');
     progName = progName ? progName + 1 : tmp;
 
     /* XXX: 'B' was used in the past but removed in 3.28,
      *      please leave some time before resuing it. */
     optstate = PL_CreateOptState(argc, argv,
-                                 "C:DJ:NP:TUV:W:a:c:d:f:gin:op:qst:uvw:z");
+                                 "46C:DJ:NP:TUV:W:a:c:d:f:gin:op:qst:uvw:z");
     while ((status = PL_GetNextOpt(optstate)) == PL_OPT_OK) {
         switch (optstate->option) {
+            case '4':
+                if (!allowIPv4) {
+                    fprintf(stderr, "Only one of [-4, -6] can be specified.\n");
+                    Usage();
+                }
+                allowIPv6 = PR_FALSE;
+                break;
+
+            case '6':
+                if (!allowIPv6) {
+                    fprintf(stderr, "Only one of [-4, -6] can be specified.\n");
+                    Usage();
+                }
+                allowIPv4 = PR_FALSE;
+                break;
+
             case 'C':
                 cipherString = optstate->value;
                 break;
 
             case 'D':
                 NoDelay = PR_TRUE;
                 break;
 
@@ -1518,17 +1545,17 @@ main(int argc, char **argv)
         if (Cert_And_Key.key == NULL) {
             fprintf(stderr, "strsclnt: Can't find Private Key for cert %s\n",
                     Cert_And_Key.nickname);
             exit(1);
         }
     }
 
     client_main(port, connections, &Cert_And_Key, hostName,
-                sniHostName);
+                sniHostName, allowIPv4, allowIPv6);
 
     /* clean up */
     if (Cert_And_Key.cert) {
         CERT_DestroyCertificate(Cert_And_Key.cert);
     }
     if (Cert_And_Key.key) {
         SECKEY_DestroyPrivateKey(Cert_And_Key.key);
     }
--- a/security/nss/coreconf/coreconf.dep
+++ b/security/nss/coreconf/coreconf.dep
@@ -5,9 +5,8 @@
 
 /*
  * A dummy header file that is a dependency for all the object files.
  * Used to force a full recompilation of NSS in Mozilla's Tinderbox
  * depend builds.  See comments in rules.mk.
  */
 
 #error "Do not include this header file."
-
--- a/security/nss/gtests/softoken_gtest/softoken_gtest.cc
+++ b/security/nss/gtests/softoken_gtest/softoken_gtest.cc
@@ -6,16 +6,17 @@
 #include "secmod.h"
 #include "secerr.h"
 
 #include "nss_scoped_ptrs.h"
 #include "util.h"
 
 #define GTEST_HAS_RTTI 0
 #include "gtest/gtest.h"
+#include <fstream>
 
 namespace nss_test {
 
 class SoftokenTest : public ::testing::Test {
  protected:
   SoftokenTest() : mNSSDBDir("SoftokenTest.d-") {}
   SoftokenTest(const std::string &prefix) : mNSSDBDir(prefix) {}
 
@@ -115,17 +116,17 @@ TEST_F(SoftokenTest, CreateObjectChangeP
   EXPECT_EQ(SECSuccess, PK11_InitPin(slot.get(), nullptr, nullptr));
   EXPECT_EQ(SECSuccess, PK11_ChangePW(slot.get(), "", "password"));
   EXPECT_EQ(SECSuccess, PK11_Logout(slot.get()));
   ScopedPK11GenericObject obj(PK11_CreateGenericObject(
       slot.get(), attributes, PR_ARRAY_SIZE(attributes), true));
   EXPECT_EQ(nullptr, obj);
 }
 
-/* The size limit for a password is 500 characters as defined in pkcs11i.h */
+// The size limit for a password is 500 characters as defined in pkcs11i.h
 TEST_F(SoftokenTest, CreateObjectChangeToBigPassword) {
   ScopedPK11SlotInfo slot(PK11_GetInternalKeySlot());
   ASSERT_TRUE(slot);
   EXPECT_EQ(SECSuccess, PK11_InitPin(slot.get(), nullptr, nullptr));
   EXPECT_EQ(
       SECSuccess,
       PK11_ChangePW(slot.get(), "",
                     "rUIFIFr2bxKnbJbitsfkyqttpk6vCJzlYMNxcxXcaN37gSZKbLk763X7iR"
@@ -152,16 +153,86 @@ TEST_F(SoftokenTest, CreateObjectChangeT
   EXPECT_EQ(SEC_ERROR_TOKEN_NOT_LOGGED_IN, PORT_GetError());
   ScopedPK11GenericObject obj(PK11_CreateGenericObject(
       slot.get(), attributes, PR_ARRAY_SIZE(attributes), true));
   // Because there's no password we can't logout and the operation should have
   // succeeded.
   EXPECT_NE(nullptr, obj);
 }
 
+// We should be able to read CRLF, LF and CR.
+// During the Initialization of the NSS Database, is called a function to load
+// PKCS11 modules defined in pkcs11.txt. This file is read to get the
+// specifications, parse them and load the modules. Here we are ensuring that
+// the parsing will work correctly, independent of the breaking line format of
+// pkcs11.txt file, which could vary depending where it was created.
+// If the parsing is not well interpreted, the database cannot initialize.
+TEST_F(SoftokenTest, CreateObjectReadBreakLine) {
+  const std::string path = mNSSDBDir.GetPath();
+  const std::string dbname_in = path + "/pkcs11.txt";
+  const std::string dbname_out_cr = path + "/pkcs11_cr.txt";
+  const std::string dbname_out_crlf = path + "/pkcs11_crlf.txt";
+  const std::string dbname_out_lf = path + "/pkcs11_lf.txt";
+
+  std::ifstream in(dbname_in);
+  ASSERT_TRUE(in);
+  std::ofstream out_cr(dbname_out_cr);
+  ASSERT_TRUE(out_cr);
+  std::ofstream out_crlf(dbname_out_crlf);
+  ASSERT_TRUE(out_crlf);
+  std::ofstream out_lf(dbname_out_lf);
+  ASSERT_TRUE(out_lf);
+
+  // Database should be correctly initialized by Setup()
+  ASSERT_TRUE(NSS_IsInitialized());
+  ASSERT_EQ(SECSuccess, NSS_Shutdown());
+
+  // Prepare the file formats with CR, CRLF and LF
+  for (std::string line; getline(in, line);) {
+    out_cr << line << "\r";
+    out_crlf << line << "\r\n";
+    out_lf << line << "\n";
+  }
+  in.close();
+  out_cr.close();
+  out_crlf.close();
+  out_lf.close();
+
+  // Change the pkcs11.txt to CR format.
+  ASSERT_TRUE(!remove(dbname_in.c_str()));
+  ASSERT_TRUE(!rename(dbname_out_cr.c_str(), dbname_in.c_str()));
+
+  // Try to initialize with CR format.
+  std::string nssInitArg("sql:");
+  nssInitArg.append(mNSSDBDir.GetUTF8Path());
+  ASSERT_EQ(SECSuccess, NSS_Initialize(nssInitArg.c_str(), "", "", SECMOD_DB,
+                                       NSS_INIT_NOROOTINIT));
+  ASSERT_TRUE(NSS_IsInitialized());
+  ASSERT_EQ(SECSuccess, NSS_Shutdown());
+
+  // Change the pkcs11.txt to CRLF format.
+  ASSERT_TRUE(!remove(dbname_in.c_str()));
+  ASSERT_TRUE(!rename(dbname_out_crlf.c_str(), dbname_in.c_str()));
+
+  // Try to initialize with CRLF format.
+  ASSERT_EQ(SECSuccess, NSS_Initialize(nssInitArg.c_str(), "", "", SECMOD_DB,
+                                       NSS_INIT_NOROOTINIT));
+  ASSERT_TRUE(NSS_IsInitialized());
+  ASSERT_EQ(SECSuccess, NSS_Shutdown());
+
+  // Change the pkcs11.txt to LF format.
+  ASSERT_TRUE(!remove(dbname_in.c_str()));
+  ASSERT_TRUE(!rename(dbname_out_lf.c_str(), dbname_in.c_str()));
+
+  // Try to initialize with LF format.
+  ASSERT_EQ(SECSuccess, NSS_Initialize(nssInitArg.c_str(), "", "", SECMOD_DB,
+                                       NSS_INIT_NOROOTINIT));
+  ASSERT_TRUE(NSS_IsInitialized());
+}
+
 class SoftokenNonAsciiTest : public SoftokenTest {
  protected:
   SoftokenNonAsciiTest() : SoftokenTest("SoftokenTest.\xF7-") {}
 };
 
 TEST_F(SoftokenNonAsciiTest, NonAsciiPathWorking) {
   ScopedPK11SlotInfo slot(PK11_GetInternalKeySlot());
   ASSERT_TRUE(slot);
--- a/security/nss/gtests/ssl_gtest/ssl_0rtt_unittest.cc
+++ b/security/nss/gtests/ssl_gtest/ssl_0rtt_unittest.cc
@@ -977,14 +977,55 @@ TEST_F(TlsConnectStreamTls13, BadAntiRep
 
   // The socket isn't a client or server until later, so configuring a client
   // should work OK.
   client_->EnsureTlsSetup();
   EXPECT_EQ(SECSuccess, SSL_SetAntiReplayContext(client_->ssl_fd(), ctx.get()));
   EXPECT_EQ(SECSuccess, SSL_SetAntiReplayContext(client_->ssl_fd(), nullptr));
 }
 
+// See also TlsConnectGenericResumption.ResumeServerIncompatibleCipher
+TEST_P(TlsConnectTls13, ZeroRttDifferentCompatibleCipher) {
+  EnsureTlsSetup();
+  server_->EnableSingleCipher(TLS_AES_128_GCM_SHA256);
+  SetupForZeroRtt();
+  client_->Set0RttEnabled(true);
+  server_->Set0RttEnabled(true);
+  // Change the ciphersuite.  Resumption is OK because the hash is the same, but
+  // early data will be rejected.
+  server_->EnableSingleCipher(TLS_CHACHA20_POLY1305_SHA256);
+  ExpectResumption(RESUME_TICKET);
+
+  StartConnect();
+  ZeroRttSendReceive(true, false);
+
+  Handshake();
+  ExpectEarlyDataAccepted(false);
+  CheckConnected();
+  SendReceive();
+}
+
+// See also TlsConnectGenericResumption.ResumeServerIncompatibleCipher
+TEST_P(TlsConnectTls13, ZeroRttDifferentIncompatibleCipher) {
+  EnsureTlsSetup();
+  server_->EnableSingleCipher(TLS_AES_256_GCM_SHA384);
+  SetupForZeroRtt();
+  client_->Set0RttEnabled(true);
+  server_->Set0RttEnabled(true);
+  // Resumption is rejected because the hash is different.
+  server_->EnableSingleCipher(TLS_CHACHA20_POLY1305_SHA256);
+  ExpectResumption(RESUME_NONE);
+
+  StartConnect();
+  ZeroRttSendReceive(true, false);
+
+  Handshake();
+  ExpectEarlyDataAccepted(false);
+  CheckConnected();
+  SendReceive();
+}
+
 #ifndef NSS_DISABLE_TLS_1_3
 INSTANTIATE_TEST_CASE_P(Tls13ZeroRttReplayTest, TlsZeroRttReplayTest,
                         TlsConnectTestBase::kTlsVariantsAll);
 #endif
 
 }  // namespace nss_test
--- a/security/nss/gtests/ssl_gtest/ssl_auth_unittest.cc
+++ b/security/nss/gtests/ssl_gtest/ssl_auth_unittest.cc
@@ -578,24 +578,37 @@ TEST_F(TlsConnectStreamTls13, PostHandsh
   EXPECT_EQ(1U, called);
   ScopedCERTCertificate cert1(SSL_PeerCertificate(server_->ssl_fd()));
   ASSERT_NE(nullptr, cert1.get());
   ScopedCERTCertificate cert2(SSL_LocalCertificate(client_->ssl_fd()));
   ASSERT_NE(nullptr, cert2.get());
   EXPECT_TRUE(SECITEM_ItemsAreEqual(&cert1->derCert, &cert2->derCert));
 }
 
-// In TLS 1.3, the client sends its cert rejection on the
-// second flight, and since it has already received the
-// server's Finished, it transitions to complete and
-// then gets an alert from the server. The test harness
-// doesn't handle this right yet.
-TEST_P(TlsConnectStream, DISABLED_ClientAuthRequiredRejected) {
+TEST_P(TlsConnectGenericPre13, ClientAuthRequiredRejected) {
+  server_->RequestClientAuth(true);
+  ConnectExpectAlert(server_, kTlsAlertBadCertificate);
+  client_->CheckErrorCode(SSL_ERROR_BAD_CERT_ALERT);
+  server_->CheckErrorCode(SSL_ERROR_NO_CERTIFICATE);
+}
+
+// In TLS 1.3, the client will claim that the connection is done and then
+// receive the alert afterwards.  So drive the handshake manually.
+TEST_P(TlsConnectTls13, ClientAuthRequiredRejected) {
   server_->RequestClientAuth(true);
-  ConnectExpectFail();
+  StartConnect();
+  client_->Handshake();  // CH
+  server_->Handshake();  // SH.. (no resumption)
+  client_->Handshake();  // Next message
+  ASSERT_EQ(TlsAgent::STATE_CONNECTED, client_->state());
+  ExpectAlert(server_, kTlsAlertCertificateRequired);
+  server_->Handshake();  // Alert
+  server_->CheckErrorCode(SSL_ERROR_NO_CERTIFICATE);
+  client_->Handshake();  // Receive Alert
+  client_->CheckErrorCode(SSL_ERROR_RX_CERTIFICATE_REQUIRED_ALERT);
 }
 
 TEST_P(TlsConnectGeneric, ClientAuthRequestedRejected) {
   server_->RequestClientAuth(false);
   Connect();
   CheckKeys();
 }
 
--- a/security/nss/gtests/ssl_gtest/ssl_resumption_unittest.cc
+++ b/security/nss/gtests/ssl_gtest/ssl_resumption_unittest.cc
@@ -594,77 +594,79 @@ TEST_P(TlsConnectTls13, WriteBeforeHands
 // tests.  Those cipher suites need to be suited to the version.
 static uint16_t ChooseOneCipher(uint16_t version) {
   if (version >= SSL_LIBRARY_VERSION_TLS_1_3) {
     return TLS_AES_128_GCM_SHA256;
   }
   return TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA;
 }
 
-static uint16_t ChooseAnotherCipher(uint16_t version) {
+static uint16_t ChooseIncompatibleCipher(uint16_t version) {
   if (version >= SSL_LIBRARY_VERSION_TLS_1_3) {
     return TLS_AES_256_GCM_SHA384;
   }
   return TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA;
 }
 
-// Test that we don't resume when we can't negotiate the same cipher.
-TEST_P(TlsConnectGenericResumption, TestResumeClientDifferentCipher) {
+// Test that we don't resume when we can't negotiate the same cipher.  Note that
+// for TLS 1.3, resumption is allowed between compatible ciphers, that is those
+// with the same KDF hash, but we choose an incompatible one here.
+TEST_P(TlsConnectGenericResumption, ResumeClientIncompatibleCipher) {
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   client_->EnableSingleCipher(ChooseOneCipher(version_));
   Connect();
   SendReceive();
   CheckKeys(ssl_kea_ecdh, ssl_auth_rsa_sign);
 
   Reset();
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   ExpectResumption(RESUME_NONE);
-  client_->EnableSingleCipher(ChooseAnotherCipher(version_));
+  client_->EnableSingleCipher(ChooseIncompatibleCipher(version_));
   uint16_t ticket_extension;
   if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
     ticket_extension = ssl_tls13_pre_shared_key_xtn;
   } else {
     ticket_extension = ssl_session_ticket_xtn;
   }
   auto ticket_capture =
       MakeTlsFilter<TlsExtensionCapture>(client_, ticket_extension);
   Connect();
   CheckKeys(ssl_kea_ecdh, ssl_auth_rsa_sign);
   EXPECT_EQ(0U, ticket_capture->extension().len());
 }
 
 // Test that we don't resume when we can't negotiate the same cipher.
-TEST_P(TlsConnectGenericResumption, TestResumeServerDifferentCipher) {
+TEST_P(TlsConnectGenericResumption, ResumeServerIncompatibleCipher) {
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   server_->EnableSingleCipher(ChooseOneCipher(version_));
   Connect();
-  SendReceive();  // Need to read so that we absorb the session ticket.
+  SendReceive();  // Absorb the session ticket.
   CheckKeys();
 
   Reset();
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   ExpectResumption(RESUME_NONE);
-  server_->EnableSingleCipher(ChooseAnotherCipher(version_));
+  server_->EnableSingleCipher(ChooseIncompatibleCipher(version_));
   Connect();
   CheckKeys();
 }
 
 // Test that the client doesn't tolerate the server picking a different cipher
 // suite for resumption.
-TEST_P(TlsConnectStream, TestResumptionOverrideCipher) {
+TEST_P(TlsConnectStream, ResumptionOverrideCipher) {
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
   server_->EnableSingleCipher(ChooseOneCipher(version_));
   Connect();
   SendReceive();
   CheckKeys(ssl_kea_ecdh, ssl_auth_rsa_sign);
 
   Reset();
   ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
-  MakeTlsFilter<SelectedCipherSuiteReplacer>(server_,
-                                             ChooseAnotherCipher(version_));
+  MakeTlsFilter<SelectedCipherSuiteReplacer>(
+      server_, ChooseIncompatibleCipher(version_));
 
   if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
     client_->ExpectSendAlert(kTlsAlertIllegalParameter);
     server_->ExpectSendAlert(kTlsAlertUnexpectedMessage);
   } else {
     ExpectAlert(client_, kTlsAlertHandshakeFailure);
   }
   ConnectExpectFail();
@@ -673,16 +675,48 @@ TEST_P(TlsConnectStream, TestResumptionO
     // The reason this test is stream only: the server is unable to decrypt
     // the alert that the client sends, see bug 1304603.
     server_->CheckErrorCode(SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE);
   } else {
     server_->CheckErrorCode(SSL_ERROR_HANDSHAKE_FAILURE_ALERT);
   }
 }
 
+// In TLS 1.3, it is possible to resume with a different cipher if it has the
+// same hash.
+TEST_P(TlsConnectTls13, ResumeClientCompatibleCipher) {
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  client_->EnableSingleCipher(TLS_AES_128_GCM_SHA256);
+  Connect();
+  SendReceive();  // Absorb the session ticket.
+  CheckKeys();
+
+  Reset();
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  ExpectResumption(RESUME_TICKET);
+  client_->EnableSingleCipher(TLS_CHACHA20_POLY1305_SHA256);
+  Connect();
+  CheckKeys();
+}
+
+TEST_P(TlsConnectTls13, ResumeServerCompatibleCipher) {
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  server_->EnableSingleCipher(TLS_AES_128_GCM_SHA256);
+  Connect();
+  SendReceive();  // Absorb the session ticket.
+  CheckKeys();
+
+  Reset();
+  ConfigureSessionCache(RESUME_BOTH, RESUME_TICKET);
+  ExpectResumption(RESUME_TICKET);
+  server_->EnableSingleCipher(TLS_CHACHA20_POLY1305_SHA256);
+  Connect();
+  CheckKeys();
+}
+
 class SelectedVersionReplacer : public TlsHandshakeFilter {
  public:
   SelectedVersionReplacer(const std::shared_ptr<TlsAgent>& a, uint16_t version)
       : TlsHandshakeFilter(a, {kTlsHandshakeServerHello}), version_(version) {}
 
  protected:
   PacketFilter::Action FilterHandshake(const HandshakeHeader& header,
                                        const DataBuffer& input,
@@ -1329,16 +1363,64 @@ TEST_P(TlsConnectGenericResumption, Conn
     ExpectResumption(RESUME_NONE);
   } else {
     ExpectResumption(RESUME_TICKET);
   }
   Connect();
   SendReceive();
 }
 
+// Check that resumption is blocked if the server requires client auth.
+TEST_P(TlsConnectGenericResumption, ClientAuthRequiredOnResumption) {
+  ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH);
+  server_->RequestClientAuth(false);
+  Connect();
+  SendReceive();
+
+  Reset();
+  client_->SetupClientAuth();
+  server_->RequestClientAuth(true);
+  ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH);
+  ExpectResumption(RESUME_NONE);
+  Connect();
+  SendReceive();
+}
+
+// Check that resumption is blocked if the server requires client auth and
+// the client fails to provide a certificate.
+TEST_P(TlsConnectGenericResumption, ClientAuthRequiredOnResumptionNoCert) {
+  ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH);
+  server_->RequestClientAuth(false);
+  Connect();
+  SendReceive();
+
+  Reset();
+  server_->RequestClientAuth(true);
+  ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH);
+  // Drive handshake manually because TLS 1.3 needs it.
+  StartConnect();
+  client_->Handshake();  // CH
+  server_->Handshake();  // SH.. (no resumption)
+  client_->Handshake();  // ...
+  if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
+    // In TLS 1.3, the client thinks that everything is OK here.
+    ASSERT_EQ(TlsAgent::STATE_CONNECTED, client_->state());
+    ExpectAlert(server_, kTlsAlertCertificateRequired);
+    server_->Handshake();  // Alert
+    client_->Handshake();  // Receive Alert
+    client_->CheckErrorCode(SSL_ERROR_RX_CERTIFICATE_REQUIRED_ALERT);
+  } else {
+    ExpectAlert(server_, kTlsAlertBadCertificate);
+    server_->Handshake();  // Alert
+    client_->Handshake();  // Receive Alert
+    client_->CheckErrorCode(SSL_ERROR_BAD_CERT_ALERT);
+  }
+  server_->CheckErrorCode(SSL_ERROR_NO_CERTIFICATE);
+}
+
 TEST_F(TlsConnectStreamTls13, ExternalTokenAfterHrr) {
   ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH);
   Connect();
   SendReceive();
 
   Reset();
   ConfigureSessionCache(RESUME_BOTH, RESUME_BOTH);
   ExpectResumption(RESUME_TICKET);
--- a/security/nss/lib/ssl/ssl3con.c
+++ b/security/nss/lib/ssl/ssl3con.c
@@ -857,16 +857,21 @@ ssl3_config_match_init(sslSocket *ss)
  * usable with the negotiated TLS version, and is otherwise usable. */
 PRBool
 ssl3_config_match(const ssl3CipherSuiteCfg *suite, PRUint8 policy,
                   const SSLVersionRange *vrange, const sslSocket *ss)
 {
     const ssl3CipherSuiteDef *cipher_def;
     const ssl3KEADef *kea_def;
 
+    if (!suite) {
+        PORT_Assert(suite);
+        return PR_FALSE;
+    }
+
     PORT_Assert(policy != SSL_NOT_ALLOWED);
     if (policy == SSL_NOT_ALLOWED)
         return PR_FALSE;
 
     if (!suite->enabled || !suite->isPresent)
         return PR_FALSE;
 
     if ((suite->policy == SSL_NOT_ALLOWED) ||
@@ -904,16 +909,37 @@ count_cipher_suites(sslSocket *ss, PRUin
             count++;
     }
     if (count == 0) {
         PORT_SetError(SSL_ERROR_SSL_DISABLED);
     }
     return count;
 }
 
+/* For TLS 1.3, when resuming, check for a ciphersuite that is both compatible
+ * with the identified ciphersuite and enabled. */
+static PRBool
+tls13_ResumptionCompatible(sslSocket *ss, ssl3CipherSuite suite)
+{
+    SSLVersionRange vrange = { SSL_LIBRARY_VERSION_TLS_1_3,
+                               SSL_LIBRARY_VERSION_TLS_1_3 };
+    SSLHashType hash = tls13_GetHashForCipherSuite(suite);
+    for (unsigned int i = 0; i < PR_ARRAY_SIZE(cipher_suite_defs); i++) {
+        if (cipher_suite_defs[i].prf_hash == hash) {
+            const ssl3CipherSuiteCfg *suiteCfg =
+                ssl_LookupCipherSuiteCfg(cipher_suite_defs[i].cipher_suite,
+                                         ss->cipherSuites);
+            if (suite && ssl3_config_match(suiteCfg, ss->ssl3.policy, &vrange, ss)) {
+                return PR_TRUE;
+            }
+        }
+    }
+    return PR_FALSE;
+}
+
 /*
  * Null compression, mac and encryption functions
  */
 SECStatus
 Null_Cipher(void *ctx, unsigned char *output, unsigned int *outputLen, unsigned int maxOutputLen,
             const unsigned char *input, unsigned int inputLen)
 {
     if (inputLen > maxOutputLen) {
@@ -4892,24 +4918,30 @@ ssl3_SendClientHello(sslSocket *ss, sslC
 
     /* We can't resume based on a different token. If the sid exists,
      * make sure the token that holds the master secret still exists ...
      * If we previously did client-auth, make sure that the token that holds
      * the private key still exists, is logged in, hasn't been removed, etc.
      */
     if (sid) {
         PRBool sidOK = PR_TRUE;
-        const ssl3CipherSuiteCfg *suite;
-
-        /* Check that the cipher suite we need is enabled. */
-        suite = ssl_LookupCipherSuiteCfg(sid->u.ssl3.cipherSuite,
+
+        if (sid->version >= SSL_LIBRARY_VERSION_TLS_1_3) {
+            if (!tls13_ResumptionCompatible(ss, sid->u.ssl3.cipherSuite)) {
+                sidOK = PR_FALSE;
+            }
+        } else {
+            /* Check that the cipher suite we need is enabled. */
+            const ssl3CipherSuiteCfg *suite =
+                ssl_LookupCipherSuiteCfg(sid->u.ssl3.cipherSuite,
                                          ss->cipherSuites);
-        PORT_Assert(suite);
-        if (!suite || !ssl3_config_match(suite, ss->ssl3.policy, &ss->vrange, ss)) {
-            sidOK = PR_FALSE;
+            SSLVersionRange vrange = { sid->version, sid->version };
+            if (!suite || !ssl3_config_match(suite, ss->ssl3.policy, &vrange, ss)) {
+                sidOK = PR_FALSE;
+            }
         }
 
         /* Check that we can recover the master secret. */
         if (sidOK) {
             PK11SlotInfo *slot = NULL;
             if (sid->u.ssl3.masterValid) {
                 slot = SECMOD_LookupSlot(sid->u.ssl3.masterModuleID,
                                          sid->u.ssl3.masterSlotID);
@@ -8675,19 +8707,19 @@ ssl3_HandleClientHello(sslSocket *ss, PR
          */
         if ((sid->peerCert == NULL) && ss->opt.requestCertificate &&
             ((ss->opt.requireCertificate == SSL_REQUIRE_ALWAYS) ||
              (ss->opt.requireCertificate == SSL_REQUIRE_NO_ERROR) ||
              ((ss->opt.requireCertificate == SSL_REQUIRE_FIRST_HANDSHAKE) &&
               !ss->firstHsDone))) {
 
             SSL_AtomicIncrementLong(&ssl3stats.hch_sid_cache_not_ok);
-            ssl_UncacheSessionID(ss);
             ssl_FreeSID(sid);
             sid = NULL;
+            ss->statelessResume = PR_FALSE;
         }
     }
 
     if (IS_DTLS(ss)) {
         ssl3_DisableNonDTLSSuites(ss);
         dtls_ReceivedFirstMessageInFlight(ss);
     }
 
--- a/security/nss/lib/util/utilmod.c
+++ b/security/nss/lib/util/utilmod.c
@@ -408,17 +408,20 @@ nssutil_ReadSecmodDB(const char *appName
         int len;
 
         if (fgets(line, sizeof(line), fd) == NULL) {
             goto endloop;
         }
 
         /* remove the ending newline */
         len = PORT_Strlen(line);
-        if (len && line[len - 1] == '\n') {
+        if (len >= 2 && line[len - 2] == '\r' && line[len - 1] == '\n') {
+            len = len - 2;
+            line[len] = 0;
+        } else if (len && (line[len - 1] == '\n' || line[len - 1] == '\r')) {
             len--;
             line[len] = 0;
         }
         if (*line == '#') {
             continue;
         }
         if (*line != 0) {
             /*
--- a/security/nss/tests/common/cleanup.sh
+++ b/security/nss/tests/common/cleanup.sh
@@ -1,19 +1,19 @@
 #!/bin/bash
 #
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 
 if [ -z "${CLEANUP}" -o "${CLEANUP}" = "${SCRIPTNAME}" ]; then
-    if [ -z "${BUILD_OPT}" ] && [ "$OBJDIR" == "Debug"  ]; then
+    if [ -z "${BUILD_OPT}" ] && [ "${OBJDIR}" == "Debug"  ]; then
         BUILD_OPT=0;
-    elif [ -z "${BUILD_OPT}" ] && [ "$OBJDIR" == "Release" ]; then
+    elif [ -z "${BUILD_OPT}" ] && [ "${OBJDIR}" == "Release" ]; then
         BUILD_OPT=1;
     fi
 
     echo
     echo "SUMMARY:"
     echo "========"
     echo "NSS variables:"
     echo "--------------"
@@ -55,13 +55,13 @@ if [ -z "${CLEANUP}" -o "${CLEANUP}" = "
         echo "TinderboxPrint:Unknown: ${LINES_CNT}"
     fi
     echo
 
     html "END_OF_TEST<BR>"
     html "</BODY></HTML>"
     rm -f ${TEMPFILES} 2>/dev/null
     if [ ${FAILED_CNT} -gt 0 ] || [ ${ASAN_CNT} -gt 0 ] ||
-       ([ ${BUILD_OPT} -eq 1 ] && [ ${CORE_CNT} -gt 0 ]); then
+       ([ ${CORE_CNT} -gt 0 ] && [ -n "${BUILD_OPT}" ] && [ ${BUILD_OPT} -eq 1 ]); then
         exit 1
     fi
 
 fi