From 296b407863a7259a04e5e8cfc19f9b8ea124777c Mon Sep 17 00:00:00 2001 From: Stephen Frost 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 76ba6f1eef402070ca1ff37f74e5dcfc639f6837 Mon Sep 17 00:00:00 2001 From: Stephen Frost 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 +++++++++++++++++++++------------ 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 @@ - 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 pg_read_server_files (to use a filename) or + the default role pg_execute_server_programs (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. 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 log_directory can be - accessed. Use a relative path for files in the cluster directory, - and a path matching the log_directory 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 + pg_read_server_files, or + pg_execute_server_program. Use a relative path for files in + the cluster directory, and a path matching the log_directory + configuration setting for log files. @@ -20016,7 +20017,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); setof text - 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. @@ -20047,7 +20048,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); text - 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. @@ -20056,7 +20057,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); bytea - 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. @@ -20065,7 +20066,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); record - 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. 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 count by the server, not by the client application, must be executable by the PostgreSQL user. COPY 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 + pg_read_server_files, + pg_write_server_files, + or pg_execute_server_program, since it allows reading + or writing any file or running a program that the server has privileges to + access. 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