Restore some default cases in tasn_dec.c and tasn_enc.c This reverts a small portion of 8c8629bfd89436e5019b6bd3c65cff4bf1a76b76. The parsers for ANY remain unchanged, but we inadvertently changed a corner case of ASN1_PRINTABLE MSTRINGs. This is a huge mess. utype in these switch cases is usually the type of the ASN1_ITEM, but, with ANY and MSTRING, it is the tag of the value we found. (An MSTRING or "multi-string" is a CHOICE of string-like types.) When parsing ANY, this is moot because the is_supported_universal_type logic ensures we'll never pass in an invalid type. When encoding ANY, this only happens if you manually construct such an ASN1_TYPE. MSTRINGs *should* be similar because of the bitmask they apply on tag types. However, there is one MSTRING type whose bitmask, B_ASN1_PRINTABLE, includes B_ASN1_UNKNOWN. ASN1_tag2bit, arbitrarily maps eight unsupported tags to B_ASN1_UNKNOWN and instead of zero. These are: - ObjectDescriptor - EXTERNAL - REAL - EMBEDDED PDV - RELATIVE-OID - TIME (note this is not the same as the X.509 Time CHOICE type) - [UNIVERSAL 15], which is not even a defined type! - CHARACTER STRING (ENUMERATED is also mapped to B_ASN1_UNKNOWN, but it's supported.) These eight tags were previously accepted in d2i_X509_NAME but 8c8629bfd89436e5019b6bd3c65cff4bf1a76b76 inadvertently started rejecting them. For now, restore the default in the switch/case so that we accept them again. Per https://crbug.com/boringssl/412, attribute values are ANY DEFINED BY types, so we actually should be accepting *all* types. We do not, because B_ASN1_PRINTABLE is completely incoherent. But because ANY is the correct type, going from the original incoherent set, to this new, smaller incoherent set is arguably a regression. This is a minimal fix. Long-term, we should handle that ANY correctly, and avoid unexpected ASN1_STRING type values, by mapping all unsupported types to V_ASN1_OTHER. This would allow us to support all types correctly. A follow-up change will do that. Update-Note: The X.509 name parser will go back to accepting a handful of universal tag types that were inadvertently rejected in 8c8629bfd89436e5019b6bd3c65cff4bf1a76b76. It is extremely unlikely that anyone uses these as they're unsupported, obscure types. This CL also makes our ASN1_TYPE encoder slightly more permissive again, if the caller manually constructs an legacy in-memory representation of an unsupported tag. But the follow-up change will restore the stricter behavior. Bug: 412, 561 Change-Id: Ia44a270f12f3021154761a1cd285707416d8787e Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58705 Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: Bob Beck <bbe@google.com> Reviewed-by: Bob Beck <bbe@google.com>
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index 5c43793..849cfe9 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc
@@ -107,8 +107,12 @@ 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 @@ 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 622ed45..24ab04f 100644 --- a/crypto/asn1/tasn_dec.c +++ b/crypto/asn1/tasn_dec.c
@@ -837,7 +837,14 @@ 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 @@ } 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 b0d72ce..e85400b 100644 --- a/crypto/asn1/tasn_enc.c +++ b/crypto/asn1/tasn_enc.c
@@ -693,15 +693,18 @@ 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 056d56a..530c24d 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc
@@ -6549,3 +6549,103 @@ 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<const uint8_t *>(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<X509_NAME> 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<uint8_t> free_der(der); + EXPECT_EQ(Bytes(der, der_len), + (Bytes(CBB_data(cbb.get()), CBB_len(cbb.get())))); + } +}