From 54e161b9dabe4bd3b08b532475294a37c7562b45 Mon Sep 17 00:00:00 2001 From: Daiki Ueno Date: Tue, 9 Mar 2021 13:07:26 +0100 Subject: gnutls_buffer_append_data: remove duplicated code The function shared the same logic as in _gnutls_buffer_resize. Signed-off-by: Daiki Ueno --- lib/str.c | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/lib/str.c b/lib/str.c index e31449937d..2247fc322b 100644 --- a/lib/str.c +++ b/lib/str.c @@ -113,7 +113,7 @@ gnutls_buffer_append_data(gnutls_buffer_t dest, const void *data, size_t data_size) { size_t const tot_len = data_size + dest->length; - size_t const unused = MEMSUB(dest->data, dest->allocd); + int ret; if (unlikely(dest->data != NULL && dest->allocd == NULL)) return gnutls_assert_val(GNUTLS_E_INVALID_REQUEST); @@ -126,25 +126,9 @@ gnutls_buffer_append_data(gnutls_buffer_t dest, const void *data, return gnutls_assert_val(GNUTLS_E_MEMORY_ERROR); } - if (dest->max_length >= tot_len) { - - if (dest->max_length - unused <= tot_len) { - align_allocd_with_data(dest); - } - } else { - size_t const new_len = - MAX(data_size, MIN_CHUNK) + MAX(dest->max_length, - MIN_CHUNK); - - dest->allocd = gnutls_realloc_fast(dest->allocd, new_len); - if (dest->allocd == NULL) { - gnutls_assert(); - return GNUTLS_E_MEMORY_ERROR; - } - dest->max_length = new_len; - dest->data = dest->allocd + unused; - - align_allocd_with_data(dest); + ret = _gnutls_buffer_resize(dest, tot_len); + if (ret < 0) { + return ret; } assert(dest->data != NULL); -- cgit v1.2.1 From 65695373e10ed0654bff60fe0dcd00137ddfffe8 Mon Sep 17 00:00:00 2001 From: Daiki Ueno Date: Tue, 9 Mar 2021 13:41:59 +0100 Subject: _gnutls_buffer_resize: add option to use allocation simpler logic This helps detect common mistakes[1] in realloc usage with valgrind, where the caller assumes that the original ptr is always returned. 1. https://bugzilla.mozilla.org/show_bug.cgi?id=1377618 Signed-off-by: Daiki Ueno Co-authored-by: Alexander Sosedkin --- lib/str.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/lib/str.c b/lib/str.c index 2247fc322b..506fe17210 100644 --- a/lib/str.c +++ b/lib/str.c @@ -138,6 +138,36 @@ gnutls_buffer_append_data(gnutls_buffer_t dest, const void *data, return 0; } +#ifdef AGGRESSIVE_REALLOC + +/* Use a simpler logic for reallocation; i.e., always call + * gnutls_realloc_fast() and do not reclaim the no-longer-used + * area which has been removed from the beginning of buffer + * with _gnutls_buffer_pop_datum(). This helps hit more + * issues when running under valgrind. + */ +int _gnutls_buffer_resize(gnutls_buffer_st * dest, size_t new_size) +{ + size_t unused; + + if (unlikely(dest->data != NULL && dest->allocd == NULL)) + return gnutls_assert_val(GNUTLS_E_INVALID_REQUEST); + + unused = MEMSUB(dest->data, dest->allocd); + dest->allocd = + gnutls_realloc_fast(dest->allocd, new_size); + if (dest->allocd == NULL) { + gnutls_assert(); + return GNUTLS_E_MEMORY_ERROR; + } + dest->max_length = new_size; + dest->data = dest->allocd + unused; + + return 0; +} + +#else + int _gnutls_buffer_resize(gnutls_buffer_st * dest, size_t new_size) { if (unlikely(dest->data != NULL && dest->allocd == NULL)) @@ -171,6 +201,8 @@ int _gnutls_buffer_resize(gnutls_buffer_st * dest, size_t new_size) } } +#endif + /* Appends the provided string. The null termination byte is appended * but not included in length. */ -- cgit v1.2.1 From 254ee88e6f20975604b9247d1764c3fcbac448ee Mon Sep 17 00:00:00 2001 From: Daiki Ueno Date: Fri, 29 Jan 2021 14:06:32 +0100 Subject: key_share: avoid use-after-free around realloc Signed-off-by: Daiki Ueno --- lib/ext/key_share.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/ext/key_share.c b/lib/ext/key_share.c index ab8abf8fe6..a8c4bb5cff 100644 --- a/lib/ext/key_share.c +++ b/lib/ext/key_share.c @@ -664,14 +664,14 @@ key_share_send_params(gnutls_session_t session, { unsigned i; int ret; - unsigned char *lengthp; - unsigned int cur_length; unsigned int generated = 0; const gnutls_group_entry_st *group; const version_entry_st *ver; /* this extension is only being sent on client side */ if (session->security_parameters.entity == GNUTLS_CLIENT) { + unsigned int length_pos; + ver = _gnutls_version_max(session); if (unlikely(ver == NULL || ver->key_shares == 0)) return 0; @@ -679,16 +679,13 @@ key_share_send_params(gnutls_session_t session, if (!have_creds_for_tls13(session)) return 0; - /* write the total length later */ - lengthp = &extdata->data[extdata->length]; + length_pos = extdata->length; ret = _gnutls_buffer_append_prefix(extdata, 16, 0); if (ret < 0) return gnutls_assert_val(ret); - cur_length = extdata->length; - if (session->internals.hsk_flags & HSK_HRR_RECEIVED) { /* we know the group */ group = get_group(session); if (unlikely(group == NULL)) @@ -736,7 +733,8 @@ key_share_send_params(gnutls_session_t session, } /* copy actual length */ - _gnutls_write_uint16(extdata->length - cur_length, lengthp); + _gnutls_write_uint16(extdata->length - length_pos - 2, + &extdata->data[length_pos]); } else { /* server */ ver = get_version(session); -- cgit v1.2.1 From 9bdb7bb60ebddaa8f167e5cfae317dbf202f0e67 Mon Sep 17 00:00:00 2001 From: Daiki Ueno Date: Fri, 29 Jan 2021 14:06:50 +0100 Subject: pre_shared_key: avoid use-after-free around realloc Signed-off-by: Daiki Ueno --- lib/ext/pre_shared_key.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/lib/ext/pre_shared_key.c b/lib/ext/pre_shared_key.c index 240be21625..c66b40e9fd 100644 --- a/lib/ext/pre_shared_key.c +++ b/lib/ext/pre_shared_key.c @@ -267,7 +267,7 @@ client_send_params(gnutls_session_t session, size_t spos; gnutls_datum_t username = {NULL, 0}; gnutls_datum_t user_key = {NULL, 0}, rkey = {NULL, 0}; - gnutls_datum_t client_hello; + unsigned client_hello_len; unsigned next_idx; const mac_entry_st *prf_res = NULL; const mac_entry_st *prf_psk = NULL; @@ -430,8 +430,7 @@ client_send_params(gnutls_session_t session, assert(extdata->length >= sizeof(mbuffer_st)); assert(ext_offset >= (ssize_t)sizeof(mbuffer_st)); ext_offset -= sizeof(mbuffer_st); - client_hello.data = extdata->data+sizeof(mbuffer_st); - client_hello.size = extdata->length-sizeof(mbuffer_st); + client_hello_len = extdata->length-sizeof(mbuffer_st); next_idx = 0; @@ -442,6 +441,11 @@ client_send_params(gnutls_session_t session, } if (prf_res && rkey.size > 0) { + gnutls_datum_t client_hello; + + client_hello.data = extdata->data+sizeof(mbuffer_st); + client_hello.size = client_hello_len; + ret = compute_psk_binder(session, prf_res, binders_len, binders_pos, ext_offset, &rkey, &client_hello, 1, @@ -476,6 +480,11 @@ client_send_params(gnutls_session_t session, } if (prf_psk && user_key.size > 0 && info) { + gnutls_datum_t client_hello; + + client_hello.data = extdata->data+sizeof(mbuffer_st); + client_hello.size = client_hello_len; + ret = compute_psk_binder(session, prf_psk, binders_len, binders_pos, ext_offset, &user_key, &client_hello, 0, -- cgit v1.2.1 From 8de3bd90e1d958147331882a51263216b5ea96d2 Mon Sep 17 00:00:00 2001 From: Daiki Ueno Date: Wed, 10 Mar 2021 16:11:29 +0100 Subject: _gnutls_buffer_resize: account for unused area if AGGRESSIVE_REALLOC Signed-off-by: Daiki Ueno --- lib/str.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/str.c b/lib/str.c index 506fe17210..bc20ebb04f 100644 --- a/lib/str.c +++ b/lib/str.c @@ -155,12 +155,12 @@ int _gnutls_buffer_resize(gnutls_buffer_st * dest, size_t new_size) unused = MEMSUB(dest->data, dest->allocd); dest->allocd = - gnutls_realloc_fast(dest->allocd, new_size); + gnutls_realloc_fast(dest->allocd, new_size + unused); if (dest->allocd == NULL) { gnutls_assert(); return GNUTLS_E_MEMORY_ERROR; } - dest->max_length = new_size; + dest->max_length = new_size + unused; dest->data = dest->allocd + unused; return 0; -- cgit v1.2.1 From dc07b8228b4bab83af5bcf7a2efd9d2a0f10f628 Mon Sep 17 00:00:00 2001 From: Daiki Ueno Date: Wed, 10 Mar 2021 16:12:23 +0100 Subject: str: suppress -Wunused-function if AGGRESSIVE_REALLOC is defined Signed-off-by: Daiki Ueno --- lib/str.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/str.c b/lib/str.c index bc20ebb04f..8007340f1e 100644 --- a/lib/str.c +++ b/lib/str.c @@ -87,15 +87,6 @@ void _gnutls_buffer_clear(gnutls_buffer_st * str) #define MIN_CHUNK 1024 -static void align_allocd_with_data(gnutls_buffer_st * dest) -{ - assert(dest->allocd != NULL); - assert(dest->data != NULL); - if (dest->length) - memmove(dest->allocd, dest->data, dest->length); - dest->data = dest->allocd; -} - /** * gnutls_buffer_append_data: * @dest: the buffer to append to @@ -168,6 +159,15 @@ int _gnutls_buffer_resize(gnutls_buffer_st * dest, size_t new_size) #else +static void align_allocd_with_data(gnutls_buffer_st * dest) +{ + assert(dest->allocd != NULL); + assert(dest->data != NULL); + if (dest->length) + memmove(dest->allocd, dest->data, dest->length); + dest->data = dest->allocd; +} + int _gnutls_buffer_resize(gnutls_buffer_st * dest, size_t new_size) { if (unlikely(dest->data != NULL && dest->allocd == NULL)) -- cgit v1.2.1