Greetings, * Michael Paquier (mich...@paquier.xyz) wrote: > On Sun, Mar 25, 2018 at 09:43:25PM -0400, Stephen Frost wrote: > > * Michael Paquier (mich...@paquier.xyz) wrote: > >> On Thu, Mar 08, 2018 at 10:15:11AM +0900, Michael Paquier wrote: > >> > Other than that the patch looks in pretty good shape to me. > >> > >> The regression tests of file_fdw are blowing up because of an error > >> string patch 2 changes. > > > > Fixed in the attached. > > Thanks for the updated version. This test is fixed.
Thanks for checking. Attached is an updated version which also includes the changes for adminpack, done in a similar manner to how pgstattuple was updated, as discussed. Regression tests updated and extended a bit, doc updates also included. If you get a chance to take a look, that'd be great. I'll do my own review of it again also after stepping away for a day or so. Thanks! Stephen
From 296b407863a7259a04e5e8cfc19f9b8ea124777c Mon Sep 17 00:00:00 2001 From: Stephen Frost <sfr...@snowman.net> Date: Wed, 7 Mar 2018 06:42:42 -0500 Subject: [PATCH 1/3] Remove explicit superuser checks in favor of ACLs This removes the explicit superuser checks in the various file-access functions in the backend, specifically pg_ls_dir(), pg_read_file(), pg_read_binary_file(), and pg_stat_file(). Instead, EXECUTE is REVOKE'd from public for these, meaning that only a superuser is able to run them by default, but access to them can be GRANT'd to other roles. Reviewed-By: Michael Paquier Discussion: https://postgr.es/m/20171231191939.GR2416%40tamriel.snowman.net --- src/backend/catalog/system_views.sql | 14 ++++++++++++++ src/backend/utils/adt/genfile.c | 20 -------------------- 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 5e6e8a64f6..559610b12f 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -1149,6 +1149,20 @@ REVOKE EXECUTE ON FUNCTION lo_export(oid, text) FROM public; REVOKE EXECUTE ON FUNCTION pg_ls_logdir() FROM public; REVOKE EXECUTE ON FUNCTION pg_ls_waldir() FROM public; +REVOKE EXECUTE ON FUNCTION pg_read_file(text) FROM public; +REVOKE EXECUTE ON FUNCTION pg_read_file(text,bigint,bigint) FROM public; +REVOKE EXECUTE ON FUNCTION pg_read_file(text,bigint,bigint,boolean) FROM public; + +REVOKE EXECUTE ON FUNCTION pg_read_binary_file(text) FROM public; +REVOKE EXECUTE ON FUNCTION pg_read_binary_file(text,bigint,bigint) FROM public; +REVOKE EXECUTE ON FUNCTION pg_read_binary_file(text,bigint,bigint,boolean) FROM public; + +REVOKE EXECUTE ON FUNCTION pg_stat_file(text) FROM public; +REVOKE EXECUTE ON FUNCTION pg_stat_file(text,boolean) FROM public; + +REVOKE EXECUTE ON FUNCTION pg_ls_dir(text) FROM public; +REVOKE EXECUTE ON FUNCTION pg_ls_dir(text,boolean,boolean) FROM public; + -- -- We also set up some things as accessible to standard roles. -- diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c index d9027fc688..a4c0f6d5ca 100644 --- a/src/backend/utils/adt/genfile.c +++ b/src/backend/utils/adt/genfile.c @@ -195,11 +195,6 @@ pg_read_file(PG_FUNCTION_ARGS) char *filename; text *result; - if (!superuser()) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - (errmsg("must be superuser to read files")))); - /* handle optional arguments */ if (PG_NARGS() >= 3) { @@ -236,11 +231,6 @@ pg_read_binary_file(PG_FUNCTION_ARGS) char *filename; bytea *result; - if (!superuser()) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - (errmsg("must be superuser to read files")))); - /* handle optional arguments */ if (PG_NARGS() >= 3) { @@ -313,11 +303,6 @@ pg_stat_file(PG_FUNCTION_ARGS) TupleDesc tupdesc; bool missing_ok = false; - if (!superuser()) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - (errmsg("must be superuser to get file information")))); - /* check the optional argument */ if (PG_NARGS() == 2) missing_ok = PG_GETARG_BOOL(1); @@ -399,11 +384,6 @@ pg_ls_dir(PG_FUNCTION_ARGS) directory_fctx *fctx; MemoryContext oldcontext; - if (!superuser()) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - (errmsg("must be superuser to get directory listings")))); - if (SRF_IS_FIRSTCALL()) { bool missing_ok = false; -- 2.14.1 From 76ba6f1eef402070ca1ff37f74e5dcfc639f6837 Mon Sep 17 00:00:00 2001 From: Stephen Frost <sfr...@snowman.net> Date: Sun, 31 Dec 2017 14:01:12 -0500 Subject: [PATCH 2/3] Add default roles for file/program access This patch adds new default roles names 'pg_read_server_files', 'pg_write_server_files', 'pg_execute_server_program' which allow an administrator to GRANT to a non-superuser role the ability to access server-side files or run programs through PostgreSQL (as the user the database is running as). Having one of these roles allows a non-superuser to use server-side COPY to read, write, or with a program, and to use file_fdw (if installed by a superuser and GRANT'd USAGE on it) to read from files or run a program. The existing misc file functions are also changed to allow a user with the 'pg_read_server_files' default role to read any files on the filesystem, matching the privileges given to that role through COPY and file_fdw above. Reviewed-By: Michael Paquier Discussion: https://postgr.es/m/20171231191939.GR2416%40tamriel.snowman.net --- contrib/file_fdw/file_fdw.c | 51 +++++++++++++++++++++------------ contrib/file_fdw/output/file_fdw.source | 2 +- doc/src/sgml/file-fdw.sgml | 8 ++++-- doc/src/sgml/func.sgml | 17 +++++------ doc/src/sgml/ref/copy.sgml | 8 ++++-- src/backend/commands/copy.c | 37 +++++++++++++++++------- src/backend/utils/adt/genfile.c | 10 +++++++ src/include/catalog/pg_authid.h | 6 ++++ 8 files changed, 96 insertions(+), 43 deletions(-) diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 3df6fc741d..2cf09aecf6 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -18,6 +18,7 @@ #include "access/htup_details.h" #include "access/reloptions.h" #include "access/sysattr.h" +#include "catalog/pg_authid.h" #include "catalog/pg_foreign_table.h" #include "commands/copy.h" #include "commands/defrem.h" @@ -201,24 +202,6 @@ file_fdw_validator(PG_FUNCTION_ARGS) List *other_options = NIL; ListCell *cell; - /* - * Only superusers are allowed to set options of a file_fdw foreign table. - * This is because we don't want non-superusers to be able to control - * which file gets read or which program gets executed. - * - * Putting this sort of permissions check in a validator is a bit of a - * crock, but there doesn't seem to be any other place that can enforce - * the check more cleanly. - * - * Note that the valid_options[] array disallows setting filename and - * program at any options level other than foreign table --- otherwise - * there'd still be a security hole. - */ - if (catalog == ForeignTableRelationId && !superuser()) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("only superuser can change options of a file_fdw foreign table"))); - /* * Check that only options supported by file_fdw, and allowed for the * current object type, are given. @@ -264,6 +247,38 @@ file_fdw_validator(PG_FUNCTION_ARGS) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"))); + + /* + * Check permissions for changing which file or program is used by + * the file_fdw. + * + * Only members of the role 'pg_read_server_files' are allowed to + * set the 'filename' option of a file_fdw foreign table, while + * only members of the role 'pg_execute_server_program' are + * allowed to set the 'program' option. This is because we don't + * want regular users to be able to control which file gets read + * or which program gets executed. + * + * Putting this sort of permissions check in a validator is a bit + * of a crock, but there doesn't seem to be any other place that + * can enforce the check more cleanly. + * + * Note that the valid_options[] array disallows setting filename + * and program at any options level other than foreign table --- + * otherwise there'd still be a security hole. + */ + if (strcmp(def->defname, "filename") == 0 && + !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_SERVER_FILES)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("only superuser or a member of the pg_read_server_files role may specify the filename option of a file_fdw foreign table"))); + + if (strcmp(def->defname, "program") == 0 && + !is_member_of_role(GetUserId(), DEFAULT_ROLE_EXECUTE_SERVER_PROGRAM)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("only superuser or a member of the pg_execute_server_program role may specify the program option of a file_fdw foreign table"))); + filename = defGetString(def); } diff --git a/contrib/file_fdw/output/file_fdw.source b/contrib/file_fdw/output/file_fdw.source index b92392fd25..f769b12cbd 100644 --- a/contrib/file_fdw/output/file_fdw.source +++ b/contrib/file_fdw/output/file_fdw.source @@ -422,7 +422,7 @@ ALTER FOREIGN TABLE agg_text OWNER TO regress_file_fdw_user; ALTER FOREIGN TABLE agg_text OPTIONS (SET format 'text'); SET ROLE regress_file_fdw_user; ALTER FOREIGN TABLE agg_text OPTIONS (SET format 'text'); -ERROR: only superuser can change options of a file_fdw foreign table +ERROR: only superuser or a member of the pg_read_server_files role may specify the filename option of a file_fdw foreign table SET ROLE regress_file_fdw_superuser; -- cleanup RESET ROLE; diff --git a/doc/src/sgml/file-fdw.sgml b/doc/src/sgml/file-fdw.sgml index e2598a07da..955a13ab7d 100644 --- a/doc/src/sgml/file-fdw.sgml +++ b/doc/src/sgml/file-fdw.sgml @@ -186,9 +186,11 @@ </para> <para> - Changing table-level options requires superuser privileges, for security - reasons: only a superuser should be able to control which file is read - or which program is run. In principle non-superusers could be allowed to + Changing table-level options requires being a superuser or having the privileges + of the default role <literal>pg_read_server_files</literal> (to use a filename) or + the default role <literal>pg_execute_server_programs</literal> (to use a program), + for security reasons: only certain users should be able to control which file is + read or which program is run. In principle regular users could be allowed to change the other options, but that's not supported at present. </para> diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 7b1a85fc71..5c6cf53600 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -19995,10 +19995,11 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); linkend="functions-admin-genfile-table"/> provide native access to files on the machine hosting the server. Only files within the database cluster directory and the <varname>log_directory</varname> can be - accessed. Use a relative path for files in the cluster directory, - and a path matching the <varname>log_directory</varname> configuration setting - for log files. Use of these functions is restricted to superusers - except where stated otherwise. + accessed unless the user is granted the role + <literal>pg_read_server_files</literal>, or + <literal>pg_execute_server_program</literal>. Use a relative path for files in + the cluster directory, and a path matching the <varname>log_directory</varname> + configuration setting for log files. </para> <table id="functions-admin-genfile-table"> @@ -20016,7 +20017,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); </entry> <entry><type>setof text</type></entry> <entry> - List the contents of a directory. + List the contents of a directory. Restricted to superusers by default, but other users can be granted EXECUTE to run the function. </entry> </row> <row> @@ -20047,7 +20048,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); </entry> <entry><type>text</type></entry> <entry> - Return the contents of a text file. + Return the contents of a text file. Restricted to superusers by default, but other users can be granted EXECUTE to run the function. </entry> </row> <row> @@ -20056,7 +20057,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); </entry> <entry><type>bytea</type></entry> <entry> - Return the contents of a file. + Return the contents of a file. Restricted to superusers by default, but other users can be granted EXECUTE to run the function. </entry> </row> <row> @@ -20065,7 +20066,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); </entry> <entry><type>record</type></entry> <entry> - Return information about a file. + Return information about a file. Restricted to superusers by default, but other users can be granted EXECUTE to run the function. </entry> </row> </tbody> diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index af2a0e91b9..344d391e4a 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -444,8 +444,12 @@ COPY <replaceable class="parameter">count</replaceable> by the server, not by the client application, must be executable by the <productname>PostgreSQL</productname> user. <command>COPY</command> naming a file or command is only allowed to - database superusers, since it allows reading or writing any file that the - server has privileges to access. + database superusers or users who are granted one of the default roles + <literal>pg_read_server_files</literal>, + <literal>pg_write_server_files</literal>, + or <literal>pg_execute_server_program</literal>, since it allows reading + or writing any file or running a program that the server has privileges to + access. </para> <para> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index a42861da0d..dfe3a00f59 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -23,6 +23,8 @@ #include "access/sysattr.h" #include "access/xact.h" #include "access/xlog.h" +#include "catalog/dependency.h" +#include "catalog/pg_authid.h" #include "catalog/pg_type.h" #include "commands/copy.h" #include "commands/defrem.h" @@ -769,8 +771,8 @@ CopyLoadRawBuf(CopyState cstate) * input/output stream. The latter could be either stdin/stdout or a * socket, depending on whether we're running under Postmaster control. * - * Do not allow a Postgres user without superuser privilege to read from - * or write to a file. + * Do not allow a Postgres user without the 'pg_access_server_files' role to + * read from or write to a file. * * Do not allow the copy if user doesn't have proper permission to access * the table or the specifically requested columns. @@ -787,21 +789,34 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, Oid relid; RawStmt *query = NULL; - /* Disallow COPY to/from file or program except to superusers. */ - if (!pipe && !superuser()) + /* + * Disallow COPY to/from file or program except to users with the + * appropriate role. + */ + if (!pipe) { - if (stmt->is_program) + if (stmt->is_program && !is_member_of_role(GetUserId(), DEFAULT_ROLE_EXECUTE_SERVER_PROGRAM)) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("must be superuser to COPY to or from an external program"), + errmsg("must be superuser or a member of the pg_execute_server_program role to COPY to or from an external program"), errhint("Anyone can COPY to stdout or from stdin. " "psql's \\copy command also works for anyone."))); else - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("must be superuser to COPY to or from a file"), - errhint("Anyone can COPY to stdout or from stdin. " - "psql's \\copy command also works for anyone."))); + { + if (is_from && !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_SERVER_FILES)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("must be superuser or a member of the pg_read_server_files role to COPY from a file"), + errhint("Anyone can COPY to stdout or from stdin. " + "psql's \\copy command also works for anyone."))); + + if (!is_from && !is_member_of_role(GetUserId(), DEFAULT_ROLE_WRITE_SERVER_FILES)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("must be superuser or a member of the pg_write_server_files role to COPY to a file"), + errhint("Anyone can COPY to stdout or from stdin. " + "psql's \\copy command also works for anyone."))); + } } if (stmt->relation) diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c index a4c0f6d5ca..c3874ad4d6 100644 --- a/src/backend/utils/adt/genfile.c +++ b/src/backend/utils/adt/genfile.c @@ -22,6 +22,7 @@ #include "access/htup_details.h" #include "access/xlog_internal.h" +#include "catalog/pg_authid.h" #include "catalog/pg_type.h" #include "funcapi.h" #include "mb/pg_wchar.h" @@ -54,6 +55,15 @@ convert_and_check_filename(text *arg) filename = text_to_cstring(arg); canonicalize_path(filename); /* filename can change length here */ + /* + * Members of the 'pg_read_server_files' role are allowed to access any + * files on the server as the PG user, so no need to do any further checks + * here. + */ + if (is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_SERVER_FILES)) + return filename; + + /* User isn't a member of the default role, so check if it's allowable */ if (is_absolute_path(filename)) { /* Disallow '/a/b/data/..' */ diff --git a/src/include/catalog/pg_authid.h b/src/include/catalog/pg_authid.h index 772e9153c4..956ba9b657 100644 --- a/src/include/catalog/pg_authid.h +++ b/src/include/catalog/pg_authid.h @@ -108,6 +108,12 @@ DATA(insert OID = 3375 ( "pg_read_all_stats" f t f f f f f -1 _null_ _null_)); #define DEFAULT_ROLE_READ_ALL_STATS 3375 DATA(insert OID = 3377 ( "pg_stat_scan_tables" f t f f f f f -1 _null_ _null_)); #define DEFAULT_ROLE_STAT_SCAN_TABLES 3377 +DATA(insert OID = 3556 ( "pg_read_server_files" f t f f f f f -1 _null_ _null_)); +#define DEFAULT_ROLE_READ_SERVER_FILES 3556 +DATA(insert OID = 3696 ( "pg_write_server_files" f t f f f f f -1 _null_ _null_)); +#define DEFAULT_ROLE_WRITE_SERVER_FILES 3696 +DATA(insert OID = 3877 ( "pg_execute_server_program" f t f f f f f -1 _null_ _null_)); +#define DEFAULT_ROLE_EXECUTE_SERVER_PROGRAM 3877 DATA(insert OID = 4200 ( "pg_signal_backend" f t f f f f f -1 _null_ _null_)); #define DEFAULT_ROLE_SIGNAL_BACKENDID 4200 -- 2.14.1 From c9080b0b49b80e74e10fcbdd469ce2499cdf8a4d Mon Sep 17 00:00:00 2001 From: Stephen Frost <sfr...@snowman.net> Date: Sun, 1 Apr 2018 09:27:31 -0400 Subject: [PATCH 3/3] Support new default roles with adminpack This provides a newer version of adminpack which works with the newly added default roles to support GRANT'ing to non-superusers access to read and write files, along with related functions (unlinking files, getting file length, renaming/removing files, scanning the log file directory) which are supported through adminpack. Note that new versions of the functions are required because an environment might have an updated version of the library but still have the old adminpack 1.0 catalog definitions (where EXECUTE is GRANT'd to PUBLIC for the functions). --- contrib/adminpack/Makefile | 2 +- contrib/adminpack/adminpack--1.0--1.1.sql | 67 ++++++++ contrib/adminpack/adminpack.c | 245 +++++++++++++++++++++++++++--- contrib/adminpack/adminpack.control | 2 +- contrib/adminpack/expected/adminpack.out | 28 +++- contrib/adminpack/sql/adminpack.sql | 18 ++- doc/src/sgml/adminpack.sgml | 9 +- src/backend/utils/adt/genfile.c | 53 ++++++- src/backend/utils/adt/misc.c | 27 +++- src/include/catalog/pg_proc.h | 8 +- 10 files changed, 417 insertions(+), 42 deletions(-) create mode 100644 contrib/adminpack/adminpack--1.0--1.1.sql diff --git a/contrib/adminpack/Makefile b/contrib/adminpack/Makefile index 89c249bc0d..afcfac4103 100644 --- a/contrib/adminpack/Makefile +++ b/contrib/adminpack/Makefile @@ -5,7 +5,7 @@ OBJS = adminpack.o $(WIN32RES) PG_CPPFLAGS = -I$(libpq_srcdir) EXTENSION = adminpack -DATA = adminpack--1.0.sql +DATA = adminpack--1.0.sql adminpack--1.0--1.1.sql PGFILEDESC = "adminpack - support functions for pgAdmin" REGRESS = adminpack diff --git a/contrib/adminpack/adminpack--1.0--1.1.sql b/contrib/adminpack/adminpack--1.0--1.1.sql new file mode 100644 index 0000000000..0219db3028 --- /dev/null +++ b/contrib/adminpack/adminpack--1.0--1.1.sql @@ -0,0 +1,67 @@ +/* contrib/adminpack/adminpack--1.0--1.1.sql */ + +-- complain if script is sourced in psql, rather than via ALTER EXTENSION +\echo Use "ALTER EXTENSION adminpack UPDATE TO '1.1'" to load this file. \quit + +/* *********************************************** + * Administrative functions for PostgreSQL + * *********************************************** */ + +/* generic file access functions */ + +CREATE OR REPLACE FUNCTION pg_catalog.pg_file_write(text, text, bool) +RETURNS bigint +AS 'MODULE_PATHNAME', 'pg_file_write_v1_1' +LANGUAGE C VOLATILE STRICT; + +REVOKE EXECUTE ON FUNCTION pg_catalog.pg_file_write(text, text, bool) FROM PUBLIC; + +CREATE OR REPLACE FUNCTION pg_catalog.pg_file_rename(text, text, text) +RETURNS bool +AS 'MODULE_PATHNAME', 'pg_file_rename_v1_1' +LANGUAGE C VOLATILE; + +REVOKE EXECUTE ON FUNCTION pg_catalog.pg_file_rename(text, text, text) FROM PUBLIC; + +CREATE OR REPLACE FUNCTION pg_catalog.pg_file_rename(text, text) +RETURNS bool +AS 'SELECT pg_catalog.pg_file_rename($1, $2, NULL::pg_catalog.text);' +LANGUAGE SQL VOLATILE STRICT; + +CREATE OR REPLACE FUNCTION pg_catalog.pg_file_unlink(text) +RETURNS bool +AS 'MODULE_PATHNAME', 'pg_file_unlink_v1_1' +LANGUAGE C VOLATILE STRICT; + +REVOKE EXECUTE ON FUNCTION pg_catalog.pg_file_unlink(text) FROM PUBLIC; + +CREATE OR REPLACE FUNCTION pg_catalog.pg_logdir_ls() +RETURNS setof record +AS 'MODULE_PATHNAME', 'pg_logdir_ls_v1_1' +LANGUAGE C VOLATILE STRICT; + +REVOKE EXECUTE ON FUNCTION pg_catalog.pg_logdir_ls() FROM PUBLIC; + +/* Renaming of existing backend functions for pgAdmin compatibility */ + +CREATE OR REPLACE FUNCTION pg_catalog.pg_file_read(text, bigint, bigint) +RETURNS text +AS 'pg_read_file_v2' +LANGUAGE INTERNAL VOLATILE STRICT; + +REVOKE EXECUTE ON FUNCTION pg_catalog.pg_file_read(text, bigint, bigint) FROM PUBLIC; + +CREATE OR REPLACE FUNCTION pg_catalog.pg_file_length(text) +RETURNS bigint +AS 'SELECT size FROM pg_catalog.pg_stat_file($1)' +LANGUAGE SQL VOLATILE STRICT; + +REVOKE EXECUTE ON FUNCTION pg_catalog.pg_file_length(text) FROM PUBLIC; + +CREATE OR REPLACE FUNCTION pg_catalog.pg_logfile_rotate() +RETURNS int4 +AS 'pg_rotate_logfile_v2' +LANGUAGE INTERNAL VOLATILE STRICT; + +REVOKE EXECUTE ON FUNCTION pg_catalog.pg_logfile_rotate() FROM PUBLIC; + diff --git a/contrib/adminpack/adminpack.c b/contrib/adminpack/adminpack.c index 1785dee3a1..122758de81 100644 --- a/contrib/adminpack/adminpack.c +++ b/contrib/adminpack/adminpack.c @@ -18,6 +18,7 @@ #include <sys/stat.h> #include <unistd.h> +#include "catalog/pg_authid.h" #include "catalog/pg_type.h" #include "funcapi.h" #include "miscadmin.h" @@ -41,9 +42,17 @@ PG_MODULE_MAGIC; PG_FUNCTION_INFO_V1(pg_file_write); +PG_FUNCTION_INFO_V1(pg_file_write_v1_1); PG_FUNCTION_INFO_V1(pg_file_rename); +PG_FUNCTION_INFO_V1(pg_file_rename_v1_1); PG_FUNCTION_INFO_V1(pg_file_unlink); +PG_FUNCTION_INFO_V1(pg_file_unlink_v1_1); PG_FUNCTION_INFO_V1(pg_logdir_ls); +PG_FUNCTION_INFO_V1(pg_logdir_ls_v1_1); + +int64 pg_file_write_internal(text *file, text *data, bool replace); +bool pg_file_rename_internal(text *file1, text *file2, text *file3); +Datum pg_logdir_ls_internal(FunctionCallInfo fcinfo); typedef struct { @@ -68,6 +77,15 @@ convert_and_check_filename(text *arg, bool logAllowed) canonicalize_path(filename); /* filename can change length here */ + /* + * Members of the 'pg_read_server_files' role are allowed to access any + * files on the server as the PG user, so no need to do any further checks + * here. + */ + if (is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_SERVER_FILES)) + return filename; + + /* User isn't a member of the default role, so check if it's allowable */ if (is_absolute_path(filename)) { /* Disallow '/a/b/data/..' */ @@ -111,23 +129,64 @@ requireSuperuser(void) /* ------------------------------------ - * generic file handling functions + * pg_file_write - old version + * + * The superuser() check here must be kept as the library might be upgraded + * without the extension being upgraded, meaning that in pre-1.1 installations + * these functions could be called by any user. */ - Datum pg_file_write(PG_FUNCTION_ARGS) { - FILE *f; - char *filename; - text *data; + text *file = PG_GETARG_TEXT_PP(0); + text *data = PG_GETARG_TEXT_PP(1); + bool replace = PG_GETARG_BOOL(2); int64 count = 0; requireSuperuser(); - filename = convert_and_check_filename(PG_GETARG_TEXT_PP(0), false); - data = PG_GETARG_TEXT_PP(1); + count = pg_file_write_internal(file, data, replace); + + PG_RETURN_INT64(count); +} + +/* ------------------------------------ + * pg_file_write_v1_1 - Version 1.1 + * + * As of adminpack version 1.1, we no longer need to check if the user + * is a superuser because we REVOKE EXECUTE on the function from PUBLIC. + * Users can then grant access to it based on their policies. + * + * Otherwise identical to pg_file_write (above). + */ +Datum +pg_file_write_v1_1(PG_FUNCTION_ARGS) +{ + text *file = PG_GETARG_TEXT_PP(0); + text *data = PG_GETARG_TEXT_PP(1); + bool replace = PG_GETARG_BOOL(2); + int64 count = 0; - if (!PG_GETARG_BOOL(2)) + count = pg_file_write_internal(file, data, replace); + + PG_RETURN_INT64(count); +} + +/* ------------------------------------ + * pg_file_write_internal - Workhorse for pg_file_write functions. + * + * This handles the actual work for pg_file_write. + */ +int64 +pg_file_write_internal(text *file, text *data, bool replace) +{ + FILE *f; + char *filename; + int64 count = 0; + + filename = convert_and_check_filename(file, false); + + if (!replace) { struct stat fst; @@ -153,29 +212,95 @@ pg_file_write(PG_FUNCTION_ARGS) (errcode_for_file_access(), errmsg("could not write file \"%s\": %m", filename))); - PG_RETURN_INT64(count); + return (count); } - +/* ------------------------------------ + * pg_file_rename - old version + * + * The superuser() check here must be kept as the library might be upgraded + * without the extension being upgraded, meaning that in pre-1.1 installations + * these functions could be called by any user. + */ Datum pg_file_rename(PG_FUNCTION_ARGS) { - char *fn1, - *fn2, - *fn3; - int rc; + text *file1; + text *file2; + text *file3; + bool result; + + if (PG_ARGISNULL(0) || PG_ARGISNULL(1)) + PG_RETURN_NULL(); + + file1 = PG_GETARG_TEXT_PP(0); + file2 = PG_GETARG_TEXT_PP(1); + + if (PG_ARGISNULL(2)) + file3 = NULL; + else + file3 = PG_GETARG_TEXT_PP(2); requireSuperuser(); + result = pg_file_rename_internal(file1, file2, file3); + + PG_RETURN_BOOL(result); +} + +/* ------------------------------------ + * pg_file_rename_v1_1 - Version 1.1 + * + * As of adminpack version 1.1, we no longer need to check if the user + * is a superuser because we REVOKE EXECUTE on the function from PUBLIC. + * Users can then grant access to it based on their policies. + * + * Otherwise identical to pg_file_write (above). + */ +Datum +pg_file_rename_v1_1(PG_FUNCTION_ARGS) +{ + text *file1; + text *file2; + text *file3; + bool result; + if (PG_ARGISNULL(0) || PG_ARGISNULL(1)) PG_RETURN_NULL(); - fn1 = convert_and_check_filename(PG_GETARG_TEXT_PP(0), false); - fn2 = convert_and_check_filename(PG_GETARG_TEXT_PP(1), false); + file1 = PG_GETARG_TEXT_PP(0); + file2 = PG_GETARG_TEXT_PP(1); + if (PG_ARGISNULL(2)) + file3 = NULL; + else + file3 = PG_GETARG_TEXT_PP(2); + + result = pg_file_rename_internal(file1, file2, file3); + + PG_RETURN_BOOL(result); +} + +/* ------------------------------------ + * pg_file_rename_internal - Workhorse for pg_file_rename functions. + * + * This handles the actual work for pg_file_rename. + */ +bool +pg_file_rename_internal(text *file1, text *file2, text *file3) +{ + char *fn1, + *fn2, + *fn3; + int rc; + + fn1 = convert_and_check_filename(file1, false); + fn2 = convert_and_check_filename(file2, false); + + if (file3 == NULL) fn3 = 0; else - fn3 = convert_and_check_filename(PG_GETARG_TEXT_PP(2), false); + fn3 = convert_and_check_filename(file3, false); if (access(fn1, W_OK) < 0) { @@ -183,7 +308,7 @@ pg_file_rename(PG_FUNCTION_ARGS) (errcode_for_file_access(), errmsg("file \"%s\" is not accessible: %m", fn1))); - PG_RETURN_BOOL(false); + return false; } if (fn3 && access(fn2, W_OK) < 0) @@ -192,7 +317,7 @@ pg_file_rename(PG_FUNCTION_ARGS) (errcode_for_file_access(), errmsg("file \"%s\" is not accessible: %m", fn2))); - PG_RETURN_BOOL(false); + return false; } rc = access(fn3 ? fn3 : fn2, 2); @@ -243,10 +368,17 @@ pg_file_rename(PG_FUNCTION_ARGS) errmsg("could not rename \"%s\" to \"%s\": %m", fn1, fn2))); } - PG_RETURN_BOOL(true); + return true; } +/* ------------------------------------ + * pg_file_unlink - old version + * + * The superuser() check here must be kept as the library might be upgraded + * without the extension being upgraded, meaning that in pre-1.1 installations + * these functions could be called by any user. + */ Datum pg_file_unlink(PG_FUNCTION_ARGS) { @@ -278,18 +410,83 @@ pg_file_unlink(PG_FUNCTION_ARGS) } +/* ------------------------------------ + * pg_file_unlink_v1_1 - Version 1.1 + * + * As of adminpack version 1.1, we no longer need to check if the user + * is a superuser because we REVOKE EXECUTE on the function from PUBLIC. + * Users can then grant access to it based on their policies. + * + * Otherwise identical to pg_file_unlink (above). + */ Datum -pg_logdir_ls(PG_FUNCTION_ARGS) +pg_file_unlink_v1_1(PG_FUNCTION_ARGS) { - FuncCallContext *funcctx; - struct dirent *de; - directory_fctx *fctx; + char *filename; + + filename = convert_and_check_filename(PG_GETARG_TEXT_PP(0), false); + + if (access(filename, W_OK) < 0) + { + if (errno == ENOENT) + PG_RETURN_BOOL(false); + else + ereport(ERROR, + (errcode_for_file_access(), + errmsg("file \"%s\" is not accessible: %m", filename))); + } + if (unlink(filename) < 0) + { + ereport(WARNING, + (errcode_for_file_access(), + errmsg("could not unlink file \"%s\": %m", filename))); + + PG_RETURN_BOOL(false); + } + PG_RETURN_BOOL(true); +} + +/* ------------------------------------ + * pg_logdir_ls - Old version + * + * The superuser() check here must be kept as the library might be upgraded + * without the extension being upgraded, meaning that in pre-1.1 installations + * these functions could be called by any user. + */ +Datum +pg_logdir_ls(PG_FUNCTION_ARGS) +{ if (!superuser()) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), (errmsg("only superuser can list the log directory")))); + return(pg_logdir_ls_internal(fcinfo)); +} + +/* ------------------------------------ + * pg_logdir_ls_v1_1 - Version 1.1 + * + * As of adminpack version 1.1, we no longer need to check if the user + * is a superuser because we REVOKE EXECUTE on the function from PUBLIC. + * Users can then grant access to it based on their policies. + * + * Otherwise identical to pg_logdir_ls (above). + */ +Datum +pg_logdir_ls_v1_1(PG_FUNCTION_ARGS) +{ + return(pg_logdir_ls_internal(fcinfo)); +} + +Datum +pg_logdir_ls_internal(FunctionCallInfo fcinfo) +{ + FuncCallContext *funcctx; + struct dirent *de; + directory_fctx *fctx; + if (strcmp(Log_filename, "postgresql-%Y-%m-%d_%H%M%S.log") != 0) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), diff --git a/contrib/adminpack/adminpack.control b/contrib/adminpack/adminpack.control index c79413f378..71f6ad5ddf 100644 --- a/contrib/adminpack/adminpack.control +++ b/contrib/adminpack/adminpack.control @@ -1,6 +1,6 @@ # adminpack extension comment = 'administrative functions for PostgreSQL' -default_version = '1.0' +default_version = '1.1' module_pathname = '$libdir/adminpack' relocatable = false schema = pg_catalog diff --git a/contrib/adminpack/expected/adminpack.out b/contrib/adminpack/expected/adminpack.out index b0d72ddab2..5bade5d001 100644 --- a/contrib/adminpack/expected/adminpack.out +++ b/contrib/adminpack/expected/adminpack.out @@ -34,7 +34,19 @@ SELECT pg_read_file('test_file1'); test1test1 (1 row) --- disallowed file paths +-- get length +SELECT pg_file_length('test_file1'); + pg_file_length +---------------- + 10 +(1 row) + +-- disallowed file paths for non-superusers and users who are +-- not members of pg_read_server_files +CREATE ROLE regress_user1; +GRANT pg_read_all_settings TO regress_user1; +GRANT EXECUTE ON FUNCTION pg_file_write(text,text,bool) TO regress_user1; +SET ROLE regress_user1; SELECT pg_file_write('../test_file0', 'test0', false); ERROR: path must be in or below the current directory SELECT pg_file_write('/tmp/test_file0', 'test0', false); @@ -47,6 +59,10 @@ SELECT pg_file_write(current_setting('data_directory') || '/test_file4', 'test4' SELECT pg_file_write(current_setting('data_directory') || '/../test_file4', 'test4', false); ERROR: reference to parent directory ("..") not allowed +RESET ROLE; +REVOKE EXECUTE ON FUNCTION pg_file_write(text,text,bool) FROM regress_user1; +REVOKE pg_read_all_settings FROM regress_user1; +DROP ROLE regress_user1; -- rename file SELECT pg_file_rename('test_file1', 'test_file2'); pg_file_rename @@ -132,14 +148,16 @@ SELECT pg_file_unlink('test_file4'); CREATE USER regress_user1; SET ROLE regress_user1; SELECT pg_file_write('test_file0', 'test0', false); -ERROR: only superuser may access generic file functions +ERROR: permission denied for function pg_file_write SELECT pg_file_rename('test_file0', 'test_file0'); -ERROR: only superuser may access generic file functions +ERROR: permission denied for function pg_file_rename CONTEXT: SQL function "pg_file_rename" statement 1 SELECT pg_file_unlink('test_file0'); -ERROR: only superuser may access generic file functions +ERROR: permission denied for function pg_file_unlink SELECT pg_logdir_ls(); -ERROR: only superuser can list the log directory +ERROR: permission denied for function pg_logdir_ls +SELECT pg_file_length('test_file0'); +ERROR: permission denied for function pg_file_length RESET ROLE; DROP USER regress_user1; -- no further tests for pg_logdir_ls() because it depends on the diff --git a/contrib/adminpack/sql/adminpack.sql b/contrib/adminpack/sql/adminpack.sql index 13621bd043..753b5b458d 100644 --- a/contrib/adminpack/sql/adminpack.sql +++ b/contrib/adminpack/sql/adminpack.sql @@ -12,12 +12,25 @@ SELECT pg_read_file('test_file1'); SELECT pg_file_write('test_file1', 'test1', false); SELECT pg_read_file('test_file1'); --- disallowed file paths +-- get length +SELECT pg_file_length('test_file1'); + +-- disallowed file paths for non-superusers and users who are +-- not members of pg_read_server_files +CREATE ROLE regress_user1; + +GRANT pg_read_all_settings TO regress_user1; +GRANT EXECUTE ON FUNCTION pg_file_write(text,text,bool) TO regress_user1; + +SET ROLE regress_user1; SELECT pg_file_write('../test_file0', 'test0', false); SELECT pg_file_write('/tmp/test_file0', 'test0', false); SELECT pg_file_write(current_setting('data_directory') || '/test_file4', 'test4', false); SELECT pg_file_write(current_setting('data_directory') || '/../test_file4', 'test4', false); - +RESET ROLE; +REVOKE EXECUTE ON FUNCTION pg_file_write(text,text,bool) FROM regress_user1; +REVOKE pg_read_all_settings FROM regress_user1; +DROP ROLE regress_user1; -- rename file SELECT pg_file_rename('test_file1', 'test_file2'); @@ -51,6 +64,7 @@ SELECT pg_file_write('test_file0', 'test0', false); SELECT pg_file_rename('test_file0', 'test_file0'); SELECT pg_file_unlink('test_file0'); SELECT pg_logdir_ls(); +SELECT pg_file_length('test_file0'); RESET ROLE; DROP USER regress_user1; diff --git a/doc/src/sgml/adminpack.sgml b/doc/src/sgml/adminpack.sgml index 1197eefbf3..447cce1e3c 100644 --- a/doc/src/sgml/adminpack.sgml +++ b/doc/src/sgml/adminpack.sgml @@ -12,7 +12,8 @@ <application>pgAdmin</application> and other administration and management tools can use to provide additional functionality, such as remote management of server log files. - Use of all these functions is restricted to superusers. + Use of all these functions is only allowed to the superuser by default but may be + allowed to other users by using the <command>GRANT</command> command. </para> <para> @@ -20,8 +21,10 @@ write access to files on the machine hosting the server. (See also the functions in <xref linkend="functions-admin-genfile-table"/>, which provide read-only access.) - Only files within the database cluster directory can be accessed, but - either a relative or absolute path is allowable. + Only files within the database cluster directory can be accessed, unless the + user is a superuser or given one of the pg_read_server_files, or pg_write_server_files + roles, as appropriate for the function, but either a relative or absolute path is + allowable. </para> <table id="functions-adminpack-table"> diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c index c3874ad4d6..b0814b7a0b 100644 --- a/src/backend/utils/adt/genfile.c +++ b/src/backend/utils/adt/genfile.c @@ -194,6 +194,8 @@ read_text_file(const char *filename, int64 seek_offset, int64 bytes_to_read, /* * Read a section of a file, returning it as text + * + * This function is kept to support adminpack 1.0. */ Datum pg_read_file(PG_FUNCTION_ARGS) @@ -205,6 +207,51 @@ pg_read_file(PG_FUNCTION_ARGS) char *filename; text *result; + if (!superuser()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + (errmsg("must be superuser to read files with adminpack 1.0"), + errhint("Consider using pg_file_read(), which is part of core, instead.")))); + + /* handle optional arguments */ + if (PG_NARGS() >= 3) + { + seek_offset = PG_GETARG_INT64(1); + bytes_to_read = PG_GETARG_INT64(2); + + if (bytes_to_read < 0) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("requested length cannot be negative"))); + } + if (PG_NARGS() >= 4) + missing_ok = PG_GETARG_BOOL(3); + + filename = convert_and_check_filename(filename_t); + + result = read_text_file(filename, seek_offset, bytes_to_read, missing_ok); + if (result) + PG_RETURN_TEXT_P(result); + else + PG_RETURN_NULL(); +} + +/* + * Read a section of a file, returning it as text + * + * No superuser check done here- instead privileges are handled by the + * GRANT system. + */ +Datum +pg_read_file_v2(PG_FUNCTION_ARGS) +{ + text *filename_t = PG_GETARG_TEXT_PP(0); + int64 seek_offset = 0; + int64 bytes_to_read = -1; + bool missing_ok = false; + char *filename; + text *result; + /* handle optional arguments */ if (PG_NARGS() >= 3) { @@ -267,7 +314,7 @@ pg_read_binary_file(PG_FUNCTION_ARGS) /* - * Wrapper functions for the 1 and 3 argument variants of pg_read_file() + * Wrapper functions for the 1 and 3 argument variants of pg_read_file_v2() * and pg_binary_read_file(). * * These are necessary to pass the sanity check in opr_sanity, which checks @@ -277,13 +324,13 @@ pg_read_binary_file(PG_FUNCTION_ARGS) Datum pg_read_file_off_len(PG_FUNCTION_ARGS) { - return pg_read_file(fcinfo); + return pg_read_file_v2(fcinfo); } Datum pg_read_file_all(PG_FUNCTION_ARGS) { - return pg_read_file(fcinfo); + return pg_read_file_v2(fcinfo); } Datum diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c index 2e1e020c4b..e1e9bdd3fe 100644 --- a/src/backend/utils/adt/misc.c +++ b/src/backend/utils/adt/misc.c @@ -341,6 +341,31 @@ pg_reload_conf(PG_FUNCTION_ARGS) } +/* + * Rotate log file + * + * This function is kept to support adminpack 1.0. + */ +Datum +pg_rotate_logfile(PG_FUNCTION_ARGS) +{ + if (!superuser()) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + (errmsg("must be superuser to rotate log files with adminpack 1.0"), + errhint("Consider using pg_logfile_rotate(), which is part of core, instead.")))); + + if (!Logging_collector) + { + ereport(WARNING, + (errmsg("rotation not possible because log collection not active"))); + PG_RETURN_BOOL(false); + } + + SendPostmasterSignal(PMSIGNAL_ROTATE_LOGFILE); + PG_RETURN_BOOL(true); +} + /* * Rotate log file * @@ -348,7 +373,7 @@ pg_reload_conf(PG_FUNCTION_ARGS) * GRANT system. */ Datum -pg_rotate_logfile(PG_FUNCTION_ARGS) +pg_rotate_logfile_v2(PG_FUNCTION_ARGS) { if (!Logging_collector) { diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h index bfc90098f8..acc6066d2e 100644 --- a/src/include/catalog/pg_proc.h +++ b/src/include/catalog/pg_proc.h @@ -3338,8 +3338,10 @@ DESCR("true if wal replay is paused"); DATA(insert OID = 2621 ( pg_reload_conf PGNSP PGUID 12 1 0 0 0 f f f t f v s 0 0 16 "" _null_ _null_ _null_ _null_ _null_ pg_reload_conf _null_ _null_ _null_ )); DESCR("reload configuration files"); -DATA(insert OID = 2622 ( pg_rotate_logfile PGNSP PGUID 12 1 0 0 0 f f f t f v s 0 0 16 "" _null_ _null_ _null_ _null_ _null_ pg_rotate_logfile _null_ _null_ _null_ )); +DATA(insert OID = 2622 ( pg_rotate_logfile PGNSP PGUID 12 1 0 0 0 f f f t f v s 0 0 16 "" _null_ _null_ _null_ _null_ _null_ pg_rotate_logfile_v2 _null_ _null_ _null_ )); DESCR("rotate log file"); +DATA(insert OID = 3437 ( pg_rotate_logfile_old PGNSP PGUID 12 1 0 0 0 f f f t f v s 0 0 16 "" _null_ _null_ _null_ _null_ _null_ pg_rotate_logfile _null_ _null_ _null_ )); +DESCR("rotate log file - old version for adminpack 1.0"); DATA(insert OID = 3800 ( pg_current_logfile PGNSP PGUID 12 1 0 0 0 f f f f f v s 0 0 25 "" _null_ _null_ _null_ _null_ _null_ pg_current_logfile _null_ _null_ _null_ )); DESCR("current logging collector file location"); DATA(insert OID = 3801 ( pg_current_logfile PGNSP PGUID 12 1 0 0 0 f f f f f v s 1 0 25 "25" _null_ _null_ _null_ _null_ _null_ pg_current_logfile_1arg _null_ _null_ _null_ )); @@ -3351,8 +3353,10 @@ DATA(insert OID = 3307 ( pg_stat_file PGNSP PGUID 12 1 0 0 0 f f f t f v s 2 0 DESCR("get information about file"); DATA(insert OID = 2624 ( pg_read_file PGNSP PGUID 12 1 0 0 0 f f f t f v s 3 0 25 "25 20 20" _null_ _null_ _null_ _null_ _null_ pg_read_file_off_len _null_ _null_ _null_ )); DESCR("read text from a file"); -DATA(insert OID = 3293 ( pg_read_file PGNSP PGUID 12 1 0 0 0 f f f t f v s 4 0 25 "25 20 20 16" _null_ _null_ _null_ _null_ _null_ pg_read_file _null_ _null_ _null_ )); +DATA(insert OID = 3293 ( pg_read_file PGNSP PGUID 12 1 0 0 0 f f f t f v s 4 0 25 "25 20 20 16" _null_ _null_ _null_ _null_ _null_ pg_read_file_v2 _null_ _null_ _null_ )); DESCR("read text from a file"); +DATA(insert OID = 3438 ( pg_read_file_old PGNSP PGUID 12 1 0 0 0 f f f t f v s 4 0 25 "25 20 20 16" _null_ _null_ _null_ _null_ _null_ pg_read_file _null_ _null_ _null_ )); +DESCR("read text from a file - old version for adminpack 1.0"); DATA(insert OID = 3826 ( pg_read_file PGNSP PGUID 12 1 0 0 0 f f f t f v s 1 0 25 "25" _null_ _null_ _null_ _null_ _null_ pg_read_file_all _null_ _null_ _null_ )); DESCR("read text from a file"); DATA(insert OID = 3827 ( pg_read_binary_file PGNSP PGUID 12 1 0 0 0 f f f t f v s 3 0 17 "25 20 20" _null_ _null_ _null_ _null_ _null_ pg_read_binary_file_off_len _null_ _null_ _null_ )); -- 2.14.1
signature.asc
Description: PGP signature
