Hi Pádraig,
Collin Funk <[email protected]> writes:
>> So in summary, I'd revert the previous patch,
>> and we'll figure out how to proceed from there.
>
> Thanks, I'll have a look later today. I'll probably just move SHA-3 to
> returning bool for errors (e.g. OOM). In coreutils we can xalloc_die, in
> libraries they decision on how to handle that can be left up to them.
Thanks again for noticing this.
What do you think of the attached patch?
# Before patch
$ valgrind ./src/cksum -a sha3 -l 256 Makefile Makefile.am \
COPYING README 2>&1 | grep -F 'definitely lost'
==148531== definitely lost: 1,664 bytes in 4 blocks
# After patch
$ valgrind ./src/cksum -a sha3 -l 256 Makefile Makefile.am \
COPYING README 2>&1 | grep -F 'definitely lost'
==151416== definitely lost: 0 bytes in 0 blocks
Since OpenSSL is just going to allocate a bunch of memory anyways, there
is no point in trying to avoid EVP_MD_CTX_create, in my opinion. It also
makes the module more library friendly, which should hopefully alleviate
Simon's concerns. And makes it so we don't have to worry about
EVP_MD_CTX increasing in future versions of OpenSSL, however unlikely
that is.
For the purposes of Coreutils, we just have to add an extra xalloc_die
ourselves, which is no big deal.
Collin
>From bfbfac66b8fb954ba3e9a611b30c7406d5c41c86 Mon Sep 17 00:00:00 2001
Message-ID: <bfbfac66b8fb954ba3e9a611b30c7406d5c41c86.1757125942.git.collin.fu...@gmail.com>
From: Collin Funk <[email protected]>
Date: Fri, 5 Sep 2025 19:19:23 -0700
Subject: [PATCH] crypto/sha3, crypto/sha3-buffer: Don't leak memory when using
OpenSSL.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Reported by Pádraig Brady in:
<https://lists.gnu.org/archive/html/bug-gnulib/2025-09/msg00058.html>.
* lib/sha3.c (DEFINE_SHA3_INIT_CTX) [!HAVE_OPENSSL_SHA3]: Always return
true.
[HAVE_OPENSSL_SHA3]: Call EVP_MD_CTX_create to allocate an EVP_MD_CTX.
(DEFINE_SHA3_BUFFER, sha3_process_bytes, sha3_process_block)
[!HAVE_OPENSSL_SHA3]: Always return true.
[HAVE_OPENSSL_SHA3]: Return NULL if any function calls fail.
(sha3_finish_ctx) [HAVE_OPENSSL_SHA3]: Free memory allocated by
EV_MD_CTX_create.
(sha3_free_ctx): New function.
* lib/sha3.h (struct sha3_ctx): Use a heap allocated EVP_MD_CTX.
(sha3_224_init_ctx, sha3_256_init_ctx, sha3_384_init_ctx)
(sha3_512_init_ctx, sha3_process_block, sha3_process_bytes): Change
prototype to return a bool. Mention that they return false if an OpenSSL
function fails.
(sha3_finish_ctx, sha3_read_ctx, sha3_224_buffer, sha3_256_buffer)
(sha3_384_buffer, sha3_512_buffer): Mention that these functions return
NULL if an OpenSSL function fails.
(sha3_free_ctx): New function.
* lib/sha3-stream.c (sha3_xxx_stream): Expect a function parameter that
returns bool. Check the return values of the sha3 functions, cleaning up
memory on failure.
* modules/crypto/sha3-buffer: Add bool.
---
ChangeLog | 26 ++++++++++++++
lib/sha3-stream.c | 34 ++++++++++++++----
lib/sha3.c | 72 +++++++++++++++++++++++++-------------
lib/sha3.h | 36 +++++++++++--------
modules/crypto/sha3-buffer | 1 +
5 files changed, 125 insertions(+), 44 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 7e019dfb78..03fe2b514a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,31 @@
2025-09-05 Collin Funk <[email protected]>
+ crypto/sha3, crypto/sha3-buffer: Don't leak memory when using OpenSSL.
+ Reported by Pádraig Brady in:
+ <https://lists.gnu.org/archive/html/bug-gnulib/2025-09/msg00058.html>.
+ * lib/sha3.c (DEFINE_SHA3_INIT_CTX) [!HAVE_OPENSSL_SHA3]: Always return
+ true.
+ [HAVE_OPENSSL_SHA3]: Call EVP_MD_CTX_create to allocate an EVP_MD_CTX.
+ (DEFINE_SHA3_BUFFER, sha3_process_bytes, sha3_process_block)
+ [!HAVE_OPENSSL_SHA3]: Always return true.
+ [HAVE_OPENSSL_SHA3]: Return NULL if any function calls fail.
+ (sha3_finish_ctx) [HAVE_OPENSSL_SHA3]: Free memory allocated by
+ EV_MD_CTX_create.
+ (sha3_free_ctx): New function.
+ * lib/sha3.h (struct sha3_ctx): Use a heap allocated EVP_MD_CTX.
+ (sha3_224_init_ctx, sha3_256_init_ctx, sha3_384_init_ctx)
+ (sha3_512_init_ctx, sha3_process_block, sha3_process_bytes): Change
+ prototype to return a bool. Mention that they return false if an OpenSSL
+ function fails.
+ (sha3_finish_ctx, sha3_read_ctx, sha3_224_buffer, sha3_256_buffer)
+ (sha3_384_buffer, sha3_512_buffer): Mention that these functions return
+ NULL if an OpenSSL function fails.
+ (sha3_free_ctx): New function.
+ * lib/sha3-stream.c (sha3_xxx_stream): Expect a function parameter that
+ returns bool. Check the return values of the sha3 functions, cleaning up
+ memory on failure.
+ * modules/crypto/sha3-buffer: Add bool.
+
quotearg: Remove an unused variable.
* lib/quotearg.c (gettext_quote): Remove a variable that is no longer
used after yesterdays changes.
diff --git a/lib/sha3-stream.c b/lib/sha3-stream.c
index 708dafa7dc..43c056e22a 100644
--- a/lib/sha3-stream.c
+++ b/lib/sha3-stream.c
@@ -41,7 +41,7 @@
successful. */
static int
sha3_xxx_stream (FILE *stream, char const *alg, void *resblock,
- ssize_t hashlen, void (*init_ctx) (struct sha3_ctx *))
+ ssize_t hashlen, bool (*init_ctx) (struct sha3_ctx *))
{
switch (afalg_stream (stream, alg, resblock, hashlen))
{
@@ -54,7 +54,11 @@ sha3_xxx_stream (FILE *stream, char const *alg, void *resblock,
return 1;
struct sha3_ctx ctx;
- init_ctx (&ctx);
+ if (! init_ctx (&ctx))
+ {
+ free (buffer);
+ return 1;
+ }
size_t sum;
/* Iterate over full file contents. */
@@ -92,6 +96,7 @@ sha3_xxx_stream (FILE *stream, char const *alg, void *resblock,
if (ferror (stream))
{
free (buffer);
+ sha3_free_ctx (&ctx);
return 1;
}
goto process_partial_block;
@@ -101,18 +106,35 @@ sha3_xxx_stream (FILE *stream, char const *alg, void *resblock,
/* Process buffer with BLOCKSIZE bytes. Note that
BLOCKSIZE % ctx.blocklen == 0
*/
- sha3_process_block (buffer, BLOCKSIZE, &ctx);
+ if (! sha3_process_block (buffer, BLOCKSIZE, &ctx))
+ {
+ free (buffer);
+ sha3_free_ctx (&ctx);
+ return 1;
+ }
}
process_partial_block:;
/* Process any remaining bytes. */
if (sum > 0)
- sha3_process_bytes (buffer, sum, &ctx);
+ {
+ if (! sha3_process_bytes (buffer, sum, &ctx))
+ {
+ free (buffer);
+ sha3_free_ctx (&ctx);
+ return 1;
+ }
+ }
- /* Construct result in desired memory. */
- sha3_finish_ctx (&ctx, resblock);
free (buffer);
+
+ /* Construct result in desired memory. */
+ if (sha3_finish_ctx (&ctx, resblock) == NULL)
+ {
+ sha3_free_ctx (&ctx);
+ return 1;
+ }
return 0;
}
diff --git a/lib/sha3.c b/lib/sha3.c
index e30210fb3e..e7b3b9c7c6 100644
--- a/lib/sha3.c
+++ b/lib/sha3.c
@@ -50,13 +50,14 @@ static const u64 rc[] = {
};
#define DEFINE_SHA3_INIT_CTX(SIZE) \
- void \
+ bool \
sha3_##SIZE##_init_ctx (struct sha3_ctx *ctx) \
{ \
memset (&ctx->state, '\0', sizeof ctx->state); \
ctx->buflen = 0; \
ctx->digestlen = SHA3_##SIZE##_DIGEST_SIZE; \
ctx->blocklen = SHA3_##SIZE##_BLOCK_SIZE; \
+ return true; \
}
DEFINE_SHA3_INIT_CTX (224)
@@ -64,6 +65,12 @@ DEFINE_SHA3_INIT_CTX (256)
DEFINE_SHA3_INIT_CTX (384)
DEFINE_SHA3_INIT_CTX (512)
+void
+sha3_free_ctx (_GL_UNUSED struct sha3_ctx *ctx)
+{
+ /* Do nothing. */
+}
+
/* Copy the value from V into the memory location pointed to by *CP,
If your architecture allows unaligned access, this is equivalent to
* (__typeof__ (v) *) cp = v */
@@ -127,7 +134,7 @@ DEFINE_SHA3_BUFFER (256)
DEFINE_SHA3_BUFFER (384)
DEFINE_SHA3_BUFFER (512)
-void
+bool
sha3_process_bytes (const void *buffer, size_t len, struct sha3_ctx *ctx)
{
if (0 < ctx->buflen)
@@ -138,7 +145,7 @@ sha3_process_bytes (const void *buffer, size_t len, struct sha3_ctx *ctx)
/* Not enough to fill a full block. */
memcpy (ctx->buffer + ctx->buflen, buffer, len);
ctx->buflen += len;
- return;
+ return true;
}
/* Process the block that already had bytes buffered. */
memcpy (ctx->buffer + ctx->buflen, buffer, left);
@@ -156,9 +163,10 @@ sha3_process_bytes (const void *buffer, size_t len, struct sha3_ctx *ctx)
memcpy (ctx->buffer, buffer, len);
ctx->buflen = len;
}
+ return true;
}
-void
+bool
sha3_process_block (const void *buffer, size_t len, struct sha3_ctx *ctx)
{
u64 *a = ctx->state;
@@ -315,21 +323,27 @@ sha3_process_block (const void *buffer, size_t len, struct sha3_ctx *ctx)
a[0] = u64xor (a[0], rc[i]);
}
}
+ return true;
}
#else /* OpenSSL implementation. */
#define DEFINE_SHA3_INIT_CTX(SIZE) \
- void \
+ bool \
sha3_##SIZE##_init_ctx (struct sha3_ctx *ctx) \
{ \
- /* EVP_DigestInit_ex expects all bytes to be zero. */ \
- memset (ctx, 0, sizeof *ctx); \
- EVP_MD_CTX *evp_ctx = (EVP_MD_CTX *) ctx->evp_ctx_buffer; \
- int rc = EVP_DigestInit_ex (evp_ctx, EVP_sha3_##SIZE (), NULL); \
- /* This should never fail. */ \
- if (rc == 0) \
- abort (); \
+ int result; \
+ ctx->evp_ctx = EVP_MD_CTX_create (); \
+ if (ctx->evp_ctx == NULL) \
+ return false; \
+ result = EVP_DigestInit_ex (ctx->evp_ctx, EVP_sha3_##SIZE (), \
+ NULL); \
+ if (result == 0) \
+ { \
+ sha3_free_ctx (ctx); \
+ return false; \
+ } \
+ return true; \
}
DEFINE_SHA3_INIT_CTX (224)
@@ -337,6 +351,12 @@ DEFINE_SHA3_INIT_CTX (256)
DEFINE_SHA3_INIT_CTX (384)
DEFINE_SHA3_INIT_CTX (512)
+void
+sha3_free_ctx (struct sha3_ctx *ctx)
+{
+ EVP_MD_CTX_free (ctx->evp_ctx);
+}
+
void *
sha3_read_ctx (const struct sha3_ctx *ctx, void *resbuf)
{
@@ -347,11 +367,10 @@ sha3_read_ctx (const struct sha3_ctx *ctx, void *resbuf)
void *
sha3_finish_ctx (struct sha3_ctx *ctx, void *resbuf)
{
- EVP_MD_CTX *evp_ctx = (EVP_MD_CTX *) ctx->evp_ctx_buffer;
- /* This should never fail. */
- int result = EVP_DigestFinal_ex (evp_ctx, resbuf, NULL);
+ int result = EVP_DigestFinal_ex (ctx->evp_ctx, resbuf, NULL);
+ sha3_free_ctx (ctx);
if (result == 0)
- abort ();
+ return NULL;
return resbuf;
}
@@ -360,8 +379,10 @@ sha3_finish_ctx (struct sha3_ctx *ctx, void *resbuf)
sha3_##SIZE##_buffer (const char *buffer, size_t len, void *resblock) \
{ \
struct sha3_ctx ctx; \
- sha3_##SIZE##_init_ctx (&ctx); \
- sha3_process_bytes (buffer, len, &ctx); \
+ if (! sha3_##SIZE##_init_ctx (&ctx)) \
+ return NULL; \
+ if (! sha3_process_bytes (buffer, len, &ctx)) \
+ return NULL; \
return sha3_finish_ctx (&ctx, resblock); \
}
@@ -370,19 +391,22 @@ DEFINE_SHA3_BUFFER (256)
DEFINE_SHA3_BUFFER (384)
DEFINE_SHA3_BUFFER (512)
-void
+bool
sha3_process_bytes (const void *buffer, size_t len, struct sha3_ctx *ctx)
{
- EVP_MD_CTX *evp_ctx = (EVP_MD_CTX *) ctx->evp_ctx_buffer;
- int result = EVP_DigestUpdate (evp_ctx, buffer, len);
+ int result = EVP_DigestUpdate (ctx->evp_ctx, buffer, len);
if (result == 0)
- abort ();
+ {
+ sha3_free_ctx (ctx);
+ return false;
+ }
+ return true;
}
-void
+bool
sha3_process_block (const void *buffer, size_t len, struct sha3_ctx *ctx)
{
- sha3_process_bytes (buffer, len, ctx);
+ return sha3_process_bytes (buffer, len, ctx);
}
#endif
diff --git a/lib/sha3.h b/lib/sha3.h
index adce43eac7..259f0d1f56 100644
--- a/lib/sha3.h
+++ b/lib/sha3.h
@@ -51,9 +51,8 @@ enum { SHA3_512_BLOCK_SIZE = 576 / 8 };
struct sha3_ctx
{
# if HAVE_OPENSSL_SHA3
- /* EVP_MD_CTX is an incomplete type. EVP_MD_CTX_create allocates 72 bytes of
- memory as of 2025-09-02. */
- max_align_t evp_ctx_buffer[256 / sizeof (max_align_t)];
+ /* EVP_MD_CTX is an incomplete type. It cannot be placed on the stack. */
+ EVP_MD_CTX *evp_ctx;
# else
u64 state[25];
uint8_t buffer[144]; /* Up to BLOCKLEN in use. */
@@ -64,40 +63,49 @@ struct sha3_ctx
};
/* Initialize structure containing state of computation. */
-extern void sha3_224_init_ctx (struct sha3_ctx *ctx);
-extern void sha3_256_init_ctx (struct sha3_ctx *ctx);
-extern void sha3_384_init_ctx (struct sha3_ctx *ctx);
-extern void sha3_512_init_ctx (struct sha3_ctx *ctx);
+extern bool sha3_224_init_ctx (struct sha3_ctx *ctx);
+extern bool sha3_256_init_ctx (struct sha3_ctx *ctx);
+extern bool sha3_384_init_ctx (struct sha3_ctx *ctx);
+extern bool sha3_512_init_ctx (struct sha3_ctx *ctx);
+
+/* Free memory allocated by the init_structure. This is only required if
+ OpenSSL is used. */
+extern void sha3_free_ctx (struct sha3_ctx *ctx);
/* Starting with the result of former calls of this function (or the
initialization function update the context for the next LEN bytes
starting at BUFFER.
- It is necessary that LEN is a multiple of the BLOCKLEN member of CTX!!! */
-extern void sha3_process_block (const void *buffer, size_t len,
+ It is necessary that LEN is a multiple of the BLOCKLEN member of CTX!!!
+ Return false if an OpenSSL function fails. */
+extern bool sha3_process_block (const void *buffer, size_t len,
struct sha3_ctx *ctx);
/* Starting with the result of former calls of this function (or the
initialization function update the context for the next LEN bytes
starting at BUFFER.
- It is NOT required that LEN is a multiple of the BLOCKLEN member of CTX. */
-extern void sha3_process_bytes (const void *buffer, size_t len,
+ It is NOT required that LEN is a multiple of the BLOCKLEN member of CTX.
+ Return false if an OpenSSL function fails. */
+extern bool sha3_process_bytes (const void *buffer, size_t len,
struct sha3_ctx *ctx);
/* Process the remaining bytes in the buffer and put result from CTX in RESBUF.
The result is always in little endian byte order, so that a byte-wise output
- yields to the wanted ASCII representation of the message digest. */
+ yields to the wanted ASCII representation of the message digest.
+ Return NULL if an OpenSSL function fails. */
extern void *sha3_finish_ctx (struct sha3_ctx *ctx, void *restrict resbuf);
/* Put result from CTX in RESBUF. The result is always in little endian byte
order, so that a byte-wise output yields to the wanted ASCII representation
- of the message digest. */
+ of the message digest.
+ Return NULL if an OpenSSL function fails. */
extern void *sha3_read_ctx (const struct sha3_ctx *ctx,
void *restrict resbuf);
/* Compute a SHA-3 message digest for LEN bytes beginning at BUFFER.
The result is always in little endian byte order, so that a byte-wise
output yields to the wanted ASCII representation of the message
- digest. */
+ digest.
+ Return NULL if an OpenSSL function fails. */
extern void *sha3_224_buffer (const char *buffer, size_t len,
void *restrict resblock);
extern void *sha3_256_buffer (const char *buffer, size_t len,
diff --git a/modules/crypto/sha3-buffer b/modules/crypto/sha3-buffer
index e0e654e563..7ab61dea30 100644
--- a/modules/crypto/sha3-buffer
+++ b/modules/crypto/sha3-buffer
@@ -8,6 +8,7 @@ m4/gl-openssl.m4
m4/sha3.m4
Depends-on:
+bool
byteswap
c99
stddef-h
--
2.51.0
