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

On Mon, Mar 3, 2025 at 12:50 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Sat, Mar 01, 2025 at 03:47:12PM -0500, Tom Lane wrote:
> > PG Bug reporting form <noreply@postgresql.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?

Yeah, that's one solution. But it would make pg_logicalinspect
functions hard to work with the pg_ls_loigcalsnapdir(). Users would
need to parse the filename by themselve and pass the LSN to them. I've
attached a draft patch for a different approach where we extract LSN
from the given file name in pg_logicalinspect functions. 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?

I think that we don't need to change the logic. It might be a good
idea to clarify the comment, though.

Regards,

--
Masahiko Sawada

Amazon Web Services: https://aws.amazon.com