pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedInsufficient memory access checks in pglz_decompress
5+ messages / 2 participants
[nested] [flat]
* Insufficient memory access checks in pglz_decompress
@ 2023-10-18 14:21 Flavien GUEDEZ <flav.pg@oopacity.net>
0 siblings, 1 reply; 5+ messages in thread
From: Flavien GUEDEZ @ 2023-10-18 14:21 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
Hi,
After some investigations about very corrupted toast data in one
postgres instance, I found that the pglz_decompress function (in
common/pg_lzcompress.c) does not check correctly where it copies data
from using memcpy(), which could result in segfault.
In this function, there are other checks to ensure that we do not copy
after the destination end, but not if we copy data from "before the
beginning".
Apologize, I am not a C developer and I am not used to submitting patches.
Though I have tried and attached kind of PoC with a relatively random
corrupted payload (it was beginning with those bytes in my storage for
obscure reasons).
Also attached a simple patch of what could be done just before the
memcpy calls.
Regards,
Flavien
Attachments:
[text/x-patch] check_for_corrupted.patch (822B, ../../01fc0593-e31e-463d-902c-dd43174acee2@oopacity.net/2-check_for_corrupted.patch)
download | inline diff:
--- a/src/common/pg_lzcompress.c 2023-10-18 12:34:30.616000000 +0000
+++ b/src/common/pg_lzcompress.c 2023-10-18 12:48:09.968000000 +0000
@@ -761,7 +761,12 @@
/*
* We can safely copy "off" bytes since that clearly
* results in non-overlapping source and destination.
+ * Check for corrupt data: if we fall before the beginning of the
+ * destination, we have problems.
*/
+ if (dp - off < dest) {
+ return -1;
+ }
memcpy(dp, dp - off, off);
len -= off;
dp += off;
@@ -792,6 +797,13 @@
*/
off += off;
}
+ /*
+ * Check for corrupt data: if we fall before the beginning of the
+ * destination, we have problems.
+ */
+ if (dp - off < dest) {
+ return -1;
+ }
memcpy(dp, dp - off, len);
dp += len;
}
[text/x-csrc] poc.c (671B, ../../01fc0593-e31e-463d-902c-dd43174acee2@oopacity.net/3-poc.c)
download | inline:
#include <ctype.h>
#include <stdio.h>
#include "postgres.h"
int
main(int argv, char **argc)
{
char *corrupted_compressed_payload = "\x21\x1C\x44\x36\x38\x56\x40\x37\0";
int corrupted_compressed_payload_size = strlen(corrupted_compressed_payload);
int decompress_tmp_buff_size = 1024; // very arbitrary
char *decompress_tmp_buff = malloc(decompress_tmp_buff_size*sizeof(char));
int decompress_ret;
decompress_ret = pglz_decompress(corrupted_compressed_payload, corrupted_compressed_payload_size,
decompress_tmp_buff, decompress_tmp_buff_size, true);
if(decompress_ret == -1) {
printf("data is corrupted\n");
}
free(decompress_tmp_buff);
return 0;
}
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Insufficient memory access checks in pglz_decompress
@ 2023-10-18 15:14 Tom Lane <tgl@sss.pgh.pa.us>
parent: Flavien GUEDEZ <flav.pg@oopacity.net>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2023-10-18 15:14 UTC (permalink / raw)
To: Flavien GUEDEZ <flav.pg@oopacity.net>; +Cc: pgsql-bugs@lists.postgresql.org
Flavien GUEDEZ <flav.pg@oopacity.net> writes:
> After some investigations about very corrupted toast data in one
> postgres instance, I found that the pglz_decompress function (in
> common/pg_lzcompress.c) does not check correctly where it copies data
> from using memcpy(), which could result in segfault.
> In this function, there are other checks to ensure that we do not copy
> after the destination end, but not if we copy data from "before the
> beginning".
Hmm, would it not be better to add this check to the existing "Check for
corrupt data" a bit further up? Then you'd only need one instance of
the test, and only need to do it once per tag (note the comment pointing
out that dp - off stays the same), and overall it'd be less surprising IMO.
regards, tom lane
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Insufficient memory access checks in pglz_decompress
@ 2023-10-18 16:46 Flavien GUEDEZ <flav.pg@oopacity.net>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 5+ messages in thread
From: Flavien GUEDEZ @ 2023-10-18 16:46 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
Thanks for your feedback, you are definitely right, I did not notice
that (dp - off) was staying the same in the while loop.
Here is another much smaller patch.
Flavien
Le 18/10/2023 à 17:14, Tom Lane a écrit :
> Flavien GUEDEZ <flav.pg@oopacity.net> writes:
>> After some investigations about very corrupted toast data in one
>> postgres instance, I found that the pglz_decompress function (in
>> common/pg_lzcompress.c) does not check correctly where it copies data
>> from using memcpy(), which could result in segfault.
>> In this function, there are other checks to ensure that we do not copy
>> after the destination end, but not if we copy data from "before the
>> beginning".
> Hmm, would it not be better to add this check to the existing "Check for
> corrupt data" a bit further up? Then you'd only need one instance of
> the test, and only need to do it once per tag (note the comment pointing
> out that dp - off stays the same), and overall it'd be less surprising IMO.
>
> regards, tom lane
Attachments:
[text/x-patch] v2-pglz_decompress_check_for_corrupted.patch (549B, ../../bb74b9f4-65c1-4d89-af20-7de623948e36@oopacity.net/2-v2-pglz_decompress_check_for_corrupted.patch)
download | inline diff:
--- a/src/common/pg_lzcompress.c 2023-10-18 12:34:30.616000000 +0000
+++ b/src/common/pg_lzcompress.c 2023-10-18 16:42:17.856000000 +0000
@@ -738,8 +738,10 @@
* source, or if we obtained off = 0, we have problems. (We
* must check this, else we risk an infinite loop below in the
* face of corrupt data.)
+ * Also check if we fell before the beginning of the
+ * destination.
*/
- if (unlikely(sp > srcend || off == 0))
+ if (unlikely(sp > srcend || off == 0 || (dp - off) < dest))
return -1;
/*
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Insufficient memory access checks in pglz_decompress
@ 2023-10-19 00:48 Tom Lane <tgl@sss.pgh.pa.us>
parent: Flavien GUEDEZ <flav.pg@oopacity.net>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2023-10-19 00:48 UTC (permalink / raw)
To: Flavien GUEDEZ <flav.pg@oopacity.net>; +Cc: pgsql-bugs@lists.postgresql.org
Flavien GUEDEZ <flav.pg@oopacity.net> writes:
> Thanks for your feedback, you are definitely right, I did not notice
> that (dp - off) was staying the same in the while loop.
> Here is another much smaller patch.
I thought of another thing we should change: it's better to perform
the test as "off > (dp - dest)" than the way you formulated it.
"dp - dest" is certainly computable, since it's the number of bytes
we've written to the output buffer so far. But "dp - off" could,
with bad luck and a buffer near the start of memory, wrap around
to look like it's after "dest".
Pushed with that change and a little fiddling with the comment.
Thanks for the report!
regards, tom lane
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Insufficient memory access checks in pglz_decompress
@ 2023-10-19 07:05 Flavien GUEDEZ <flav.pg@oopacity.net>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 5+ messages in thread
From: Flavien GUEDEZ @ 2023-10-19 07:05 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
Le 19/10/2023 à 02:48, Tom Lane a écrit :
> I thought of another thing we should change: it's better to perform
> the test as "off > (dp - dest)" than the way you formulated it.
> "dp - dest" is certainly computable, since it's the number of bytes
> we've written to the output buffer so far. But "dp - off" could,
> with bad luck and a buffer near the start of memory, wrap around
> to look like it's after "dest".
>
> Pushed with that change and a little fiddling with the comment.
> Thanks for the report!
>
> regards, tom lane
Thank you for the details !
Best,
Flavien
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-10-19 07:05 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-10-18 14:21 Insufficient memory access checks in pglz_decompress Flavien GUEDEZ <flav.pg@oopacity.net>
2023-10-18 15:14 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-18 16:46 ` Flavien GUEDEZ <flav.pg@oopacity.net>
2023-10-19 00:48 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-19 07:05 ` Flavien GUEDEZ <flav.pg@oopacity.net>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox