From c394713d0d84558865a5ed434310e8fefe445be6 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 27 Sep 2023 15:24:58 -0400 Subject: [PATCH 1/2] Elaborate a bit on static vs dynamic EC_GROUPs in documentation Also move EC_GROUP_free and EC_GROUP_dup to the deprecated section, because it's now usually unnecessary. Change-Id: Ica4aa8d2b993555f977792bf284a5dbf29a3d710 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/63245 Commit-Queue: David Benjamin Auto-Submit: David Benjamin Reviewed-by: Adam Langley --- include/openssl/ec.h | 36 +++++++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/include/openssl/ec.h b/include/openssl/ec.h index e8a2db0a0..085222c9e 100644 --- a/include/openssl/ec.h +++ b/include/openssl/ec.h @@ -100,6 +100,16 @@ typedef enum { // Elliptic curve groups. +// +// Elliptic curve groups are represented by |EC_GROUP| objects. Unlike OpenSSL, +// if limited to the APIs in this section, callers may treat |EC_GROUP|s as +// static, immutable objects which do not need to be copied or released. In +// BoringSSL, only custom |EC_GROUP|s created by |EC_GROUP_new_curve_GFp| +// (deprecated) are dynamic. +// +// Callers may cast away |const| and use |EC_GROUP_dup| and |EC_GROUP_free| with +// static groups, for compatibility with OpenSSL or dynamic groups, but it is +// otherwise unnecessary. // EC_group_p224 returns an |EC_GROUP| for P-224, also known as secp224r1. OPENSSL_EXPORT const EC_GROUP *EC_group_p224(void); @@ -133,12 +143,6 @@ OPENSSL_EXPORT const EC_GROUP *EC_group_p521(void); // more modern primitives. OPENSSL_EXPORT EC_GROUP *EC_GROUP_new_by_curve_name(int nid); -// EC_GROUP_free releases a reference to |group|. -OPENSSL_EXPORT void EC_GROUP_free(EC_GROUP *group); - -// EC_GROUP_dup takes a reference to |a| and returns it. -OPENSSL_EXPORT EC_GROUP *EC_GROUP_dup(const EC_GROUP *a); - // EC_GROUP_cmp returns zero if |a| and |b| are the same group and non-zero // otherwise. OPENSSL_EXPORT int EC_GROUP_cmp(const EC_GROUP *a, const EC_GROUP *b, @@ -363,9 +367,27 @@ OPENSSL_EXPORT int EC_hash_to_curve_p384_xmd_sha384_sswu( // Deprecated functions. +// EC_GROUP_free releases a reference to |group|, if |group| was created by +// |EC_GROUP_new_curve_GFp|. If |group| is static, it does nothing. +// +// This function exists for OpenSSL compatibilty, and to manage dynamic +// |EC_GROUP|s constructed by |EC_GROUP_new_curve_GFp|. Callers that do not need +// either may ignore this function. +OPENSSL_EXPORT void EC_GROUP_free(EC_GROUP *group); + +// EC_GROUP_dup increments |group|'s reference count and returns it, if |group| +// was created by |EC_GROUP_new_curve_GFp|. If |group| is static, it simply +// returns |group|. +// +// This function exists for OpenSSL compatibilty, and to manage dynamic +// |EC_GROUP|s constructed by |EC_GROUP_new_curve_GFp|. Callers that do not need +// either may ignore this function. +OPENSSL_EXPORT EC_GROUP *EC_GROUP_dup(const EC_GROUP *group); + // EC_GROUP_new_curve_GFp creates a new, arbitrary elliptic curve group based // on the equation y² = x³ + a·x + b. It returns the new group or NULL on -// error. +// error. The lifetime of the resulting object must be managed with +// |EC_GROUP_dup| and |EC_GROUP_free|. // // This new group has no generator. It is an error to use a generator-less group // with any functions except for |EC_GROUP_free|, |EC_POINT_new|, From 313f9b0c6034e3337f50a91466bd8977442bdc21 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 23 May 2023 17:15:19 -0400 Subject: [PATCH 2/2] Replace CONF's internal representation with something more typesafe Sections are stored in a CONF structure as having name == NULL and value being a STACK_OF(CONF_VALUE) with the wrong pointer type. This loses type safety and complicates all the cleanup functions. (E.g. crypto/x509 has its own X509V3_conf_free which is distinct from the copy in crypto/conf.c.) These objects are, happily, never exported outside the file. Replace them with a CONF_SECTION and store the two values in separate hash tables. This also means a CONF_VALUE's name is no longer nullable, so all the comparisons and hashes become simpler. Also fix up add_string slightly. It left the CONF in a slightly precarious state if a malloc failed in the middle. Also v->section would leak if add_string failed. Change-Id: Ib54e9dd5037766804c8ddcd80d357237d2d357ea Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60106 Commit-Queue: David Benjamin Reviewed-by: Adam Langley --- crypto/conf/conf.c | 176 +++++++++++++++++++-------------------- crypto/conf/conf_test.cc | 3 +- crypto/conf/internal.h | 6 +- 3 files changed, 94 insertions(+), 91 deletions(-) diff --git a/crypto/conf/conf.c b/crypto/conf/conf.c index 024fa7448..40e8ffba2 100644 --- a/crypto/conf/conf.c +++ b/crypto/conf/conf.c @@ -70,48 +70,51 @@ #include "../internal.h" +struct conf_section_st { + char *name; + // values contains non-owning pointers to the values in the section. + STACK_OF(CONF_VALUE) *values; +}; + static const char kDefaultSectionName[] = "default"; +static uint32_t conf_section_hash(const CONF_SECTION *s) { + return OPENSSL_strhash(s->name); +} + +static int conf_section_cmp(const CONF_SECTION *a, const CONF_SECTION *b) { + return strcmp(a->name, b->name); +} + static uint32_t conf_value_hash(const CONF_VALUE *v) { - const uint32_t section_hash = v->section ? OPENSSL_strhash(v->section) : 0; - const uint32_t name_hash = v->name ? OPENSSL_strhash(v->name) : 0; + const uint32_t section_hash = OPENSSL_strhash(v->section); + const uint32_t name_hash = OPENSSL_strhash(v->name); return (section_hash << 2) ^ name_hash; } static int conf_value_cmp(const CONF_VALUE *a, const CONF_VALUE *b) { - int i; - - if (a->section != b->section) { - i = strcmp(a->section, b->section); - if (i) { - return i; - } + int cmp = strcmp(a->section, b->section); + if (cmp != 0) { + return cmp; } - if (a->name != NULL && b->name != NULL) { - return strcmp(a->name, b->name); - } else if (a->name == b->name) { - return 0; - } else { - return (a->name == NULL) ? -1 : 1; - } + return strcmp(a->name, b->name); } CONF *NCONF_new(void *method) { - CONF *conf; - if (method != NULL) { return NULL; } - conf = OPENSSL_malloc(sizeof(CONF)); + CONF *conf = OPENSSL_malloc(sizeof(CONF)); if (conf == NULL) { return NULL; } - conf->data = lh_CONF_VALUE_new(conf_value_hash, conf_value_cmp); - if (conf->data == NULL) { - OPENSSL_free(conf); + conf->sections = lh_CONF_SECTION_new(conf_section_hash, conf_section_cmp); + conf->values = lh_CONF_VALUE_new(conf_value_hash, conf_value_cmp); + if (conf->sections == NULL || conf->values == NULL) { + NCONF_free(conf); return NULL; } @@ -120,69 +123,64 @@ CONF *NCONF_new(void *method) { CONF_VALUE *CONF_VALUE_new(void) { return OPENSSL_zalloc(sizeof(CONF_VALUE)); } -static void value_free_contents(CONF_VALUE *value) { - OPENSSL_free(value->section); - if (value->name) { - OPENSSL_free(value->name); - OPENSSL_free(value->value); - } else { - // TODO(davidben): When |value->name| is NULL, |CONF_VALUE| is actually an - // entirely different structure. This is fragile and confusing. Make a - // proper |CONF_SECTION| type that doesn't require this. - sk_CONF_VALUE_free((STACK_OF(CONF_VALUE) *)value->value); +static void value_free(CONF_VALUE *value) { + if (value == NULL) { + return; } + OPENSSL_free(value->section); + OPENSSL_free(value->name); + OPENSSL_free(value->value); + OPENSSL_free(value); } -static void value_free(CONF_VALUE *value) { - if (value != NULL) { - value_free_contents(value); - OPENSSL_free(value); +static void section_free(CONF_SECTION *section) { + if (section == NULL) { + return; } + OPENSSL_free(section->name); + sk_CONF_VALUE_free(section->values); + OPENSSL_free(section); } static void value_free_arg(CONF_VALUE *value, void *arg) { value_free(value); } +static void section_free_arg(CONF_SECTION *section, void *arg) { + section_free(section); +} + void NCONF_free(CONF *conf) { - if (conf == NULL || conf->data == NULL) { + if (conf == NULL) { return; } - lh_CONF_VALUE_doall_arg(conf->data, value_free_arg, NULL); - lh_CONF_VALUE_free(conf->data); + lh_CONF_SECTION_doall_arg(conf->sections, section_free_arg, NULL); + lh_CONF_SECTION_free(conf->sections); + lh_CONF_VALUE_doall_arg(conf->values, value_free_arg, NULL); + lh_CONF_VALUE_free(conf->values); OPENSSL_free(conf); } -static CONF_VALUE *NCONF_new_section(const CONF *conf, const char *section) { - STACK_OF(CONF_VALUE) *sk = NULL; - int ok = 0; - CONF_VALUE *v = NULL, *old_value; - - sk = sk_CONF_VALUE_new_null(); - v = CONF_VALUE_new(); - if (sk == NULL || v == NULL) { - goto err; +static CONF_SECTION *NCONF_new_section(const CONF *conf, const char *section) { + CONF_SECTION *s = OPENSSL_malloc(sizeof(CONF_SECTION)); + if (!s) { + return NULL; } - v->section = OPENSSL_strdup(section); - if (v->section == NULL) { + s->name = OPENSSL_strdup(section); + s->values = sk_CONF_VALUE_new_null(); + if (s->name == NULL || s->values == NULL) { goto err; } - v->name = NULL; - v->value = (char *)sk; - - if (!lh_CONF_VALUE_insert(conf->data, &old_value, v)) { + CONF_SECTION *old_section; + if (!lh_CONF_SECTION_insert(conf->sections, &old_section, s)) { goto err; } - value_free(old_value); - ok = 1; + section_free(old_section); + return s; err: - if (!ok) { - sk_CONF_VALUE_free(sk); - OPENSSL_free(v); - v = NULL; - } - return v; + section_free(s); + return NULL; } static int str_copy(CONF *conf, char *section, char **pto, char *from) { @@ -254,21 +252,20 @@ err: return 0; } -static CONF_VALUE *get_section(const CONF *conf, const char *section) { - CONF_VALUE template; - +static CONF_SECTION *get_section(const CONF *conf, const char *section) { + CONF_SECTION template; OPENSSL_memset(&template, 0, sizeof(template)); - template.section = (char *) section; - return lh_CONF_VALUE_retrieve(conf->data, &template); + template.name = (char *) section; + return lh_CONF_SECTION_retrieve(conf->sections, &template); } const STACK_OF(CONF_VALUE) *NCONF_get_section(const CONF *conf, const char *section) { - const CONF_VALUE *section_value = get_section(conf, section); - if (section_value == NULL) { + const CONF_SECTION *section_obj = get_section(conf, section); + if (section_obj == NULL) { return NULL; } - return (STACK_OF(CONF_VALUE)*) section_value->value; + return section_obj->values; } const char *NCONF_get_string(const CONF *conf, const char *section, @@ -280,30 +277,35 @@ const char *NCONF_get_string(const CONF *conf, const char *section, } OPENSSL_memset(&template, 0, sizeof(template)); - template.section = (char *) section; - template.name = (char *) name; - value = lh_CONF_VALUE_retrieve(conf->data, &template); + template.section = (char *)section; + template.name = (char *)name; + value = lh_CONF_VALUE_retrieve(conf->values, &template); if (value == NULL) { return NULL; } return value->value; } -static int add_string(const CONF *conf, CONF_VALUE *section, +static int add_string(const CONF *conf, CONF_SECTION *section, CONF_VALUE *value) { - STACK_OF(CONF_VALUE) *section_stack = (STACK_OF(CONF_VALUE)*) section->value; - CONF_VALUE *old_value; - - value->section = OPENSSL_strdup(section->section); - if (!sk_CONF_VALUE_push(section_stack, value)) { + value->section = OPENSSL_strdup(section->name); + if (value->section == NULL) { return 0; } - if (!lh_CONF_VALUE_insert(conf->data, &old_value, value)) { + if (!sk_CONF_VALUE_push(section->values, value)) { + return 0; + } + + CONF_VALUE *old_value; + if (!lh_CONF_VALUE_insert(conf->values, &old_value, value)) { + // Remove |value| from |section->values|, so we do not leave a dangling + // pointer. + sk_CONF_VALUE_pop(section->values); return 0; } if (old_value != NULL) { - (void)sk_CONF_VALUE_delete_ptr(section_stack, old_value); + (void)sk_CONF_VALUE_delete_ptr(section->values, old_value); value_free(old_value); } @@ -388,8 +390,8 @@ int NCONF_load_bio(CONF *conf, BIO *in, long *out_error_line) { int again; long eline = 0; char btmp[DECIMAL_SIZE(eline) + 1]; - CONF_VALUE *v = NULL, *tv; - CONF_VALUE *sv = NULL; + CONF_VALUE *v = NULL; + CONF_SECTION *sv = NULL; char *section = NULL, *buf; char *start, *psection, *pname; @@ -540,6 +542,7 @@ int NCONF_load_bio(CONF *conf, BIO *in, long *out_error_line) { goto err; } + CONF_SECTION *tv; if (strcmp(psection, section) != 0) { if ((tv = get_section(conf, psection)) == NULL) { tv = NCONF_new_section(conf, psection); @@ -569,12 +572,7 @@ err: } snprintf(btmp, sizeof btmp, "%ld", eline); ERR_add_error_data(2, "line ", btmp); - - if (v != NULL) { - OPENSSL_free(v->name); - OPENSSL_free(v->value); - OPENSSL_free(v); - } + value_free(v); return 0; } diff --git a/crypto/conf/conf_test.cc b/crypto/conf/conf_test.cc index b243411db..544ac9661 100644 --- a/crypto/conf/conf_test.cc +++ b/crypto/conf/conf_test.cc @@ -94,7 +94,8 @@ static void ExpectConfEquals(const CONF *conf, const ConfModel &model) { // There should not be any other values in |conf|. |conf| currently stores // both sections and values in the same map. - EXPECT_EQ(lh_CONF_VALUE_num_items(conf->data), total_values + model.size()); + EXPECT_EQ(lh_CONF_SECTION_num_items(conf->sections), model.size()); + EXPECT_EQ(lh_CONF_VALUE_num_items(conf->values), total_values); } TEST(ConfTest, Parse) { diff --git a/crypto/conf/internal.h b/crypto/conf/internal.h index 04359ad25..1e39ac498 100644 --- a/crypto/conf/internal.h +++ b/crypto/conf/internal.h @@ -24,10 +24,14 @@ extern "C" { #endif +typedef struct conf_section_st CONF_SECTION; + +DEFINE_LHASH_OF(CONF_SECTION) DEFINE_LHASH_OF(CONF_VALUE) struct conf_st { - LHASH_OF(CONF_VALUE) *data; + LHASH_OF(CONF_VALUE) *values; + LHASH_OF(CONF_SECTION) *sections; }; // CONF_VALUE_new returns a freshly allocated and zeroed |CONF_VALUE|.