v8-0001-pg_logicalinspect-Add-validation-check-for-input-.patch

From 8b579191a75e28cec9b16bdda67acdacbd6a9ab8 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Tue, 4 Mar 2025 05:43:15 +0000
Subject: [PATCH v8] pg_logicalinspect: Add validation check for input
 filename.

Previously, pg_logicalinspect functions were too trusting of their
input and blindly passed it to SnapBuildRestoreSnapshot(). If the
input pointed to a directory, the server could a PANIC error while
attempting to fsync_fname() with isdir=false on a directory.

This commit adds validation checks for input filenames and passes the
LSN extracted from the filename to SnapBuildRestoreSnapshot() instead
of the filename itself. It also adds regression tests for various
input patterns and permission checks.

Bug: #18828
Co-authored-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Co-authored-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/18828-0f4701c635064211@postgresql.org
---
 contrib/pg_logicalinspect/Makefile            |  1 +
 .../expected/pg_logicalinspect.out            | 81 +++++++++++++++++++
 contrib/pg_logicalinspect/pg_logicalinspect.c | 55 ++++++++++---
 .../sql/pg_logicalinspect.sql                 | 48 +++++++++++
 src/backend/replication/logical/snapbuild.c   | 14 ++--
 src/include/replication/snapbuild_internal.h  |  2 +-
 6 files changed, 183 insertions(+), 18 deletions(-)
 create mode 100644 contrib/pg_logicalinspect/expected/pg_logicalinspect.out
 create mode 100644 contrib/pg_logicalinspect/sql/pg_logicalinspect.sql

diff --git a/contrib/pg_logicalinspect/Makefile b/contrib/pg_logicalinspect/Makefile
index 55124514d42..17cee50d5c8 100644
--- a/contrib/pg_logicalinspect/Makefile
+++ b/contrib/pg_logicalinspect/Makefile
@@ -11,6 +11,7 @@ DATA = pg_logicalinspect--1.0.sql
 
 EXTRA_INSTALL = contrib/test_decoding
 
+REGRESS = pg_logicalinspect
 ISOLATION = logical_inspect
 
 ISOLATION_OPTS = --temp-config $(top_srcdir)/contrib/pg_logicalinspect/logicalinspect.conf
diff --git a/contrib/pg_logicalinspect/expected/pg_logicalinspect.out b/contrib/pg_logicalinspect/expected/pg_logicalinspect.out
new file mode 100644
index 00000000000..f6c9b625d6b
--- /dev/null
+++ b/contrib/pg_logicalinspect/expected/pg_logicalinspect.out
@@ -0,0 +1,81 @@
+CREATE EXTENSION pg_logicalinspect;
+-- ===================================================================
+-- Tests for input validation
+-- ===================================================================
+SELECT pg_get_logical_snapshot_info('0-40796E18.foo');
+ERROR:  invalid snapshot file name "0-40796E18.foo"
+SELECT pg_get_logical_snapshot_info('0--40796E18.snap');
+ERROR:  invalid snapshot file name "0--40796E18.snap"
+SELECT pg_get_logical_snapshot_info('-1--40796E18.snap');
+ERROR:  invalid snapshot file name "-1--40796E18.snap"
+SELECT pg_get_logical_snapshot_info('0/40796E18.foo.snap');
+ERROR:  invalid snapshot file name "0/40796E18.foo.snap"
+SELECT pg_get_logical_snapshot_info('0/40796E18.snap');
+ERROR:  invalid snapshot file name "0/40796E18.snap"
+SELECT pg_get_logical_snapshot_info('');
+ERROR:  invalid snapshot file name ""
+SELECT pg_get_logical_snapshot_info(NULL);
+ pg_get_logical_snapshot_info 
+------------------------------
+ 
+(1 row)
+
+SELECT pg_get_logical_snapshot_info('../snapshots');
+ERROR:  invalid snapshot file name "../snapshots"
+SELECT pg_get_logical_snapshot_info('../snapshots/0-40796E18.snap');
+ERROR:  invalid snapshot file name "../snapshots/0-40796E18.snap"
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo');
+ERROR:  invalid snapshot file name "0-40796E18.foo"
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo.snap');
+ERROR:  invalid snapshot file name "0-40796E18.foo.snap"
+SELECT pg_get_logical_snapshot_meta('0/40796E18.snap');
+ERROR:  invalid snapshot file name "0/40796E18.snap"
+SELECT pg_get_logical_snapshot_meta('');
+ERROR:  invalid snapshot file name ""
+SELECT pg_get_logical_snapshot_meta(NULL);
+ pg_get_logical_snapshot_meta 
+------------------------------
+ 
+(1 row)
+
+SELECT pg_get_logical_snapshot_meta('../snapshots');
+ERROR:  invalid snapshot file name "../snapshots"
+-- ===================================================================
+-- Tests for permissions
+-- ===================================================================
+CREATE ROLE regress_pg_logicalinspect;
+SELECT has_function_privilege('regress_pg_logicalinspect',
+  'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- no
+ has_function_privilege 
+------------------------
+ f
+(1 row)
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+  'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- no
+ has_function_privilege 
+------------------------
+ f
+(1 row)
+
+-- Functions accessible by users with role pg_read_server_files.
+GRANT pg_read_server_files TO regress_pg_logicalinspect;
+SELECT has_function_privilege('regress_pg_logicalinspect',
+  'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- yes
+ has_function_privilege 
+------------------------
+ t
+(1 row)
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+  'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- yes
+ has_function_privilege 
+------------------------
+ t
+(1 row)
+
+-- ===================================================================
+-- Clean up
+-- ===================================================================
+DROP ROLE regress_pg_logicalinspect;
+DROP EXTENSION pg_logicalinspect;
diff --git a/contrib/pg_logicalinspect/pg_logicalinspect.c b/contrib/pg_logicalinspect/pg_logicalinspect.c
index cd575c6bd36..3b64d8ad880 100644
--- a/contrib/pg_logicalinspect/pg_logicalinspect.c
+++ b/contrib/pg_logicalinspect/pg_logicalinspect.c
@@ -48,6 +48,45 @@ get_snapbuild_state_desc(SnapBuildState state)
 	return stateDesc;
 }
 
+/*
+ * Extract the LSN from the given serialized snapshot file name.
+ */
+static XLogRecPtr
+parse_snapshot_filename(const char *filename)
+{
+	uint32		hi;
+	uint32		lo;
+	XLogRecPtr	lsn;
+	char		tmpfname[MAXPGPATH];
+
+	/*
+	 * Extract the values to build the LSN.
+	 *
+	 * Note: Including ".snap" doesn't mean that sscanf() also does the format
+	 * check including the suffix. The subsequent check validates if the given
+	 * filename has the expected suffix.
+	 */
+	if (sscanf(filename, "%X-%X.snap", &hi, &lo) != 2)
+		goto parse_error;
+
+	/*
+	 * Bring back the extracted LSN to the snapshot file format and compare it
+	 * to the given filename. This check strictly checks if the given filename
+	 * follows the format of the snapshot filename.
+	 */
+	sprintf(tmpfname, "%X-%X.snap", hi, lo);
+	if (strcmp(tmpfname, filename) != 0)
+		goto parse_error;
+
+	lsn = ((uint64) hi) << 32 | lo;
+
+	return lsn;
+
+parse_error:
+	ereport(ERROR,
+			errmsg("invalid snapshot file name \"%s\"", filename));
+}
+
 /*
  * Retrieve the logical snapshot file metadata.
  */
@@ -60,7 +99,7 @@ pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
 	Datum		values[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
 	bool		nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
 	TupleDesc	tupdesc;
-	char		path[MAXPGPATH];
+	XLogRecPtr	lsn;
 	int			i = 0;
 	text	   *filename_t = PG_GETARG_TEXT_PP(0);
 
@@ -68,12 +107,10 @@ pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
 	if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
 		elog(ERROR, "return type must be a row type");
 
-	sprintf(path, "%s/%s",
-			PG_LOGICAL_SNAPSHOTS_DIR,
-			text_to_cstring(filename_t));
+	lsn = parse_snapshot_filename(text_to_cstring(filename_t));
 
 	/* Validate and restore the snapshot to 'ondisk' */
-	SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
+	SnapBuildRestoreSnapshot(&ondisk, lsn, CurrentMemoryContext, false);
 
 	values[i++] = UInt32GetDatum(ondisk.magic);
 	values[i++] = Int64GetDatum((int64) ondisk.checksum);
@@ -97,7 +134,7 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
 	Datum		values[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
 	bool		nulls[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
 	TupleDesc	tupdesc;
-	char		path[MAXPGPATH];
+	XLogRecPtr	lsn;
 	int			i = 0;
 	text	   *filename_t = PG_GETARG_TEXT_PP(0);
 
@@ -105,12 +142,10 @@ pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
 	if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
 		elog(ERROR, "return type must be a row type");
 
-	sprintf(path, "%s/%s",
-			PG_LOGICAL_SNAPSHOTS_DIR,
-			text_to_cstring(filename_t));
+	lsn = parse_snapshot_filename(text_to_cstring(filename_t));
 
 	/* Validate and restore the snapshot to 'ondisk' */
-	SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
+	SnapBuildRestoreSnapshot(&ondisk, lsn, CurrentMemoryContext, false);
 
 	values[i++] = CStringGetTextDatum(get_snapbuild_state_desc(ondisk.builder.state));
 	values[i++] = TransactionIdGetDatum(ondisk.builder.xmin);
diff --git a/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql b/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql
new file mode 100644
index 00000000000..143cf45dc12
--- /dev/null
+++ b/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql
@@ -0,0 +1,48 @@
+CREATE EXTENSION pg_logicalinspect;
+
+-- ===================================================================
+-- Tests for input validation
+-- ===================================================================
+
+SELECT pg_get_logical_snapshot_info('0-40796E18.foo');
+SELECT pg_get_logical_snapshot_info('0--40796E18.snap');
+SELECT pg_get_logical_snapshot_info('-1--40796E18.snap');
+SELECT pg_get_logical_snapshot_info('0/40796E18.foo.snap');
+SELECT pg_get_logical_snapshot_info('0/40796E18.snap');
+SELECT pg_get_logical_snapshot_info('');
+SELECT pg_get_logical_snapshot_info(NULL);
+SELECT pg_get_logical_snapshot_info('../snapshots');
+SELECT pg_get_logical_snapshot_info('../snapshots/0-40796E18.snap');
+
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo');
+SELECT pg_get_logical_snapshot_meta('0-40796E18.foo.snap');
+SELECT pg_get_logical_snapshot_meta('0/40796E18.snap');
+SELECT pg_get_logical_snapshot_meta('');
+SELECT pg_get_logical_snapshot_meta(NULL);
+SELECT pg_get_logical_snapshot_meta('../snapshots');
+
+-- ===================================================================
+-- Tests for permissions
+-- ===================================================================
+CREATE ROLE regress_pg_logicalinspect;
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+  'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- no
+SELECT has_function_privilege('regress_pg_logicalinspect',
+  'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- no
+
+-- Functions accessible by users with role pg_read_server_files.
+GRANT pg_read_server_files TO regress_pg_logicalinspect;
+
+SELECT has_function_privilege('regress_pg_logicalinspect',
+  'pg_get_logical_snapshot_info(text)', 'EXECUTE'); -- yes
+SELECT has_function_privilege('regress_pg_logicalinspect',
+  'pg_get_logical_snapshot_meta(text)', 'EXECUTE'); -- yes
+
+-- ===================================================================
+-- Clean up
+-- ===================================================================
+
+DROP ROLE regress_pg_logicalinspect;
+
+DROP EXTENSION pg_logicalinspect;
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index bd0680dcbe5..b64e53de017 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1691,12 +1691,17 @@ out:
  * If 'missing_ok' is true, will not throw an error if the file is not found.
  */
 bool
-SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, XLogRecPtr lsn,
 						 MemoryContext context, bool missing_ok)
 {
 	int			fd;
 	pg_crc32c	checksum;
 	Size		sz;
+	char		path[MAXPGPATH];
+
+	sprintf(path, "%s/%X-%X.snap",
+			PG_LOGICAL_SNAPSHOTS_DIR,
+			LSN_FORMAT_ARGS(lsn));
 
 	fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
 
@@ -1788,18 +1793,13 @@ static bool
 SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
 {
 	SnapBuildOnDisk ondisk;
-	char		path[MAXPGPATH];
 
 	/* no point in loading a snapshot if we're already there */
 	if (builder->state == SNAPBUILD_CONSISTENT)
 		return false;
 
-	sprintf(path, "%s/%X-%X.snap",
-			PG_LOGICAL_SNAPSHOTS_DIR,
-			LSN_FORMAT_ARGS(lsn));
-
 	/* validate and restore the snapshot to 'ondisk' */
-	if (!SnapBuildRestoreSnapshot(&ondisk, path, builder->context, true))
+	if (!SnapBuildRestoreSnapshot(&ondisk, lsn, builder->context, true))
 		return false;
 
 	/*
diff --git a/src/include/replication/snapbuild_internal.h b/src/include/replication/snapbuild_internal.h
index 081b01b890a..3b915dc8793 100644
--- a/src/include/replication/snapbuild_internal.h
+++ b/src/include/replication/snapbuild_internal.h
@@ -193,7 +193,7 @@ typedef struct SnapBuildOnDisk
 	/* variable amount of TransactionIds follows */
 } SnapBuildOnDisk;
 
-extern bool SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+extern bool SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, XLogRecPtr lsn,
 									 MemoryContext context, bool missing_ok);
 
 #endif							/* SNAPBUILD_INTERNAL_H */
-- 
2.43.5