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 <agl@google.com>

crypto/asn1/a_strnid.c

@@ -56,22 +56,23 @@
 
 #include <openssl/asn1.h>
 
-#include <stdlib.h>             /* For bsearch */
+#include <assert.h>
+#include <stdlib.h>
 #include <string.h>
 
 #include <openssl/err.h>
 #include <openssl/mem.h>
 #include <openssl/obj.h>
-#include <openssl/stack.h>
 
 #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,

crypto/asn1/asn1_test.cc

@@ -32,6 +32,10 @@
 #include "../test/test_util.h"
 #include "internal.h"
 
+#if defined(OPENSSL_THREADS)
+#include <thread>
+#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<std::thread> threads;
+  threads.emplace_back([&] {
+    ASSERT_TRUE(ASN1_STRING_TABLE_add(nid1, 5, 10, V_ASN1_PRINTABLESTRING,
+                                      STABLE_NO_MASK));
+    bssl::UniquePtr<ASN1_STRING> str(ASN1_STRING_set_by_NID(
+        nullptr, reinterpret_cast<const uint8_t *>("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<ASN1_STRING> str(ASN1_STRING_set_by_NID(
+        nullptr, reinterpret_cast<const uint8_t *>("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.

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(

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