Magnus, all, * Magnus Hagander (mag...@hagander.net) wrote: > On Tue, Jan 2, 2018 at 1:08 PM, Stephen Frost <sfr...@snowman.net> wrote: > > Suggestions on a name for this..? pg_server_copy_program? > > Presumably it would also be used in postgres_fdw, so that seems like a bad > name. Maybe pg_exec_server_command?
I went with 'pg_execute_server_program', since 'program' is what we use in the COPY syntax, et al. 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. Thanks! Stephen
From 02c13fcb8a41f320f178fad29e9949f3846420ce Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfr...@snowman.net>
Date: Sun, 31 Dec 2017 14:01:12 -0500
Subject: [PATCH] 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.
Further, this patch moves the privilege check for the remaining misc
file functions from explicit superuser checks to the GRANT system,
similar to what's done for pg_ls_logdir() and others. Lastly, these
functions are changed to allow a user with the 'pg_read_server_files'
role to be able to access files outside of the PG data directory.
---
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/catalog/system_views.sql | 14 ++++++++++
src/backend/commands/copy.c | 37 ++++++++++++++++++--------
src/backend/utils/adt/genfile.c | 30 +++++++--------------
src/include/catalog/pg_authid.h | 6 +++++
8 files changed, 109 insertions(+), 62 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/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/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 d9027fc688..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/..' */
@@ -195,11 +205,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 +241,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 +313,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 +394,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;
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
