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.

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

4 files changed, 85 insertions, 3 deletions

diff --git a/gtests/util_gtest/manifest.mn b/gtests/util_gtest/manifest.mn
index c09146c83..87ebdfb33 100644
--- a/gtests/util_gtest/manifest.mn
+++ b/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)
 
diff --git a/gtests/util_gtest/util_gtest.gyp b/gtests/util_gtest/util_gtest.gyp
index ab803b761..e924df04f 100644
--- a/gtests/util_gtest/util_gtest.gyp
+++ b/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': [
diff --git a/gtests/util_gtest/util_secasn1d_unittest.cc b/gtests/util_gtest/util_secasn1d_unittest.cc
new file mode 100644
index 000000000..c6cd0c044
--- /dev/null
+++ b/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
diff --git a/lib/util/secasn1d.c b/lib/util/secasn1d.c
index 9b5586228..d219ee0c2 100644
--- a/lib/util/secasn1d.c
+++ b/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;