Bug 1767934 - land NSS 2efccbd85918 UPGRADE_NSS_RELEASE, r=nss-reviewers,djackson

2022-05-19  John M. Schanck  <jschanck@mozilla.com>

	* lib/ckfw/wrap.c:
	Bug 1766978 - improve error handling after
	nssCKFWInstance_CreateObjectHandle. r=djackson

	[2efccbd85918] [tip]

2022-03-18  Robert Relyea  <rrelyea@redhat.com>

	* cmd/pk12util/pk12util.c, lib/pkcs12/p12local.c,
	tests/common/init.sh, tests/tools/tools.sh:
	Bug 1757075 NSS does not properly import or export pkcs12 files with
	large passwords and pkcs5v2 encoding.

	Don't use NULL when encoding UTF8 with pkcs5v2. Fix a bug here when
	converting from UCS2 to UTF8 we would add a double NULL when adding
	a NULL.

	[0f4664512bd0]

2022-05-17  Dennis Jackson  <djackson@mozilla.com>

	* nspr.patch:
	Remove nspr.patch mistakenly committed in e3ac914bc684
	[99e32fcca1c7]

2022-05-17  Leander Schwarz  <lschwarz@mozilla.com>

	* 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:
	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.

	[7f4b0af3a526]

	* gtests/ssl_gtest/ssl_version_unittest.cc, lib/ssl/ssl3con.c:
	Bug 1765753 - TLS 1.3 Server: Send protocol_version alert on
	unsupported ClientHello.legacy_version. r=djackson

	[bc7bfba47e0a]

	* gtests/ssl_gtest/ssl_extension_unittest.cc, lib/ssl/ssl3exthandle.c:
	Bug 1765753 - Added RFC8422 compliant TLS <= 1.2
	undefined/compressed ECPointFormat extension alerts. r=djackson

	[d06a8831ec84]

2022-05-16  John M. Schanck  <jschanck@mozilla.com>

	* gtests/util_gtest/manifest.mn, gtests/util_gtest/util_gtest.gyp,
	gtests/util_gtest/util_secasn1d_unittest.cc, lib/util/secasn1d.c:
	Bug 1387919 - Fix secasn1d parsing of indefinite SEQUENCE inside
	indefinite GROUP. r=keeler,nss-reviewers,djackson

	In an iteration over elements of an indefinite-length encoded GROUP
	(sec_asn1d_next_in_group), the child of the current state is
	responsible for parsing the GROUP's end-of-contents octets---a call
	to sec_asn1d_parse_end_of_contents(state->child) sets the
	endofcontents flag for state->child and a later call to
	sec_asn1d_next_in_group checks state->child->endofcontents and
	terminates the iteration.

	In an iteration over elements of an indefinite-length encoded
	SEQUENCE (sec_asn1d_next_in_sequence), on the other hand, the
	current state, not its child, handles the end-of-contents octets.

	Prior to this commit, an error would occur when state pointed to an
	indefinite-length encoded GROUP and state->child pointed to an
	indefinite-length encoded SEQUENCE. In this case, state->child would
	be passed to sec_asn1d_parse_end_of_contents to parse the SEQUENCE's
	end-of-contents octets. This would set the endofcontents flag for
	state->child, and this would be misinterpreted as an end-of-
	iteration signal for the surrounding GROUP.

	[1811eec24997]

	* automation/abi-check/expected-report-libnss3.so.txt,
	lib/nss/nss.def, lib/pk11wrap/pk11list.c, lib/pk11wrap/pk11util.c,
	lib/pk11wrap/secmod.h, lib/util/nssrwlk.h:
	Bug 1753315 - Add SECMOD_LockedModuleHasRemovableSlots. r=rrelyea

	[499ae15c18ad]

2022-05-13  Kai Engert  <kaie@kuix.de>

	* automation/abi-check/expected-report-libnspr4.so.txt,
	cmd/selfserv/selfserv.c, cmd/tstclnt/tstclnt.c, nspr.patch:
	Bug 1769295 - selfserv and tstclnt should use
	PR_GetPrefLoopbackAddrInfo. r=rrelyea

	[e3ac914bc684]

2022-05-11  John M. Schanck  <jschanck@mozilla.com>

	* lib/softoken/legacydb/lginit.c:
	Bug 1454072 - Use of uninitialized pointer in lg_init after alloc
	fail. r=nss-reviewers,nkulatova

	[927d47dcc509]

2022-05-06  John M. Schanck  <jschanck@mozilla.com>

	* automation/clang-format/Dockerfile:
	Bug 1766907 - Update mercurial in clang-format docker image. r=mt

	[83a89ed9f527]

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

security/nss/TAG-INFO

@@ -1 +1 @@
-85bf9240f3e1
+2efccbd85918

security/nss/automation/abi-check/expected-report-libnspr4.so.txt

@@ -0,0 +1,5 @@
+
+1 Added function:
+
+  'function PRStatus PR_GetPrefLoopbackAddrInfo(PRNetAddr*, PRUint16)'    {PR_GetPrefLoopbackAddrInfo}
+

security/nss/automation/abi-check/expected-report-libnss3.so.txt

@@ -0,0 +1,5 @@
+
+1 Added function:
+
+  'function PRBool SECMOD_LockedModuleHasRemovableSlots(SECMODModule*)'    {SECMOD_LockedModuleHasRemovableSlots@@NSS_3.79}
+

security/nss/automation/clang-format/Dockerfile

@@ -7,10 +7,17 @@ RUN apt-get update \
     ca-certificates \
     clang-format-3.9 \
     locales \
-    mercurial \
+    python-dev \
+    python-pip \
+    python-setuptools \
+    python-wheel \
+    build-essential \
  && rm -rf /var/lib/apt/lists/* \
  && apt-get autoremove -y && apt-get clean -y
 
+RUN pip install mercurial==6.1.1
+
+
 RUN update-alternatives --install /usr/bin/clang-format \
     clang-format $(which clang-format-3.9) 10
 

security/nss/cmd/pk12util/pk12util.c

@@ -665,6 +665,17 @@ P12U_ExportPKCS12Object(char *nn, char *outfile, PK11SlotInfo *inSlot,
         goto loser;
     }
 
+    /* we are passing UTF8, drop the NULL in the normal password value.
+     * UCS2 conversion will add it back if necessary. This only affects
+     * password > Blocksize of the Hash function and pkcs5v2 pbe (if password
+     * <=Blocksize then the password is zero padded anyway, so an extra NULL
+     * at the end has not effect). This is allows us to work with openssl and
+     * gnutls. Older versions of NSS already fail to decrypt long passwords
+     * in this case, so we aren't breaking anyone with this code */
+    if ((pwitem->len > 0) && (!pwitem->data[pwitem->len - 1])) {
+        pwitem->len--;
+    }
+
     p12cxt = p12u_InitContext(PR_FALSE, outfile);
     if (!p12cxt) {
         SECU_PrintError(progName, "Initialization failed: %s", outfile);

security/nss/cmd/selfserv/selfserv.c

@@ -1711,19 +1711,30 @@ do_accepts(
 PRFileDesc *
 getBoundListenSocket(unsigned short port)
 {
-    PRFileDesc *listen_sock;
+    PRFileDesc *listen_sock = NULL;
     int listenQueueDepth = 5 + (2 * maxThreads);
     PRStatus prStatus;
     PRNetAddr addr;
     PRSocketOptionData opt;
 
-    addr.inet.family = PR_AF_INET;
+    // We want to listen on the IP family that tstclnt will use.
-    addr.inet.ip = PR_INADDR_ANY;
+    // tstclnt uses PR_GetPrefLoopbackAddrInfo to decide, if it's
-    addr.inet.port = PR_htons(port);
+    // asked to connect to localhost.
 
-    listen_sock = PR_NewTCPSocket();
+    prStatus = PR_GetPrefLoopbackAddrInfo(&addr, port);
+    if (prStatus == PR_FAILURE) {
+        addr.inet.family = PR_AF_INET;
+        addr.inet.ip = PR_INADDR_ANY;
+        addr.inet.port = PR_htons(port);
+    }
+
+    if (addr.inet.family == PR_AF_INET6) {
+        listen_sock = PR_OpenTCPSocket(PR_AF_INET6);
+    } else if (addr.inet.family == PR_AF_INET) {
+        listen_sock = PR_NewTCPSocket();
+    }
     if (listen_sock == NULL) {
-        errExit("PR_NewTCPSocket");
+        errExit("Couldn't create socket");
     }
 
     opt.option = PR_SockOpt_Nonblocking;

security/nss/cmd/tstclnt/tstclnt.c

@@ -2190,32 +2190,45 @@ main(int argc, char **argv)
     if (status == PR_SUCCESS) {
         addr.inet.port = PR_htons(portno);
     } else {
-        /* Lookup host */
+        PRBool gotLoopbackIP = PR_FALSE;
-        PRAddrInfo *addrInfo;
+        if ((!strcmp(host, "localhost") || !strcmp(host, "localhost.localdomain"))
-        void *enumPtr = NULL;
+            /* only check for preference if both types are allowed */
+            && allowIPv4 && allowIPv6) {
+            /* make a decision which IP to prefer */
+            status = PR_GetPrefLoopbackAddrInfo(&addr, portno);
+            if (status != PR_FAILURE) {
+                gotLoopbackIP = PR_TRUE;
+            }
+        }
 
-        addrInfo = PR_GetAddrInfoByName(host, PR_AF_UNSPEC,
+        if (!gotLoopbackIP) {
-                                        PR_AI_ADDRCONFIG | PR_AI_NOCANONNAME);
+            /* Lookup host */
-        if (!addrInfo) {
+            PRAddrInfo *addrInfo;
-            fprintf(stderr, "HOSTNAME=%s\n", host);
+            void *enumPtr = NULL;
-            SECU_PrintError(progName, "error looking up host");
+
-            error = 1;
+            addrInfo = PR_GetAddrInfoByName(host, PR_AF_UNSPEC,
-            goto done;
+                                            PR_AI_ADDRCONFIG | PR_AI_NOCANONNAME);
-        }
+            if (!addrInfo) {
-        for (;;) {
+                fprintf(stderr, "HOSTNAME=%s\n", host);
-            enumPtr = PR_EnumerateAddrInfo(enumPtr, addrInfo, portno, &addr);
+                SECU_PrintError(progName, "error looking up host");
-            if (enumPtr == NULL)
+                error = 1;
-                break;
+                goto done;
-            if (addr.raw.family == PR_AF_INET && allowIPv4)
+            }
-                break;
+            for (;;) {
-            if (addr.raw.family == PR_AF_INET6 && allowIPv6)
+                enumPtr = PR_EnumerateAddrInfo(enumPtr, addrInfo, portno, &addr);
-                break;
+                if (enumPtr == NULL)
-        }
+                    break;
-        PR_FreeAddrInfo(addrInfo);
+                if (addr.raw.family == PR_AF_INET && allowIPv4)
-        if (enumPtr == NULL) {
+                    break;
-            SECU_PrintError(progName, "error looking up host address");
+                if (addr.raw.family == PR_AF_INET6 && allowIPv6)
-            error = 1;
+                    break;
-            goto done;
+            }
+            PR_FreeAddrInfo(addrInfo);
+            if (enumPtr == NULL) {
+                SECU_PrintError(progName, "error looking up host address");
+                error = 1;
+                goto done;
+            }
         }
     }
 

security/nss/coreconf/coreconf.dep

@@ -10,3 +10,4 @@
  */
 
 #error "Do not include this header file."
+

security/nss/gtests/ssl_gtest/ssl_extension_unittest.cc

@@ -597,6 +597,22 @@ TEST_P(TlsExtensionTestPre13, SupportedPointsTrailingData) {
       client_, ssl_ec_point_formats_xtn, extension));
 }
 
+TEST_P(TlsExtensionTestPre13, SupportedPointsCompressed) {
+  const uint8_t val[] = {0x01, 0x02};
+  DataBuffer extension(val, sizeof(val));
+  ClientHelloErrorTest(std::make_shared<TlsExtensionReplacer>(
+                           client_, ssl_ec_point_formats_xtn, extension),
+                       kTlsAlertIllegalParameter);
+}
+
+TEST_P(TlsExtensionTestPre13, SupportedPointsUndefined) {
+  const uint8_t val[] = {0x01, 0xAA};
+  DataBuffer extension(val, sizeof(val));
+  ClientHelloErrorTest(std::make_shared<TlsExtensionReplacer>(
+                           client_, ssl_ec_point_formats_xtn, extension),
+                       kTlsAlertIllegalParameter);
+}
+
 TEST_P(TlsExtensionTestPre13, RenegotiationInfoBadLength) {
   const uint8_t val[] = {0x99};
   DataBuffer extension(val, sizeof(val));

security/nss/gtests/ssl_gtest/ssl_record_unittest.cc

@@ -310,22 +310,16 @@ 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
+/* Filter to modify record header and content */
- * type to the specified one (default handshake) and optionally add padding,
+class Tls13RecordModifier : public TlsRecordFilter {
- * 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(
+  Tls13RecordModifier(const std::shared_ptr<TlsAgent>& a,
-      const std::shared_ptr<TlsAgent>& a,
+                      uint8_t contentType = ssl_ct_handshake, size_t size = 0,
-      SSLContentType contentType = ssl_ct_handshake, size_t padding = 0)
+                      size_t padding = 0)
-      : TlsRecordFilter(a), contentType_(contentType), padding_(padding) {}
+      : TlsRecordFilter(a),
+        contentType_(contentType),
+        size_(size),
+        padding_(padding) {}
 
  protected:
   PacketFilter::Action FilterRecord(const TlsRecordHeader& header,
@@ -350,7 +344,7 @@ class ZeroLengthInnerPlaintextCreatorFilter : public TlsRecordFilter {
 
     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;
@@ -361,7 +355,8 @@ class ZeroLengthInnerPlaintextCreatorFilter : public TlsRecordFilter {
   }
 
  private:
-  SSLContentType contentType_;
+  uint8_t contentType_;
+  size_t size_;
   size_t padding_;
 };
 
@@ -389,13 +384,18 @@ class ZeroLengthInnerPlaintextSetupTls13
 };
 
 /* 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>(
+  auto filter =
-      client_, contentType_, padding_);
+      MakeTlsFilter<Tls13RecordModifier>(client_, contentType_, 0, padding_);
   filter->EnableDecryption();
   filter->Disable();
 
@@ -603,4 +603,202 @@ INSTANTIATE_TEST_SUITE_P(
       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

security/nss/gtests/ssl_gtest/ssl_v2_client_hello_unittest.cc

@@ -214,7 +214,7 @@ TEST_P(SSLv2ClientHelloTest, ConnectDisabled) {
   // 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();
 }
@@ -237,8 +237,8 @@ TEST_P(SSLv2ClientHelloTest, ConnectAfterEmptyV3Record) {
   // as the record length.
   SetPadding(255);
 
-  ConnectExpectAlert(server_, kTlsAlertIllegalParameter);
+  ConnectExpectAlert(server_, kTlsAlertUnexpectedMessage);
-  EXPECT_EQ(SSL_ERROR_BAD_CLIENT, server_->error_code());
+  EXPECT_EQ(SSL_ERROR_RX_UNKNOWN_RECORD_TYPE, server_->error_code());
 }
 
 // Test negotiating TLS 1.3.
@@ -277,7 +277,7 @@ TEST_P(SSLv2ClientHelloTest, SendSecurityEscape) {
   // 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.

security/nss/gtests/ssl_gtest/ssl_version_unittest.cc

@@ -70,10 +70,8 @@ TEST_F(TlsConnectTest, TestDowngradeDetectionToTls11) {
 // Attempt to negotiate the bogus DTLS 1.1 version.
 TEST_F(DtlsConnectTest, TestDtlsVersion11) {
   MakeTlsFilter<TlsClientHelloVersionSetter>(client_, ((~0x0101) & 0xffff));
-  ConnectExpectAlert(server_, kTlsAlertHandshakeFailure);
+  ConnectExpectAlert(server_, kTlsAlertProtocolVersion);
-  // It's kind of surprising that SSL_ERROR_NO_CYPHER_OVERLAP is
+  client_->CheckErrorCode(SSL_ERROR_PROTOCOL_VERSION_ALERT);
-  // what is returned here, but this is deliberate in ssl3_HandleAlert().
-  client_->CheckErrorCode(SSL_ERROR_NO_CYPHER_OVERLAP);
   server_->CheckErrorCode(SSL_ERROR_UNSUPPORTED_VERSION);
 }
 
@@ -419,6 +417,33 @@ TEST_F(TlsConnectTest, TlsSupportedVersionsEncoding) {
   EXPECT_EQ(SSL_LIBRARY_VERSION_TLS_1_0, static_cast<int>(version));
 }
 
+/* Test that on reception of unsupported ClientHello.legacy_version the TLS 1.3
+ * server sends the correct alert.
+ *
+ * If the "supported_versions" extension is absent and the server only supports
+ * versions greater than ClientHello.legacy_version, the server MUST abort the
+ * handshake with a "protocol_version" alert [RFC8446, Appendix D.2]. */
+TEST_P(TlsConnectGenericPre13, ClientHelloUnsupportedTlsVersion) {
+  StartConnect();
+
+  if (variant_ == ssl_variant_stream) {
+    server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_3,
+                             SSL_LIBRARY_VERSION_TLS_1_3);
+  } else {
+    server_->SetVersionRange(SSL_LIBRARY_VERSION_DTLS_1_3,
+                             SSL_LIBRARY_VERSION_DTLS_1_3);
+  }
+
+  // Try to handshake
+  client_->Handshake();
+  // Expect protocol version alert
+  server_->ExpectSendAlert(kTlsAlertProtocolVersion);
+  server_->Handshake();
+  // Digest alert at peer
+  client_->ExpectReceiveAlert(kTlsAlertProtocolVersion);
+  client_->ReadBytes();
+}
+
 INSTANTIATE_TEST_SUITE_P(
     TlsDowngradeSentinelTest, TlsDowngradeTest,
     ::testing::Combine(TlsConnectTestBase::kTlsVariantsStream,

security/nss/gtests/util_gtest/manifest.mn

@@ -12,6 +12,7 @@ CPPSRCS = \
         util_gtests.cc \
         util_memcmpzero_unittest.cc \
         util_pkcs11uri_unittest.cc \
+        util_secasn1d_unittest.cc \
         util_utf8_unittest.cc \
         $(NULL)
 

security/nss/gtests/util_gtest/util_gtest.gyp

@@ -16,6 +16,7 @@
         'util_gtests.cc',
         'util_memcmpzero_unittest.cc',
         'util_pkcs11uri_unittest.cc',
+        'util_secasn1d_unittest.cc',
         'util_utf8_unittest.cc',
       ],
       'dependencies': [

security/nss/gtests/util_gtest/util_secasn1d_unittest.cc

@@ -0,0 +1,69 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=2 et sw=2 tw=80: */
+/* 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/. */
+
+#include "secasn1.h"
+
+#include "gtest/gtest.h"
+
+namespace nss_test {
+
+class SECASN1DTest : public ::testing::Test {};
+
+struct InnerSequenceItem {
+  SECItem value;
+};
+
+struct OuterSequence {
+  InnerSequenceItem *item;
+};
+
+static const SEC_ASN1Template InnerSequenceTemplate[] = {
+    {SEC_ASN1_SEQUENCE, 0, NULL, sizeof(InnerSequenceItem)},
+    {SEC_ASN1_ANY, offsetof(InnerSequenceItem, value)},
+    {0}};
+
+static const SEC_ASN1Template OuterSequenceTemplate[] = {
+    {SEC_ASN1_SEQUENCE_OF, offsetof(OuterSequence, item), InnerSequenceTemplate,
+     sizeof(OuterSequence)}};
+
+TEST_F(SECASN1DTest, IndefiniteSequenceInIndefiniteGroup) {
+  PLArenaPool *arena = PORT_NewArena(4096);
+  OuterSequence *outer = nullptr;
+  SECStatus rv;
+
+  // echo "SEQUENCE indefinite {
+  //         SEQUENCE indefinite {
+  //            PrintableString { \"Test for Bug 1387919\" }
+  //         }
+  //       }" | ascii2der | xxd -i
+  unsigned char ber[] = {0x30, 0x80, 0x30, 0x80, 0x13, 0x14, 0x54, 0x65,
+                         0x73, 0x74, 0x20, 0x66, 0x6f, 0x72, 0x20, 0x42,
+                         0x75, 0x67, 0x20, 0x31, 0x33, 0x38, 0x37, 0x39,
+                         0x31, 0x39, 0x00, 0x00, 0x00, 0x00};
+
+  // Decoding should fail if the trailing EOC is omitted (Bug 1387919)
+  SECItem missingEOC = {siBuffer, ber, sizeof(ber) - 2};
+  rv = SEC_ASN1DecodeItem(arena, &outer, OuterSequenceTemplate, &missingEOC);
+  EXPECT_EQ(SECFailure, rv);
+
+  // With the trailing EOC, this is well-formed BER.
+  SECItem goodEncoding = {siBuffer, ber, sizeof(ber)};
+  rv = SEC_ASN1DecodeItem(arena, &outer, OuterSequenceTemplate, &goodEncoding);
+  EXPECT_EQ(SECSuccess, rv);
+
+  // |outer| should now be a null terminated array of InnerSequenceItems
+
+  // The first item is PrintableString { \"Test for Bug 1387919\" }
+  EXPECT_EQ(outer[0].item->value.len, 22U);
+  EXPECT_EQ(0, memcmp(outer[0].item->value.data, ber + 4, 22));
+
+  // The second item is the null terminator
+  EXPECT_EQ(outer[1].item, nullptr);
+
+  PORT_FreeArena(arena, PR_FALSE);
+}
+
+}  // namespace nss_test

security/nss/lib/ckfw/wrap.c

@@ -2570,11 +2570,17 @@ NSSCKFWC_FindObjects(
         phObject[i] = nssCKFWInstance_FindObjectHandle(fwInstance, fwObject);
         if ((CK_OBJECT_HANDLE)0 == phObject[i]) {
             phObject[i] = nssCKFWInstance_CreateObjectHandle(fwInstance, fwObject, &error);
-        }
+            /* CreateObjectHandle returns CKR_GENERAL_ERROR if fwObject already
-        if ((CK_OBJECT_HANDLE)0 == phObject[i]) {
+             * has a handle. This happens when another thread creates a handle
-            /* This isn't right either, is it? */
+             * between our FindObjectHandle and CreateObjectHandle calls.
-            nssCKFWObject_Destroy(fwObject);
+             */
-            goto loser;
+            if (error == CKR_GENERAL_ERROR) {
+                error = CKR_OK;
+                phObject[i] = nssCKFWInstance_FindObjectHandle(fwInstance, fwObject);
+            }
+            if (error != CKR_OK || (CK_OBJECT_HANDLE)0 == phObject[i]) {
+                goto loser;
+            }
         }
     }
 

security/nss/lib/nss/nss.def

@@ -1247,3 +1247,9 @@ PK11_FindObjectForCert;
 ;+    local:
 ;+       *;
 ;+};
+;+NSS_3.79 { 	# NSS 3.79 release
+;+    global:
+SECMOD_LockedModuleHasRemovableSlots;
+;+    local:
+;+       *;
+;+};

security/nss/lib/pk11wrap/pk11list.c

@@ -32,8 +32,8 @@ SECMOD_DestroyListLock(SECMODListLock *lock)
 }
 
 /*
- * Lock the List for Read: NOTE: this assumes the reading isn't so common
+ * Lock the list for reading.
- * the writing will be starved.
+ * Note: this uses a non-reentrant lock. Writers are given preference.
  */
 void
 SECMOD_GetReadLock(SECMODListLock *modLock)

security/nss/lib/pk11wrap/pk11util.c

@@ -1002,6 +1002,8 @@ SECMOD_CanDeleteInternalModule(void)
  * C_GetSlotList(flag, &data, &count) so that the array doesn't accidently
  * grow on the caller. It is permissible for the slots to increase between
  * successive calls with NULL to get the size.
+ *
+ * Caller must not hold a module list read lock.
  */
 SECStatus
 SECMOD_UpdateSlotList(SECMODModule *mod)
@@ -1344,14 +1346,27 @@ loser:
 PRBool
 SECMOD_HasRemovableSlots(SECMODModule *mod)
 {
-    int i;
     PRBool ret = PR_FALSE;
-
     if (!moduleLock) {
         PORT_SetError(SEC_ERROR_NOT_INITIALIZED);
         return ret;
     }
     SECMOD_GetReadLock(moduleLock);
+    ret = SECMOD_LockedModuleHasRemovableSlots(mod);
+    SECMOD_ReleaseReadLock(moduleLock);
+    return ret;
+}
+
+PRBool
+SECMOD_LockedModuleHasRemovableSlots(SECMODModule *mod)
+{
+    int i;
+    PRBool ret;
+    if (mod->slotCount == 0) {
+        return PR_TRUE;
+    }
+
+    ret = PR_FALSE;
     for (i = 0; i < mod->slotCount; i++) {
         PK11SlotInfo *slot = mod->slots[i];
         /* perm modules are not inserted or removed */
@@ -1361,10 +1376,6 @@ SECMOD_HasRemovableSlots(SECMODModule *mod)
         ret = PR_TRUE;
         break;
     }
-    if (mod->slotCount == 0) {
-        ret = PR_TRUE;
-    }
-    SECMOD_ReleaseReadLock(moduleLock);
     return ret;
 }
 

security/nss/lib/pk11wrap/secmod.h

@@ -143,7 +143,31 @@ extern unsigned long SECMOD_PubMechFlagstoInternal(unsigned long publicFlags);
 extern unsigned long SECMOD_InternaltoPubMechFlags(unsigned long internalFlags);
 extern unsigned long SECMOD_PubCipherFlagstoInternal(unsigned long publicFlags);
 
+/*
+ * Check to see if the module has removable slots that we may need to
+ * watch for.
+ *
+ * NB: This function acquires the module list lock in order to access
+ * mod->slotCount and mod->slots. Deadlock can occur if the caller holds the
+ * module list lock. Callers that already hold the module list lock must use
+ * SECMOD_LockedModuleHasRemovableSlots instead.
+ */
 PRBool SECMOD_HasRemovableSlots(SECMODModule *mod);
+
+/*
+ * Like SECMOD_HasRemovableSlots but this function does not acquire the module
+ * list lock.
+ */
+PRBool SECMOD_LockedModuleHasRemovableSlots(SECMODModule *mod);
+
+/*
+ * this function waits for a token event on any slot of a given module
+ * This function should not be called from more than one thread of the
+ * same process (though other threads can make other library calls
+ * on this module while this call is blocked).
+ *
+ * Caller must not hold a module list read lock.
+ */
 PK11SlotInfo *SECMOD_WaitForAnyTokenEvent(SECMODModule *mod,
                                           unsigned long flags, PRIntervalTime latency);
 /*
@@ -153,6 +177,7 @@ PK11SlotInfo *SECMOD_WaitForAnyTokenEvent(SECMODModule *mod,
  * shutting down  the module.
  */
 SECStatus SECMOD_CancelWait(SECMODModule *mod);
+
 /*
  * check to see if the module has added new slots. PKCS 11 v2.20 allows for
  * modules to add new slots, but never remove them. Slots not be added between
@@ -160,6 +185,8 @@ SECStatus SECMOD_CancelWait(SECMODModule *mod);
  * C_GetSlotList(flag, &data, &count) so that the array doesn't accidently
  * grow on the caller. It is permissible for the slots to increase between
  * corresponding calls with NULL to get the size.
+ *
+ * Caller must not hold a module list read lock.
  */
 SECStatus SECMOD_UpdateSlotList(SECMODModule *mod);
 SEC_END_PROTOS

security/nss/lib/pkcs12/p12local.c

@@ -908,8 +908,12 @@ sec_pkcs12_find_object(SEC_PKCS12SafeContents *safe,
     return NULL;
 }
 
-/* this function converts a password to unicode and encures that the
+/* this function converts a password to UCS2 and ensures that the
- * required double 0 byte be placed at the end of the string
+ * required double 0 byte be placed at the end of the string (if zeroTerm
+ * is set), or the 0 bytes at the end are dropped (if zeroTerm is not set).
+ * If toUnicode is false, we convert from UCS2 to UTF8/ASCII (latter is a
+ * proper subset of the former) depending on the state of the asciiCovert
+ * flag)
  */
 PRBool
 sec_pkcs12_convert_item_to_unicode(PLArenaPool *arena, SECItem *dest,
@@ -917,12 +921,15 @@ sec_pkcs12_convert_item_to_unicode(PLArenaPool *arena, SECItem *dest,
                                    PRBool asciiConvert, PRBool toUnicode)
 {
     PRBool success = PR_FALSE;
+    int bufferSize;
+
     if (!src || !dest) {
         PORT_SetError(SEC_ERROR_INVALID_ARGS);
         return PR_FALSE;
     }
 
-    dest->len = src->len * 3 + 2;
+    bufferSize = src->len * 3 + 2;
+    dest->len = bufferSize;
     if (arena) {
         dest->data = (unsigned char *)PORT_ArenaZAlloc(arena, dest->len);
     } else {
@@ -956,24 +963,36 @@ sec_pkcs12_convert_item_to_unicode(PLArenaPool *arena, SECItem *dest,
         return PR_FALSE;
     }
 
-    if ((dest->len >= 2) &&
+    /* in some cases we need to add NULL terminations and in others
-        (dest->data[dest->len - 1] || dest->data[dest->len - 2]) && zeroTerm) {
+     * we need to drop null terminations */
-        if (dest->len + 2 > 3 * src->len) {
+    if (zeroTerm) {
-            if (arena) {
+        /* unicode adds two nulls at the end */
-                dest->data = (unsigned char *)PORT_ArenaGrow(arena,
+        if (toUnicode) {
-                                                             dest->data, dest->len,
+            if ((dest->len >= 2) &&
-                                                             dest->len + 2);
+                (dest->data[dest->len - 1] || dest->data[dest->len - 2])) {
-            } else {
+                /* we've already allocated space for these new NULLs */
-                dest->data = (unsigned char *)PORT_Realloc(dest->data,
+                PORT_Assert(dest->len + 2 <= bufferSize);
-                                                           dest->len + 2);
+                dest->len += 2;
+                dest->data[dest->len - 1] = dest->data[dest->len - 2] = 0;
             }
-
+            /* ascii/utf-8 adds just 1 */
-            if (!dest->data) {
+        } else if ((dest->len >= 1) && dest->data[dest->len - 1]) {
-                return PR_FALSE;
+            PORT_Assert(dest->len + 1 <= bufferSize);
+            dest->len++;
+            dest->data[dest->len - 1] = 0;
+        }
+    } else {
+        /* handle the drop case, no need to do any allocations here. */
+        if (toUnicode) {
+            while ((dest->len >= 2) && !dest->data[dest->len - 1] &&
+                   !dest->data[dest->len - 2]) {
+                dest->len -= 2;
+            }
+        } else {
+            while (dest->len && !dest->data[dest->len - 1]) {
+                dest->len--;
             }
         }
-        dest->len += 2;
-        dest->data[dest->len - 1] = dest->data[dest->len - 2] = 0;
     }
 
     return PR_TRUE;
@@ -1011,7 +1030,8 @@ sec_pkcs12_is_pkcs12_pbe_algorithm(SECOidTag algorithm)
  *
  * we assume that the pwitem is already encoded in Unicode by the
  * caller.  if the encryption scheme is not the one defined in PKCS
- * #12, decode the pwitem back into UTF-8. */
+ * #12, decode the pwitem back into UTF-8. NOTE: UTF-8 strings are
+ * used in the PRF without the trailing NULL */
 PRBool
 sec_pkcs12_decode_password(PLArenaPool *arena,
                            SECItem *result,
@@ -1021,7 +1041,7 @@ sec_pkcs12_decode_password(PLArenaPool *arena,
     if (!sec_pkcs12_is_pkcs12_pbe_algorithm(algorithm))
         return sec_pkcs12_convert_item_to_unicode(arena, result,
                                                   (SECItem *)pwitem,
-                                                  PR_TRUE, PR_FALSE, PR_FALSE);
+                                                  PR_FALSE, PR_FALSE, PR_FALSE);
 
     return SECITEM_CopyItem(arena, result, pwitem) == SECSuccess;
 }

security/nss/lib/softoken/legacydb/lginit.c

@@ -515,6 +515,7 @@ lg_init(SDB **pSdb, int flags, NSSLOWCERTCertDBHandle *certdbPtr,
     lgdb_p->hashTable = PL_NewHashTable(64, lg_HashNumber, PL_CompareValues,
                                         SECITEM_HashCompare, NULL, 0);
     if (lgdb_p->hashTable == NULL) {
+        PR_DestroyLock(lgdb_p->dbLock);
         goto loser;
     }
 
@@ -548,12 +549,6 @@ loser:
         PORT_Free(sdb);
     }
     if (lgdb_p) {
-        if (lgdb_p->dbLock) {
-            PR_DestroyLock(lgdb_p->dbLock);
-        }
-        if (lgdb_p->hashTable) {
-            PL_HashTableDestroy(lgdb_p->hashTable);
-        }
         PORT_Free(lgdb_p);
     }
     return error;

security/nss/lib/ssl/ssl3con.c

@@ -8991,9 +8991,15 @@ ssl3_HandleClientHello(sslSocket *ss, PRUint8 *b, PRUint32 length)
                                    PR_MIN(ss->clientHelloVersion,
                                           SSL_LIBRARY_VERSION_TLS_1_2),
                                    PR_TRUE);
+        /* Send protocol version alert if the ClientHello.legacy_version is not
+         * supported by the server.
+         *
+         * If the "supported_versions" extension is absent and the server only
+         * supports versions greater than ClientHello.legacy_version, the
+         * server MUST abort the handshake with a "protocol_version" alert
+         * [RFC8446, Appendix D.2]. */
         if (rv != SECSuccess) {
-            desc = (ss->clientHelloVersion > SSL_LIBRARY_VERSION_3_0) ? protocol_version
+            desc = protocol_version;
-                                                                      : handshake_failure;
             errCode = SSL_ERROR_UNSUPPORTED_VERSION;
             goto alert_loser;
         }
@@ -13191,10 +13197,25 @@ ssl3_HandleNonApplicationData(sslSocket *ss, SSLContentType rType,
             }
         /* 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;
     }

security/nss/lib/ssl/ssl3exthandle.c

@@ -1813,8 +1813,16 @@ ssl3_HandleSupportedPointFormatsXtn(const sslSocket *ss,
         }
     }
 
-    /* Poor client doesn't support uncompressed points. */
+    /* Poor client doesn't support uncompressed points.
+     *
+     * If the client sends the extension and the extension does not contain the
+     * uncompressed point format, and the client has used the Supported Groups
+     * extension to indicate support for any of the curves defined in this
+     * specification, then the server MUST abort the handshake and return an
+     * illegal_parameter alert. [RFC8422, Section 5.1.2] */
+    ssl3_ExtSendAlert(ss, alert_fatal, illegal_parameter);
     PORT_SetError(SSL_ERROR_RX_MALFORMED_HANDSHAKE);
+
     return SECFailure;
 }
 

security/nss/lib/ssl/ssl3gthr.c

@@ -348,7 +348,15 @@ dtls_GatherData(sslSocket *ss, sslGather *gs, int flags)
     } 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);

security/nss/lib/ssl/tls13con.c

@@ -5817,7 +5817,14 @@ 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.
+    /* 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) &&

security/nss/lib/util/nssrwlk.h

@@ -5,12 +5,13 @@
 /*
 ** File:        nsrwlock.h
 ** Description: API to basic reader-writer lock functions of NSS.
-**  These are re-entrant reader writer locks; that is,
+**  These locks allow re-entry from writers but not readers. That is,
 **  If I hold the write lock, I can ask for it and get it again.
 **  If I hold the write lock, I can also ask for and get a read lock.
 **      I can then release the locks in any order (read or write).
+**  If I hold a read lock, I must not ask for another read lock or
+**      the write lock.
 **  I must release each lock type as many times as I acquired it.
-**  Otherwise, these are normal reader/writer locks.
 **
 ** For deadlock detection, locks should be ranked, and no lock may be aquired
 ** while I hold a lock of higher rank number.

security/nss/lib/util/secasn1d.c

@@ -248,7 +248,7 @@ typedef struct sec_asn1d_state_struct {
 
     PRPackedBool
         allocate,      /* when true, need to allocate the destination */
-        endofcontents, /* this state ended up parsing end-of-contents octets */
+        endofcontents, /* this state ended up parsing its parent's end-of-contents octets */
         explicit,      /* we are handling an explicit header */
         indefinite,    /* the current item has indefinite-length encoding */
         missing,       /* an optional field that was not present */
@@ -1114,7 +1114,7 @@ sec_asn1d_prepare_for_contents(sec_asn1d_state *state)
          * inspection, too) then move this code into the switch statement
          * below under cases SET_OF and SEQUENCE_OF; it will be cleaner.
          */
-        PORT_Assert(state->underlying_kind == SEC_ASN1_SET_OF || state->underlying_kind == SEC_ASN1_SEQUENCE_OF || state->underlying_kind == (SEC_ASN1_SEQUENCE_OF | SEC_ASN1_DYNAMIC) || state->underlying_kind == (SEC_ASN1_SEQUENCE_OF | SEC_ASN1_DYNAMIC));
+        PORT_Assert(state->underlying_kind == SEC_ASN1_SET_OF || state->underlying_kind == SEC_ASN1_SEQUENCE_OF || state->underlying_kind == (SEC_ASN1_SET_OF | SEC_ASN1_DYNAMIC) || state->underlying_kind == (SEC_ASN1_SEQUENCE_OF | SEC_ASN1_DYNAMIC));
         if (state->contents_length != 0 || state->indefinite) {
             const SEC_ASN1Template *subt;
 
@@ -2470,7 +2470,18 @@ sec_asn1d_parse_end_of_contents(sec_asn1d_state *state,
 
     if (state->pending == 0) {
         state->place = afterEndOfContents;
-        state->endofcontents = PR_TRUE;
+        /* These end-of-contents octets either terminate a SEQUENCE, a GROUP,
+         * or a constructed string. The SEQUENCE case is unique in that the
+         * state parses its own end-of-contents octets and therefore should not
+         * have its `endofcontents` flag set. We identify the SEQUENCE case by
+         * checking whether the child state's template is pointing at a
+         * template terminator (see `sec_asn1d_next_in_sequence`).
+         */
+        if (state->child && state->child->theTemplate->kind == 0) {
+            state->endofcontents = PR_FALSE;
+        } else {
+            state->endofcontents = PR_TRUE;
+        }
     }
 
     return len;

security/nss/tests/common/init.sh

@@ -83,6 +83,7 @@ if [ -z "${INIT_SOURCED}" -o "${INIT_SOURCED}" != "TRUE" ]; then
         GTESTDIR=${HOSTDIR}/gtests
 
         PWFILE=${HOSTDIR}/tests.pw
+        LONGPWFILE=${HOSTDIR}/tests.longpw
         EMPTY_FILE=${HOSTDIR}/tests_empty
         NOISE_FILE=${HOSTDIR}/tests_noise
         CORELIST_FILE=${HOSTDIR}/clist
@@ -92,6 +93,7 @@ if [ -z "${INIT_SOURCED}" -o "${INIT_SOURCED}" != "TRUE" ]; then
         FIPSP12PWFILE=${HOSTDIR}/tests.fipsp12pw
 
         echo nss > ${PWFILE}
+        echo "nss123456789012345678901234567890123456789012345678901234567890_" > ${LONGPWFILE}
         echo > ${EMPTY_FILE}
         echo "fIps140" > ${FIPSPWFILE}
         echo "fips104" > ${FIPSBADPWFILE}
@@ -661,6 +663,7 @@ if [ -z "${INIT_SOURCED}" -o "${INIT_SOURCED}" != "TRUE" ]; then
     fi
 
     R_PWFILE=../tests.pw
+    R_LONGPWFILE=../tests.longpw
     R_EMPTY_FILE=../tests_empty
     R_NOISE_FILE=../tests_noise
 

security/nss/tests/tools/tools.sh

@@ -387,6 +387,30 @@ tools_p12_export_list_import_with_default_ciphers()
   ret=$?
   html_msg $ret 0 "Listing Alice's pk12 EC file (pk12util -l)"
   check_tmpfile
+
+  echo "$SCRIPTNAME: Exporting Alice's email EC cert & key with long pw------"
+  echo "pk12util -o Alice-ec-long.p12 -n \"Alice-ec\" -d ${P_R_ALICEDIR} -k ${R_PWFILE} \\"
+  echo "         -w ${R_LONGPWFILE}"
+  ${BINDIR}/pk12util -o Alice-ec-long.p12 -n "Alice-ec" -d ${P_R_ALICEDIR} -k ${R_PWFILE} \
+       -w ${R_LONGPWFILE} 2>&1
+  ret=$?
+  html_msg $ret 0 "Exporting Alice's email EC cert & key with long pw (pk12util -o)"
+  check_tmpfile
+  verify_p12 Alice-ec-long.p12 "default" "default" "default"
+
+  echo "$SCRIPTNAME: Importing Alice's email EC cert & key with long pw-----"
+  echo "pk12util -i Alice-ec-long.p12 -d ${P_R_COPYDIR} -k ${R_PWFILE} -w ${R_LONGPWFILE}"
+  ${BINDIR}/pk12util -i Alice-ec-long.p12 -d ${P_R_COPYDIR} -k ${R_PWFILE} -w ${R_LONGPWFILE} 2>&1
+  ret=$?
+  html_msg $ret 0 "Importing Alice's email EC cert & key with long pw (pk12util -i)"
+  check_tmpfile
+
+  echo "$SCRIPTNAME: Listing Alice's pk12 EC file with long pw ------------"
+  echo "pk12util -l Alice-ec-long.p12 -w ${R_LONGPWFILE}"
+  ${BINDIR}/pk12util -l Alice-ec-long.p12 -w ${R_LONGPWFILE} 2>&1
+  ret=$?
+  html_msg $ret 0 "Listing Alice's pk12 EC file with long pw (pk12util -l)"
+  check_tmpfile
 }
 
 tools_p12_import_old_files()