From c9ecb13191fe902c1e78e3bca7c36c293bba4bc6 Mon Sep 17 00:00:00 2001 From: Pauli Date: Wed, 4 Jul 2018 09:30:43 +1000 Subject: [PATCH] NCONF_get_number refix. Fix the NULL check lack in a different way that is more compatible with non-NULL branch. Refer #6632 Also mark and pop the error stack instead of clearing all errors when something goes awry in CONF_get_number. Reviewed-by: Rich Salz Reviewed-by: Andy Polyakov (Merged from https://github.com/openssl/openssl/pull/6643) --- crypto/conf/conf_err.c | 1 + crypto/conf/conf_lib.c | 44 ++++++++++++++++++++++++++++----------- crypto/err/openssl.txt | 1 + include/openssl/conferr.h | 1 + test/conf_include_test.c | 26 +++++++++++++++++++++++ 5 files changed, 61 insertions(+), 12 deletions(-) diff --git a/crypto/conf/conf_err.c b/crypto/conf/conf_err.c index 01f98da87a..f7613584ec 100644 --- a/crypto/conf/conf_err.c +++ b/crypto/conf/conf_err.c @@ -60,6 +60,7 @@ static const ERR_STRING_DATA CONF_str_reasons[] = { {ERR_PACK(ERR_LIB_CONF, 0, CONF_R_NO_SECTION), "no section"}, {ERR_PACK(ERR_LIB_CONF, 0, CONF_R_NO_SUCH_FILE), "no such file"}, {ERR_PACK(ERR_LIB_CONF, 0, CONF_R_NO_VALUE), "no value"}, + {ERR_PACK(ERR_LIB_CONF, 0, CONF_R_NUMBER_TOO_LARGE), "number too large"}, {ERR_PACK(ERR_LIB_CONF, 0, CONF_R_RECURSIVE_DIRECTORY_INCLUDE), "recursive directory include"}, {ERR_PACK(ERR_LIB_CONF, 0, CONF_R_SSL_COMMAND_SECTION_EMPTY), diff --git a/crypto/conf/conf_lib.c b/crypto/conf/conf_lib.c index 5f976f35c4..1833b15ebe 100644 --- a/crypto/conf/conf_lib.c +++ b/crypto/conf/conf_lib.c @@ -11,6 +11,7 @@ #include #include #include "internal/conf.h" +#include "internal/ctype.h" #include #include #include @@ -123,6 +124,7 @@ long CONF_get_number(LHASH_OF(CONF_VALUE) *conf, const char *group, int status; long result = 0; + ERR_set_mark(); if (conf == NULL) { status = NCONF_get_number_e(NULL, group, name, &result); } else { @@ -130,12 +132,8 @@ long CONF_get_number(LHASH_OF(CONF_VALUE) *conf, const char *group, CONF_set_nconf(&ctmp, conf); status = NCONF_get_number_e(&ctmp, group, name, &result); } - - if (status == 0) { - /* This function does not believe in errors... */ - ERR_clear_error(); - } - return result; + ERR_pop_to_mark(); + return status == 0 ? 0L : result; } void CONF_free(LHASH_OF(CONF_VALUE) *conf) @@ -277,10 +275,23 @@ char *NCONF_get_string(const CONF *conf, const char *group, const char *name) return NULL; } +static int default_is_number(const CONF *conf, char c) +{ + return ossl_isdigit(c); +} + +static int default_to_int(const CONF *conf, char c) +{ + return (int)(c - '0'); +} + int NCONF_get_number_e(const CONF *conf, const char *group, const char *name, long *result) { char *str; + long res; + int (*is_number)(const CONF *, char) = &default_is_number; + int (*to_int)(const CONF *, char) = &default_to_int; if (result == NULL) { CONFerr(CONF_F_NCONF_GET_NUMBER_E, ERR_R_PASSED_NULL_PARAMETER); @@ -292,14 +303,23 @@ int NCONF_get_number_e(const CONF *conf, const char *group, const char *name, if (str == NULL) return 0; - if (conf == NULL) - *result = strtol(str, &str, 10); - else - for (*result = 0; conf->meth->is_number(conf, *str);) { - *result = (*result) * 10 + conf->meth->to_int(conf, *str); - str++; + if (conf != NULL) { + if (conf->meth->is_number != NULL) + is_number = conf->meth->is_number; + if (conf->meth->to_int != NULL) + to_int = conf->meth->to_int; + } + for (res = 0; is_number(conf, *str); str++) { + const int d = to_int(conf, *str); + + if (res > (LONG_MAX - d) / 10L) { + CONFerr(CONF_F_NCONF_GET_NUMBER_E, CONF_R_NUMBER_TOO_LARGE); + return 0; } + res = res * 10 + d; + } + *result = res; return 1; } diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index ee68388b50..007560a171 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt @@ -2012,6 +2012,7 @@ CONF_R_NO_CONF_OR_ENVIRONMENT_VARIABLE:106:no conf or environment variable CONF_R_NO_SECTION:107:no section CONF_R_NO_SUCH_FILE:114:no such file CONF_R_NO_VALUE:108:no value +CONF_R_NUMBER_TOO_LARGE:121:number too large CONF_R_RECURSIVE_DIRECTORY_INCLUDE:111:recursive directory include CONF_R_SSL_COMMAND_SECTION_EMPTY:117:ssl command section empty CONF_R_SSL_COMMAND_SECTION_NOT_FOUND:118:ssl command section not found diff --git a/include/openssl/conferr.h b/include/openssl/conferr.h index 4574636b01..d1c92f45d8 100644 --- a/include/openssl/conferr.h +++ b/include/openssl/conferr.h @@ -58,6 +58,7 @@ int ERR_load_CONF_strings(void); # define CONF_R_NO_SECTION 107 # define CONF_R_NO_SUCH_FILE 114 # define CONF_R_NO_VALUE 108 +# define CONF_R_NUMBER_TOO_LARGE 121 # define CONF_R_RECURSIVE_DIRECTORY_INCLUDE 111 # define CONF_R_SSL_COMMAND_SECTION_EMPTY 117 # define CONF_R_SSL_COMMAND_SECTION_NOT_FOUND 118 diff --git a/test/conf_include_test.c b/test/conf_include_test.c index ba79d2c208..ee02d9b3fb 100644 --- a/test/conf_include_test.c +++ b/test/conf_include_test.c @@ -153,6 +153,31 @@ static int test_check_null_numbers(void) return 1; } +static int test_check_overflow(void) +{ +#if defined(_BSD_SOURCE) \ + || (defined(_POSIX_C_SOURCE) && _POSIX_C_SOURCE >= 200112L) \ + || (defined(_XOPEN_SOURCE) && _XOPEN_SOURCE >= 600) + long val = 0; + char max[(sizeof(long) * 8) / 3 + 3]; + char *p; + + p = max + sprintf(max, "0%ld", LONG_MAX) - 1; + setenv("FNORD", max, 1); + if (!TEST_true(NCONF_get_number(NULL, "missing", "FNORD", &val)) + || !TEST_long_eq(val, LONG_MAX)) + return 0; + + while (++*p > '9') + *p-- = '0'; + + setenv("FNORD", max, 1); + if (!TEST_false(NCONF_get_number(NULL, "missing", "FNORD", &val))) + return 0; +#endif + return 1; +} + int setup_tests(void) { const char *conf_file; @@ -181,6 +206,7 @@ int setup_tests(void) ADD_TEST(test_load_config); ADD_TEST(test_check_null_numbers); + ADD_TEST(test_check_overflow); return 1; } -- 2.34.1