yggdrasil/ring
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Restore some default cases in tasn_dec.c and tasn_enc.c

Browse Source 
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>
This commit is contained in:
David Benjamin 2023-04-12 16:30:01 -04:00 committed by Boringssl LUCI CQ
parent 8cacbd93b8
commit abfd5ebc87
4 changed files with 122 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
crypto/asn1/asn1_test.cc
View File

@ -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|.

12
crypto/asn1/tasn_dec.c
View File

@ -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)) {

11
crypto/asn1/tasn_enc.c
View File

@ -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);

100
crypto/x509/x509_test.cc
View File

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