Greetings Michael, * Michael Paquier (mich...@paquier.xyz) wrote: > On Tue, Mar 06, 2018 at 10:00:54AM -0500, Stephen Frost wrote: > > Attached is an updated patch which splits up the permissions as > > suggested up-thread by Magnus: > > > > The default roles added are: > > > > * pg_read_server_files > > * pg_write_server_files > > * pg_execute_server_program > > > > Reviews certainly welcome. > > It seems to me that we have two different concepts here in one single > patch: > 1) Replace superuser checks by execution ACLs for FS-related functions. > 2) Introduce new administration roles to control COPY and file_fdw
> First, could it be possible to do a split for 1) and 2)? Done, because it was less work than arguing about it, but I'm not convinced that we really need to split out patches to this level of granularity. Perhaps something to consider for the future. > + /* > + * 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; > Second, I don't quite understand what is the link between COPY/file_fdw > and the possibility to use absolute paths in all the functions of > genfile.c. Is the use-case the possibility to check for the existence > of a file using pg_stat_file before doing a copy? This seems rather > limited because COPY or file_fdw would complain similarly for a missing > file. So I don't quite get the use-case for authorizing that. There's absolutely a use-case for being able to work with files outside of the data directory using the misc file functions, which is what's being addressed here, while also bringing into line the privileges given to this new default role. To address the use-case of accessing files generically through pg_read_file() or pg_read_binary_file() by having to go through COPY instead would be unnecessairly complex. Consider a case like postgresql.conf residing outside of the data directory. For an application to be able to read that with pg_read_file() is very straight-forward and applications already exist which do. Forcing that application to go through COPY would require creating a TEMP table and then coming up with a COPY command that doesn't actually *do* what COPY is meant to do- that is, parse the file. By default, you'd get errors from such a COPY command as it would think there's extra columns defined in the "copy-format" or "csv" or what-have-you file. > Could you update the documentation of pg_rewind please? It seems to me > that with your patch then superuser rights are not necessary mandatory, This will require actual testing to be done before I'd make such a change. I'll see if I can do that soon, but I also wouldn't complain if someone else wanted to actually go through and set that up and test that it works. Thanks! Stephen
From 8cf834c1c09caf1bcb19702bf42994ca8c2be479 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/2] 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 2cd8194d8742c4caa14eaf4294ff70aaac4352b6 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/2] 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 ++++++++++++++++++++++++++--------------- 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 +++++ 7 files changed, 95 insertions(+), 42 deletions(-) diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index cf0a3629bc..4176d98aeb 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/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 2f59af25a6..398326ce52 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 4562a5121d..c07ad67b07 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
signature.asc
Description: PGP signature
