Support ANY attribute values in X509_NAME
An attribute in an X.509 name is defined as
AttributeTypeAndValue ::= SEQUENCE {
type AttributeType,
value AttributeValue }
AttributeType ::= OBJECT IDENTIFIER
AttributeValue ::= ANY -- DEFINED BY AttributeType
However, OpenSSL only supported an ad-hoc subset of types. (See
crbug.com/42290275 for details). Fix this by doing two things:
1. Stop trying to use the MSTRING framework for AttributeValue as many
of these are decidedly not strings.
2. Reuse ASN1_TYPE's V_ASN1_OTHER representation for non-universal tags.
3. For NULL, OBJECT, and BOOLEAN, use V_ASN1_OTHER because the normal
representation is not an ASN1_STRING.
This is all wrapped up in a new, internal-only ASN1_ANY_AS_STRING type.
With this, MSTRING is no longer exposed to random unknown tags and we
can remove some default cases in our big switch/cases.
Update-Note: X.509 name attributes may now be any ASN.1 type, matching
the spec.
Additionally, a few obscure, unimplemented ASN.1 types changed
representation: ObjectDescriptor, EXTERNAL, REAL, EMBEDDED PDV,
RELATIVE-OID, and TIME (no connection to time types used in X.509) used
to be represented as ASN1_STRINGs with the type fields set to the
corresponding tag number, and their contents uninterpreted. Now they're
wrapped in V_ASN1_OTHER, consistent with ASN1_TYPE. This is not expected
to impact anyone.
Bug: 42290275
Change-Id: I54b0d72fcbfaeebae416aa96cdf74abdba8b4c88
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/78329
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 d2df1fb..f2604ac 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -106,22 +106,6 @@
obj->value.asn1_string->length));
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 128. This is no
- // longer used but is still accepted by the encoder.
- //
- // TODO(crbug.com/42290275): The encoder should reject it. However, it is
- // still needed to support some edge cases in |ASN1_ANY_AS_STRING|. When that
- // is fixed, test that we reject it.
- obj.reset(ASN1_TYPE_new());
- ASSERT_TRUE(obj);
- obj->type = 128;
- obj->value.asn1_string = ASN1_STRING_type_new(128);
- ASSERT_TRUE(obj->value.asn1_string);
- const uint8_t zero = 0;
- ASSERT_TRUE(ASN1_STRING_set(obj->value.asn1_string, &zero, sizeof(zero)));
- 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};
@@ -1997,6 +1981,16 @@
ASSERT_TRUE(obj);
EXPECT_EQ(-1, obj->type);
EXPECT_EQ(-1, i2d_ASN1_TYPE(obj.get(), nullptr));
+
+ // The historical in-memory representation of [UNIVERSAL 128] was for both
+ // |obj->type| and |obj->value.asn1_string->type| to be 128. This is no longer
+ // used and should be rejected by the encoder.
+ obj.reset(ASN1_TYPE_new());
+ ASSERT_TRUE(obj);
+ obj->type = 128;
+ obj->value.asn1_string = ASN1_STRING_type_new(128);
+ ASSERT_TRUE(obj->value.asn1_string);
+ EXPECT_EQ(-1, i2d_ASN1_TYPE(obj.get(), nullptr));
}
// Encoding invalid MSTRING types should fail. An MSTRING is a CHOICE of
diff --git a/crypto/asn1/internal.h b/crypto/asn1/internal.h
index 5732c02..8d863fc 100644
--- a/crypto/asn1/internal.h
+++ b/crypto/asn1/internal.h
@@ -214,17 +214,9 @@
ASN1_ex_i2d *asn1_ex_i2d;
} ASN1_EXTERN_FUNCS;
-// ASN1_ANY_AS_STRING is an MSTRING that supports an arbitrary subset of types
-// representable in ASN1_STRING, used by X.509 name attribute values.
-//
-// TODO(crbug.com/42290275): This should support all types and also encode
-// non-ASN1_STRING values in a V_ASN1_OTHER structure.
-#define B_ASN1_ANY_AS_STRING \
- (B_ASN1_NUMERICSTRING | B_ASN1_PRINTABLESTRING | B_ASN1_T61STRING | \
- B_ASN1_IA5STRING | B_ASN1_BIT_STRING | B_ASN1_UNIVERSALSTRING | \
- B_ASN1_BMPSTRING | B_ASN1_UTF8STRING | B_ASN1_SEQUENCE | \
- B_ASN1_OCTET_STRING | B_ASN1_UNKNOWN)
-
+// ASN1_ANY_AS_STRING is an |ASN1_ITEM| with ASN.1 type ANY and C type
+// |ASN1_STRING*|. Types which are not represented with |ASN1_STRING|, such as
+// |ASN1_OBJECT|, are represented with type |V_ASN1_OTHER|.
DECLARE_ASN1_ITEM(ASN1_ANY_AS_STRING)
diff --git a/crypto/asn1/tasn_dec.cc b/crypto/asn1/tasn_dec.cc
index 6267120..13dc114 100644
--- a/crypto/asn1/tasn_dec.cc
+++ b/crypto/asn1/tasn_dec.cc
@@ -629,6 +629,7 @@
return 0; // Should never happen
}
+ assert(it->itype == ASN1_ITYPE_PRIMITIVE || it->itype == ASN1_ITYPE_MSTRING);
if (it->itype == ASN1_ITYPE_MSTRING) {
utype = tag;
tag = -1;
@@ -636,7 +637,7 @@
utype = it->utype;
}
- if (utype == V_ASN1_ANY) {
+ if (utype == V_ASN1_ANY || utype == V_ASN1_ANY_AS_STRING) {
// If type is ANY need to figure out type from tag
unsigned char oclass;
if (tag >= 0) {
@@ -647,8 +648,9 @@
OPENSSL_PUT_ERROR(ASN1, ASN1_R_ILLEGAL_OPTIONAL_ANY);
return 0;
}
+ const int is_string = utype == V_ASN1_ANY_AS_STRING;
p = *in;
- ret = asn1_check_tlen(NULL, &utype, &oclass, NULL, &p, inlen, -1, 0, 0);
+ ret = asn1_check_tlen(&plen, &utype, &oclass, &cst, &p, inlen, -1, 0, 0);
if (!ret) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR);
return 0;
@@ -656,6 +658,37 @@
if (!is_supported_universal_type(utype, oclass)) {
utype = V_ASN1_OTHER;
}
+ // These three types are not represented as |ASN1_STRING|, so they must be
+ // parsed separately and then treated as an opaque |V_ASN1_OTHER|.
+ if (is_string && (utype == V_ASN1_OBJECT || utype == V_ASN1_NULL ||
+ utype == V_ASN1_BOOLEAN)) {
+ if (cst) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_TYPE_NOT_PRIMITIVE);
+ return 0;
+ }
+ CBS cbs;
+ CBS_init(&cbs, p, plen);
+ if (utype == V_ASN1_OBJECT && !CBS_is_valid_asn1_oid(&cbs)) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_INVALID_OBJECT_ENCODING);
+ return 0;
+ }
+ if (utype == V_ASN1_NULL && CBS_len(&cbs) != 0) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_NULL_IS_WRONG_LENGTH);
+ return 0;
+ }
+ if (utype == V_ASN1_BOOLEAN) {
+ if (CBS_len(&cbs) != 1) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_BOOLEAN_IS_WRONG_LENGTH);
+ return 0;
+ }
+ uint8_t v = CBS_data(&cbs)[0];
+ if (v != 0 && v != 0xff) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_DECODE_ERROR);
+ return 0;
+ }
+ }
+ utype = V_ASN1_OTHER;
+ }
}
if (tag == -1) {
tag = utype;
@@ -738,6 +771,10 @@
opval = pval;
pval = &typ->value.asn1_value;
}
+
+ // If implementing a type that is not represented in |ASN1_STRING|, the
+ // |V_ASN1_ANY_AS_STRING| logic must be modified to redirect it to
+ // |V_ASN1_OTHER|.
switch (utype) {
case V_ASN1_OBJECT:
if (!c2i_ASN1_OBJECT((ASN1_OBJECT **)pval, &cont, len)) {
@@ -796,14 +833,7 @@
case V_ASN1_UTF8STRING:
case V_ASN1_OTHER:
case V_ASN1_SET:
- case V_ASN1_SEQUENCE:
- // TODO(crbug.com/42290275): This default case should be removed, now
- // that we've resolved https://crbug.com/42290430. However, it is still
- // needed to support some edge cases in |ASN1_ANY_AS_STRING|. It broadly
- // doesn't tolerate unrecognized universal tags, except for eight values
- // that map to |B_ASN1_UNKNOWN| instead of zero. See the
- // X509Test.NameAttributeValues test.
- default: {
+ case V_ASN1_SEQUENCE: {
CBS cbs;
CBS_init(&cbs, cont, (size_t)len);
if (utype == V_ASN1_BMPSTRING) {
@@ -866,6 +896,10 @@
}
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.cc b/crypto/asn1/tasn_enc.cc
index 8a5bae6..9aa2039 100644
--- a/crypto/asn1/tasn_enc.cc
+++ b/crypto/asn1/tasn_enc.cc
@@ -527,21 +527,23 @@
unsigned char c;
int len;
+ assert(it->itype == ASN1_ITYPE_PRIMITIVE || it->itype == ASN1_ITYPE_MSTRING);
// Historically, |it->funcs| for primitive types contained an
// |ASN1_PRIMITIVE_FUNCS| table of callbacks.
assert(it->funcs == NULL);
*out_omit = 0;
- // Should type be omitted?
- if ((it->itype != ASN1_ITYPE_PRIMITIVE) || (it->utype != V_ASN1_BOOLEAN)) {
+ // Handle omitted optional values for all but BOOLEAN, which uses a
+ // non-pointer representation.
+ if (it->itype != ASN1_ITYPE_PRIMITIVE || it->utype != V_ASN1_BOOLEAN) {
if (!*pval) {
*out_omit = 1;
return 0;
}
}
- if (it->itype == ASN1_ITYPE_MSTRING) {
+ if (it->itype == ASN1_ITYPE_MSTRING || it->utype == V_ASN1_ANY_AS_STRING) {
// If MSTRING type set the underlying type
strtmp = (ASN1_STRING *)*pval;
utype = strtmp->type;
@@ -550,15 +552,8 @@
OPENSSL_PUT_ERROR(ASN1, ASN1_R_WRONG_TYPE);
return -1;
}
- // Negative INTEGER and ENUMERATED values use |ASN1_STRING| type values
- // that do not match their corresponding utype values. INTEGERs cannot
- // participate in MSTRING types, but ENUMERATEDs can.
- //
- // TODO(davidben): Is this a bug? Although arguably one of the MSTRING
- // types should contain more values, rather than less. See
- // https://crbug.com/42290275. But it is not possible to fit all possible
- // ANY values into an |ASN1_STRING|, so matching the spec here is somewhat
- // hopeless.
+ // Negative INTEGER and ENUMERATED values use |ASN1_STRING| type values that
+ // do not match their corresponding utype values.
if (utype == V_ASN1_NEG_INTEGER) {
utype = V_ASN1_INTEGER;
} else if (utype == V_ASN1_NEG_ENUMERATED) {
@@ -649,18 +644,15 @@
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/42290275): This default case should be removed, now that
- // we've resolved https://crbug.com/42290430. However, it is still needed to
- // support some edge cases in |ASN1_ANY_AS_STRING|. It broadly doesn't
- // tolerate unrecognized universal tags, 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/asn1/tasn_typ.cc b/crypto/asn1/tasn_typ.cc
index 48b0f01..1d11ae8 100644
--- a/crypto/asn1/tasn_typ.cc
+++ b/crypto/asn1/tasn_typ.cc
@@ -46,6 +46,7 @@
IMPLEMENT_ASN1_TYPE(ASN1_OBJECT)
IMPLEMENT_ASN1_TYPE(ASN1_ANY)
+IMPLEMENT_ASN1_TYPE(ASN1_ANY_AS_STRING)
// Just swallow an ASN1_SEQUENCE in an ASN1_STRING
IMPLEMENT_ASN1_TYPE(ASN1_SEQUENCE)
@@ -59,8 +60,6 @@
IMPLEMENT_ASN1_FUNCTIONS_const_fname(ASN1_STRING, DIRECTORYSTRING,
DIRECTORYSTRING)
-IMPLEMENT_ASN1_MSTRING(ASN1_ANY_AS_STRING, B_ASN1_ANY_AS_STRING)
-
// Three separate BOOLEAN type: normal, DEFAULT TRUE and DEFAULT FALSE
IMPLEMENT_ASN1_TYPE_ex(ASN1_BOOLEAN, ASN1_BOOLEAN, ASN1_BOOLEAN_NONE)
IMPLEMENT_ASN1_TYPE_ex(ASN1_TBOOLEAN, ASN1_BOOLEAN, ASN1_BOOLEAN_TRUE)
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 3b7a57d..4bed472 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -7166,35 +7166,51 @@
{CBS_ASN1_BMPSTRING, std::string("\0a", 2), V_ASN1_BMPSTRING,
std::string("\0a", 2)},
{CBS_ASN1_OCTETSTRING, "abc", V_ASN1_OCTET_STRING, "abc"},
+ {CBS_ASN1_UTCTIME, "700101000000Z", V_ASN1_UTCTIME, "700101000000Z"},
+ {CBS_ASN1_GENERALIZEDTIME, "19700101000000Z", V_ASN1_GENERALIZEDTIME,
+ "19700101000000Z"},
// ENUMERATED is supported but, currently, INTEGER is not.
+ {CBS_ASN1_INTEGER, "\x01", V_ASN1_INTEGER, "\x01"},
{CBS_ASN1_ENUMERATED, "\x01", V_ASN1_ENUMERATED, "\x01"},
// Test negative values. These are interesting because, when encoding, the
// ASN.1 type must be determined from the string type, but the string type
// has an extra |V_ASN1_NEG| bit.
+ {CBS_ASN1_INTEGER, "\xff", V_ASN1_NEG_INTEGER, "\x01"},
{CBS_ASN1_ENUMERATED, "\xff", V_ASN1_NEG_ENUMERATED, "\x01"},
- // SEQUENCE is supported but, currently, SET is not. Note the
- // |ASN1_STRING| representation will include the tag and length.
+ // SEQUENCE and SET use their |ASN1_STRING| representation, which includes
+ // the tag and length.
{CBS_ASN1_SEQUENCE, "", V_ASN1_SEQUENCE, std::string("\x30\x00", 2)},
+ {CBS_ASN1_SET, "", V_ASN1_SET, std::string("\x31\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 */, ""},
+ // NULL, BOOLEAN, and OBJECT IDENTIFIER use non-|ASN1_STRING|
+ // representations, so they are represented with |V_ASN1_OTHER|.
+ {CBS_ASN1_NULL, "", V_ASN1_OTHER, std::string("\x05\x00", 2)},
+ {CBS_ASN1_BOOLEAN, std::string("\x00", 1), V_ASN1_OTHER,
+ std::string("\x01\x01\x00", 3)},
+ {CBS_ASN1_BOOLEAN, "\xff", V_ASN1_OTHER, "\x01\x01\xff"},
+ {CBS_ASN1_OBJECT, "\x01\x02\x03\x04", V_ASN1_OTHER,
+ "\x06\x04\x01\x02\x03\x04"},
- // TODO(crbug.com/42290275): Attribute values are an ANY DEFINED BY type,
- // so we actually should be accepting all ASN.1 types. We currently
- // do not and only accept the above types. Extend this test when we fix
- // this.
+ // These types are not actually supported by the library, but we accept
+ // them as |V_ASN1_OTHER|.
+ {7 /* ObjectDescriptor */, "", V_ASN1_OTHER, std::string("\x07\x00", 2)},
+ {8 /* EXTERNAL */, "", V_ASN1_OTHER, std::string("\x08\x00", 2)},
+ {9 /* REAL */, "", V_ASN1_OTHER, std::string("\x09\x00", 2)},
+ {11 /* EMBEDDED PDV */, "", V_ASN1_OTHER, std::string("\x0b\x00", 2)},
+ {13 /* RELATIVE-OID */, "", V_ASN1_OTHER, std::string("\x0d\x00", 2)},
+ {14 /* TIME */, "", V_ASN1_OTHER, std::string("\x0e\x00", 2)},
+ {15 /* not a type; reserved value */, "", V_ASN1_OTHER,
+ std::string("\x0f\x00", 2)},
+ {29 /* CHARACTER STRING */, "", V_ASN1_OTHER, std::string("\x1d\x00", 2)},
+
+ // Non-universal tags are allowed as |V_ASN1_OTHER| too.
+ {CBS_ASN1_APPLICATION | CBS_ASN1_CONSTRUCTED | 42, "", V_ASN1_OTHER,
+ std::string("\x7f\x2a\x00", 3)},
+ {CBS_ASN1_APPLICATION | 42, "", V_ASN1_OTHER,
+ std::string("\x5f\x2a\x00", 3)},
};
for (const auto &t : kTests) {
SCOPED_TRACE(t.der_tag);
@@ -7257,7 +7273,9 @@
// Errors in supported universal types should be handled.
{CBS_ASN1_NULL, "not null"},
{CBS_ASN1_BOOLEAN, "not bool"},
+ {CBS_ASN1_BOOLEAN, "\1"}, // BOOL in DER must be 0x00 or 0xff.
{CBS_ASN1_OBJECT, ""},
+ {CBS_ASN1_OBJECT, "\x80\x01"}, // Non-minimal OID encoding
{CBS_ASN1_INTEGER, std::string("\0\0", 2)},
{CBS_ASN1_ENUMERATED, std::string("\0\0", 2)},
{CBS_ASN1_BITSTRING, ""},
@@ -7266,24 +7284,11 @@
{CBS_ASN1_UNIVERSALSTRING, "not utf-32"},
{CBS_ASN1_UTCTIME, "not utctime"},
{CBS_ASN1_GENERALIZEDTIME, "not generalizedtime"},
+ {CBS_ASN1_NULL | CBS_ASN1_CONSTRUCTED, ""},
+ {CBS_ASN1_OBJECT | CBS_ASN1_CONSTRUCTED, "\x01\x02\x03\x04"},
+ {CBS_ASN1_BOOLEAN | CBS_ASN1_CONSTRUCTED, "\xff"},
{CBS_ASN1_UTF8STRING | CBS_ASN1_CONSTRUCTED, ""},
{CBS_ASN1_SEQUENCE & ~CBS_ASN1_CONSTRUCTED, ""},
-
- // TODO(crbug.com/42290275): The following inputs should parse, but are
- // currently rejected because they cannot be represented in
- // |ASN1_ANY_AS_STRING|, either because they don't fit in |ASN1_STRING| or
- // simply in the |B_ASN1_ANY_AS_STRING| bitmask.
- {CBS_ASN1_NULL, ""},
- {CBS_ASN1_BOOLEAN, std::string("\x00", 1)},
- {CBS_ASN1_BOOLEAN, "\xff"},
- {CBS_ASN1_OBJECT, "\x01\x02\x03\x04"},
- {CBS_ASN1_INTEGER, "\x01"},
- {CBS_ASN1_INTEGER, "\xff"},
- {CBS_ASN1_UTCTIME, "700101000000Z"},
- {CBS_ASN1_GENERALIZEDTIME, "19700101000000Z"},
- {CBS_ASN1_SET, ""},
- {CBS_ASN1_APPLICATION | CBS_ASN1_CONSTRUCTED | 42, ""},
- {CBS_ASN1_APPLICATION | 42, ""},
};
for (const auto &t : kInvalidTests) {
SCOPED_TRACE(t.der_tag);
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h
index 6e8e4ae..ad4905c 100644
--- a/include/openssl/asn1.h
+++ b/include/openssl/asn1.h
@@ -77,6 +77,10 @@
// V_ASN1_ANY is used by the ASN.1 templates to indicate an ANY type.
#define V_ASN1_ANY (-4)
+// V_ASN1_ANY_AS_STRING is used by the ASN.1 templates to indicate an ANY type
+// represented with |ASN1_STRING| instead of |ASN1_TYPE|.
+#define V_ASN1_ANY_AS_STRING (-5)
+
// The following constants are tag numbers for universal types.
#define V_ASN1_EOC 0
#define V_ASN1_BOOLEAN 1
