Merge branch 'wip/dueno/realloc-3.6.x' into 'gnutls_3_6_x'

key_share, pre_shared_key: avoid use-after-free around realloc [3.6.x]

See merge request gnutls/gnutls!1408

3 files changed, 62 insertions, 39 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);
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,
diff --git a/lib/str.c b/lib/str.c
index e31449937d..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
@@ -113,7 +104,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 +117,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);
 
@@ -154,6 +129,45 @@ 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 + unused);
+	if (dest->allocd == NULL) {
+		gnutls_assert();
+		return GNUTLS_E_MEMORY_ERROR;
+	}
+	dest->max_length = new_size + unused;
+	dest->data = dest->allocd + unused;
+
+	return 0;
+}
+
+#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))
@@ -187,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.
  */