From 247a1786e25dbf77548168572e383d57aa743af4 Mon Sep 17 00:00:00 2001 From: Richard Levitte Date: Fri, 12 Feb 2021 20:30:40 +0100 Subject: OSSL_PARAM: Correct the assumptions on the UTF8 string length When the string "ABCDEFGH" is passed, what's considered its data, this? { 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H' } or this? { 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', '\0' } If it's passed as a pass phrase, should the terminating NUL byte be considered part of the pass phrase, or not? Our treatment of OSSL_PARAMs with the data type OSSL_PARAM_UTF8_STRING set the length of the string to include the terminating NUL byte, which is quite confusing. What should the recipient of such a string believe? Instead of perpetuating this confusion, we change the assumption to set the OSSL_PARAM to the length of the string, not including the terminating NUL byte, thereby giving it the same value as a strlen() call would give. Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/14168) --- crypto/params.c | 57 +++++++++++++++++++++++++++++---------- doc/man3/OSSL_PARAM.pod | 7 +++++ doc/man3/OSSL_PARAM_int.pod | 20 +++++++------- doc/man7/EVP_KDF-SSHKDF.pod | 2 +- test/params_api_test.c | 5 ++-- test/params_test.c | 66 ++++++++++++++++++++++----------------------- 6 files changed, 98 insertions(+), 59 deletions(-) diff --git a/crypto/params.c b/crypto/params.c index e28affe708..a3263e93c3 100644 --- a/crypto/params.c +++ b/crypto/params.c @@ -1070,15 +1070,21 @@ OSSL_PARAM OSSL_PARAM_construct_double(const char *key, double *buf) return ossl_param_construct(key, OSSL_PARAM_REAL, buf, sizeof(double)); } -static int get_string_internal(const OSSL_PARAM *p, void **val, size_t max_len, - size_t *used_len, unsigned int type) +static int get_string_internal(const OSSL_PARAM *p, void **val, + size_t *max_len, size_t *used_len, + unsigned int type) { - size_t sz; + size_t sz, alloc_sz; if ((val == NULL && used_len == NULL) || p == NULL || p->data_type != type) return 0; sz = p->data_size; + /* + * If the input size is 0, or the input string needs NUL byte + * termination, allocate an extra byte. + */ + alloc_sz = sz + (type == OSSL_PARAM_UTF8_STRING || sz == 0); if (used_len != NULL) *used_len = sz; @@ -1090,16 +1096,15 @@ static int get_string_internal(const OSSL_PARAM *p, void **val, size_t max_len, return 1; if (*val == NULL) { - char *const q = OPENSSL_malloc(sz > 0 ? sz : 1); + char *const q = OPENSSL_malloc(alloc_sz); if (q == NULL) return 0; *val = q; - if (sz != 0) - memcpy(q, p->data, sz); - return 1; + *max_len = alloc_sz; } - if (max_len < sz) + + if (*max_len < sz) return 0; memcpy(*val, p->data, sz); return 1; @@ -1107,14 +1112,35 @@ static int get_string_internal(const OSSL_PARAM *p, void **val, size_t max_len, int OSSL_PARAM_get_utf8_string(const OSSL_PARAM *p, char **val, size_t max_len) { - return get_string_internal(p, (void **)val, max_len, NULL, - OSSL_PARAM_UTF8_STRING); + int ret = get_string_internal(p, (void **)val, &max_len, NULL, + OSSL_PARAM_UTF8_STRING); + + /* + * We try to ensure that the copied string is terminated with a + * NUL byte. That should be easy, just place a NUL byte at + * |((char*)*val)[p->data_size]|. + * Unfortunately, we have seen cases where |p->data_size| doesn't + * correctly reflect the length of the string, and just happens + * to be out of bounds according to |max_len|, so in that case, we + * make the extra step of trying to find the true length of the + * string that |p->data| points at, and use that as an index to + * place the NUL byte in |*val|. + */ + size_t data_length = p->data_size; + + if (data_length >= max_len) + data_length = OPENSSL_strnlen(p->data, data_length); + if (data_length >= max_len) + return 0; /* No space for a terminating NUL byte */ + ((char *)*val)[data_length] = '\0'; + + return ret; } int OSSL_PARAM_get_octet_string(const OSSL_PARAM *p, void **val, size_t max_len, size_t *used_len) { - return get_string_internal(p, val, max_len, used_len, + return get_string_internal(p, val, &max_len, used_len, OSSL_PARAM_OCTET_STRING); } @@ -1128,6 +1154,9 @@ static int set_string_internal(OSSL_PARAM *p, const void *val, size_t len, return 0; memcpy(p->data, val, len); + /* If possible within the size of p->data, add a NUL terminator byte */ + if (type == OSSL_PARAM_UTF8_STRING && p->data_size > len) + ((char *)p->data)[len] = '\0'; return 1; } @@ -1139,7 +1168,7 @@ int OSSL_PARAM_set_utf8_string(OSSL_PARAM *p, const char *val) p->return_size = 0; if (val == NULL) return 0; - return set_string_internal(p, val, strlen(val) + 1, OSSL_PARAM_UTF8_STRING); + return set_string_internal(p, val, strlen(val), OSSL_PARAM_UTF8_STRING); } int OSSL_PARAM_set_octet_string(OSSL_PARAM *p, const void *val, @@ -1158,7 +1187,7 @@ OSSL_PARAM OSSL_PARAM_construct_utf8_string(const char *key, char *buf, size_t bsize) { if (buf != NULL && bsize == 0) - bsize = strlen(buf) + 1; + bsize = strlen(buf); return ossl_param_construct(key, OSSL_PARAM_UTF8_STRING, buf, bsize); } @@ -1207,7 +1236,7 @@ int OSSL_PARAM_set_utf8_ptr(OSSL_PARAM *p, const char *val) return 0; p->return_size = 0; return set_ptr_internal(p, val, OSSL_PARAM_UTF8_PTR, - val == NULL ? 0 : strlen(val) + 1); + val == NULL ? 0 : strlen(val)); } int OSSL_PARAM_set_octet_ptr(OSSL_PARAM *p, const void *val, diff --git a/doc/man3/OSSL_PARAM.pod b/doc/man3/OSSL_PARAM.pod index fdf376a206..99f4e2ce62 100644 --- a/doc/man3/OSSL_PARAM.pod +++ b/doc/man3/OSSL_PARAM.pod @@ -97,6 +97,13 @@ setting parameters) or shall (when requesting parameters) be stored, and I is its size in bytes. The organization of the data depends on the parameter type and flag. +The I needs special attention with the parameter type +B in relation to C strings. When setting +parameters, the size should be set to the length of the string, not +counting the terminating NUL byte. When requesting parameters, the +size should be set to the size of the buffer to be populated, which +should accomodate enough space for a terminating NUL byte. + When I, it's acceptable for I to be NULL. This can be used by the I to figure out dynamically exactly how much buffer space is needed to store the parameter data. diff --git a/doc/man3/OSSL_PARAM_int.pod b/doc/man3/OSSL_PARAM_int.pod index d637d94f8a..25b87014b7 100644 --- a/doc/man3/OSSL_PARAM_int.pod +++ b/doc/man3/OSSL_PARAM_int.pod @@ -184,8 +184,7 @@ size I is created. OSSL_PARAM_construct_utf8_string() is a function that constructs a UTF8 string OSSL_PARAM structure. A parameter with name I, storage I and size I is created. -If I is zero, the string length is determined using strlen(3) + 1 for the -null termination byte. +If I is zero, the string length is determined using strlen(3). Generally pass zero for I instead of calling strlen(3) yourself. OSSL_PARAM_construct_octet_string() is a function that constructs an OCTET @@ -232,15 +231,18 @@ will be assigned the size the parameter's I buffer should have. OSSL_PARAM_get_utf8_string() retrieves a UTF8 string from the parameter pointed to by I

. -The string is either stored into I<*val> with a length limit of I or, -in the case when I<*val> is NULL, memory is allocated for the string and -I is ignored. +The string is stored into I<*val> with a size limit of I, +which must be large enough to accomodate a terminating NUL byte, +otherwise this function will fail. +If I<*val> is NULL, memory is allocated for the string and I +is ignored. If memory is allocated by this function, it must be freed by the caller. OSSL_PARAM_set_utf8_string() sets a UTF8 string from the parameter pointed to by I

to the value referenced by I. If the parameter's I field is NULL, then only its I field -will be assigned the size the parameter's I buffer should have. +will be assigned the minimum size the parameter's I buffer should have +to accomodate the string, including a terminating NUL byte. OSSL_PARAM_get_octet_string() retrieves an OCTET string from the parameter pointed to by I

. @@ -334,11 +336,11 @@ This example is for setting parameters on some object: #include const char *foo = "some string"; - size_t foo_l = strlen(foo) + 1; + size_t foo_l = strlen(foo); const char bar[] = "some other string"; const OSSL_PARAM set[] = { OSSL_PARAM_utf8_ptr("foo", &foo, foo_l), - OSSL_PARAM_utf8_string("bar", bar, sizeof(bar)), + OSSL_PARAM_utf8_string("bar", bar, sizeof(bar) - 1), OSSL_PARAM_END }; @@ -366,7 +368,7 @@ could fill in the parameters like this: if ((p = OSSL_PARAM_locate(params, "foo")) != NULL) OSSL_PARAM_set_utf8_ptr(p, "foo value"); if ((p = OSSL_PARAM_locate(params, "bar")) != NULL) - OSSL_PARAM_set_utf8_ptr(p, "bar value"); + OSSL_PARAM_set_utf8_string(p, "bar value"); if ((p = OSSL_PARAM_locate(params, "cookie")) != NULL) OSSL_PARAM_set_utf8_ptr(p, "cookie value"); diff --git a/doc/man7/EVP_KDF-SSHKDF.pod b/doc/man7/EVP_KDF-SSHKDF.pod index f0e113c6c8..a2ff902cce 100644 --- a/doc/man7/EVP_KDF-SSHKDF.pod +++ b/doc/man7/EVP_KDF-SSHKDF.pod @@ -124,7 +124,7 @@ This example derives an 8 byte IV using SHA-256 with a 1K "key" and appropriate *p++ = OSSL_PARAM_construct_octet_string(OSSL_KDF_PARAM_SALT, session_id, (size_t)32); *p++ = OSSL_PARAM_construct_utf8_string(OSSL_KDF_PARAM_SSHKDF_TYPE, - type, sizeof(type)); + type, sizeof(type) - 1); *p = OSSL_PARAM_construct_end(); if (EVP_KDF_CTX_set_params(kctx, params) <= 0) /* Error */ diff --git a/test/params_api_test.c b/test/params_api_test.c index 0a68b9c462..2d69b5c327 100644 --- a/test/params_api_test.c +++ b/test/params_api_test.c @@ -538,7 +538,7 @@ static int test_param_construct(void) bufp = NULL; if (!TEST_ptr(cp = OSSL_PARAM_locate(params, "utf8str")) || !TEST_true(OSSL_PARAM_set_utf8_string(cp, "abcdef")) - || !TEST_size_t_eq(cp->return_size, sizeof("abcdef")) + || !TEST_size_t_eq(cp->return_size, sizeof("abcdef") - 1) || !TEST_true(OSSL_PARAM_get_utf8_string(cp, &bufp, 0)) || !TEST_str_eq(bufp, "abcdef")) goto err; @@ -548,10 +548,11 @@ static int test_param_construct(void) || !TEST_str_eq(buf2, "abcdef")) goto err; /* UTF8 pointer */ + /* Note that the size of a UTF8 string does *NOT* include the NUL byte */ bufp = buf; if (!TEST_ptr(cp = OSSL_PARAM_locate(params, "utf8ptr")) || !TEST_true(OSSL_PARAM_set_utf8_ptr(cp, "tuvwxyz")) - || !TEST_size_t_eq(cp->return_size, sizeof("tuvwxyz")) + || !TEST_size_t_eq(cp->return_size, sizeof("tuvwxyz") - 1) || !TEST_str_eq(bufp, "tuvwxyz") || !TEST_true(OSSL_PARAM_get_utf8_ptr(cp, (const char **)&bufp2)) || !TEST_ptr_eq(bufp2, bufp)) diff --git a/test/params_test.c b/test/params_test.c index 9729ab892b..dd2d13b862 100644 --- a/test/params_test.c +++ b/test/params_test.c @@ -141,9 +141,20 @@ static int raw_set_params(void *vobj, const OSSL_PARAM *params) if (!TEST_ptr(obj->p4 = OPENSSL_strndup(params->data, params->data_size))) return 0; + obj->p4_l = strlen(obj->p4); } else if (strcmp(params->key, "p5") == 0) { - strncpy(obj->p5, params->data, params->data_size); - obj->p5_l = strlen(obj->p5) + 1; + /* + * Protect obj->p5 against too much data. This should not + * happen, we don't use that long strings. + */ + size_t data_length = + OPENSSL_strnlen(params->data, params->data_size); + + if (!TEST_size_t_lt(data_length, sizeof(obj->p5))) + return 0; + strncpy(obj->p5, params->data, data_length); + obj->p5[data_length] = '\0'; + obj->p5_l = strlen(obj->p5); } else if (strcmp(params->key, "p6") == 0) { obj->p6 = *(const char **)params->data; obj->p6_l = params->data_size; @@ -164,36 +175,22 @@ static int raw_get_params(void *vobj, OSSL_PARAM *params) params->return_size = sizeof(obj->p2); *(double *)params->data = obj->p2; } else if (strcmp(params->key, "p3") == 0) { - size_t bytes = BN_num_bytes(obj->p3); - - params->return_size = bytes; - if (!TEST_size_t_ge(params->data_size, bytes)) + params->return_size = BN_num_bytes(obj->p3); + if (!TEST_size_t_ge(params->data_size, params->return_size)) return 0; - BN_bn2nativepad(obj->p3, params->data, bytes); + BN_bn2nativepad(obj->p3, params->data, params->return_size); } else if (strcmp(params->key, "p4") == 0) { - size_t bytes = strlen(obj->p4) + 1; - - params->return_size = bytes; - if (!TEST_size_t_ge(params->data_size, bytes)) + params->return_size = strlen(obj->p4); + if (!TEST_size_t_gt(params->data_size, params->return_size)) return 0; strcpy(params->data, obj->p4); } else if (strcmp(params->key, "p5") == 0) { - size_t bytes = strlen(obj->p5) + 1; - - params->return_size = bytes; - if (!TEST_size_t_ge(params->data_size, bytes)) + params->return_size = strlen(obj->p5); + if (!TEST_size_t_gt(params->data_size, params->return_size)) return 0; strcpy(params->data, obj->p5); } else if (strcmp(params->key, "p6") == 0) { - /* - * We COULD also use OPENSSL_FULL_VERSION_STR directly and - * use sizeof(OPENSSL_FULL_VERSION_STR) instead of calling - * strlen(). - * The caller wouldn't know the difference. - */ - size_t bytes = strlen(obj->p6) + 1; - - params->return_size = bytes; + params->return_size = strlen(obj->p6); *(const char **)params->data = obj->p6; } @@ -229,12 +226,12 @@ static int api_set_params(void *vobj, const OSSL_PARAM *params) char *p5_ptr = obj->p5; if (!TEST_true(OSSL_PARAM_get_utf8_string(p, &p5_ptr, sizeof(obj->p5)))) return 0; - obj->p5_l = strlen(obj->p5) + 1; + obj->p5_l = strlen(obj->p5); } if ((p = OSSL_PARAM_locate_const(params, "p6")) != NULL) { if (!TEST_true(OSSL_PARAM_get_utf8_ptr(p, &obj->p6))) return 0; - obj->p6_l = strlen(obj->p6) + 1; + obj->p6_l = strlen(obj->p6); } return 1; @@ -353,8 +350,8 @@ static OSSL_PARAM static_raw_params[] = { { "p3", OSSL_PARAM_UNSIGNED_INTEGER, &bignumbin, sizeof(bignumbin), 0 }, { "p4", OSSL_PARAM_UTF8_STRING, &app_p4, sizeof(app_p4), 0 }, { "p5", OSSL_PARAM_UTF8_STRING, &app_p5, sizeof(app_p5), 0 }, - /* sizeof(app_p6_init), because we know that's what we're using */ - { "p6", OSSL_PARAM_UTF8_PTR, &app_p6, sizeof(app_p6_init), 0 }, + /* sizeof(app_p6_init) - 1, because we know that's what we're using */ + { "p6", OSSL_PARAM_UTF8_PTR, &app_p6, sizeof(app_p6_init) - 1, 0 }, { "foo", OSSL_PARAM_OCTET_STRING, &foo, sizeof(foo), 0 }, { NULL, 0, NULL, 0, 0 } }; @@ -366,7 +363,8 @@ static OSSL_PARAM static_api_params[] = { OSSL_PARAM_DEFN("p4", OSSL_PARAM_UTF8_STRING, &app_p4, sizeof(app_p4)), OSSL_PARAM_DEFN("p5", OSSL_PARAM_UTF8_STRING, &app_p5, sizeof(app_p5)), /* sizeof(app_p6_init), because we know that's what we're using */ - OSSL_PARAM_DEFN("p6", OSSL_PARAM_UTF8_PTR, &app_p6, sizeof(app_p6_init)), + OSSL_PARAM_DEFN("p6", OSSL_PARAM_UTF8_PTR, &app_p6, + sizeof(app_p6_init) - 1), OSSL_PARAM_DEFN("foo", OSSL_PARAM_OCTET_STRING, &foo, sizeof(foo)), OSSL_PARAM_END }; @@ -461,10 +459,12 @@ static int test_case_variant(OSSL_PARAM *params, const struct provider_dispatch_ || !TEST_BN_eq(app_p3, verify_p3) /* "provider" value */ || !TEST_str_eq(app_p4, p4_init) /* "provider" value */ || !TEST_ptr(p = OSSL_PARAM_locate(params, "p5")) - || !TEST_size_t_eq(p->return_size, sizeof(p5_init)) /* "provider" value */ + || !TEST_size_t_eq(p->return_size, + sizeof(p5_init) - 1) /* "provider" value */ || !TEST_str_eq(app_p5, p5_init) /* "provider" value */ || !TEST_ptr(p = OSSL_PARAM_locate(params, "p6")) - || !TEST_size_t_eq(p->return_size, sizeof(p6_init)) /* "provider" value */ + || !TEST_size_t_eq(p->return_size, + sizeof(p6_init) - 1) /* "provider" value */ || !TEST_str_eq(app_p6, p6_init) /* "provider" value */ || !TEST_char_eq(foo[0], app_foo_init) /* Should remain untouched */ || !TEST_ptr(p = OSSL_PARAM_locate(params, "foo"))) @@ -511,11 +511,11 @@ static int test_case_variant(OSSL_PARAM *params, const struct provider_dispatch_ || !TEST_str_eq(app_p4, app_p4_init) /* app value */ || !TEST_ptr(p = OSSL_PARAM_locate(params, "p5")) || !TEST_size_t_eq(p->return_size, - sizeof(app_p5_init)) /* app value */ + sizeof(app_p5_init) - 1) /* app value */ || !TEST_str_eq(app_p5, app_p5_init) /* app value */ || !TEST_ptr(p = OSSL_PARAM_locate(params, "p6")) || !TEST_size_t_eq(p->return_size, - sizeof(app_p6_init)) /* app value */ + sizeof(app_p6_init) - 1) /* app value */ || !TEST_str_eq(app_p6, app_p6_init) /* app value */ || !TEST_char_eq(foo[0], app_foo_init) /* Should remain untouched */ || !TEST_ptr(p = OSSL_PARAM_locate(params, "foo"))) -- cgit v1.2.3