From 6fbff7fc8aabeee2254405f254220bbe8c05c67d Mon Sep 17 00:00:00 2001 From: Daiki Ueno Date: Fri, 5 Jun 2020 16:26:33 +0200 Subject: [PATCH] crypto-api: always allocate memory when serializing iovec_t The AEAD iov interface falls back to serializing the input buffers if the low-level cipher doesn't support scatter/gather encryption. However, there was a bug in the functions used for the serialization, which causes memory leaks under a certain condition (i.e. the number of input buffers is 1). This patch makes the logic of the functions simpler, by removing a micro-optimization that tries to minimize the number of calls to malloc/free. The original problem was reported by Marius Steffen in: https://bugzilla.samba.org/show_bug.cgi?id=14399 and the cause was investigated by Alexander Haase in: https://gitlab.com/gnutls/gnutls/-/merge_requests/1277 Signed-off-by: Daiki Ueno --- lib/crypto-api.c | 36 +++++++++++------------------------- tests/aead-cipher-vec.c | 33 ++++++++++++++++++--------------- 2 files changed, 29 insertions(+), 40 deletions(-) diff --git a/lib/crypto-api.c b/lib/crypto-api.c index 45be64ed1..8524f5ed4 100644 --- a/lib/crypto-api.c +++ b/lib/crypto-api.c @@ -891,32 +891,23 @@ gnutls_aead_cipher_encrypt(gnutls_aead_cipher_hd_t handle, struct iov_store_st { void *data; size_t size; - unsigned allocated; }; static void iov_store_free(struct iov_store_st *s) { - if (s->allocated) { - gnutls_free(s->data); - s->allocated = 0; - } + gnutls_free(s->data); } static int iov_store_grow(struct iov_store_st *s, size_t length) { - if (s->allocated || s->data == NULL) { - s->size += length; - s->data = gnutls_realloc(s->data, s->size); - if (s->data == NULL) - return gnutls_assert_val(GNUTLS_E_MEMORY_ERROR); - s->allocated = 1; - } else { - void *data = s->data; - size_t size = s->size + length; - s->data = gnutls_malloc(size); - memcpy(s->data, data, s->size); - s->size += length; - } + void *data; + + s->size += length; + data = gnutls_realloc(s->data, s->size); + if (data == NULL) + return gnutls_assert_val(GNUTLS_E_MEMORY_ERROR); + + s->data = data; return 0; } @@ -926,11 +917,6 @@ copy_from_iov(struct iov_store_st *dst, const giovec_t *iov, int iovcnt) memset(dst, 0, sizeof(*dst)); if (iovcnt == 0) { return 0; - } else if (iovcnt == 1) { - dst->data = iov[0].iov_base; - dst->size = iov[0].iov_len; - /* implies: dst->allocated = 0; */ - return 0; } else { int i; uint8_t *p; @@ -944,11 +930,11 @@ copy_from_iov(struct iov_store_st *dst, const giovec_t *iov, int iovcnt) p = dst->data; for (i=0;i 0) + memcpy(p, iov[i].iov_base, iov[i].iov_len); p += iov[i].iov_len; } - dst->allocated = 1; return 0; } } diff --git a/tests/aead-cipher-vec.c b/tests/aead-cipher-vec.c index fba9010d9..6a30a35f7 100644 --- a/tests/aead-cipher-vec.c +++ b/tests/aead-cipher-vec.c @@ -49,6 +49,7 @@ static void start(const char *name, int algo) giovec_t auth_iov[2]; uint8_t tag[64]; size_t tag_size = 0; + size_t i; key.data = key16; key.size = gnutls_cipher_get_key_size(algo); @@ -82,21 +83,23 @@ static void start(const char *name, int algo) if (ret < 0) fail("gnutls_cipher_init: %s\n", gnutls_strerror(ret)); - ret = gnutls_aead_cipher_encryptv2(ch, - iv.data, iv.size, - auth_iov, 2, - iov, 3, - tag, &tag_size); - if (ret < 0) - fail("could not encrypt data: %s\n", gnutls_strerror(ret)); - - ret = gnutls_aead_cipher_decryptv2(ch, - iv.data, iv.size, - auth_iov, 2, - iov, 3, - tag, tag_size); - if (ret < 0) - fail("could not decrypt data: %s\n", gnutls_strerror(ret)); + for (i = 0; i < 2; i++) { + ret = gnutls_aead_cipher_encryptv2(ch, + iv.data, iv.size, + auth_iov, 2, + iov, i + 1, + tag, &tag_size); + if (ret < 0) + fail("could not encrypt data: %s\n", gnutls_strerror(ret)); + + ret = gnutls_aead_cipher_decryptv2(ch, + iv.data, iv.size, + auth_iov, 2, + iov, i + 1, + tag, tag_size); + if (ret < 0) + fail("could not decrypt data: %s\n", gnutls_strerror(ret)); + } gnutls_aead_cipher_deinit(ch); } -- 2.25.4