mirror/openssl
2
0
Fork 0
mirror of https://github.com/openssl/openssl.git synced 2025-02-17 14:32:04 +08:00
Code Issues Packages Projects Releases Wiki Activity

OSSL_PARAM: Correct the assumptions on the UTF8 string length

Browse Source 
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 <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/14168)
This commit is contained in:
Richard Levitte 2021-02-12 20:30:40 +01:00
parent c1be4d617c
commit 247a1786e2
6 changed files with 98 additions and 59 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

57
crypto/params.c
View File

@ -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,

7
doc/man3/OSSL_PARAM.pod
View File

@ -97,6 +97,13 @@ setting parameters) or shall (when requesting parameters) be stored,
and I<data_size> is its size in bytes.
The organization of the data depends on the parameter type and flag.
The I<data_size> needs special attention with the parameter type
B<OSSL_PARAM_UTF8_STRING> 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<requesting parameters>, it's acceptable for I<data> to be NULL.
This can be used by the I<requestor> to figure out dynamically exactly
how much buffer space is needed to store the parameter data.

20
doc/man3/OSSL_PARAM_int.pod
View File

@ -184,8 +184,7 @@ size I<rsize> is created.
OSSL_PARAM_construct_utf8_string() is a function that constructs a UTF8
string OSSL_PARAM structure.
A parameter with name I<key>, storage I<buf> and size I<bsize> is created.
If I<bsize> is zero, the string length is determined using strlen(3) + 1 for the
null termination byte.
If I<bsize> is zero, the string length is determined using strlen(3).
Generally pass zero for I<bsize> 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<data> buffer should have.
OSSL_PARAM_get_utf8_string() retrieves a UTF8 string from the parameter
pointed to by I<p>.
The string is either stored into I<*val> with a length limit of I<max_len> or,
in the case when I<*val> is NULL, memory is allocated for the string and
I<max_len> is ignored.
The string is stored into I<*val> with a size limit of I<max_len>,
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<max_len>
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<p> to the value referenced by I<val>.
If the parameter's I<data> field is NULL, then only its I<return_size> field
will be assigned the size the parameter's I<data> buffer should have.
will be assigned the minimum size the parameter's I<data> 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<p>.
@ -334,11 +336,11 @@ This example is for setting parameters on some object:
#include <openssl/core.h>
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");

2
doc/man7/EVP_KDF-SSHKDF.pod
View File

@ -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 */

5
test/params_api_test.c
View File

@ -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))

66
test/params_test.c
View File

@ -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")))