From 919a973930adf7fb9853fd5c3ae722a1d5e2cf59 Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Thu, 1 Jul 2021 11:44:40 -0700 Subject: [PATCH] conf: don't crash when parsing. lh_strhash mapped nullptr to zero. ec8c67dfbc switched CONF's use to OPENSSL_strhash, which crashes on nullptr. But CONF depends on the nullptr handling. Change-Id: I131c752aa089fb99b01c9e406b6994f3a6236976 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48385 Commit-Queue: Adam Langley Reviewed-by: David Benjamin --- crypto/CMakeLists.txt | 1 + crypto/conf/conf.c | 4 +++- crypto/conf/conf_test.cc | 45 ++++++++++++++++++++++++++++++++++++++++ include/openssl/conf.h | 13 +++++++----- 4 files changed, 57 insertions(+), 6 deletions(-) create mode 100644 crypto/conf/conf_test.cc diff --git a/crypto/CMakeLists.txt b/crypto/CMakeLists.txt index c06cbb792..7832edbec 100644 --- a/crypto/CMakeLists.txt +++ b/crypto/CMakeLists.txt @@ -501,6 +501,7 @@ add_executable( cipher_extra/cipher_test.cc cmac/cmac_test.cc compiler_test.cc + conf/conf_test.cc constant_time_test.cc cpu-arm-linux_test.cc crypto_test.cc diff --git a/crypto/conf/conf.c b/crypto/conf/conf.c index bc147f325..9e7dfe56f 100644 --- a/crypto/conf/conf.c +++ b/crypto/conf/conf.c @@ -82,7 +82,9 @@ struct conf_st { #define MAX_CONF_VALUE_LENGTH 65536 static uint32_t conf_value_hash(const CONF_VALUE *v) { - return (OPENSSL_strhash(v->section) << 2) ^ OPENSSL_strhash(v->name); + const uint32_t section_hash = v->section ? OPENSSL_strhash(v->section) : 0; + const uint32_t name_hash = v->name ? OPENSSL_strhash(v->name) : 0; + return (section_hash << 2) ^ name_hash; } static int conf_value_cmp(const CONF_VALUE *a, const CONF_VALUE *b) { diff --git a/crypto/conf/conf_test.cc b/crypto/conf/conf_test.cc new file mode 100644 index 000000000..25f4b4e7f --- /dev/null +++ b/crypto/conf/conf_test.cc @@ -0,0 +1,45 @@ +/* Copyright (c) 2021, Google Inc. + * + * Permission to use, copy, modify, and/or distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY + * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION + * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ + +#include +#include + +#include + + +TEST(ConfTest, Parse) { + // Check that basic parsing works. (We strongly recommend that people don't + // use the [N]CONF functions.) + + static const char kConf[] = R"( +# Comment + +key=value + +[section_name] +key=value2 +)"; + + bssl::UniquePtr bio(BIO_new_mem_buf(kConf, sizeof(kConf) - 1)); + ASSERT_TRUE(bio); + bssl::UniquePtr conf(NCONF_new(nullptr)); + ASSERT_TRUE(conf); + ASSERT_TRUE(NCONF_load_bio(conf.get(), bio.get(), nullptr)); + EXPECT_TRUE(NCONF_get_section(conf.get(), "section_name")); + EXPECT_FALSE(NCONF_get_section(conf.get(), "other_section")); + // Doesn't work but should. + // EXPECT_STREQ(NCONF_get_string(conf.get(), nullptr, "key"), "value"); + EXPECT_STREQ(NCONF_get_string(conf.get(), "section_name", "key"), "value2"); + EXPECT_STREQ(NCONF_get_string(conf.get(), "other_section", "key"), nullptr); +} diff --git a/include/openssl/conf.h b/include/openssl/conf.h index ae718699e..6890c7d25 100644 --- a/include/openssl/conf.h +++ b/include/openssl/conf.h @@ -100,22 +100,25 @@ OPENSSL_EXPORT void NCONF_free(CONF *conf); // |conf|. It returns one on success and zero on error. In the event of an // error, if |out_error_line| is not NULL, |*out_error_line| is set to the // number of the line that contained the error. -int NCONF_load(CONF *conf, const char *filename, long *out_error_line); +OPENSSL_EXPORT int NCONF_load(CONF *conf, const char *filename, + long *out_error_line); // NCONF_load_bio acts like |NCONF_load| but reads from |bio| rather than from // a named file. -int NCONF_load_bio(CONF *conf, BIO *bio, long *out_error_line); +OPENSSL_EXPORT int NCONF_load_bio(CONF *conf, BIO *bio, long *out_error_line); // NCONF_get_section returns a stack of values for a given section in |conf|. // If |section| is NULL, the default section is returned. It returns NULL on // error. -STACK_OF(CONF_VALUE) *NCONF_get_section(const CONF *conf, const char *section); +OPENSSL_EXPORT STACK_OF(CONF_VALUE) *NCONF_get_section(const CONF *conf, + const char *section); // NCONF_get_string returns the value of the key |name|, in section |section|. // The |section| argument may be NULL to indicate the default section. It // returns the value or NULL on error. -const char *NCONF_get_string(const CONF *conf, const char *section, - const char *name); +OPENSSL_EXPORT const char *NCONF_get_string(const CONF *conf, + const char *section, + const char *name); // Utility functions