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()))));
+ }
+}
