Wrong buffer limits check
Hi hackers,
I have tried to analyse Postgres code with Svace static analyzer [1]https://svace.pages.ispras.ru/svace-website/en/ and
found something I think is a real bug.
In pgp-decrypt.c, in prefix_init function the following check:
if (len > sizeof(tmpbuf))
seem to be erroneous and should really look this way:
if (len > PGP_MAX_BLOCK)
Otherwise the below checks in this line could lead to buffer overflows:
if (buf[len - 2] != buf[len] || buf[len - 1] != buf[len + 1])
This is because buf will point to tmpbuf, while tmpbuf have a size of
PGP_MAX_BLOCK + 2.
What do you think? The proposed patch towarts the current master branch is
attached.
[1]: https://svace.pages.ispras.ru/svace-website/en/
--
best regards,
Mikhail A. Gribkov
e-mail: youzhick@gmail.com
*http://www.flickr.com/photos/youzhick/albums
<http://www.flickr.com/photos/youzhick/albums>*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick
Attachments:
v001-Fix_buffer_len_check.patchapplication/octet-stream; name=v001-Fix_buffer_len_check.patchDownload
diff --git a/contrib/pgcrypto/pgp-decrypt.c b/contrib/pgcrypto/pgp-decrypt.c
index d12dcad194..c49c70dc0c 100644
--- a/contrib/pgcrypto/pgp-decrypt.c
+++ b/contrib/pgcrypto/pgp-decrypt.c
@@ -250,7 +250,7 @@ prefix_init(void **priv_p, void *arg, PullFilter *src)
uint8 tmpbuf[PGP_MAX_BLOCK + 2];
len = pgp_get_cipher_block_size(ctx->cipher_algo);
- if (len > sizeof(tmpbuf))
+ if (len > PGP_MAX_BLOCK)
return PXE_BUG;
res = pullf_read_max(src, len + 2, &buf, tmpbuf);
On 29 Jan 2024, at 14:37, Mikhail Gribkov <youzhick@gmail.com> wrote:
I have tried to analyse Postgres code with Svace static analyzer [1] and found something I think is a real bug.
In pgp-decrypt.c, in prefix_init function the following check:
if (len > sizeof(tmpbuf))seem to be erroneous and should really look this way:
if (len > PGP_MAX_BLOCK)
Studying the code I think you're right, we should be ensuring that the cipher
block size isn't exceeding PGP_MAX_BLOCK. In practice it seems night
impossible to hit given the ciphers in cipher_list, but we should still fix it.
Unsurprisingly this seems to have been there forever (since July 2005) so needs
to be backpatched to all supported branches for the sake of consistency
--
Daniel Gustafsson