From: Petr Gotthard Date: Sat, 6 Feb 2021 20:47:20 +0000 (+0100) Subject: Enhanced integer parsing in OSSL_PARAM_allocate_from_text X-Git-Tag: openssl-3.0.0-alpha12~101 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=604b86d8d360e36fc2fc0d1611d05bf38699d297;p=thirdparty%2Fopenssl.git Enhanced integer parsing in OSSL_PARAM_allocate_from_text Fixes #14041 and additional bugs discovered by the newly created tests. This patch: - Introduces support for 0x prefixed integers - Fixes parsing of negative integers (negative numbers were shifted by -2) - Fixes ability to parse maximal unsigned numbers ("too small buffer" error used to be reported incorrectly) - Fixes a memory leak when OSSL_PARAM_allocate_from_text fails leaving a temporary BN allocated Reviewed-by: Matt Caswell Reviewed-by: Paul Dale Reviewed-by: Richard Levitte (Merged from https://github.com/openssl/openssl/pull/14093) --- diff --git a/crypto/params_from_text.c b/crypto/params_from_text.c index ddc3c38aa4..b019744f9b 100644 --- a/crypto/params_from_text.c +++ b/crypto/params_from_text.c @@ -28,6 +28,7 @@ static int prepare_from_text(const OSSL_PARAM *paramdefs, const char *key, size_t *buf_n, BIGNUM **tmpbn, int *found) { const OSSL_PARAM *p; + size_t buf_bits; /* * ishex is used to translate legacy style string controls in hex format @@ -50,7 +51,7 @@ static int prepare_from_text(const OSSL_PARAM *paramdefs, const char *key, if (*ishex) BN_hex2bn(tmpbn, value); else - BN_dec2bn(tmpbn, value); + BN_asc2bn(tmpbn, value); if (*tmpbn == NULL) return 0; @@ -62,20 +63,25 @@ static int prepare_from_text(const OSSL_PARAM *paramdefs, const char *key, * buffer, i.e. if it's negative, we need to deal with it. We do * it by subtracting 1 here and inverting the bytes in * construct_from_text() below. + * To subtract 1 from an absolute value of a negative number we + * actually have to add 1: -3 - 1 = -4, |-3| = 3 + 1 = 4. */ if (p->data_type == OSSL_PARAM_INTEGER && BN_is_negative(*tmpbn) - && !BN_sub_word(*tmpbn, 1)) { + && !BN_add_word(*tmpbn, 1)) { return 0; } - *buf_n = BN_num_bytes(*tmpbn); + buf_bits = (size_t)BN_num_bits(*tmpbn); + *buf_n = (buf_bits + 7) / 8; /* * TODO(v3.0) is this the right way to do this? This code expects * a zero data size to simply mean "arbitrary size". */ if (p->data_size > 0) { - if (*buf_n >= p->data_size) { + if (buf_bits > p->data_size * 8 + || (p->data_type == OSSL_PARAM_INTEGER + && buf_bits == p->data_size * 8)) { ERR_raise(ERR_LIB_CRYPTO, CRYPTO_R_TOO_SMALL_BUFFER); /* Since this is a different error, we don't break */ return 0; @@ -184,11 +190,11 @@ int OSSL_PARAM_allocate_from_text(OSSL_PARAM *to, if (!prepare_from_text(paramdefs, key, value, value_n, ¶mdef, &ishex, &buf_n, &tmpbn, found)) - return 0; + goto err; if ((buf = OPENSSL_zalloc(buf_n > 0 ? buf_n : 1)) == NULL) { ERR_raise(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE); - return 0; + goto err; } ok = construct_from_text(to, paramdef, value, value_n, ishex, @@ -197,4 +203,7 @@ int OSSL_PARAM_allocate_from_text(OSSL_PARAM *to, if (!ok) OPENSSL_free(buf); return ok; + err: + BN_free(tmpbn); + return 0; } diff --git a/doc/man3/OSSL_PARAM_allocate_from_text.pod b/doc/man3/OSSL_PARAM_allocate_from_text.pod index ef68f0e10c..80ba555a8f 100644 --- a/doc/man3/OSSL_PARAM_allocate_from_text.pod +++ b/doc/man3/OSSL_PARAM_allocate_from_text.pod @@ -55,15 +55,16 @@ depending on that item's I, as follows: =item B and B -If I started with "hex", I is assumed to contain -I hexadecimal characters, which are decoded, and the -resulting bytes become the number stored in the I<< to->data >> -storage. - If I didn't start with "hex", I is assumed to contain I decimal characters, which are decoded, and the resulting bytes become the number stored in the I<< to->data >> storage. +If I starts with "0x", it is assumed to contain I +hexadecimal characters. + +If I started with "hex", I is assumed to contain +I hexadecimal characters without the "0x" prefix. + If I contains characters that couldn't be decoded as hexadecimal or decimal characters, OSSL_PARAM_allocate_from_text() considers that an error. diff --git a/test/params_test.c b/test/params_test.c index 8ee2e1594c..913df9eb8a 100644 --- a/test/params_test.c +++ b/test/params_test.c @@ -541,8 +541,81 @@ static int test_case(int i) test_cases[i].prov)); } +/*- + * OSSL_PARAM_allocate_from_text() tests + * ===================================== + */ + +static const OSSL_PARAM params_from_text[] = { + OSSL_PARAM_int32("int", NULL), + OSSL_PARAM_DEFN("short", OSSL_PARAM_INTEGER, NULL, sizeof(int16_t)), + OSSL_PARAM_DEFN("ushort", OSSL_PARAM_UNSIGNED_INTEGER, NULL, sizeof(uint16_t)), + OSSL_PARAM_END, +}; + +struct int_from_text_test_st { + const char *argname; + const char *strval; + long int intval; + int res; +}; + +static struct int_from_text_test_st int_from_text_test_cases[] = { + { "int", "", 0, 0 }, + { "int", "0", 0, 1 }, + { "int", "101", 101, 1 }, + { "int", "-102", -102, 1 }, + { "int", "12A", 12, 1 }, /* incomplete */ + { "int", "0x12B", 0x12B, 1 }, + { "hexint", "12C", 0x12C, 1 }, + { "hexint", "0x12D", 0, 1 }, /* zero */ + /* test check of the target buffer size */ + { "int", "0x7fffffff", INT32_MAX, 1 }, + { "int", "2147483647", INT32_MAX, 1 }, + { "int", "2147483648", 0, 0 }, /* too small buffer */ + { "int", "-2147483648", INT32_MIN, 1 }, + { "int", "-2147483649", 0, 0 }, /* too small buffer */ + { "short", "0x7fff", INT16_MAX, 1 }, + { "short", "32767", INT16_MAX, 1 }, + { "short", "32768", 0, 0 }, /* too small buffer */ + { "ushort", "0xffff", UINT16_MAX, 1 }, + { "ushort", "65535", UINT16_MAX, 1 }, + { "ushort", "65536", 0, 0 }, /* too small buffer */ +}; + +static int check_int_from_text(const struct int_from_text_test_st a) +{ + OSSL_PARAM param; + long int val = 0; + int res; + + if (!OSSL_PARAM_allocate_from_text(¶m, params_from_text, + a.argname, a.strval, 0, NULL)) { + if (a.res) + TEST_error("errant %s param \"%s\"", a.argname, a.strval); + return !a.res; + } + + res = OSSL_PARAM_get_long(¶m, &val); + OPENSSL_free(param.data); + + if (res ^ a.res || val != a.intval) { + TEST_error("errant %s \"%s\" %li != %li", + a.argname, a.strval, a.intval, val); + return 0; + } + + return a.res; +} + +static int test_allocate_from_text(int i) +{ + return check_int_from_text(int_from_text_test_cases[i]); +} + int setup_tests(void) { ADD_ALL_TESTS(test_case, OSSL_NELEM(test_cases)); + ADD_ALL_TESTS(test_allocate_from_text, OSSL_NELEM(int_from_text_test_cases)); return 1; }