From: Stephen Frost Date: Fri, 6 Apr 2018 18:47:10 +0000 (-0400) Subject: Add default roles for file/program access X-Git-Tag: REL_11_BETA1~331 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=0fdc8495bff02684142a44ab3bc5b18a8ca1863a;p=postgresql Add default roles for file/program access This patch adds new default roles named '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 from above. Reviewed-By: Michael Paquier Discussion: https://postgr.es/m/20171231191939.GR2416%40tamriel.snowman.net --- 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 6257563eaa..a86d3f40f1 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -20119,10 +20119,21 @@ 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. Use a relative path for files in + the cluster directory, and a path matching the log_directory + configuration setting for log files. + + + + Note that granting users the EXECUTE privilege on the + pg_read_file(), or related, functions allows them the + ability to read any file on the server which the database can read and + that those reads bypass all in-database privilege checks. This means that, + among other things, a user with this access is able to read the contents of the + pg_authid table where authentication information is contained, + as well as read any file in the database. Therefore, granting access to these + functions should be carefully considered. @@ -20140,7 +20151,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. @@ -20171,7 +20182,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. @@ -20180,7 +20191,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. @@ -20189,7 +20200,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/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml index 94fd4ebf58..81b44a8c41 100644 --- a/doc/src/sgml/user-manag.sgml +++ b/doc/src/sgml/user-manag.sgml @@ -534,6 +534,21 @@ DROP ROLE doomed_role; pg_signal_backend Send signals to other backends (eg: cancel query, terminate). + + pg_read_server_files + Allow reading files from any location the database can access on the server with COPY and + other file-access functions. + + + pg_write_server_files + Allow writing to files in any location the database can access on the server with COPY and + other file-access functions. + + + pg_execute_server_program + Allow executing programs on the database server as the user the database runs as with + COPY and other functions which allow executing a server-side program. + pg_monitor Read/execute various monitoring views and functions. @@ -545,6 +560,16 @@ DROP ROLE doomed_role;
+ + The pg_read_server_files, pg_write_server_files and + pg_execute_server_program roles are intended to allow administrators to have + trusted, but non-superuser, roles which are able to access files and run programs on the + database server as the user the database runs as. As these roles are able to access any file on + the server filesystem, they bypass all database-level permission checks when accessing files + directly and they could be used to gain superuser-level access, therefore care should be taken + when granting these roles to users. + + The pg_monitor, pg_read_all_settings, pg_read_all_stats and pg_stat_scan_tables @@ -556,7 +581,8 @@ DROP ROLE doomed_role; Care should be taken when granting these roles to ensure they are only used where - needed to perform the desired monitoring. + needed and with the understanding that these roles grant access to privileged + information. diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index ae06609a1e..a5084dc3cd 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,37 @@ 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) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("must be superuser 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."))); + { + if (!is_member_of_role(GetUserId(), DEFAULT_ROLE_EXECUTE_SERVER_PROGRAM)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + 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..9e85df18aa 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" @@ -45,6 +46,12 @@ typedef struct * * Filename may be absolute or relative to the DataDir, but we only allow * absolute paths that match DataDir or Log_directory. + * + * This does a privilege check against the 'pg_read_server_files' role, so + * this function is really only appropriate for callers who are only checking + * 'read' access. Do not use this function if you are looking for a check + * for 'write' or 'program' access without updating it to access the type + * of check as an argument and checking the appropriate role membership. */ static char * convert_and_check_filename(text *arg) @@ -54,6 +61,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..00c84a33b5 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 = 4569 ( "pg_read_server_files" f t f f f f f -1 _null_ _null_)); +#define DEFAULT_ROLE_READ_SERVER_FILES 4569 +DATA(insert OID = 4570 ( "pg_write_server_files" f t f f f f f -1 _null_ _null_)); +#define DEFAULT_ROLE_WRITE_SERVER_FILES 4570 +DATA(insert OID = 4571 ( "pg_execute_server_program" f t f f f f f -1 _null_ _null_)); +#define DEFAULT_ROLE_EXECUTE_SERVER_PROGRAM 4571 DATA(insert OID = 4200 ( "pg_signal_backend" f t f f f f f -1 _null_ _null_)); #define DEFAULT_ROLE_SIGNAL_BACKENDID 4200