Support OCTET STRING attribute values in X509_NAME
This is part of a broader issue with OpenSSL's goofy ASN1_PRINTABLE type
(which seems to have no connection to any standard), but this is a
minimal fix intended for easy cherry-picking.
This now leaves ASN1_PRINTABLE unused, but for now I've forked it into
an ASN1_ANY_AS_STRING type. It is still very far from an actual ANY, but
the intent is to capture AttributeValue's status as an ANY type, but
which OpenSSL's API forces us to represent as an ASN1_STRING. (Later
changes will make it actually match the name.)
Update-Note: X.509 name attributes now may be OCTET STRINGs.
Bug: 42290275
Change-Id: I1389533595a972bd0b4aa3c1840dc0f15c0fa645
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/78327
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 c4d6f0e..151874d 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -110,9 +110,9 @@
// |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.
+ // TODO(crbug.com/42290275): The encoder should reject it. However, it is
+ // still needed to support some edge cases in |ASN1_ANY_AS_STRING| and
+ // |ASN1_PRINTABLE|. When that is fixed, test that we reject it.
obj.reset(ASN1_TYPE_new());
ASSERT_TRUE(obj);
obj->type = 128;
diff --git a/crypto/asn1/internal.h b/crypto/asn1/internal.h
index 8c31f52..5732c02 100644
--- a/crypto/asn1/internal.h
+++ b/crypto/asn1/internal.h
@@ -214,6 +214,19 @@
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)
+
+DECLARE_ASN1_ITEM(ASN1_ANY_AS_STRING)
+
#if defined(__cplusplus)
} // extern C
diff --git a/crypto/asn1/tasn_dec.cc b/crypto/asn1/tasn_dec.cc
index bf5e030..5138786 100644
--- a/crypto/asn1/tasn_dec.cc
+++ b/crypto/asn1/tasn_dec.cc
@@ -797,12 +797,12 @@
case V_ASN1_OTHER:
case V_ASN1_SET:
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.
+ // 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_PRINTABLE| and
+ // |ASN1_ANY_AS_STRING|. They broadly don't tolerate unrecognized universal
+ // tags, 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);
diff --git a/crypto/asn1/tasn_enc.cc b/crypto/asn1/tasn_enc.cc
index 9156e16..f184a22 100644
--- a/crypto/asn1/tasn_enc.cc
+++ b/crypto/asn1/tasn_enc.cc
@@ -556,9 +556,9 @@
//
// TODO(davidben): Is this a bug? Although arguably one of the MSTRING
// types should contain more values, rather than less. See
- // https://crbug.com/boringssl/412. But it is not possible to fit all
- // possible ANY values into an |ASN1_STRING|, so matching the spec here
- // is somewhat hopeless.
+ // 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.
if (utype == V_ASN1_NEG_INTEGER) {
utype = V_ASN1_INTEGER;
} else if (utype == V_ASN1_NEG_ENUMERATED) {
@@ -649,11 +649,11 @@
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
+ // 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_PRINTABLE| and |ASN1_ANY_AS_STRING|.
+ // They broadly don'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
diff --git a/crypto/asn1/tasn_typ.cc b/crypto/asn1/tasn_typ.cc
index 97d0e44..5ccafbc 100644
--- a/crypto/asn1/tasn_typ.cc
+++ b/crypto/asn1/tasn_typ.cc
@@ -16,7 +16,8 @@
#include <openssl/asn1t.h>
-// Declarations for string types
+#include "internal.h"
+
#define IMPLEMENT_ASN1_STRING_FUNCTIONS(sname) \
IMPLEMENT_ASN1_TYPE(sname) \
@@ -64,6 +65,8 @@
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 2008957..3b7a57d 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -7165,6 +7165,7 @@
V_ASN1_UNIVERSALSTRING, std::string("\0\0\0a", 4)},
{CBS_ASN1_BMPSTRING, std::string("\0a", 2), V_ASN1_BMPSTRING,
std::string("\0a", 2)},
+ {CBS_ASN1_OCTETSTRING, "abc", V_ASN1_OCTET_STRING, "abc"},
// ENUMERATED is supported but, currently, INTEGER is not.
{CBS_ASN1_ENUMERATED, "\x01", V_ASN1_ENUMERATED, "\x01"},
@@ -7190,8 +7191,8 @@
{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
+ // 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.
};
@@ -7268,17 +7269,16 @@
{CBS_ASN1_UTF8STRING | CBS_ASN1_CONSTRUCTED, ""},
{CBS_ASN1_SEQUENCE & ~CBS_ASN1_CONSTRUCTED, ""},
- // TODO(crbug.com/boringssl/412): The following inputs should parse, but
- // are currently rejected because they cannot be represented in
- // |ASN1_PRINTABLE|, either because they don't fit in |ASN1_STRING| or
- // simply in the |B_ASN1_PRINTABLE| bitmask.
+ // 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_OCTETSTRING, ""},
{CBS_ASN1_UTCTIME, "700101000000Z"},
{CBS_ASN1_GENERALIZEDTIME, "19700101000000Z"},
{CBS_ASN1_SET, ""},
diff --git a/crypto/x509/x_name.cc b/crypto/x509/x_name.cc
index 767ebfd..16e60e4 100644
--- a/crypto/x509/x_name.cc
+++ b/crypto/x509/x_name.cc
@@ -55,7 +55,7 @@
ASN1_SEQUENCE(X509_NAME_ENTRY) = {
ASN1_SIMPLE(X509_NAME_ENTRY, object, ASN1_OBJECT),
- ASN1_SIMPLE(X509_NAME_ENTRY, value, ASN1_PRINTABLE),
+ ASN1_SIMPLE(X509_NAME_ENTRY, value, ASN1_ANY_AS_STRING),
} ASN1_SEQUENCE_END(X509_NAME_ENTRY)
IMPLEMENT_ASN1_ALLOC_FUNCTIONS(X509_NAME_ENTRY)
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h
index 1bb16ad..1288cb1 100644
--- a/include/openssl/asn1.h
+++ b/include/openssl/asn1.h
@@ -140,9 +140,9 @@
// |V_ASN1_*| constant to it. Otherwise, whether it returns |B_ASN1_UNKNOWN| or
// zero is ill-defined and callers should not rely on it.
//
-// TODO(https://crbug.com/boringssl/412): Figure out what |B_ASN1_UNNOWN| vs
-// zero is meant to be. The main impact is what values go in |B_ASN1_PRINTABLE|.
-// To that end, we must return zero on types that can't go in |ASN1_STRING|.
+// TODO(crbug.com/42290275): Figure out what |B_ASN1_UNKNOWN| vs zero is meant
+// to be. The main impact is what values go in |B_ASN1_PRINTABLE|. To that end,
+// we must return zero on types that can't go in |ASN1_STRING|.
OPENSSL_EXPORT unsigned long ASN1_tag2bit(int tag);
// ASN1_tag2str returns a string representation of |tag|, interpret as a tag
@@ -1860,7 +1860,7 @@
// maps to |B_ASN1_UNKNOWN|.
//
// Do not use this. Despite the name, it has no connection to PrintableString or
-// printable characters. See https://crbug.com/boringssl/412.
+// printable characters. See https://crbug.com/42290275.
#define B_ASN1_PRINTABLE \
(B_ASN1_NUMERICSTRING | B_ASN1_PRINTABLESTRING | B_ASN1_T61STRING | \
B_ASN1_IA5STRING | B_ASN1_BIT_STRING | B_ASN1_UNIVERSALSTRING | \
@@ -1879,7 +1879,7 @@
// |d2i_SAMPLE|.
//
// Do not use this. Despite, the name it has no connection to PrintableString or
-// printable characters. See https://crbug.com/boringssl/412.
+// printable characters. See https://crbug.com/42290275.
//
// TODO(https://crbug.com/boringssl/354): This function currently also accepts
// BER, but this will be removed in the future.
@@ -1889,14 +1889,14 @@
// i2d_ASN1_PRINTABLE marshals |in| as DER, as described in |i2d_SAMPLE|.
//
// Do not use this. Despite the name, it has no connection to PrintableString or
-// printable characters. See https://crbug.com/boringssl/412.
+// printable characters. See https://crbug.com/42290275.
OPENSSL_EXPORT int i2d_ASN1_PRINTABLE(const ASN1_STRING *in, uint8_t **outp);
// ASN1_PRINTABLE is an |ASN1_ITEM| whose ASN.1 type is a CHOICE of an ad-hoc
// subset of string-like types, and whose C type is |ASN1_STRING*|.
//
// Do not use this. Despite the name, it has no connection to PrintableString or
-// printable characters. See https://crbug.com/boringssl/412.
+// printable characters. See https://crbug.com/42290275.
DECLARE_ASN1_ITEM(ASN1_PRINTABLE)
// ASN1_INTEGER_set sets |a| to an INTEGER with value |v|. It returns one on
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index e2f899e..2802893 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -1504,9 +1504,9 @@
// pointer for OpenSSL compatibility, but callers should not mutate the result.
// Doing so will break internal invariants in the library.
//
-// TODO(https://crbug.com/boringssl/412): Although the spec says any ASN.1 type
-// is allowed, we currently only allow an ad-hoc set of types. Additionally, it
-// is unclear if some types can even be represented by this function.
+// TODO(crbug.com/42290275): Although the spec says any ASN.1 type is allowed,
+// we currently only allow an ad-hoc set of types. Additionally, it is unclear
+// if some types can even be represented by this function.
OPENSSL_EXPORT ASN1_STRING *X509_NAME_ENTRY_get_data(
const X509_NAME_ENTRY *entry);
