diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index 5c4379370..849cfe90c 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc @@ -107,8 +107,12 @@ TEST(ASN1Test, UnknownTags) { TestSerialize(obj.get(), i2d_ASN1_TYPE, kTag128); // The historical in-memory representation of |kTag128| was for both - // |obj->type| and |obj->value.asn1_string->type| to be NULL. This is no - // longer used and should be rejected by the encoder. + // |obj->type| and |obj->value.asn1_string->type| to be 128. This is no + // longer used but is still accepted by the encoder. + // + // TODO(crbug.com/boringssl/412): The encoder should reject it. However, it is + // still needed to support some edge cases in |ASN1_PRINTABLE|. When that is + // fixed, test that we reject it. obj.reset(ASN1_TYPE_new()); ASSERT_TRUE(obj); obj->type = 128; @@ -116,7 +120,7 @@ TEST(ASN1Test, UnknownTags) { ASSERT_TRUE(obj->value.asn1_string); const uint8_t zero = 0; ASSERT_TRUE(ASN1_STRING_set(obj->value.asn1_string, &zero, sizeof(zero))); - EXPECT_EQ(-1, i2d_ASN1_TYPE(obj.get(), nullptr)); + 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|. diff --git a/crypto/asn1/tasn_dec.c b/crypto/asn1/tasn_dec.c index 622ed458a..24ab04f27 100644 --- a/crypto/asn1/tasn_dec.c +++ b/crypto/asn1/tasn_dec.c @@ -837,7 +837,14 @@ static int asn1_ex_c2i(ASN1_VALUE **pval, const unsigned char *cont, long len, case V_ASN1_UTF8STRING: case V_ASN1_OTHER: case V_ASN1_SET: - case V_ASN1_SEQUENCE: { + case V_ASN1_SEQUENCE: + // TODO(crbug.com/boringssl/412): This default case should be removed, now + // that we've resolved https://crbug.com/boringssl/561. However, it is still + // needed to support some edge cases in |ASN1_PRINTABLE|. |ASN1_PRINTABLE| + // broadly doesn't tolerate unrecognized universal tags, but except for + // eight values that map to |B_ASN1_UNKNOWN| instead of zero. See the + // X509Test.NameAttributeValues test. + default: { CBS cbs; CBS_init(&cbs, cont, (size_t)len); if (utype == V_ASN1_BMPSTRING) { @@ -900,9 +907,6 @@ static int asn1_ex_c2i(ASN1_VALUE **pval, const unsigned char *cont, long len, } break; } - default: - OPENSSL_PUT_ERROR(ASN1, ASN1_R_BAD_TEMPLATE); - goto err; } // If ASN1_ANY and NULL type fix up value if (typ && (utype == V_ASN1_NULL)) { diff --git a/crypto/asn1/tasn_enc.c b/crypto/asn1/tasn_enc.c index b0d72ce86..e85400b2c 100644 --- a/crypto/asn1/tasn_enc.c +++ b/crypto/asn1/tasn_enc.c @@ -693,15 +693,18 @@ static int asn1_ex_i2c(ASN1_VALUE **pval, unsigned char *cout, int *out_omit, case V_ASN1_SET: // This is not a valid |ASN1_ITEM| type, but it appears in |ASN1_TYPE|. case V_ASN1_OTHER: + // TODO(crbug.com/boringssl/412): This default case should be removed, now + // that we've resolved https://crbug.com/boringssl/561. However, it is still + // needed to support some edge cases in |ASN1_PRINTABLE|. |ASN1_PRINTABLE| + // broadly doesn't tolerate unrecognized universal tags, but except for + // eight values that map to |B_ASN1_UNKNOWN| instead of zero. See the + // X509Test.NameAttributeValues test. + default: // All based on ASN1_STRING and handled the same strtmp = (ASN1_STRING *)*pval; cont = strtmp->data; len = strtmp->length; break; - - default: - OPENSSL_PUT_ERROR(ASN1, ASN1_R_BAD_TEMPLATE); - return -1; } if (cout && len) { OPENSSL_memcpy(cout, cont, len); diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 056d56a4b..530c24d5c 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -6549,3 +6549,103 @@ TEST(X509Test, SortRDN) { 0x02, 0x41, 0x42}; EXPECT_EQ(Bytes(kExpected), Bytes(der, der_len)); } + +TEST(X509Test, NameAttributeValues) { + // 1.2.840.113554.4.1.72585.0. We use an unrecognized OID because using an + // arbitrary ASN.1 type as the value for commonName is invalid. Our parser + // does not check this, but best to avoid unrelated errors in tests, in case + // we decide to later. + static const uint8_t kOID[] = {0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, + 0x04, 0x01, 0x84, 0xb7, 0x09, 0x00}; + + const struct { + CBS_ASN1_TAG der_tag; + std::string der_contents; + int str_type; + std::string str_contents; + } kTests[] = { + // String types are parsed as string types. + {CBS_ASN1_BITSTRING, std::string("\0", 1), V_ASN1_BIT_STRING, ""}, + {CBS_ASN1_UTF8STRING, "abc", V_ASN1_UTF8STRING, "abc"}, + {CBS_ASN1_NUMERICSTRING, "123", V_ASN1_NUMERICSTRING, "123"}, + {CBS_ASN1_PRINTABLESTRING, "abc", V_ASN1_PRINTABLESTRING, "abc"}, + {CBS_ASN1_T61STRING, "abc", V_ASN1_T61STRING, "abc"}, + {CBS_ASN1_IA5STRING, "abc", V_ASN1_IA5STRING, "abc"}, + {CBS_ASN1_UNIVERSALSTRING, std::string("\0\0\0a", 4), + V_ASN1_UNIVERSALSTRING, std::string("\0\0\0a", 4)}, + {CBS_ASN1_BMPSTRING, std::string("\0a", 2), V_ASN1_BMPSTRING, + std::string("\0a", 2)}, + + // ENUMERATED is supported but, currently, INTEGER is not. + {CBS_ASN1_ENUMERATED, "\x01", V_ASN1_ENUMERATED, "\x01"}, + + // SEQUENCE is supported but, currently, SET is not. Note the + // |ASN1_STRING| representation will include the tag and length. + {CBS_ASN1_SEQUENCE, "", V_ASN1_SEQUENCE, std::string("\x30\x00", 2)}, + + // These types are not actually supported by the library but, + // historically, we would parse them, and not other unsupported types, due + // to quirks of |ASN1_tag2bit|. + {7, "", V_ASN1_OBJECT_DESCRIPTOR, ""}, + {8, "", V_ASN1_EXTERNAL, ""}, + {9, "", V_ASN1_REAL, ""}, + {11, "", 11 /* EMBEDDED PDV */, ""}, + {13, "", 13 /* RELATIVE-OID */, ""}, + {14, "", 14 /* TIME */, ""}, + {15, "", 15 /* not a type; reserved value */, ""}, + {29, "", 29 /* CHARACTER STRING */, ""}, + + // TODO(crbug.com/boringssl/412): Attribute values are an ANY DEFINED BY + // type, so we actually shoudl be accepting all ASN.1 types. We currently + // do not and only accept the above types. Extend this test when we fix + // this. + }; + for (const auto &t : kTests) { + SCOPED_TRACE(t.der_tag); + SCOPED_TRACE(Bytes(t.der_contents)); + + // Construct an X.509 name containing a single RDN with a single attribute: + // kOID with the specified value. + bssl::ScopedCBB cbb; + ASSERT_TRUE(CBB_init(cbb.get(), 128)); + CBB seq, rdn, attr, attr_type, attr_value; + ASSERT_TRUE(CBB_add_asn1(cbb.get(), &seq, CBS_ASN1_SEQUENCE)); + ASSERT_TRUE(CBB_add_asn1(&seq, &rdn, CBS_ASN1_SET)); + ASSERT_TRUE(CBB_add_asn1(&rdn, &attr, CBS_ASN1_SEQUENCE)); + ASSERT_TRUE(CBB_add_asn1(&attr, &attr_type, CBS_ASN1_OBJECT)); + ASSERT_TRUE(CBB_add_bytes(&attr_type, kOID, sizeof(kOID))); + ASSERT_TRUE(CBB_add_asn1(&attr, &attr_value, t.der_tag)); + ASSERT_TRUE(CBB_add_bytes( + &attr_value, reinterpret_cast(t.der_contents.data()), + t.der_contents.size())); + ASSERT_TRUE(CBB_flush(cbb.get())); + SCOPED_TRACE(Bytes(CBB_data(cbb.get()), CBB_len(cbb.get()))); + + // The input should parse. + const uint8_t *inp = CBB_data(cbb.get()); + bssl::UniquePtr name( + d2i_X509_NAME(nullptr, &inp, CBB_len(cbb.get()))); + ASSERT_TRUE(name); + EXPECT_EQ(inp, CBB_data(cbb.get()) + CBB_len(cbb.get())) + << "input was not fully consumed"; + + // Check there is a single attribute with the expected in-memory + // representation. + ASSERT_EQ(1, X509_NAME_entry_count(name.get())); + const X509_NAME_ENTRY *entry = X509_NAME_get_entry(name.get(), 0); + const ASN1_OBJECT *obj = X509_NAME_ENTRY_get_object(entry); + EXPECT_EQ(Bytes(OBJ_get0_data(obj), OBJ_length(obj)), Bytes(kOID)); + const ASN1_STRING *value = X509_NAME_ENTRY_get_data(entry); + EXPECT_EQ(ASN1_STRING_type(value), t.str_type); + EXPECT_EQ(Bytes(ASN1_STRING_get0_data(value), ASN1_STRING_length(value)), + Bytes(t.str_contents)); + + // The name should re-encode with the same input. + uint8_t *der = nullptr; + int der_len = i2d_X509_NAME(name.get(), &der); + ASSERT_GE(der_len, 0); + bssl::UniquePtr free_der(der); + EXPECT_EQ(Bytes(der, der_len), + (Bytes(CBB_data(cbb.get()), CBB_len(cbb.get())))); + } +}