Test ASN1_TYPE parsing more extensively

We should be able to parse a variety of valid types, and also reject
syntax errors in any types that we recognize. Also double-check that
nothing went wrong in the translation from i2d_ASN1_TYPE to X509_ALGOR
(basically the only use of ASN1_TYPE in the library).

Change-Id: I825f44c35a7e8ea6374641cf5e0ea5daaf471ad7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81727
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index e2cf0d3..54d2cb4 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -71,55 +71,6 @@
   EXPECT_EQ(Bytes(expected), Bytes(buf));
 }
 
-// Historically, unknown universal tags were represented in |ASN1_TYPE| as
-// |ASN1_STRING|s with the type matching the tag number. This can collide with
-// |V_ASN_NEG|, which was one of the causes of CVE-2016-2108. We now represent
-// unsupported values with |V_ASN1_OTHER|, but retain the |V_ASN1_MAX_UNIVERSAL|
-// limit.
-TEST(ASN1Test, UnknownTags) {
-  // kTag258 is an ASN.1 structure with a universal tag with number 258.
-  static const uint8_t kTag258[] = {0x1f, 0x82, 0x02, 0x01, 0x00};
-  static_assert(
-      V_ASN1_NEG_INTEGER == 258,
-      "V_ASN1_NEG_INTEGER changed. Update kTag258 to collide with it.");
-  const uint8_t *p = kTag258;
-  bssl::UniquePtr<ASN1_TYPE> obj(d2i_ASN1_TYPE(NULL, &p, sizeof(kTag258)));
-  EXPECT_FALSE(obj) << "Parsed value with illegal tag" << obj->type;
-  ERR_clear_error();
-
-  // kTagOverflow is an ASN.1 structure with a universal tag with number 2^35-1,
-  // which will not fit in an int.
-  static const uint8_t kTagOverflow[] = {0x1f, 0xff, 0xff, 0xff,
-                                         0xff, 0x7f, 0x01, 0x00};
-  p = kTagOverflow;
-  obj.reset(d2i_ASN1_TYPE(NULL, &p, sizeof(kTagOverflow)));
-  EXPECT_FALSE(obj) << "Parsed value with tag overflow" << obj->type;
-  ERR_clear_error();
-
-  // kTag128 is an ASN.1 structure with a universal tag with number 128. It
-  // should be parsed as |V_ASN1_OTHER|.
-  static const uint8_t kTag128[] = {0x1f, 0x81, 0x00, 0x01, 0x00};
-  p = kTag128;
-  obj.reset(d2i_ASN1_TYPE(NULL, &p, sizeof(kTag128)));
-  ASSERT_TRUE(obj);
-  EXPECT_EQ(V_ASN1_OTHER, obj->type);
-  EXPECT_EQ(Bytes(kTag128), Bytes(obj->value.asn1_string->data,
-                                  obj->value.asn1_string->length));
-  TestSerialize(obj.get(), i2d_ASN1_TYPE, kTag128);
-
-  // If a tag is known, but has the wrong constructed bit, it should be
-  // rejected, not placed in |V_ASN1_OTHER|.
-  static const uint8_t kConstructedOctetString[] = {0x24, 0x00};
-  p = kConstructedOctetString;
-  obj.reset(d2i_ASN1_TYPE(nullptr, &p, sizeof(kConstructedOctetString)));
-  EXPECT_FALSE(obj);
-
-  static const uint8_t kPrimitiveSequence[] = {0x10, 0x00};
-  p = kPrimitiveSequence;
-  obj.reset(d2i_ASN1_TYPE(nullptr, &p, sizeof(kPrimitiveSequence)));
-  EXPECT_FALSE(obj);
-}
-
 static bssl::UniquePtr<BIGNUM> BIGNUMPow2(unsigned bit) {
   bssl::UniquePtr<BIGNUM> bn(BN_new());
   if (!bn ||  //
@@ -599,6 +550,22 @@
   TestSerialize(val.get(), i2d_BASIC_CONSTRAINTS, kCA);
 }
 
+static std::vector<uint8_t> EmbedParamInAlgorithmIdentifier(
+    bssl::Span<const uint8_t> param) {
+  bssl::ScopedCBB cbb;
+  CBB seq;
+  BSSL_CHECK(CBB_init(cbb.get(), 64));
+  BSSL_CHECK(CBB_add_asn1(cbb.get(), &seq, CBS_ASN1_SEQUENCE));
+  static const uint8_t kTestOID[] = {0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12,
+                                     0x04, 0x01, 0x84, 0xb7, 0x09, 0x02};
+  BSSL_CHECK(
+      CBB_add_asn1_element(&seq, CBS_ASN1_OBJECT, kTestOID, sizeof(kTestOID)));
+  BSSL_CHECK(CBB_add_bytes(&seq, param.data(), param.size()));
+  BSSL_CHECK(CBB_flush(cbb.get()));
+  return std::vector(CBB_data(cbb.get()),
+                     CBB_data(cbb.get()) + CBB_len(cbb.get()));
+}
+
 TEST(ASN1Test, ASN1Type) {
   const struct {
     int type;
@@ -616,6 +583,12 @@
       {V_ASN1_BIT_STRING, {0x03, 0x02, 0x01, 0x00}},
       // INTEGER { -1 }
       {V_ASN1_INTEGER, {0x02, 0x01, 0xff}},
+      // INTEGER { 1 }
+      {V_ASN1_INTEGER, {0x02, 0x01, 0x01}},
+      // ENUMERATED { -1 }
+      {V_ASN1_ENUMERATED, {0x0a, 0x01, 0xff}},
+      // ENUMERATED { 1 }
+      {V_ASN1_ENUMERATED, {0x0a, 0x01, 0x01}},
       // OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2 }
       {V_ASN1_OBJECT,
        {0x06, 0x0c, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04, 0x01, 0x84, 0xb7,
@@ -626,20 +599,114 @@
       {V_ASN1_SEQUENCE, {0x30, 0x00}},
       // SET {}
       {V_ASN1_SET, {0x31, 0x00}},
+      // IA5String { "a" }
+      {V_ASN1_IA5STRING, {0x16, 0x01, 0x61}},
+      // UTF8String { "a" }
+      {V_ASN1_UTF8STRING, {0x0c, 0x01, 0x61}},
+      // BMPString { u"a" }
+      {V_ASN1_BMPSTRING, {0x1e, 0x02, 0x00, 0x61}},
+      // UniversalString { U"a" }
+      {V_ASN1_UNIVERSALSTRING, {0x1c, 0x04, 0x00, 0x00, 0x00, 0x61}},
       // [0] { UTF8String { "a" } }
       {V_ASN1_OTHER, {0xa0, 0x03, 0x0c, 0x01, 0x61}},
+      // [UNIVERSAL 128] { `00` }
+      // Unknown universal tags are parsed as |V_ASN1_OTHER|.
+      {V_ASN1_OTHER, {0x1f, 0x81, 0x00, 0x01, 0x00}},
   };
   for (const auto &t : kTests) {
     SCOPED_TRACE(Bytes(t.der));
 
+    auto check_asn1_type = [&](const ASN1_TYPE *typ) {
+      EXPECT_EQ(ASN1_TYPE_get(typ), t.type);
+      EXPECT_EQ(typ->type, t.type);
+      // If we parsed a string type, check that is consistent.
+      if (t.type != V_ASN1_NULL && t.type != V_ASN1_OBJECT &&
+          t.type != V_ASN1_BOOLEAN) {
+        int str_type = ASN1_STRING_type(typ->value.asn1_string);
+        if (str_type == V_ASN1_NEG_INTEGER ||
+            str_type == V_ASN1_NEG_ENUMERATED) {
+          str_type &= ~V_ASN1_NEG;
+        }
+        EXPECT_EQ(str_type, t.type);
+      }
+    };
+
     // The input should successfully parse.
     const uint8_t *ptr = t.der.data();
     bssl::UniquePtr<ASN1_TYPE> val(d2i_ASN1_TYPE(nullptr, &ptr, t.der.size()));
     ASSERT_TRUE(val);
 
-    EXPECT_EQ(ASN1_TYPE_get(val.get()), t.type);
-    EXPECT_EQ(val->type, t.type);
+    check_asn1_type(val.get());
     TestSerialize(val.get(), i2d_ASN1_TYPE, t.der);
+
+    // Test the same thing wrapped in an AlgorithmIdentifier.
+    std::vector<uint8_t> alg_der = EmbedParamInAlgorithmIdentifier(t.der);
+    ptr = alg_der.data();
+    bssl::UniquePtr<X509_ALGOR> alg(
+        d2i_X509_ALGOR(nullptr, &ptr, alg_der.size()));
+    ASSERT_TRUE(alg);
+
+    check_asn1_type(alg->parameter);
+    TestSerialize(alg.get(), i2d_X509_ALGOR, alg_der);
+  }
+
+  static_assert(V_ASN1_NEG_INTEGER == 258,
+                "V_ASN1_NEG_INTEGER changed. Update first test below to "
+                "collide with it.");
+  const std::vector<uint8_t> kInvalidTests[] = {
+      // Tag [UNIVERSAL 258] should be rejected.
+      //
+      // Historically, unknown universal tags were represented in |ASN1_TYPE| as
+      // |ASN1_STRING|s with the type matching the tag number. This can collide
+      // with |V_ASN_NEG|, which was one of the causes of CVE-2016-2108. (258
+      // matches |V_ASN1_NEG_INTEGER|.) We now represent unsupported values with
+      // |V_ASN1_OTHER|, but retain the |V_ASN1_MAX_UNIVERSAL| limit.
+      {0x1f, 0x82, 0x02, 0x01, 0x00},
+      // Tag [UNIVERSAL 2^35-1] will not fit in an int and should not
+      // overflow.
+      {0x1f, 0xff, 0xff, 0xff, 0xff, 0x7f, 0x01, 0x00},
+      // EOC is not a value.
+      {0x00, 0x00},
+      // Supported primitive types should be rejected if constructed.
+      {0x21, 0x00},  // [BOOLEAN CONSTRUCTED] {}
+      {0x22, 0x00},  // [INTEGER CONSTRUCTED] {}
+      {0x23, 0x00},  // [BIT STRING CONSTRUCTED] {}
+      {0x24, 0x00},  // [OCTET STRING CONSTRUCTED] {}
+      {0x25, 0x00},  // [NULL CONSTRUCTED] {}
+      {0x26, 0x00},  // [OBJECT IDENTIFIER CONSTRUCTED] {}
+      {0x2a, 0x00},  // [ENUMERATED CONSTRUCTED] {}
+      {0x2c, 0x00},  // [UTF8String CONSTRUCTED] {}
+      {0x3c, 0x00},  // [UniversalString CONSTRUCTED] {}
+      {0x3e, 0x00},  // [BMPString CONSTRUCTED] {}
+      // Supported constructed types should be rejected if primitive.
+      {0x10, 0x00},  // [SEQUENCE PRIMITIVE] {}
+      {0x11, 0x00},  // [SET PRIMITIVE] {}
+      // Invalid recognized types should be rejected.
+      {0x0c, 0x01, 0xff},        // UTF8String { `ff` }
+      {0x1c, 0x01, 0xff},        // UniversalString { `ff` }
+      {0x1e, 0x01, 0xff},        // BMPString { `ff` }
+      {0x02, 0x02, 0x00, 0x00},  // INTEGER { `0000` }
+      {0x0a, 0x02, 0x00, 0x00},  // ENUMERATED { `0000` }
+      {0x06, 0x00},              // OBJECT_IDENTIFIER { }
+      {0x06, 0x01, 0xff},        // OBJECT_IDENTIFIER { `ff` }
+      {0x03, 0x01, 0xff},        // BIT_STRING { `ff` }
+      {0x05, 0x01, 0xff},        // NULL { `ff` }
+      {0x01, 0x00},              // BOOLEAN {}
+      {0x01, 0x02, 0x00, 0x00},  // BOOLEAN { `0000` }
+  };
+  for (const auto &t : kInvalidTests) {
+    SCOPED_TRACE(Bytes(t));
+    const uint8_t *ptr = t.data();
+    bssl::UniquePtr<ASN1_TYPE> val(d2i_ASN1_TYPE(nullptr, &ptr, t.size()));
+    EXPECT_FALSE(val);
+    ERR_clear_error();
+
+    std::vector<uint8_t> alg_der = EmbedParamInAlgorithmIdentifier(t);
+    ptr = alg_der.data();
+    bssl::UniquePtr<X509_ALGOR> alg(
+        d2i_X509_ALGOR(nullptr, &ptr, alg_der.size()));
+    EXPECT_FALSE(alg);
+    ERR_clear_error();
   }
 }
 
@@ -1977,7 +2044,7 @@
 
 // Encoding invalid |ASN1_TYPE|s should fail. |ASN1_TYPE|s are
 // default-initialized to an invalid type.
-TEST(ASN1Test, InvalidASN1Type) {
+TEST(ASN1Test, EncodeInvalidASN1Type) {
   bssl::UniquePtr<ASN1_TYPE> obj(ASN1_TYPE_new());
   ASSERT_TRUE(obj);
   EXPECT_EQ(-1, obj->type);