From 3ac79fb7ed71854ddacc51a871de3f9ecd52502e 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 v5] Fix bug in pg_logicalinspect functions

The functions did not handle passed empty strings correctly and were not sanitizing
the request to prevent reference to files outside the snapshots directory.

Per bug #18828 from Robins Tharakan.
---
 contrib/pg_logicalinspect/Makefile            |  1 +
 .../expected/pg_logicalinspect.out            | 75 +++++++++++++++++++
 contrib/pg_logicalinspect/pg_logicalinspect.c | 54 ++++++++++---
 .../sql/pg_logicalinspect.sql                 | 45 +++++++++++
 src/backend/replication/logical/snapbuild.c   | 14 ++--
 src/include/replication/snapbuild_internal.h  |  2 +-
 6 files changed, 173 insertions(+), 18 deletions(-)
  42.3% contrib/pg_logicalinspect/expected/
  28.6% contrib/pg_logicalinspect/sql/
  22.3% contrib/pg_logicalinspect/
   4.2% src/backend/replication/logical/

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..f1f0712bec2
--- /dev/null
+++ b/contrib/pg_logicalinspect/expected/pg_logicalinspect.out
@@ -0,0 +1,75 @@
+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.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_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..57bc622f06b 100644
--- a/contrib/pg_logicalinspect/pg_logicalinspect.c
+++ b/contrib/pg_logicalinspect/pg_logicalinspect.c
@@ -48,6 +48,44 @@ 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;
+	bool		report_error = false;
+
+	/* Extract the values to build the LSN */
+	if (sscanf(filename, "%X-%X", &hi, &lo) != 2)
+		report_error = true;
+
+	/* Ensure filename is %X-%X.snap */
+	if (!report_error)
+	{
+		size_t		buffer_size = strlen(filename) + 1;
+		char	   *buffer = palloc(buffer_size);
+
+		snprintf(buffer, buffer_size, "%X-%X.snap", hi, lo);
+
+		if (strcmp(buffer, filename) != 0)
+			report_error = true;
+
+		pfree(buffer);
+	}
+
+	if (report_error)
+		ereport(ERROR,
+				errmsg("invalid snapshot file name \"%s\"", filename));
+
+	lsn = ((uint64) hi) << 32 | lo;
+
+	return lsn;
+}
+
 /*
  * Retrieve the logical snapshot file metadata.
  */
@@ -60,7 +98,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 +106,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 +133,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 +141,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..7d4951f30b0
--- /dev/null
+++ b/contrib/pg_logicalinspect/sql/pg_logicalinspect.sql
@@ -0,0 +1,45 @@
+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.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_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.34.1

