This is an automated email from the ASF dual-hosted git repository. leborchuk pushed a commit to branch REL_2_STABLE in repository https://gitbox.apache.org/repos/asf/cloudberry.git
commit 2c46d9a2e8f2992fb566c3a0b3859d1217e9b3b4 Author: Jianghua Yang <[email protected]> AuthorDate: Thu Sep 24 21:26:55 2026 +0800 Restrict pg_file_write/rename/unlink and pg_logdir_ls to privileged roles pg_file_write(text,text,bool), pg_file_rename(text,text,text), pg_file_unlink(text) and pg_logdir_ls() had proacl = NULL, i.e. PUBLIC EXECUTE. Any role, with no GRANT at all, could create, overwrite, rename or delete files under the data directory and the log directory, and list the log directory. postgresql.auto.conf is writable that way, which turns into code execution as the postgres OS user through shared_preload_libraries or archive_command after a reload or restart. The catalog entries for these functions point at the _v1_1 C symbols, whose bodies deliberately carry no privilege check: they were written for contrib/adminpack, where every CREATE FUNCTION is immediately followed by a REVOKE EXECUTE FROM PUBLIC (adminpack--1.1--2.0.sql). The bodies were brought into core in genfile.c, but neither the REVOKE nor an equivalent in-function check came along. The path confinement in convert_and_check_filename() is a read-side check by its own definition and never covered this. Add requireWriteServerFiles() for the three write-side functions and requireReadServerFiles() for pg_logdir_ls(), mirroring the pg_read_server_files check that convert_and_check_filename() already does; both accept superusers. These take effect as soon as the new binary is in place, which matters because an in-place upgraded cluster keeps proacl = NULL forever. Also add the matching REVOKE and GRANT in system_functions.sql, so a freshly initdb'd cluster is protected at the ACL layer too. gp_toolkit.gp_move_orphaned_files, the only in-tree caller, is run by administrators and is unaffected, and contrib/adminpack keeps its own copies of these C functions. The new genfile_privileges test covers both layers: a plain role is rejected by the ACL, and it is still rejected by genfile.c once it has been granted EXECUTE explicitly, while pg_write_server_files and pg_read_server_files members and superusers are allowed. --- src/backend/catalog/system_functions.sql | 28 +++++++++ src/backend/utils/adt/genfile.c | 50 +++++++++++++--- src/test/regress/expected/genfile_privileges.out | 73 ++++++++++++++++++++++++ src/test/regress/parallel_schedule | 2 +- src/test/regress/sql/genfile_privileges.sql | 50 ++++++++++++++++ 5 files changed, 194 insertions(+), 9 deletions(-) diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql index 5e3db477e61..efa24a36be2 100644 --- a/src/backend/catalog/system_functions.sql +++ b/src/backend/catalog/system_functions.sql @@ -725,10 +725,38 @@ REVOKE EXECUTE ON FUNCTION pg_ls_dir(text) FROM public; REVOKE EXECUTE ON FUNCTION pg_ls_dir(text,boolean,boolean) FROM public; REVOKE EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer) FROM PUBLIC; + +-- pg_file_write/pg_file_rename/pg_file_unlink have proacl NULL (PUBLIC +-- EXECUTE) because their _v1_1 bodies were copied in-core from adminpack +-- without adminpack's matching REVOKE. genfile.c now also checks for +-- superuser/pg_write_server_files membership at call time, but this +-- REVOKE is defense in depth for new initdbs. The matching GRANT keeps +-- pg_write_server_files members able to call them (the EXECUTE ACL check +-- happens before genfile.c's own membership check ever runs). +REVOKE EXECUTE ON FUNCTION pg_file_write(text,text,boolean) FROM public; + +REVOKE EXECUTE ON FUNCTION pg_file_rename(text,text,text) FROM public; + +REVOKE EXECUTE ON FUNCTION pg_file_unlink(text) FROM public; + +-- pg_logdir_ls has the same omission (proacl NULL, _v1_1 body with no +-- privilege check): it's a read-side listing, so gate it like the other +-- read-side functions above (pg_read_server_files) rather than write. +REVOKE EXECUTE ON FUNCTION pg_logdir_ls() FROM public; + + -- -- We also set up some things as accessible to standard roles. -- +GRANT EXECUTE ON FUNCTION pg_file_write(text,text,boolean) TO pg_write_server_files; + +GRANT EXECUTE ON FUNCTION pg_file_rename(text,text,text) TO pg_write_server_files; + +GRANT EXECUTE ON FUNCTION pg_file_unlink(text) TO pg_write_server_files; + +GRANT EXECUTE ON FUNCTION pg_logdir_ls() TO pg_read_server_files; + GRANT EXECUTE ON FUNCTION pg_ls_logdir() TO pg_monitor; GRANT EXECUTE ON FUNCTION pg_ls_waldir() TO pg_monitor; diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c index e7936572633..3c1b41ef132 100644 --- a/src/backend/utils/adt/genfile.c +++ b/src/backend/utils/adt/genfile.c @@ -134,6 +134,36 @@ requireSuperuser(void) (errmsg("only superuser may access generic file functions")))); } +/* + * check for superuser or privileges of the 'pg_write_server_files' role, + * bark if neither. Unlike convert_and_check_filename()'s read-side check, + * this does not also gate the path itself: it is only appropriate for + * callers that need a write-access privilege check. + */ +static void +requireWriteServerFiles(void) +{ + if (!has_privs_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("must be superuser or a member of the pg_write_server_files role to use this function"))); +} + +/* + * check for superuser or privileges of the 'pg_read_server_files' role, + * bark if neither. Same idea as requireWriteServerFiles(), for + * callers that only need a read-access privilege check (e.g. listing + * filenames rather than confining a path). + */ +static void +requireReadServerFiles(void) +{ + if (!has_privs_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("must be superuser or a member of the pg_read_server_files role to use this function"))); +} + /* * Read a section of a file, returning it as bytea * @@ -669,8 +699,7 @@ pg_file_write(PG_FUNCTION_ARGS) /* ------------------------------------ * pg_file_write_v1_1 - Version 1.1 * - * No superuser check done here- instead privileges are handled by the - * GRANT system. + * Restricted to superuser or pg_write_server_files members. */ Datum pg_file_write_v1_1(PG_FUNCTION_ARGS) @@ -680,6 +709,8 @@ pg_file_write_v1_1(PG_FUNCTION_ARGS) bool replace = PG_GETARG_BOOL(2); int64 count = 0; + requireWriteServerFiles(); + count = pg_file_write_internal(file, data, replace); PG_RETURN_INT64(count); @@ -810,8 +841,7 @@ pg_file_rename(PG_FUNCTION_ARGS) /* ------------------------------------ * pg_file_rename_v1_1 - Version 1.1 * - * No superuser check done here- instead privileges are handled by the - * GRANT system. + * Restricted to superuser or pg_write_server_files members. */ Datum pg_file_rename_v1_1(PG_FUNCTION_ARGS) @@ -821,6 +851,8 @@ pg_file_rename_v1_1(PG_FUNCTION_ARGS) text *file3; bool result; + requireWriteServerFiles(); + if (PG_ARGISNULL(0) || PG_ARGISNULL(1)) PG_RETURN_NULL(); @@ -875,14 +907,15 @@ pg_file_unlink(PG_FUNCTION_ARGS) /* ------------------------------------ * pg_file_unlink_v1_1 - Version 1.1 * - * No superuser check done here- instead privileges are handled by the - * GRANT system. + * Restricted to superuser or pg_write_server_files members. */ Datum pg_file_unlink_v1_1(PG_FUNCTION_ARGS) { char *filename; + requireWriteServerFiles(); + filename = convert_and_check_filename(PG_GETARG_TEXT_PP(0)); if (access(filename, W_OK) < 0) @@ -1063,12 +1096,13 @@ pg_logdir_ls(PG_FUNCTION_ARGS) /* ------------------------------------ * pg_logdir_ls_v1_1 - Version 1.1 * - * No superuser check done here- instead privileges are handled by the - * GRANT system. + * Restricted to superuser or pg_read_server_files members. */ Datum pg_logdir_ls_v1_1(PG_FUNCTION_ARGS) { + requireReadServerFiles(); + return (pg_logdir_ls_internal(fcinfo)); } diff --git a/src/test/regress/expected/genfile_privileges.out b/src/test/regress/expected/genfile_privileges.out new file mode 100644 index 00000000000..799b565cb79 --- /dev/null +++ b/src/test/regress/expected/genfile_privileges.out @@ -0,0 +1,73 @@ +-- +-- pg_file_write()/pg_file_rename()/pg_file_unlink() must only be usable by +-- superusers and members of pg_write_server_files; pg_logdir_ls() is the +-- read-side equivalent, gated on pg_read_server_files. +-- +CREATE ROLE regress_genfile_plain; +CREATE ROLE regress_genfile_writer IN ROLE pg_write_server_files; +CREATE ROLE regress_genfile_reader IN ROLE pg_read_server_files; +-- A plain role is denied at the ACL layer by the REVOKE in +-- system_functions.sql. +SET SESSION AUTHORIZATION regress_genfile_plain; +SELECT pg_file_write('regress_genfile.txt', 'hello', false); +ERROR: permission denied for function pg_file_write +SELECT pg_file_rename('regress_genfile.txt', 'regress_genfile2.txt', NULL); +ERROR: permission denied for function pg_file_rename +SELECT pg_file_unlink('regress_genfile.txt'); +ERROR: permission denied for function pg_file_unlink +SELECT count(*) >= 0 AS ok FROM pg_logdir_ls() AS t(starttime timestamp, filename text); +ERROR: permission denied for function pg_logdir_ls +RESET SESSION AUTHORIZATION; +-- On a cluster upgraded in place proacl stays NULL, so the checks in +-- genfile.c are the only defense. Simulate that by granting EXECUTE. +GRANT EXECUTE ON FUNCTION pg_file_write(text,text,boolean), + pg_file_rename(text,text,text), + pg_file_unlink(text), + pg_logdir_ls() TO regress_genfile_plain; +SET SESSION AUTHORIZATION regress_genfile_plain; +SELECT pg_file_write('regress_genfile.txt', 'hello', false); +ERROR: must be superuser or a member of the pg_write_server_files role to use this function +SELECT pg_file_rename('regress_genfile.txt', 'regress_genfile2.txt', NULL); +ERROR: must be superuser or a member of the pg_write_server_files role to use this function +SELECT pg_file_unlink('regress_genfile.txt'); +ERROR: must be superuser or a member of the pg_write_server_files role to use this function +SELECT count(*) >= 0 AS ok FROM pg_logdir_ls() AS t(starttime timestamp, filename text); +ERROR: must be superuser or a member of the pg_read_server_files role to use this function +RESET SESSION AUTHORIZATION; +-- A pg_write_server_files member is allowed; the superuser cleans up after +-- it, which covers the superuser path too. +SET SESSION AUTHORIZATION regress_genfile_writer; +SELECT pg_file_write('regress_genfile.txt', 'hello', false); + pg_file_write +--------------- + 5 +(1 row) + +SELECT pg_file_rename('regress_genfile.txt', 'regress_genfile2.txt', NULL); + pg_file_rename +---------------- + t +(1 row) + +RESET SESSION AUTHORIZATION; +SELECT pg_file_unlink('regress_genfile2.txt'); + pg_file_unlink +---------------- + t +(1 row) + +-- Likewise for pg_logdir_ls(). Which log files exist is not deterministic, +-- so only assert that the call succeeds. +SET SESSION AUTHORIZATION regress_genfile_reader; +SELECT count(*) >= 0 AS ok FROM pg_logdir_ls() AS t(starttime timestamp, filename text); + ok +---- + t +(1 row) + +RESET SESSION AUTHORIZATION; +REVOKE ALL ON FUNCTION pg_file_write(text,text,boolean), + pg_file_rename(text,text,text), + pg_file_unlink(text), + pg_logdir_ls() FROM regress_genfile_plain; +DROP ROLE regress_genfile_plain, regress_genfile_writer, regress_genfile_reader; diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule index e5551f42ecb..b435c0ba792 100644 --- a/src/test/regress/parallel_schedule +++ b/src/test/regress/parallel_schedule @@ -102,7 +102,7 @@ test: transactions # ---------- # Another group of parallel tests # ---------- -test: brin gin gist spgist privileges init_privs security_label collate matview lock replica_identity rowsecurity object_address tablesample groupingsets drop_operator password identity generated join_hash appendonly_sample aocs_sample +test: brin gin gist spgist privileges init_privs genfile_privileges security_label collate matview lock replica_identity rowsecurity object_address tablesample groupingsets drop_operator password identity generated join_hash appendonly_sample aocs_sample # ---------- # Additional BRIN tests diff --git a/src/test/regress/sql/genfile_privileges.sql b/src/test/regress/sql/genfile_privileges.sql new file mode 100644 index 00000000000..b5f41c43977 --- /dev/null +++ b/src/test/regress/sql/genfile_privileges.sql @@ -0,0 +1,50 @@ +-- +-- pg_file_write()/pg_file_rename()/pg_file_unlink() must only be usable by +-- superusers and members of pg_write_server_files; pg_logdir_ls() is the +-- read-side equivalent, gated on pg_read_server_files. +-- +CREATE ROLE regress_genfile_plain; +CREATE ROLE regress_genfile_writer IN ROLE pg_write_server_files; +CREATE ROLE regress_genfile_reader IN ROLE pg_read_server_files; + +-- A plain role is denied at the ACL layer by the REVOKE in +-- system_functions.sql. +SET SESSION AUTHORIZATION regress_genfile_plain; +SELECT pg_file_write('regress_genfile.txt', 'hello', false); +SELECT pg_file_rename('regress_genfile.txt', 'regress_genfile2.txt', NULL); +SELECT pg_file_unlink('regress_genfile.txt'); +SELECT count(*) >= 0 AS ok FROM pg_logdir_ls() AS t(starttime timestamp, filename text); +RESET SESSION AUTHORIZATION; + +-- On a cluster upgraded in place proacl stays NULL, so the checks in +-- genfile.c are the only defense. Simulate that by granting EXECUTE. +GRANT EXECUTE ON FUNCTION pg_file_write(text,text,boolean), + pg_file_rename(text,text,text), + pg_file_unlink(text), + pg_logdir_ls() TO regress_genfile_plain; +SET SESSION AUTHORIZATION regress_genfile_plain; +SELECT pg_file_write('regress_genfile.txt', 'hello', false); +SELECT pg_file_rename('regress_genfile.txt', 'regress_genfile2.txt', NULL); +SELECT pg_file_unlink('regress_genfile.txt'); +SELECT count(*) >= 0 AS ok FROM pg_logdir_ls() AS t(starttime timestamp, filename text); +RESET SESSION AUTHORIZATION; + +-- A pg_write_server_files member is allowed; the superuser cleans up after +-- it, which covers the superuser path too. +SET SESSION AUTHORIZATION regress_genfile_writer; +SELECT pg_file_write('regress_genfile.txt', 'hello', false); +SELECT pg_file_rename('regress_genfile.txt', 'regress_genfile2.txt', NULL); +RESET SESSION AUTHORIZATION; +SELECT pg_file_unlink('regress_genfile2.txt'); + +-- Likewise for pg_logdir_ls(). Which log files exist is not deterministic, +-- so only assert that the call succeeds. +SET SESSION AUTHORIZATION regress_genfile_reader; +SELECT count(*) >= 0 AS ok FROM pg_logdir_ls() AS t(starttime timestamp, filename text); +RESET SESSION AUTHORIZATION; + +REVOKE ALL ON FUNCTION pg_file_write(text,text,boolean), + pg_file_rename(text,text,text), + pg_file_unlink(text), + pg_logdir_ls() FROM regress_genfile_plain; +DROP ROLE regress_genfile_plain, regress_genfile_writer, regress_genfile_reader; --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
