diff options
author | John M. Schanck <jschanck@mozilla.com> | 2022-05-16 20:48:14 +0000 |
---|---|---|
committer | John M. Schanck <jschanck@mozilla.com> | 2022-05-16 20:48:14 +0000 |
commit | de1cd863c7a8393d91ca0317954c3e3af556c086 (patch) | |
tree | c6e4b7a0fcefd2bcb2261ecd43a35b8ab698bdfa | |
parent | 61eaca3238b91403831e796e19976e7acce53a6f (diff) | |
download | nss-hg-de1cd863c7a8393d91ca0317954c3e3af556c086.tar.gz |
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
-rw-r--r-- | gtests/util_gtest/manifest.mn | 1 | ||||
-rw-r--r-- | gtests/util_gtest/util_gtest.gyp | 1 | ||||
-rw-r--r-- | gtests/util_gtest/util_secasn1d_unittest.cc | 69 | ||||
-rw-r--r-- | lib/util/secasn1d.c | 17 |
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; |