Memory leak in pg_hmac_final
Attachments:
fix-hmac-leak.patchtext/plain; charset=UTF-8; name=fix-hmac-leak.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/src/common/hmac.c b/src/common/hmac.c
index 1089db67443..bfe2e7cb5e9 100644
--- a/src/common/hmac.c
+++ b/src/common/hmac.c
@@ -232,7 +232,10 @@ pg_hmac_final(pg_hmac_ctx *ctx, uint8 *dest, size_t len)
memset(h, 0, ctx->digest_size);
if (pg_cryptohash_final(ctx->hash, h, ctx->digest_size) < 0)
+ {
+ FREE(h);
return -1;
+ }
/* H(K XOR opad, tmp) */
if (pg_cryptohash_init(ctx->hash) < 0 ||
@@ -240,9 +243,11 @@ pg_hmac_final(pg_hmac_ctx *ctx, uint8 *dest, size_t len)
pg_cryptohash_update(ctx->hash, h, ctx->digest_size) < 0 ||
pg_cryptohash_final(ctx->hash, dest, len) < 0)
{
+ FREE(h);
return -1;
}
+ FREE(h);
return 0;
}
On 1 Oct 2021, at 12:39, Sergey Shinderuk <s.shinderuk@postgrespro.ru> wrote:
Here is a patch fixing the subject.
Seems reasonable on a quick glance, the interim h buffer should be freed (this
is present since 14). I'll have another look at this in a bit and will take
care of it.
--
Daniel Gustafsson https://vmware.com/
On 01.10.2021 15:05, Daniel Gustafsson wrote:
On 1 Oct 2021, at 12:39, Sergey Shinderuk <s.shinderuk@postgrespro.ru> wrote:
Here is a patch fixing the subject.
Seems reasonable on a quick glance, the interim h buffer should be freed (this
is present since 14). I'll have another look at this in a bit and will take
care of it.
Thanks. I found it using the leaks tool on macOS.
Without the patch:
% MallocStackLogging=1 leaks -quiet -atExit -- psql -d 'dbname=postgres
user=alice password=secret' -XAtc 'select 1'
...
Process 91635: 4390 nodes malloced for 252 KB
Process 91635: 4103 leaks for 131296 total leaked bytes.
...
(User alice has a SCRAM-encrypted password.)
With the patch:
Process 98250: 290 nodes malloced for 124 KB
Process 98250: 3 leaks for 96 total leaked bytes.
The remaining leaks are expected and not worth fixing, I guess:
STACK OF 1 INSTANCE OF 'ROOT LEAK: malloc<32>':
4 libdyld.dylib 0x7fff68d80cc9 start + 1
3 psql 0x10938b9f9 main + 2393
startup.c:207
2 psql 0x1093ab5a5 pg_malloc + 21
fe_memutils.c:49
1 libsystem_malloc.dylib 0x7fff68f36cf5 malloc + 21
0 libsystem_malloc.dylib 0x7fff68f36d9e malloc_zone_malloc
+ 140
====
2 (48 bytes) ROOT LEAK: 0x7ffbb75040d0 [32]
1 (16 bytes) 0x7ffbb75040f0 [16] length: 8 "select 1"
STACK OF 1 INSTANCE OF 'ROOT LEAK: malloc<48>':
5 libdyld.dylib 0x7fff68d80cc9 start + 1
4 psql 0x10938b8b0 main + 2064
startup.c:207
3 psql 0x1093ab78e pg_strdup + 14
fe_memutils.c:96
2 libsystem_c.dylib 0x7fff68e26ce6 strdup + 32
1 libsystem_malloc.dylib 0x7fff68f36cf5 malloc + 21
0 libsystem_malloc.dylib 0x7fff68f36d9e malloc_zone_malloc
+ 140
====
1 (48 bytes) ROOT LEAK: 0x7ffbb75040a0 [48] length: 42
"dbname=postgres user=alice password=secret"
--
Sergey Shinderuk https://postgrespro.com/
On 1 Oct 2021, at 14:31, Sergey Shinderuk <s.shinderuk@postgrespro.ru> wrote:
On 01.10.2021 15:05, Daniel Gustafsson wrote:
On 1 Oct 2021, at 12:39, Sergey Shinderuk <s.shinderuk@postgrespro.ru> wrote:
Here is a patch fixing the subject.
Seems reasonable on a quick glance, the interim h buffer should be freed (this
is present since 14). I'll have another look at this in a bit and will take
care of it.
Patch pushed to master and 14.
Thanks. I found it using the leaks tool on macOS.
Nice, I hadn't heard of that before but it seems quite neat.
--
Daniel Gustafsson https://vmware.com/
On Fri, Oct 01, 2021 at 10:58:07PM +0200, Daniel Gustafsson wrote:
Nice, I hadn't heard of that before but it seems quite neat.
Thanks for the fix, it looks fine. I just saw the thread. Perhaps
the commit log should have said that this only impacts non-OpenSSL
builds. Worth noting that in ~13 we used a static buffer for "h" in
the SCRAM code, as its size was known thanks to SHA-256.
--
Michael