From 3ac79fb7ed71854ddacc51a871de3f9ecd52502e Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot 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