bug 1579060 - fix handling of issuerUniqueID and subjectUniqueID in mozilla::pkix::BackCert r=jcj

According to RFC 5280, the definitions of issuerUniqueID and subjectUniqueID in
TBSCertificate are as follows:

  issuerUniqueID  [1]  IMPLICIT UniqueIdentifier OPTIONAL,
  subjectUniqueID [2]  IMPLICIT UniqueIdentifier OPTIONAL,

where UniqueIdentifier is a BIT STRING.

IMPLICIT tags replace the tag of the underlying type. For these fields, there is
no specified class (just a tag number within the class), and the underlying type
of BIT STRING is "primitive" (i.e. not constructed). Thus, the tags should be of
the form CONTEXT SPECIFIC | [number in class], which comes out to 0x81 and 0x82,
respectively.

When originally implemented, mozilla::pkix incorrectly required that the
CONSTRUCTED bit also be set for these fields. Consequently, the library would
reject any certificate that actually contained these fields. Evidently such
certificates are rare.

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

3 files changed, 68 insertions, 12 deletions

diff --git a/gtests/mozpkix_gtest/pkixder_universal_types_tests.cpp b/gtests/mozpkix_gtest/pkixder_universal_types_tests.cpp
index 260c735ec..0dc8555d9 100644
--- a/gtests/mozpkix_gtest/pkixder_universal_types_tests.cpp
+++ b/gtests/mozpkix_gtest/pkixder_universal_types_tests.cpp
@@ -1224,3 +1224,53 @@ TEST_F(pkixder_universal_types_tests, OID)
 
   ASSERT_EQ(Success, OID(reader, expectedOID));
 }
+
+TEST_F(pkixder_universal_types_tests, SkipOptionalImplicitPrimitiveTag)
+{
+  const uint8_t DER_IMPLICIT_BIT_STRING_WITH_CLASS_NUMBER_1[] = {
+    0x81,
+    0x04,
+      0x00,
+      0x0A,
+      0x0B,
+      0x0C,
+  };
+  Input input(DER_IMPLICIT_BIT_STRING_WITH_CLASS_NUMBER_1);
+  Reader reader(input);
+
+  ASSERT_EQ(Success, SkipOptionalImplicitPrimitiveTag(reader, 1));
+  ASSERT_TRUE(reader.AtEnd());
+}
+
+TEST_F(pkixder_universal_types_tests, SkipOptionalImplicitPrimitiveTagMismatch)
+{
+  const uint8_t DER_IMPLICIT_BIT_STRING_WITH_CLASS_NUMBER_1[] = {
+    0x81,
+    0x04,
+      0x00,
+      0x0A,
+      0x0B,
+      0x0C,
+  };
+  Input input(DER_IMPLICIT_BIT_STRING_WITH_CLASS_NUMBER_1);
+  Reader reader(input);
+
+  ASSERT_EQ(Success, SkipOptionalImplicitPrimitiveTag(reader, 2));
+  ASSERT_FALSE(reader.AtEnd());
+}
+
+TEST_F(pkixder_universal_types_tests, NoSkipOptionalImplicitConstructedTag)
+{
+  const uint8_t DER_IMPLICIT_SEQUENCE_WITH_CLASS_NUMBER_1[] = {
+    0xA1,
+    0x03,
+      0x05,
+      0x01,
+      0x00,
+  };
+  Input input(DER_IMPLICIT_SEQUENCE_WITH_CLASS_NUMBER_1);
+  Reader reader(input);
+
+  ASSERT_EQ(Success, SkipOptionalImplicitPrimitiveTag(reader, 1));
+  ASSERT_FALSE(reader.AtEnd());
+}
diff --git a/lib/mozpkix/include/pkix/pkixder.h b/lib/mozpkix/include/pkix/pkixder.h
index 3aae0ecf6..379106ef4 100644
--- a/lib/mozpkix/include/pkix/pkixder.h
+++ b/lib/mozpkix/include/pkix/pkixder.h
@@ -114,6 +114,17 @@ inline Result ExpectTagAndSkipValue(Reader& input, uint8_t tag) {
   return ExpectTagAndGetValue(input, tag, ignoredValue);
 }
 
+// This skips IMPLICIT OPTIONAL tags that are "primitive" (not constructed),
+// given the number in the class of the tag (i.e. the number in the brackets in
+// `issuerUniqueID  [1]  IMPLICIT UniqueIdentifier OPTIONAL`).
+inline Result SkipOptionalImplicitPrimitiveTag(Reader& input,
+                                               uint8_t numberInClass) {
+  if (input.Peek(CONTEXT_SPECIFIC | numberInClass)) {
+    return ExpectTagAndSkipValue(input, CONTEXT_SPECIFIC | numberInClass);
+  }
+  return Success;
+}
+
 // Like ExpectTagAndGetValue, except the output Input will contain the
 // encoded tag and length along with the value.
 inline Result ExpectTagAndGetTLV(Reader& input, uint8_t tag,
diff --git a/lib/mozpkix/lib/pkixcert.cpp b/lib/mozpkix/lib/pkixcert.cpp
index a30483738..7789bd57d 100644
--- a/lib/mozpkix/lib/pkixcert.cpp
+++ b/lib/mozpkix/lib/pkixcert.cpp
@@ -105,29 +105,24 @@ BackCert::Init()
     return rv;
   }
 
-  static const uint8_t CSC = der::CONTEXT_SPECIFIC | der::CONSTRUCTED;
-
   // According to RFC 5280, all fields below this line are forbidden for
   // certificate versions less than v3.  However, for compatibility reasons,
   // we parse v1/v2 certificates in the same way as v3 certificates.  So if
   // these fields appear in a v1 certificate, they will be used.
 
   // Ignore issuerUniqueID if present.
-  if (tbsCertificate.Peek(CSC | 1)) {
-    rv = der::ExpectTagAndSkipValue(tbsCertificate, CSC | 1);
-    if (rv != Success) {
-      return rv;
-    }
+  rv = der::SkipOptionalImplicitPrimitiveTag(tbsCertificate, 1);
+  if (rv != Success) {
+    return rv;
   }
 
   // Ignore subjectUniqueID if present.
-  if (tbsCertificate.Peek(CSC | 2)) {
-    rv = der::ExpectTagAndSkipValue(tbsCertificate, CSC | 2);
-    if (rv != Success) {
-      return rv;
-    }
+  rv = der::SkipOptionalImplicitPrimitiveTag(tbsCertificate, 2);
+  if (rv != Success) {
+    return rv;
   }
 
+  static const uint8_t CSC = der::CONTEXT_SPECIFIC | der::CONSTRUCTED;
   rv = der::OptionalExtensions(
          tbsCertificate, CSC | 3,
          [this](Reader& extnID, const Input& extnValue, bool critical,