pg.ddx.io pgsql-bugs@postgresql.org mailing list archive help / color / mirror / Atom feed From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Subject: [PATCH v7] Fix bug in pg_logicalinspect functions
Date: Tue, 4 Mar 2025 05:43:15 +0000
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 | 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(-)
42.6% contrib/pg_logicalinspect/expected/
28.1% contrib/pg_logicalinspect/sql/
23.2% contrib/pg_logicalinspect/
3.7% src/backend/replication/logical/
inline patch 1: contrib/pg_logicalinspect/Makefile, contrib/pg_logicalinspect/expected/pg_logicalinspect.out, contrib/pg_logicalinspect/pg_logicalinspect.c and 3 more (+183/-18 lines) download inline-1.patch 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..29f7f296653 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" documents the expected format, but be aware
+ * that sscanf only counts converted values (%X) in its return value, not
+ * literal text matches (meaning that ".snap" has no effect here).
+ */
+ 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.34.1
Message-ID: <no-message-id-25384@localhost>
Permalink: ../no-message-id-25384@localhost/
Also on: postgresql.org/message-id/no-message-id-25384@localhost
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: bertranddrouvot.pg@gmail.com
Subject: Re: [PATCH v7] Fix bug in pg_logicalinspect functions
In-Reply-To: <no-message-id-25384@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox