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

On Sat, Mar 1, 2025 at 12:47 PM Tom Lane <tgl@sss.pgh.pa.us> 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 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.

Agreed.

>
> 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?

SnapBuildRestoreSnapshot() is called by pg_logicalinspect functions
(pg_get_logical_snapshot_meta() and pg_get_logical_snapshot_info())
and SnapBuildRestore(). For the latter, SnapBuildRestore() surely
passes the file name, so it would not be a problem.

One way to fix this issue is that SnapBuildRestoreSnapshot() takes an
LSN corresponding to the serialized snapshot instead of a path, and it
constructs the .snap file path correctly. This fix would also prevent
it from being a security bug.

pg_logicalinspect functions, pg_get_logical_snapshot_info() and
pg_get_logical_snapshot_meta(), would also need to be changed so that
they extract the LSN from the given file name. If the given file name
is incorrect, it should raise an error.

>
> 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.
>

This PANIC occurred because fsync_fname() with isdir=false was called
for the directory, pg_logical/snapshots/' in the above case
(data_sync_retry is false by default). I think you referred to the
following comment in SnapBuildRestoreSnapshot():

    /* ----
     * Make sure the snapshot had been stored safely to disk, that's normally
     * cheap.
     * Note that we do not need PANIC here, nobody will be able to use the
     * slot without fsyncing, and saving it won't succeed without an fsync()
     * either...
     * ----
     */
    fsync_fname(path, false);
    fsync_fname(PG_LOGICAL_SNAPSHOTS_DIR, true);

These comments have been present since the initial logical decoding
commit, whereas data_sync_retry was introduced at a later stage. While
the comment's reference to PANIC suggests that a critical section
isn't necessary here, the actual PANIC occurred in an unanticipated
manner. SnapBuildRestoreSnapshot() works correctly as long as it takes
a correct file path but 7cdfeee320e72 violated this assumption.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com