Thread

[PATCH v6] Fix bug in pg_logicalinspect functions

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-03-04T05:43:15Z

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            | 81 +++++++++++++++++++
 contrib/pg_logicalinspect/pg_logicalinspect.c | 56 ++++++++++---
 .../sql/pg_logicalinspect.sql                 | 48 +++++++++++
 src/backend/replication/logical/snapbuild.c   | 14 ++--
 src/include/replication/snapbuild_internal.h  |  2 +-
 6 files changed, 184 insertions(+), 18 deletions(-)
  43.0% contrib/pg_logicalinspect/expected/
  28.3% contrib/pg_logicalinspect/sql/
  22.5% contrib/pg_logicalinspect/
   3.8% 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..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..7963e52f9d1 100644
--- a/contrib/pg_logicalinspect/pg_logicalinspect.c
+++ b/contrib/pg_logicalinspect/pg_logicalinspect.c
@@ -48,6 +48,46 @@ get_snapbuild_state_desc(SnapBuildState state)
 	return stateDesc;
 }
 
+/*
+ * Extract the LSN from the given serialized snapshot file name.
+ */
+static XLogRecPtr
+parse_snapshot_filename(char *filename)
+{
+	uint32		hi;
+	uint32		lo;
+	XLogRecPtr	lsn;
+	char		tmpfname[MAXPGPATH];
+
+	/*
+	 * Extract the values to build the LSN.
+	 *
+	 * Note: We deliberately don't use "%X-%X.snap" because sscanf only counts
+	 * converted values (%X), not literal text matches.
+	 */
+	if (sscanf(filename, "%X-%X", &hi, &lo) != 2)
+		goto parse_error;
+
+	/*
+	 * Bring back the LSN to the snapshot file format and compare it to the
+	 * given name to see if the extracted LSN is sane.
+	 *
+	 * This also ensures there are no extra characters between the LSN values
+	 * and the .snap extension.
+	 */
+	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 +100,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 +108,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 +135,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 +143,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.34.1


--fkkW88tUwgX1xg5l--