Re: BUG #18828: Crash when pg_get_logical_snapshot_meta() passed empty string

Hi,

On Sat, Mar 01, 2025 at 03:47:12PM -0500, Tom Lane wrote:
> PG Bug reporting form <noreply(at)postgresql(dot)org> writes:
> > Passing an empty string to the recently added extension function
> > pg_get_logical_snapshot_meta() triggers PANIC / Abort.

Thanks for the report!

> Thanks for noticing. It looks like this function is far too trusting
> of its input. Another way to crash it is to point to a directory:
>
> regression=# SELECT pg_get_logical_snapshot_meta('../snapshots');
> PANIC: could not open file "pg_logical/snapshots/../snapshots": Is a directory
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
>
> This example incidentally shows that it's not sanitizing the request
> to prevent reference to files outside the snapshots directory, which
> possibly would be a security bug down the road. I realize that we
> restrict access to the function, but still we shouldn't be *this*
> trusting of the argument.
>

Indeed that's bad.

> Possibly some of the defense for this ought to be in
> SnapBuildRestoreSnapshot: it looks like that function isn't
> considering the possibility that OpenTransientFile will succeed on a
> directory. Does it have any other callers passing
> less-than-fully-trustworthy paths?

I don't think so. The other caller is in snapbuild.c where the input is
build based on a lsn.

Now I wonder if pg_get_logical_snapshot_meta() (and pg_get_logical_snapshot_info()
which suffers for the same issues reported above) should take a lsn as input
parameter. That's how it has been proposed initially and that would simplify
things. Thoughts?

> Another interesting question is why the error is being promoted
> to PANIC. That sure seems unnecessary, and there's a comment
> in SnapBuildRestoreSnapshot that claims it won't happen.
>

Yeah. That's the fsync_fname() call in SnapBuildRestoreSnapshot() that promotes
to PANIC due to (data_sync_elevel(ERROR)). This behavior has been introduced in
9ccdd7f66e3 (that made the comment in SnapBuildRestore() (at that time) inaccurate).

I'm not sure we should change the logic here, maybe just update the comment?

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com