This is an automated email from the ASF dual-hosted git repository.
yjhjstz pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/cloudberry.git
The following commit(s) were added to refs/heads/main by this push:
new d01cb3ce955 Restrict pg_file_write/rename/unlink and pg_logdir_ls to
privileged roles
d01cb3ce955 is described below
commit d01cb3ce9555e91025f7426f618dcbe0f6444f0d
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 | 26 +++++++++
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, 192 insertions(+), 9 deletions(-)
diff --git a/src/backend/catalog/system_functions.sql
b/src/backend/catalog/system_functions.sql
index 39c4c309a0b..5cb5958557f 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -747,11 +747,37 @@ REVOKE EXECUTE ON FUNCTION pg_ls_replslotdir(text) 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 e378a9419ad..9b15557818b 100644
--- a/src/backend/utils/adt/genfile.c
+++ b/src/backend/utils/adt/genfile.c
@@ -129,6 +129,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
*
@@ -709,8 +739,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)
@@ -720,6 +749,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);
@@ -850,8 +881,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)
@@ -861,6 +891,8 @@ pg_file_rename_v1_1(PG_FUNCTION_ARGS)
text *file3;
bool result;
+ requireWriteServerFiles();
+
if (PG_ARGISNULL(0) || PG_ARGISNULL(1))
PG_RETURN_NULL();
@@ -915,14 +947,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)
@@ -1104,12 +1137,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 f8e9c70bc8d..0096b72b20c 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -90,7 +90,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]
