Thread: Insufficient memory access checks in pglz_decompress

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

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

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

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

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