From f61997b4d70e3ef3836e9f16b30a4a6bfae5de56 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 2 Oct 2021 10:57:24 -0400 Subject: [PATCH] Make ASN1_STRING_TABLE_add thread-safe and document. This function is a little awkward. It mutates global data, so if two libraries in the address space both attempt to define a custom OID, they will conflict. But some existing code uses it so, as long as it does so, we should make it thread-safe. Along the way, I've switched it to a hash table and removed the ability to overwrite existing entries. Previously, overwriting a built-in table would crash (on platforms where const structures are write-protected). Overwriting a dynamic table implemented this weird merging algorithm. The one caller I've seen does not appear to need this feature. I've also switched ASN1_STRING_TABLE_cleanup to a no-op, matching our other global cleanup functions. This function is not safe to call without global knowledge of all other uses of the library. Update-Note: ASN1_STRING_TABLE_add no longer allows overwrite existing entries. In most cases, this would crash or trigger a race condition anyway. Bug: 426 Change-Id: Ie024cca87feaef3ff10064b452f3a860844544da Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49769 Reviewed-by: Adam Langley --- crypto/asn1/a_strnid.c | 187 +++++++++++++++++++++------------------ crypto/asn1/asn1_test.cc | 57 +++++++++++- crypto/asn1/internal.h | 8 ++ include/openssl/asn1.h | 103 +++++++++++++-------- 4 files changed, 229 insertions(+), 126 deletions(-) diff --git a/crypto/asn1/a_strnid.c b/crypto/asn1/a_strnid.c index 62c2f5d25..bc9226a3a 100644 --- a/crypto/asn1/a_strnid.c +++ b/crypto/asn1/a_strnid.c @@ -56,22 +56,23 @@ #include -#include /* For bsearch */ +#include +#include #include #include #include #include -#include #include "../internal.h" +#include "../lhash/internal.h" #include "internal.h" -DEFINE_STACK_OF(ASN1_STRING_TABLE) +DEFINE_LHASH_OF(ASN1_STRING_TABLE) -static STACK_OF(ASN1_STRING_TABLE) *stable = NULL; -static void st_free(ASN1_STRING_TABLE *tbl); +static LHASH_OF(ASN1_STRING_TABLE) *string_tables = NULL; +static struct CRYPTO_STATIC_MUTEX string_tables_lock = CRYPTO_STATIC_MUTEX_INIT; void ASN1_STRING_set_default_mask(unsigned long mask) { @@ -87,34 +88,36 @@ int ASN1_STRING_set_default_mask_asc(const char *p) return 1; } +static const ASN1_STRING_TABLE *asn1_string_table_get(int nid); + /* * The following function generates an ASN1_STRING based on limits in a * table. Frequently the types and length of an ASN1_STRING are restricted by * a corresponding OID. For example certificates and certificate requests. */ -ASN1_STRING *ASN1_STRING_set_by_NID(ASN1_STRING **out, - const unsigned char *in, int inlen, - int inform, int nid) +ASN1_STRING *ASN1_STRING_set_by_NID(ASN1_STRING **out, const unsigned char *in, + int len, int inform, int nid) { - ASN1_STRING_TABLE *tbl; ASN1_STRING *str = NULL; - unsigned long mask; int ret; - if (!out) + if (!out) { out = &str; - tbl = ASN1_STRING_TABLE_get(nid); - if (tbl) { - mask = tbl->mask; - if (!(tbl->flags & STABLE_NO_MASK)) + } + const ASN1_STRING_TABLE *tbl = asn1_string_table_get(nid); + if (tbl != NULL) { + unsigned long mask = tbl->mask; + if (!(tbl->flags & STABLE_NO_MASK)) { mask &= B_ASN1_UTF8STRING; - ret = ASN1_mbstring_ncopy(out, in, inlen, inform, mask, - tbl->minsize, tbl->maxsize); + } + ret = ASN1_mbstring_ncopy(out, in, len, inform, mask, tbl->minsize, + tbl->maxsize); } else { - ret = ASN1_mbstring_copy(out, in, inlen, inform, B_ASN1_UTF8STRING); + ret = ASN1_mbstring_copy(out, in, len, inform, B_ASN1_UTF8STRING); } - if (ret <= 0) + if (ret <= 0) { return NULL; + } return *out; } @@ -159,91 +162,101 @@ static const ASN1_STRING_TABLE tbl_standard[] = { {NID_ms_csp_name, -1, -1, B_ASN1_BMPSTRING, STABLE_NO_MASK} }; -static int sk_table_cmp(const ASN1_STRING_TABLE **a, - const ASN1_STRING_TABLE **b) +static int table_cmp(const ASN1_STRING_TABLE *a, const ASN1_STRING_TABLE *b) { - return (*a)->nid - (*b)->nid; + if (a->nid < b->nid) { + return -1; + } + if (a->nid > b->nid) { + return 1; + } + return 0; } -static int table_cmp(const void *in_a, const void *in_b) +static int table_cmp_void(const void *a, const void *b) { - const ASN1_STRING_TABLE *a = in_a; - const ASN1_STRING_TABLE *b = in_b; - return a->nid - b->nid; + return table_cmp(a, b); } -ASN1_STRING_TABLE *ASN1_STRING_TABLE_get(int nid) +static uint32_t table_hash(const ASN1_STRING_TABLE *tbl) { - int found; - size_t idx; - ASN1_STRING_TABLE *ttmp; - ASN1_STRING_TABLE fnd; - fnd.nid = nid; - - ttmp = - bsearch(&fnd, tbl_standard, - sizeof(tbl_standard) / sizeof(ASN1_STRING_TABLE), - sizeof(ASN1_STRING_TABLE), table_cmp); - if (ttmp) - return ttmp; - if (!stable) - return NULL; - sk_ASN1_STRING_TABLE_sort(stable); - found = sk_ASN1_STRING_TABLE_find(stable, &idx, &fnd); - if (!found) - return NULL; - return sk_ASN1_STRING_TABLE_value(stable, idx); + return OPENSSL_hash32(&tbl->nid, sizeof(tbl->nid)); } -int ASN1_STRING_TABLE_add(int nid, - long minsize, long maxsize, unsigned long mask, - unsigned long flags) +static const ASN1_STRING_TABLE *asn1_string_table_get(int nid) { - ASN1_STRING_TABLE *tmp; - char new_nid = 0; - flags &= ~STABLE_FLAGS_MALLOC; - if (!stable) - stable = sk_ASN1_STRING_TABLE_new(sk_table_cmp); - if (!stable) { - OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE); - return 0; + ASN1_STRING_TABLE key; + key.nid = nid; + const ASN1_STRING_TABLE *tbl = + bsearch(&key, tbl_standard, OPENSSL_ARRAY_SIZE(tbl_standard), + sizeof(ASN1_STRING_TABLE), table_cmp_void); + if (tbl != NULL) { + return tbl; } - if (!(tmp = ASN1_STRING_TABLE_get(nid))) { - tmp = OPENSSL_malloc(sizeof(ASN1_STRING_TABLE)); - if (!tmp) { - OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE); - return 0; - } - tmp->flags = flags | STABLE_FLAGS_MALLOC; - tmp->nid = nid; - tmp->minsize = tmp->maxsize = -1; - new_nid = 1; - } else - tmp->flags = (tmp->flags & STABLE_FLAGS_MALLOC) | flags; - if (minsize != -1) - tmp->minsize = minsize; - if (maxsize != -1) - tmp->maxsize = maxsize; - tmp->mask = mask; - if (new_nid) - sk_ASN1_STRING_TABLE_push(stable, tmp); - return 1; + + CRYPTO_STATIC_MUTEX_lock_read(&string_tables_lock); + if (string_tables != NULL) { + tbl = lh_ASN1_STRING_TABLE_retrieve(string_tables, &key); + } + CRYPTO_STATIC_MUTEX_unlock_read(&string_tables_lock); + /* Note returning |tbl| without the lock is only safe because + * |ASN1_STRING_TABLE_add| cannot modify or delete existing entries. If we + * wish to support that, this function must copy the result under a lock. */ + return tbl; } -void ASN1_STRING_TABLE_cleanup(void) +int ASN1_STRING_TABLE_add(int nid, long minsize, long maxsize, + unsigned long mask, unsigned long flags) { - STACK_OF(ASN1_STRING_TABLE) *tmp; - tmp = stable; - if (!tmp) - return; - stable = NULL; - sk_ASN1_STRING_TABLE_pop_free(tmp, st_free); + /* Existing entries cannot be overwritten. */ + if (asn1_string_table_get(nid) != NULL) { + OPENSSL_PUT_ERROR(ASN1, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); + return 0; + } + + int ret = 0; + CRYPTO_STATIC_MUTEX_lock_write(&string_tables_lock); + + if (string_tables == NULL) { + string_tables = lh_ASN1_STRING_TABLE_new(table_hash, table_cmp); + if (string_tables == NULL) { + goto err; + } + } else { + /* Check again for an existing entry. One may have been added while + * unlocked. */ + ASN1_STRING_TABLE key; + key.nid = nid; + if (lh_ASN1_STRING_TABLE_retrieve(string_tables, &key) != NULL) { + OPENSSL_PUT_ERROR(ASN1, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); + goto err; + } + } + + ASN1_STRING_TABLE *tbl = OPENSSL_malloc(sizeof(ASN1_STRING_TABLE)); + if (tbl == NULL) { + goto err; + } + tbl->nid = nid; + tbl->flags = flags; + tbl->minsize = minsize; + tbl->maxsize = maxsize; + tbl->mask = mask; + ASN1_STRING_TABLE *old_tbl; + if (!lh_ASN1_STRING_TABLE_insert(string_tables, &old_tbl, tbl)) { + OPENSSL_free(tbl); + goto err; + } + assert(old_tbl == NULL); + ret = 1; + +err: + CRYPTO_STATIC_MUTEX_unlock_write(&string_tables_lock); + return ret; } -static void st_free(ASN1_STRING_TABLE *tbl) +void ASN1_STRING_TABLE_cleanup(void) { - if (tbl->flags & STABLE_FLAGS_MALLOC) - OPENSSL_free(tbl); } void asn1_get_string_table_for_testing(const ASN1_STRING_TABLE **out_ptr, diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index 439afc6e7..7c37cb6c8 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc @@ -32,6 +32,10 @@ #include "../test/test_util.h" #include "internal.h" +#if defined(OPENSSL_THREADS) +#include +#endif + // kTag128 is an ASN.1 structure with a universal tag with number 128. static const uint8_t kTag128[] = { @@ -1160,11 +1164,11 @@ TEST(ASN1Test, StringByNID) { TEST(ASN1Test, StringByCustomNID) { // This test affects library-global state. We rely on nothing else in the test // suite using these OIDs. - int nid1 = OBJ_create("1.2.840.113554.4.1.72585.1000", "custom OID 1", - "custom OID 1"); + int nid1 = OBJ_create("1.2.840.113554.4.1.72585.1000", "custom OID 1000", + "custom OID 1000"); ASSERT_NE(NID_undef, nid1); - int nid2 = OBJ_create("1.2.840.113554.4.1.72585.1001", "custom OID 2", - "custom OID 2"); + int nid2 = OBJ_create("1.2.840.113554.4.1.72585.1001", "custom OID 1001", + "custom OID 1001"); ASSERT_NE(NID_undef, nid2); // Values registered in the string table should be picked up. @@ -1200,7 +1204,52 @@ TEST(ASN1Test, StringByCustomNID) { EXPECT_EQ(V_ASN1_UTF8STRING, ASN1_STRING_type(str.get())); EXPECT_EQ(Bytes("12345"), Bytes(ASN1_STRING_get0_data(str.get()), ASN1_STRING_length(str.get()))); + + // Overriding existing entries, built-in or custom, is an error. + EXPECT_FALSE( + ASN1_STRING_TABLE_add(NID_countryName, -1, -1, DIRSTRING_TYPE, 0)); + EXPECT_FALSE(ASN1_STRING_TABLE_add(nid1, -1, -1, DIRSTRING_TYPE, 0)); +} + +#if defined(OPENSSL_THREADS) +TEST(ASN1Test, StringByCustomNIDThreads) { + // This test affects library-global state. We rely on nothing else in the test + // suite using these OIDs. + int nid1 = OBJ_create("1.2.840.113554.4.1.72585.1002", "custom OID 1002", + "custom OID 1002"); + ASSERT_NE(NID_undef, nid1); + int nid2 = OBJ_create("1.2.840.113554.4.1.72585.1003", "custom OID 1003", + "custom OID 1003"); + ASSERT_NE(NID_undef, nid2); + + std::vector threads; + threads.emplace_back([&] { + ASSERT_TRUE(ASN1_STRING_TABLE_add(nid1, 5, 10, V_ASN1_PRINTABLESTRING, + STABLE_NO_MASK)); + bssl::UniquePtr str(ASN1_STRING_set_by_NID( + nullptr, reinterpret_cast("12345"), 5, MBSTRING_UTF8, + nid1)); + ASSERT_TRUE(str); + EXPECT_EQ(V_ASN1_PRINTABLESTRING, ASN1_STRING_type(str.get())); + EXPECT_EQ(Bytes("12345"), Bytes(ASN1_STRING_get0_data(str.get()), + ASN1_STRING_length(str.get()))); + }); + threads.emplace_back([&] { + ASSERT_TRUE(ASN1_STRING_TABLE_add(nid2, 5, 10, V_ASN1_PRINTABLESTRING, + STABLE_NO_MASK)); + bssl::UniquePtr str(ASN1_STRING_set_by_NID( + nullptr, reinterpret_cast("12345"), 5, MBSTRING_UTF8, + nid2)); + ASSERT_TRUE(str); + EXPECT_EQ(V_ASN1_PRINTABLESTRING, ASN1_STRING_type(str.get())); + EXPECT_EQ(Bytes("12345"), Bytes(ASN1_STRING_get0_data(str.get()), + ASN1_STRING_length(str.get()))); + }); + for (auto &thread : threads) { + thread.join(); + } } +#endif // OPENSSL_THREADS // Test that multi-string types correctly encode negative ENUMERATED. // Multi-string types cannot contain INTEGER, so we only test ENUMERATED. diff --git a/crypto/asn1/internal.h b/crypto/asn1/internal.h index cab0c7735..dcb6fcf7b 100644 --- a/crypto/asn1/internal.h +++ b/crypto/asn1/internal.h @@ -175,6 +175,14 @@ const void *asn1_type_value_as_pointer(const ASN1_TYPE *a); * ASN.1 PrintableString, and zero otherwise. */ int asn1_is_printable(uint32_t value); +typedef struct { + int nid; + long minsize; + long maxsize; + unsigned long mask; + unsigned long flags; +} ASN1_STRING_TABLE; + /* asn1_get_string_table_for_testing sets |*out_ptr| and |*out_len| to the table * of built-in |ASN1_STRING_TABLE| values. It is exported for testing. */ OPENSSL_EXPORT void asn1_get_string_table_for_testing( diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h index 4f6fb3b4b..0f22de196 100644 --- a/include/openssl/asn1.h +++ b/include/openssl/asn1.h @@ -370,6 +370,50 @@ OPENSSL_EXPORT int ASN1_mbstring_ncopy(ASN1_STRING **out, const uint8_t *in, int len, int inform, unsigned long mask, long minsize, long maxsize); +// ASN1_STRING_set_by_NID behaves like |ASN1_mbstring_ncopy|, but determines +// |mask|, |minsize|, and |maxsize| based on |nid|. When |nid| is a recognized +// X.509 attribute type, it will pick a suitable ASN.1 string type and bounds. +// For most attribute types, it preferentially chooses UTF8String. If |nid| is +// unrecognized, it uses UTF8String by default. +// +// Slightly unlike |ASN1_mbstring_ncopy|, this function interprets |out| and +// returns its result as follows: If |out| is NULL, it returns a newly-allocated +// |ASN1_STRING| containing the result. If |out| is non-NULL and +// |*out| is NULL, it additionally sets |*out| to the result. If both |out| and +// |*out| are non-NULL, it instead updates the object at |*out| and returns +// |*out|. In all cases, it returns NULL on error. +// +// This function supports the following NIDs: |NID_countryName|, +// |NID_dnQualifier|, |NID_domainComponent|, |NID_friendlyName|, +// |NID_givenName|, |NID_initials|, |NID_localityName|, |NID_ms_csp_name|, +// |NID_name|, |NID_organizationalUnitName|, |NID_organizationName|, +// |NID_pkcs9_challengePassword|, |NID_pkcs9_emailAddress|, +// |NID_pkcs9_unstructuredAddress|, |NID_pkcs9_unstructuredName|, +// |NID_serialNumber|, |NID_stateOrProvinceName|, and |NID_surname|. Additional +// NIDs may be registered with |ASN1_STRING_set_by_NID|, but it is recommended +// to call |ASN1_mbstring_ncopy| directly instead. +OPENSSL_EXPORT ASN1_STRING *ASN1_STRING_set_by_NID(ASN1_STRING **out, + const unsigned char *in, + int len, int inform, + int nid); + +// STABLE_NO_MASK causes |ASN1_STRING_TABLE_add| to allow types other than +// UTF8String. +#define STABLE_NO_MASK 0x02 + +// ASN1_STRING_TABLE_add registers the corresponding parameters with |nid|, for +// use with |ASN1_STRING_set_by_NID|. It returns one on success and zero on +// error. It is an error to call this function if |nid| is a built-in NID, or +// was already registered by a previous call. +// +// WARNING: This function affects global state in the library. If two libraries +// in the same address space register information for the same OID, one call +// will fail. Prefer directly passing the desired parametrs to +// |ASN1_mbstring_copy| or |ASN1_mbstring_ncopy| instead. +OPENSSL_EXPORT int ASN1_STRING_TABLE_add(int nid, long minsize, long maxsize, + unsigned long mask, + unsigned long flags); + // TODO(davidben): Expand and document function prototypes generated in macros. @@ -839,6 +883,30 @@ OPENSSL_EXPORT int ASN1_STRING_print_ex_fp(FILE *fp, const ASN1_STRING *str, unsigned long flags); +// Deprecated functions. + +// ASN1_PRINTABLE_type interprets |len| bytes from |s| as a Latin-1 string. It +// returns the first of |V_ASN1_PRINTABLESTRING|, |V_ASN1_IA5STRING|, or +// |V_ASN1_T61STRING| that can represent every character. If |len| is negative, +// |strlen(s)| is used instead. +// +// TODO(davidben): Remove this once all copies of Conscrypt have been updated +// past https://github.com/google/conscrypt/pull/1032. +OPENSSL_EXPORT int ASN1_PRINTABLE_type(const unsigned char *s, int len); + +// ASN1_STRING_set_default_mask does nothing. +OPENSSL_EXPORT void ASN1_STRING_set_default_mask(unsigned long mask); + +// ASN1_STRING_set_default_mask_asc returns one. +OPENSSL_EXPORT int ASN1_STRING_set_default_mask_asc(const char *p); + +// ASN1_STRING_get_default_mask returns |B_ASN1_UTF8STRING|. +OPENSSL_EXPORT unsigned long ASN1_STRING_get_default_mask(void); + +// ASN1_STRING_TABLE_cleanup does nothing. +OPENSSL_EXPORT void ASN1_STRING_TABLE_cleanup(void); + + // Underdocumented functions. // // The following functions are not yet documented and organized. @@ -863,17 +931,6 @@ typedef struct ASN1_ENCODING_st { unsigned alias_only_on_next_parse : 1; } ASN1_ENCODING; -#define STABLE_FLAGS_MALLOC 0x01 -#define STABLE_NO_MASK 0x02 - -typedef struct asn1_string_table_st { - int nid; - long minsize; - long maxsize; - unsigned long mask; - unsigned long flags; -} ASN1_STRING_TABLE; - // Declarations for template structures: for full definitions // see asn1t.h typedef struct ASN1_TEMPLATE_st ASN1_TEMPLATE; @@ -1114,12 +1171,6 @@ OPENSSL_EXPORT ASN1_OBJECT *ASN1_OBJECT_create(int nid, int len, const char *sn, const char *ln); -// ASN1_PRINTABLE_type interprets |len| bytes from |s| as a Latin-1 string. It -// returns the first of |V_ASN1_PRINTABLESTRING|, |V_ASN1_IA5STRING|, or -// |V_ASN1_T61STRING| that can represent every character. If |len| is negative, -// |strlen(s)| is used instead. -OPENSSL_EXPORT int ASN1_PRINTABLE_type(const unsigned char *s, int len); - OPENSSL_EXPORT unsigned long ASN1_tag2bit(int tag); // SPECIALS @@ -1146,24 +1197,6 @@ OPENSSL_EXPORT void *ASN1_item_unpack(const ASN1_STRING *oct, OPENSSL_EXPORT ASN1_STRING *ASN1_item_pack(void *obj, const ASN1_ITEM *it, ASN1_OCTET_STRING **oct); -// ASN1_STRING_set_default_mask does nothing. -OPENSSL_EXPORT void ASN1_STRING_set_default_mask(unsigned long mask); - -// ASN1_STRING_set_default_mask_asc returns one. -OPENSSL_EXPORT int ASN1_STRING_set_default_mask_asc(const char *p); - -// ASN1_STRING_get_default_mask returns |B_ASN1_UTF8STRING|. -OPENSSL_EXPORT unsigned long ASN1_STRING_get_default_mask(void); - -OPENSSL_EXPORT ASN1_STRING *ASN1_STRING_set_by_NID(ASN1_STRING **out, - const unsigned char *in, - int inlen, int inform, - int nid); -OPENSSL_EXPORT ASN1_STRING_TABLE *ASN1_STRING_TABLE_get(int nid); -OPENSSL_EXPORT int ASN1_STRING_TABLE_add(int, long, long, unsigned long, - unsigned long); -OPENSSL_EXPORT void ASN1_STRING_TABLE_cleanup(void); - // ASN1 template functions // Old API compatible functions