Add default role 'pg_access_server_files'

Started by Stephen Frostabout 8 years ago31 messages
#1Stephen Frost
sfrost@snowman.net
1 attachment(s)

Greetings,

This patch adds a new default role called 'pg_access_server_files' which
allows an administrator to GRANT to a non-superuser role the ability to
access server-side files through PostgreSQL (as the user the database is
running as). By itself, having this role allows a non-superuser to use
server-side COPY and to use file_fdw (if installed by a superuser and
GRANT'd USAGE on it).

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_access_server_files'
role to be able to access files outside of the PG data directory.

This follows on and continues what was recently done with the
lo_import/export functions. There's other superuser checks to replace
with grant'able default roles, but those probably make more sense as
independent patches. I continue to be of the opinion that it'd be nice
to have more fine-grained control over these functions to limit the
access granted, but nothing here prevents that from being done and this
at least allows some movement away from having to have roles with
superuser access.

Thanks!

Stephen

Attachments:

add_default_role_access_server_files_v1-master.patchtext/x-diff; charset=us-asciiDownload
From eb8be8ffbadcc37418dc12d59c6767e073028e35 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@snowman.net>
Date: Sun, 31 Dec 2017 14:01:12 -0500
Subject: [PATCH] Add default role pg_access_server_files

This patch adds a new default role called 'pg_access_server_files' which
allows an administrator to GRANT to a non-superuser role the ability to
access server-side files through PostgreSQL (as the user the database is
running as).  By itself, having this role allows a non-superuser to use
server-side COPY and to use file_fdw (if installed by a superuser and
GRANT'd USAGE on it).

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_access_server_files'
role to be able to access files outside of the PG data directory.
---
 contrib/file_fdw/file_fdw.c          | 13 ++++++++-----
 doc/src/sgml/file-fdw.sgml           |  9 +++++----
 doc/src/sgml/func.sgml               | 16 ++++++++--------
 doc/src/sgml/ref/copy.sgml           |  5 +++--
 src/backend/catalog/system_views.sql | 14 ++++++++++++++
 src/backend/commands/copy.c          | 16 ++++++++++------
 src/backend/utils/adt/genfile.c      | 30 ++++++++++--------------------
 src/include/catalog/pg_authid.h      |  2 ++
 8 files changed, 60 insertions(+), 45 deletions(-)

diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index 370cc365d6..08d8d61f10 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"
@@ -202,9 +203,10 @@ file_fdw_validator(PG_FUNCTION_ARGS)
 	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.
+	 * Only members of the special role 'pg_access_server_files' are allowed
+	 * to set options of a file_fdw foreign table.  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
@@ -214,10 +216,11 @@ file_fdw_validator(PG_FUNCTION_ARGS)
 	 * program at any options level other than foreign table --- otherwise
 	 * there'd still be a security hole.
 	 */
-	if (catalog == ForeignTableRelationId && !superuser())
+	if (catalog == ForeignTableRelationId &&
+		!is_member_of_role(GetUserId(), DEFAULT_ROLE_ACCESS_SERVER_FILES))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-				 errmsg("only superuser can change options of a file_fdw foreign table")));
+				 errmsg("only superuser or a member of the pg_access_server_files role can change options of a file_fdw foreign table")));
 
 	/*
 	 * Check that only options supported by file_fdw, and allowed for the
diff --git a/doc/src/sgml/file-fdw.sgml b/doc/src/sgml/file-fdw.sgml
index e2598a07da..119f55add4 100644
--- a/doc/src/sgml/file-fdw.sgml
+++ b/doc/src/sgml/file-fdw.sgml
@@ -186,10 +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
-  change the other options, but that's not supported at present.
+  Changing table-level options requires begin a superuser or having the privileges
+  of the default role <literal>pg_access_server_files</literal>, 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>
 
  <para>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 4dd9d029e6..29148839dc 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19896,10 +19896,10 @@ 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_access_server_files</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">
@@ -19917,7 +19917,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>
@@ -19948,7 +19948,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>
@@ -19957,7 +19957,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>
@@ -19966,7 +19966,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..411fb77b1a 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -444,8 +444,9 @@ 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 the default role
+    <literal>pg_access_server_files</literal>, since it allows reading or
+    writing any file 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 394aea8e0f..323741a73a 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1147,6 +1147,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 254be28ae4..6d5e30f7e6 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -24,6 +24,7 @@
 #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"
@@ -774,8 +775,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.
@@ -792,19 +793,22 @@ 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
+	 * 'pg_access_server_files' role.
+	 */
+	if (!pipe && !is_member_of_role(GetUserId(), DEFAULT_ROLE_ACCESS_SERVER_FILES))
 	{
 		if (stmt->is_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_access_server_files 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"),
+					 errmsg("must be superuser or a member of the pg_access_server_files role to COPY to or from a file"),
 					 errhint("Anyone can COPY to stdout or from stdin. "
 							 "psql's \\copy command also works for anyone.")));
 	}
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index 04f1efbe4b..7a2e6752af 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_access_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_ACCESS_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 9b6b52c9f9..168fc47f3c 100644
--- a/src/include/catalog/pg_authid.h
+++ b/src/include/catalog/pg_authid.h
@@ -108,6 +108,8 @@ 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_access_server_files" f t f f f f f -1 _null_ _null_));
+#define DEFAULT_ROLE_ACCESS_SERVER_FILES	3556
 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

#2Magnus Hagander
magnus@hagander.net
In reply to: Stephen Frost (#1)
Re: Add default role 'pg_access_server_files'

On Sun, Dec 31, 2017 at 8:19 PM, Stephen Frost <sfrost@snowman.net> wrote:

Greetings,

This patch adds a new default role called 'pg_access_server_files' which
allows an administrator to GRANT to a non-superuser role the ability to
access server-side files through PostgreSQL (as the user the database is
running as). By itself, having this role allows a non-superuser to use
server-side COPY and to use file_fdw (if installed by a superuser and
GRANT'd USAGE on it).

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_access_server_files'
role to be able to access files outside of the PG data directory.

This follows on and continues what was recently done with the
lo_import/export functions. There's other superuser checks to replace
with grant'able default roles, but those probably make more sense as
independent patches. I continue to be of the opinion that it'd be nice
to have more fine-grained control over these functions to limit the
access granted, but nothing here prevents that from being done and this
at least allows some movement away from having to have roles with
superuser access.

Would it make sense to separate out:
* write from read. E.g. a pg_write_server_files/pg_read_server_files? ISTM
that will turn into a pretty common request...
* execute from read/write, so COPY FROM PROGRAM etc would be a separate
role?

I realize we don't want to go overboard with the number of roles here, but
at least separating read from write seems useful.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

#3Stephen Frost
sfrost@snowman.net
In reply to: Magnus Hagander (#2)
Re: Add default role 'pg_access_server_files'

Magnus,

* Magnus Hagander (magnus@hagander.net) wrote:

On Sun, Dec 31, 2017 at 8:19 PM, Stephen Frost <sfrost@snowman.net> wrote:

This patch adds a new default role called 'pg_access_server_files' which
allows an administrator to GRANT to a non-superuser role the ability to
access server-side files through PostgreSQL (as the user the database is
running as). By itself, having this role allows a non-superuser to use
server-side COPY and to use file_fdw (if installed by a superuser and
GRANT'd USAGE on it).

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_access_server_files'
role to be able to access files outside of the PG data directory.

This follows on and continues what was recently done with the
lo_import/export functions. There's other superuser checks to replace
with grant'able default roles, but those probably make more sense as
independent patches. I continue to be of the opinion that it'd be nice
to have more fine-grained control over these functions to limit the
access granted, but nothing here prevents that from being done and this
at least allows some movement away from having to have roles with
superuser access.

Would it make sense to separate out:
* write from read. E.g. a pg_write_server_files/pg_read_server_files? ISTM
that will turn into a pretty common request...

Ok.

* execute from read/write, so COPY FROM PROGRAM etc would be a separate
role?

Suggestions on a name for this..? pg_server_copy_program?

I realize we don't want to go overboard with the number of roles here, but
at least separating read from write seems useful.

Yeah, these are certainly good suggestions for the COPY case. I had set
out thihking about pg_read/write_file and we have the read/write
seperation there through the GRANT rights on the functions themselves,
but we don't have that for COPY without different roles.

I'll add those in and publish a new patch soon.

Thanks!

Stephen

#4Magnus Hagander
magnus@hagander.net
In reply to: Stephen Frost (#3)
Re: Add default role 'pg_access_server_files'

On Tue, Jan 2, 2018 at 1:08 PM, Stephen Frost <sfrost@snowman.net> wrote:

Magnus,

* Magnus Hagander (magnus@hagander.net) wrote:

On Sun, Dec 31, 2017 at 8:19 PM, Stephen Frost <sfrost@snowman.net>

wrote:

This patch adds a new default role called 'pg_access_server_files'

which

allows an administrator to GRANT to a non-superuser role the ability to
access server-side files through PostgreSQL (as the user the database

is

running as). By itself, having this role allows a non-superuser to use
server-side COPY and to use file_fdw (if installed by a superuser and
GRANT'd USAGE on it).

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_access_server_files'
role to be able to access files outside of the PG data directory.

This follows on and continues what was recently done with the
lo_import/export functions. There's other superuser checks to replace
with grant'able default roles, but those probably make more sense as
independent patches. I continue to be of the opinion that it'd be nice
to have more fine-grained control over these functions to limit the
access granted, but nothing here prevents that from being done and this
at least allows some movement away from having to have roles with
superuser access.

Would it make sense to separate out:
* write from read. E.g. a pg_write_server_files/pg_read_server_files?

ISTM

that will turn into a pretty common request...

Ok.

* execute from read/write, so COPY FROM PROGRAM etc would be a separate
role?

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?

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

#5Stephen Frost
sfrost@snowman.net
In reply to: Magnus Hagander (#4)
Re: Add default role 'pg_access_server_files'

Magnus,

* Magnus Hagander (magnus@hagander.net) wrote:

On Tue, Jan 2, 2018 at 1:08 PM, Stephen Frost <sfrost@snowman.net> wrote:

* Magnus Hagander (magnus@hagander.net) wrote:

On Sun, Dec 31, 2017 at 8:19 PM, Stephen Frost <sfrost@snowman.net>

wrote:

This patch adds a new default role called 'pg_access_server_files'

which

allows an administrator to GRANT to a non-superuser role the ability to
access server-side files through PostgreSQL (as the user the database

is

running as). By itself, having this role allows a non-superuser to use
server-side COPY and to use file_fdw (if installed by a superuser and
GRANT'd USAGE on it).

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_access_server_files'
role to be able to access files outside of the PG data directory.

This follows on and continues what was recently done with the
lo_import/export functions. There's other superuser checks to replace
with grant'able default roles, but those probably make more sense as
independent patches. I continue to be of the opinion that it'd be nice
to have more fine-grained control over these functions to limit the
access granted, but nothing here prevents that from being done and this
at least allows some movement away from having to have roles with
superuser access.

Would it make sense to separate out:
* write from read. E.g. a pg_write_server_files/pg_read_server_files?

ISTM

that will turn into a pretty common request...

Ok.

* execute from read/write, so COPY FROM PROGRAM etc would be a separate
role?

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'm guessing you mean file_fdw.. :)

That name sounds alright to me though.

Thanks!

Stephen

#6Ryan Murphy
ryanfmurphy@gmail.com
In reply to: Stephen Frost (#5)
Re: Add default role 'pg_access_server_files'

Stephen, so far I've read thru your patch and familiarized myself with some of the auth functionality in pg_authid.h and src/backend/utils/adt/acl.c

The only question I have so far about your patch is the last several hunks of the diff, which remove superuser checks without adding anything immediately obvious in their place:

...
@@ -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);
...

I wanted to ask if you have reason to believe that these checks were not necessary (and therefore can be deleted instead of replaced by is_member_of_role() checks like you did elsewhere). I still have limited understanding of the overall code, so really just asking because it's the first thing that jumped out.

Best,
Ryan

#7Stephen Frost
sfrost@snowman.net
In reply to: Ryan Murphy (#6)
Re: Add default role 'pg_access_server_files'

Greetings Ryan!

* Ryan Murphy (ryanfmurphy@gmail.com) wrote:

Stephen, so far I've read thru your patch and familiarized myself with some of the auth functionality in pg_authid.h and src/backend/utils/adt/acl.c

The only question I have so far about your patch is the last several hunks of the diff, which remove superuser checks without adding anything immediately obvious in their place:

Ah, I realize it's not immediately obvious, but they *are* replaced by
something else- REVOKE statements in the "system_views.sql" file in
src/backend/catalog:

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;

That script is run as part of 'initdb' to set things up in the system.
By issueing those REVOKE statements, no one but the cluster owner (a
superuser) is able to run those functions- unless a superuser decides
that it's ok for others to run them, in which case they would run:

GRANT EXECUTE ON FUNCTION pg_read_file(text) TO myuser;

I wanted to ask if you have reason to believe that these checks were not necessary (and therefore can be deleted instead of replaced by is_member_of_role() checks like you did elsewhere). I still have limited understanding of the overall code, so really just asking because it's the first thing that jumped out.

The places where is_member_of_role() is being used are cases where it's
not possible to use the GRANT system. For example, we don't have a way
to say "GRANT read-files-outside-the-data-directory TO role1;" in the
normal GRANT system, and so a default role is added to allow that
specific right to be GRANT'd to non-superuser.

One would need to have both the default role AND EXECUTE rights on the
function to be able to, say, run:

SELECT pg_read_file('/data/load_area/load_file');

With just EXECUTE on the function, they could use pg_read_file() to read
files under the data directory but not elsewhere on the system, which
may be overly limiting for some use-cases.

Of course, all of these functions allow a great deal of access to the
system and that's why they started out being superuser-only.
Administrators will need to carefully consider who, if anyone, should
have the level of access which these functions and default roles
provide.

Thanks!

Stephen

#8Ryan Murphy
ryanfmurphy@gmail.com
In reply to: Stephen Frost (#7)
Re: Add default role 'pg_access_server_files'

Hi Stephen,

I have another question then based on what you said earlier today, and some testing I did using your patch.

TLDR: I created a role "tester" and was (as expected) not able to perform pg_read_file() on files outside the data directory.
But then I granted EXECUTE on that function for that role, and then I was able to (which is not what I expected).

Here's what I did (I apologize if this is too verbose):

* I successfully applied your patch to HEAD, and built Postgres from source:

make clean
configure (with options including a specific --prefix)
make
make install

* Then I went into the "install_dir/bin" and did the following to setup a data directory:

$ ./initdb ~/sql-data/2018-01-06
The files belonging to this database system will be owned by user "postgres".
This user must also own the server process.

The database cluster will be initialized with locale "en_US.UTF-8".
The default database encoding has accordingly been set to "UTF8".
The default text search configuration will be set to "english".

Data page checksums are disabled.

creating directory /Users/postgres/sql-data/2018-01-06 ... ok
creating subdirectories ... ok
selecting default max_connections ... 100
selecting default shared_buffers ... 128MB
selecting dynamic shared memory implementation ... posix
creating configuration files ... ok
running bootstrap script ... ok
performing post-bootstrap initialization ... ok
syncing data to disk ... ok

WARNING: enabling "trust" authentication for local connections
You can change this by editing pg_hba.conf or using the option -A, or
--auth-local and --auth-host, the next time you run initdb.

Success. You can now start the database server using:

./pg_ctl -D /Users/postgres/sql-data/2018-01-06 -l logfile start

* Then I started the database:

$ ./pg_ctl -D /Users/postgres/sql-data/2018-01-06 -l logfile start
waiting for server to start.... done
server started

* I went into the database and tried a pg_read_file:

$ psql postgres
psql (9.4.5, server 11devel)
Type "help" for help.

postgres=# select pg_read_file('/Users/postgres/temp');
pg_read_file
-----------------------------------------------------------
here is the file content

(1 row)

* Of course that worked as superuser, so created a new role:

postgres=# create role tester;
CREATE ROLE
postgres=# \q
postgres=# alter role tester with login;
ALTER ROLE
postgres=# \q

$ psql postgres tester
psql (9.4.5, server 11devel)
Type "help" for help.

postgres=> select pg_read_file('/Users/postgres/temp');
ERROR: permission denied for function pg_read_file
postgres=> \q

* My current understanding at this point is that EXECUTE permissions would only allow "tester" to pg_read_file() on files in the data directory. I try GRANTing EXECUTE:

$ psql postgres
psql (9.4.5, server 11devel)
Type "help" for help.

postgres=# grant execute on function pg_read_file(text) to tester;
GRANT
postgres=# select pg_read_file('/Users/postgres/temp');
pg_read_file
-----------------------------------------------------------
here is the file content

(1 row)

Is this expected behavior? I thought I would need to GRANT that new "pg_access_server_files" role to "tester" in order to do this. I may have misunderstood how your new feature works, just doublechecking.

Regards,
Ryan

#9Ryan Murphy
ryanfmurphy@gmail.com
In reply to: Ryan Murphy (#8)
Re: Add default role 'pg_access_server_files'

Oops! I made a mistake, which clearly showed up in my last email: I forgot
to psql back in as "tester".

Now I get the right behavior:

$ psql postgres tester
psql (9.4.5, server 11devel)
Type "help" for help.

postgres=> select pg_read_file('/Users/postgres/temp');
ERROR: absolute path not allowed

Thanks for bearing with me. So far so good on this feature, going to run
the tests too.

Best,
Ryan

#10Ryan Murphy
ryanfmurphy@gmail.com
In reply to: Ryan Murphy (#8)
Re: Add default role 'pg_access_server_files'

(Duplicated to make sure it's in the commitfest Thread, didn't seem to get in there when I replied to the email)

Oops! I made a mistake, which clearly showed up in my last email: I forgot to psql back in as "tester".

Now I get the right behavior:

$ psql postgres tester
postgres=> select pg_read_file('/Users/postgres/temp');
ERROR: absolute path not allowed

Thanks for bearing with me. So far so good on this feature, going to run the tests too.

Best,
Ryan

#11Ryan Murphy
ryanfmurphy@gmail.com
In reply to: Ryan Murphy (#10)
Re: Add default role 'pg_access_server_files'

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: tested, passed

I ran make installcheck-world and all tests passed except one that is a known issue with the way I have my environment setup (ecpg tests unrelated to this patch).

Manual tests I ran to see if it Implements the Feature:

1) confirmed that superuser can call pg_read_file() to read files in or out of data directory
2) confirmed that "tester" can call pg_read_file() only if given EXECUTE privilege
3) confirmed that "tester" can only call pg_read_file() on a file OUTSIDE of the data directory iff I "grant pg_access_server_files to tester;"

Documentation seems reasonable.

I believe this patch to be Ready for Committer.

The new status of this patch is: Ready for Committer

#12Thomas Munro
thomas.munro@enterprisedb.com
In reply to: Stephen Frost (#1)
Re: Add default role 'pg_access_server_files'

On Mon, Jan 1, 2018 at 8:19 AM, Stephen Frost <sfrost@snowman.net> wrote:

Greetings,

This patch adds a new default role called 'pg_access_server_files' which
allows an administrator to GRANT to a non-superuser role the ability to
access server-side files through PostgreSQL (as the user the database is
running as). By itself, having this role allows a non-superuser to use
server-side COPY and to use file_fdw (if installed by a superuser and
GRANT'd USAGE on it).

Hi Stephen,

Not sure if you are aware of this failure?

test file_fdw ... FAILED

Because:

! ERROR: only superuser can change options of a file_fdw foreign table
...
! ERROR: only superuser or a member of the pg_access_server_files role
can change options of a file_fdw foreign table

--
Thomas Munro
http://www.enterprisedb.com

#13Stephen Frost
sfrost@snowman.net
In reply to: Thomas Munro (#12)
Re: Add default role 'pg_access_server_files'

Thomas,

* Thomas Munro (thomas.munro@enterprisedb.com) wrote:

On Mon, Jan 1, 2018 at 8:19 AM, Stephen Frost <sfrost@snowman.net> wrote:

This patch adds a new default role called 'pg_access_server_files' which
allows an administrator to GRANT to a non-superuser role the ability to
access server-side files through PostgreSQL (as the user the database is
running as). By itself, having this role allows a non-superuser to use
server-side COPY and to use file_fdw (if installed by a superuser and
GRANT'd USAGE on it).

Not sure if you are aware of this failure?

test file_fdw ... FAILED

Thanks, I did do a make check-world, but I tend to do them in parallel
and evidently missed this. The patch needs to be reworked based on
discussion with Magnus anyhow, which I hope to do this weekend.
Currently trying to push other patches along. :)

Thanks!

Stephen

#14Ryan Murphy
ryanfmurphy@gmail.com
In reply to: Ryan Murphy (#11)
Re: Add default role 'pg_access_server_files'

Just circling back on this.

I did have a question that came up about the behavior of the server as it is without the patch. I logged into psql with my superuser "postgres":

postgres=# select pg_read_file('/Users/postgres/temp');
ERROR: absolute path not allowed

I had not tried this before with my unpatched build of postgres. (In retrospect of course I should have). I expected my superuser to be able to perform this task, but it seems that for security reasons we presently don't allow access to absolute path names (except in the data dir and log dir) - not even for a superuser. Is that correct? In that case the security implications of this patch would need more consideration.

Stephen, looking at the patch, I see that in src/backend/utils/adt/genfile.c you've made some changes to the function convert_and_check_filename(). These changes, I believe, loosen the security checks in ways other than just adding the granularity of a new role which can be GRANTed to non superusers:

    +   /*
    +    * Members of the 'pg_access_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_ACCESS_SERVER_FILES))
    +       return filename;
    +
    +   /* User isn't a member of the default role, so check if it's allowable */
        if (is_absolute_path(filename))
        {

As you can see, you've added a short-circuit "return" statement for anyone who has the DEFAULT_ROLE_ACCESS_SERVER_FILES. Prior to this patch, even a Superuser would be subject to the following is_absolute_path() logic. But with it, the return statement short-circuits the is_absolute_path() check.

Is this an intended behavior of the patch - to allow file access to absolute paths where previously it was impossible? Or, was the intention just to add granularity via the new role? I had assumed the latter.

Best regards,
Ryan

The new status of this patch is: Waiting on Author

#15Michael Paquier
michael.paquier@gmail.com
In reply to: Ryan Murphy (#14)
Re: Add default role 'pg_access_server_files'

On Thu, Jan 18, 2018 at 02:04:45PM +0000, Ryan Murphy wrote:

I had not tried this before with my unpatched build of postgres. (In
retrospect of course I should have). I expected my superuser to be
able to perform this task, but it seems that for security reasons we
presently don't allow access to absolute path names (except in the
data dir and log dir) - not even for a superuser. Is that correct?

Correct.

In that case the security implications of this patch would need more
consideration.

Stephen, looking at the patch, I see that in
src/backend/utils/adt/genfile.c you've made some changes to the
function convert_and_check_filename(). These changes, I believe,
loosen the security checks in ways other than just adding the
granularity of a new role which can be GRANTed to non superusers:

+   /*
+    * Members of the 'pg_access_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_ACCESS_SERVER_FILES))
+       return filename;
+
+   /* User isn't a member of the default role, so check if it's allowable */
if (is_absolute_path(filename))
{

It seems to me that this is the intention behind the patch as the
comment points out and as Stephen has stated in
/messages/by-id/20171231191939.GR2416@tamriel.snowman.net.

As you can see, you've added a short-circuit "return" statement for
anyone who has the DEFAULT_ROLE_ACCESS_SERVER_FILES. Prior to this
patch, even a Superuser would be subject to the following
is_absolute_path() logic. But with it, the return statement
short-circuits the is_absolute_path() check.

I agree that it is a strange concept to loosen the access while even
superusers cannot do that. By concept superusers are assumed to be able
to do anything on the server by the way.
--
Michael

#16Stephen Frost
sfrost@snowman.net
In reply to: Michael Paquier (#15)
Re: Add default role 'pg_access_server_files'

Michael, all,

* Michael Paquier (michael.paquier@gmail.com) wrote:

On Thu, Jan 18, 2018 at 02:04:45PM +0000, Ryan Murphy wrote:

I had not tried this before with my unpatched build of postgres. (In
retrospect of course I should have). I expected my superuser to be
able to perform this task, but it seems that for security reasons we
presently don't allow access to absolute path names (except in the
data dir and log dir) - not even for a superuser. Is that correct?

Correct.

That's how it currently is, yes, though that doesn't really prevent a
superuser from accessing files outside of the data dir, they would just
have to use another mechanism to do so than this (but it's not hard).

In that case the security implications of this patch would need more
consideration.

Stephen, looking at the patch, I see that in
src/backend/utils/adt/genfile.c you've made some changes to the
function convert_and_check_filename(). These changes, I believe,
loosen the security checks in ways other than just adding the
granularity of a new role which can be GRANTed to non superusers:

+   /*
+    * Members of the 'pg_access_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_ACCESS_SERVER_FILES))
+       return filename;
+
+   /* User isn't a member of the default role, so check if it's allowable */
if (is_absolute_path(filename))
{

It seems to me that this is the intention behind the patch as the
comment points out and as Stephen has stated in
/messages/by-id/20171231191939.GR2416@tamriel.snowman.net.

Yes, this change is intentional. Note that superusers are members of
all roles explicitly (see the check in is_member_of_role()).

As you can see, you've added a short-circuit "return" statement for
anyone who has the DEFAULT_ROLE_ACCESS_SERVER_FILES. Prior to this
patch, even a Superuser would be subject to the following
is_absolute_path() logic. But with it, the return statement
short-circuits the is_absolute_path() check.

I agree that it is a strange concept to loosen the access while even
superusers cannot do that. By concept superusers are assumed to be able
to do anything on the server by the way.

As best as I can tell, the checks in these functions weren't because of
security concerns but simply because the original justification for them
was to be able to read files in the data directory and so they were
written specifically for that purpose. There's no such check in
lo_import(), for example, and it is, as Michael says, assumed that
superusers are equivilant to having full access to the server as the
user the database is running as.

This patch still needs updating for Magnus' comments, of course, and
I'm still planning to make that happen, so Waiting on Author is the
right status currently.

Thanks!

Stephen

#17Ryan Murphy
ryanfmurphy@gmail.com
In reply to: Stephen Frost (#16)
Re: Add default role 'pg_access_server_files'

Ok great. Thanks Michael and Stephen for the explanations.

#18Andres Freund
andres@anarazel.de
In reply to: Stephen Frost (#16)
Re: Add default role 'pg_access_server_files'

Hi,

On 2018-01-19 09:28:33 -0500, Stephen Frost wrote:

This patch still needs updating for Magnus' comments, of course, and
I'm still planning to make that happen, so Waiting on Author is the
right status currently.

Given that this hasn't happened, and that the next CF has started, ISTM,
the patch should be marked as returned with feedback. If not, it at the
very least should be updated soon...

Greetings,

Andres Freund

#19Stephen Frost
sfrost@snowman.net
In reply to: Magnus Hagander (#4)
1 attachment(s)
Re: Add default role 'pg_access_server_files'

Magnus, all,

* Magnus Hagander (magnus@hagander.net) wrote:

On Tue, Jan 2, 2018 at 1:08 PM, Stephen Frost <sfrost@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

Attachments:

add_default_role_access_server_files_v2-master.patchtext/x-diff; charset=us-asciiDownload
From 02c13fcb8a41f320f178fad29e9949f3846420ce Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@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

#20Michael Paquier
michael@paquier.xyz
In reply to: Stephen Frost (#19)
Re: Add default role 'pg_access_server_files'

On Tue, Mar 06, 2018 at 10:00:54AM -0500, Stephen Frost wrote:

* Magnus Hagander (magnus@hagander.net) wrote:

On Tue, Jan 2, 2018 at 1:08 PM, Stephen Frost <sfrost@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.

Okay.

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)?

+   /*
+    * 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.

Could you update the documentation of pg_rewind please? It seems to me
that with your patch then superuser rights are not necessary mandatory,
as the role connecting to the source server does not need to have
superuser right, and needs just execution rights to pg_ls_dir,
pg_read_binary_files and pg_stat_file. Such a user does not need to be
granted access to pg_read_server_files either as no absolute paths are
involved in pg_rewind. Listing the functions directly would be also
nice. Personally I care a lot about 1), way less about 2).
--
Michael

#21Stephen Frost
sfrost@snowman.net
In reply to: Michael Paquier (#20)
1 attachment(s)
Re: Add default role 'pg_access_server_files'

Greetings Michael,

* Michael Paquier (michael@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

Attachments:

add_default_role_access_server_files_v3-master.patchtext/x-diff; charset=us-asciiDownload
From 8cf834c1c09caf1bcb19702bf42994ca8c2be479 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@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 <sfrost@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

#22Michael Paquier
michael@paquier.xyz
In reply to: Stephen Frost (#21)
1 attachment(s)
Re: Add default role 'pg_access_server_files'

On Wed, Mar 07, 2018 at 07:00:03AM -0500, Stephen Frost wrote:

* Michael Paquier (michael@paquier.xyz) wrote:

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.

One patch should defend one idea, this makes the git history easier to
understand in my opinion.

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.

Hm, OK...

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.

That's what I did, manually, using the attached SQL script with your
patch 1 applied. You can check for the list of functions used by
pg_rewind in libpq_fetch.c where you just need to grant execution access
to those functions for a login user and you are good to go:
pg_ls_dir(text, boolean, boolean)
pg_stat_file(text, boolean)
pg_read_binary_file(text)
pg_read_binary_file(text, bigint, bigint, boolean)

So getting this documented properly would be nice. Of course feel free
to test this by yourself as you wish.

Could you send separate files for each patch by the way? This eases
testing, and I don't recall that git am has a way to enforce only a
subset of patches to be applied based on one file, though my git-fu is
limited in this area.

+ 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.
Well, the parsing of file_fdw complains if "program" or "filename" is
not defined, so a user has to be in either pg_read_server_files to use
"filename" or in pg_execute_server_program to use "program", so I am not
sure that the last sentence of this paragraph makes much sense from the
beginning.

-        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>
You should make those paragraphs spawn into multiple lines.  EXECUTE
should use <literal> markup, and I think that you should use "EXECUTE
privilege to run the function" on those doc portions.  That's all for
the nits.

Other than that the patch looks in pretty good shape to me.
--
Michael

Attachments:

rewind_grant.sqlapplication/x-sqlDownload
#23Michael Paquier
michael@paquier.xyz
In reply to: Michael Paquier (#22)
Re: Add default role 'pg_access_server_files'

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.
--
Michael

#24Stephen Frost
sfrost@snowman.net
In reply to: Michael Paquier (#23)
1 attachment(s)
Re: Add default role 'pg_access_server_files'

Greetings,

* Michael Paquier (michael@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.

Does anyone have an opinion regarding the adminpack functions? I was
just reviewing the patch and considering if we should adjust the
privileges there also and it seems like we should. That'd be a pretty
straight-forward change, of course, so unless there's some reason not to
then I'll see about providing an updated patch tomorrow which covers
those functions as well.

Note that it'll be a bit more complicated since we can't just remove the
checks from the existing functions- we'll need to have new functions
where the checks are removed and a new extension version that updates to
the new functions and then REVOKE's access to them. Not a big deal,
just pointing out that it's not quite as straight-forward since it's an
extension and we need to deal with environments where the server's been
upgraded and the .so changed, but the existing functions are still in
place with their current public-execute rights.

Thanks!

Stephen

Attachments:

add_default_role_access_server_files_v4-master.patchtext/x-diff; charset=us-asciiDownload
From 296b407863a7259a04e5e8cfc19f9b8ea124777c Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@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 76ba6f1eef402070ca1ff37f74e5dcfc639f6837 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@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 +++++++++++++++++++++------------
 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

#25Michael Paquier
michael@paquier.xyz
In reply to: Stephen Frost (#24)
Re: Add default role 'pg_access_server_files'

On Sun, Mar 25, 2018 at 09:43:25PM -0400, Stephen Frost wrote:

* Michael Paquier (michael@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.

Patch 2 includes the documentation changes from patch 1, which would
matter only if you decide to keep things splitted. As far as my brain
sees the patch is logically correct.

Note that it'll be a bit more complicated since we can't just remove the
checks from the existing functions- we'll need to have new functions
where the checks are removed and a new extension version that updates to
the new functions and then REVOKE's access to them. Not a big deal,
just pointing out that it's not quite as straight-forward since it's an
extension and we need to deal with environments where the server's been
upgraded and the .so changed, but the existing functions are still in
place with their current public-execute rights.

Yeah, that's basically what you did for pgstattuple in fd321a1. I am
not sure that I would have time to double-check what you code and the
commit fest ends in 5 days. There are many other patches in need of
attention, so I would be incline to just do this portion in the future
and keep the proposal as-is. My 2c.
--
Michael

#26Stephen Frost
sfrost@snowman.net
In reply to: Michael Paquier (#25)
1 attachment(s)
Re: Add default role 'pg_access_server_files'

Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:

On Sun, Mar 25, 2018 at 09:43:25PM -0400, Stephen Frost wrote:

* Michael Paquier (michael@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

Attachments:

add_default_role_access_server_files_v5-master.patchtext/x-diff; charset=us-asciiDownload
From 296b407863a7259a04e5e8cfc19f9b8ea124777c Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@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 <sfrost@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 <sfrost@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

#27Michael Paquier
michael@paquier.xyz
In reply to: Stephen Frost (#26)
Re: Add default role 'pg_access_server_files'

On Sun, Apr 01, 2018 at 09:39:02AM -0400, Stephen Frost wrote:

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.

I have spotted some issues mainly in patch 3.

I am not sure what has happened to your editor, but git diff --check is
throwing a dozen of warnings coming from adminpack.c.

c0cbe00 has stolen the OIDs your patch is using for the new roles, so
patch 2 needs a refresh.

@@ -68,6 +77,15 @@ convert_and_check_filename(text *arg, bool logAllowed)
[...]
+   /*
+    * 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;
So...  If a user has loaded adminpack v1.0 with Postgres v11, then
convert_and_check_filename would actually be able to read paths out of
the data folder for a user within pg_read_server_files, while with
Postgres v10 then only paths within the data folder were allowed.  And
that7s actually fine because a superuser check happens before entering
in this code path.
 pg_file_rename(PG_FUNCTION_ARGS)
+{
+   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();
Here requireSuperuser() should be called before looking at the
argument values.

No refactoring for pg_file_unlink and its v1.1?

The argument checks are exactly the same for +pg_file_rename and
pg_file_rename_v1_1. Why about just passing fcinfo around and simplify
the patch?

+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;
You forgot a REVOKE clause for pg_file_rename(text, text).

In adminpack.c, convert_and_check_filename is always called with false
as second argument. Why not dropping it and use the version in
genfile.c instead? As far as I can see, both functions are the same.

pg_read_file and pg_read_file_v2 could be refactored as well with an
internal routine. Having to support v1 and v2 functions in the backend
code is not elegant. Would it actually work to keep the v1 function in
adminpack.c and the v2 function in genfile.c even if adminpack 1.0 is
loaded? This way you keep in core only one function. What matters is
that the function name matches, right?

+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)
Those three functions should be static.
--
Michael
#28Stephen Frost
sfrost@snowman.net
In reply to: Michael Paquier (#27)
1 attachment(s)
Re: Add default role 'pg_access_server_files'

Michael, all,

* Michael Paquier (michael@paquier.xyz) wrote:

On Sun, Apr 01, 2018 at 09:39:02AM -0400, Stephen Frost wrote:

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.

I have spotted some issues mainly in patch 3.

Thanks for taking a look.

I am not sure what has happened to your editor, but git diff --check is
throwing a dozen of warnings coming from adminpack.c.

Ahhh, those came from switching over to tmux.. I need to figure out how
to get it to copy/paste like I had set up before with screen. Anyhow,
they were all in patch 3 and I've fixed them all.

c0cbe00 has stolen the OIDs your patch is using for the new roles, so
patch 2 needs a refresh.

Fixed and generally rebased.

@@ -68,6 +77,15 @@ convert_and_check_filename(text *arg, bool logAllowed)
[...]
+   /*
+    * 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;

So... If a user has loaded adminpack v1.0 with Postgres v11, then
convert_and_check_filename would actually be able to read paths out of
the data folder for a user within pg_read_server_files, while with
Postgres v10 then only paths within the data folder were allowed. And
that7s actually fine because a superuser check happens before entering
in this code path.

Yes, all of the adminpack v1.0 code paths still have superuser checks,
similar to how the older pgstattuple code paths do. When an upgrade to
adminpack v1.1 is done, the new v1_1 functions don't have those
superuser checks but the upgrade script REVOKE's execute rights from
public, so the right to execute the functions has to be explicitly
GRANT'd for non-superusers.

pg_file_rename(PG_FUNCTION_ARGS)
+{
+   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();

Here requireSuperuser() should be called before looking at the
argument values.

Moved up.

No refactoring for pg_file_unlink and its v1.1?

I considered each function and thought about if it'd make sense to
refactor them or if they were simple enough that the additional function
wouldn't really be all that useful. I'm open to revisiting that, but
just want to make it clear that it was something I thought about and
considered. Since pg_file_unlink is basically two function calls, I
didn't think it worthwhile to refactor those into their own function.

The argument checks are exactly the same for +pg_file_rename and
pg_file_rename_v1_1. Why about just passing fcinfo around and simplify
the patch?

In general, I prefer to keep the PG_FUNCTION_ARGS abstraction when we
can. Unfortunately, that does fall apart when wrapping an SRF as in
pg_logdir_ls(), but with pg_file_rename we can maintain it and it's
really not that much code to do so. As with the refactoring of
pg_file_unlink, this is something which could really go either way.

+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;

You forgot a REVOKE clause for pg_file_rename(text, text).

No, I explicitly didn't include it because that's a security-invoker SQL
function that basically doesn't do anything but turn around and call
pg_file_rename(), which will handle the privilege check:

=*> select pg_file_rename('abc','123');
ERROR: permission denied for function pg_file_rename
CONTEXT: SQL function "pg_file_rename" statement 1

I'm not sure how useful it is to REVOKE the rights on the simple SQL
function; that would just mean that an admin has to go GRANT the rights
on that function as well as the three-argument version.

In adminpack.c, convert_and_check_filename is always called with false
as second argument. Why not dropping it and use the version in
genfile.c instead? As far as I can see, both functions are the same.

Hmm. I'm pretty sure that function was actually copied from adminpack
into core, so I'm not surprised that they're basically the same, but it
was implemented in core as static and I'm not really sure that we want
to export it- it wasn't when it was first copied into core, after all.

Also, they actually should be different- the functions in core are for
reading files while the ones in adminpack are for writing to files, so
that should really be checking against the pg_write_server_files role
instead, as updated in the patch.

pg_read_file and pg_read_file_v2 could be refactored as well with an
internal routine. Having to support v1 and v2 functions in the backend
code is not elegant. Would it actually work to keep the v1 function in
adminpack.c and the v2 function in genfile.c even if adminpack 1.0 is
loaded? This way you keep in core only one function. What matters is
that the function name matches, right?

The function name in core would still have to be different and it's
also possible that there are other callers of it beyond those in
adminpack, so keeping them in core also avoids breaking those callers.
That's not a huge deal in general, of course, but I think it tends to
tip the scales towards just having two versions in core, at least for
now.

Thinking about this a bit more though, those old function names were
listed as deprecated and have been such for quite a while. If a user
upgrades to adminpack 1.1, which they would have to do pretty much
explicitly, maybe they'll be expecting and understanding that they might
have to change their user code and instead of trying to support these
old names in adminpack for compatibility we should just drop the old
function names? Users could still install adminpack 1.0 if they wish
to, after all.

The more I think about it, the more I like the apporach of just dropping
these deprecated "alternative names for things in core" with the new
adminpack version. In terms of applications, as I understand it, they
aren't used in the latest version of pgAdmin3 and they also aren't used
with pgAdmin4, so I don't think we need to be worrying about supporting
them in v11.

I've dropped those functions from the new patch. We still need to keep
the backend functions for adminpack v1.0, of course, but perhaps in a
few releases we can decide that adminpack v1.0 is no longer supported
and drop it and the functions which are supporting it in core.

+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)

Those three functions should be static.

Fixed.

I also went through and updated the docs for the new default roles (we
have a list of the default roles in the docs with what they're for). In
those docs I also pointed out that users with these roles can bypass the
in-database permissions checks, etc. I also went back and added similar
warnings to other places in the docs to hopefully make sure everyone
realizes that these roles are very nearly "superuser" equivilant.

Updated patch attached.

I still want to do more review of it, but this is defintiely starting to
feel better to me. I'm going to spend tomorrow making sure that the
patch to update the pg_dump-related regression tests is good to go and
hopefully push that, after which I'll come back to this for another
review.

Thanks!

Stephen

Attachments:

add_default_role_access_server_files_v6-master.patchtext/x-diff; charset=us-asciiDownload
From e49e307334834267e885a1ea05ee1b4163676c6b Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@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 e9e188682f..8cd8bf40ac 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1151,6 +1151,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 aa7e6050d2ad6d79b39e82bc84b9fee2ffe37748 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@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                  | 26 +++++++++++------
 doc/src/sgml/ref/copy.sgml              |  8 ++++--
 doc/src/sgml/user-manag.sgml            | 25 ++++++++++++++++
 src/backend/commands/copy.c             | 37 +++++++++++++++++-------
 src/backend/utils/adt/genfile.c         | 10 +++++++
 src/include/catalog/pg_authid.h         |  6 ++++
 9 files changed, 130 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 5abb1c46fb..89f5408f84 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -20021,10 +20021,20 @@ 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>.  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>
+
+   <para>
+    Note that granting users the <literal>pg_read_server_files</literal> role 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
+    <literal>pg_authid</literal> table where authentication information is contained,
+    as well as read any file in the database.  Therefore, granting access to this
+    role should be carefully considered.
    </para>
 
    <table id="functions-admin-genfile-table">
@@ -20042,7 +20052,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>
@@ -20073,7 +20083,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>
@@ -20082,7 +20092,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>
@@ -20091,7 +20101,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/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index 94fd4ebf58..38c1c5ca53 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -534,6 +534,21 @@ DROP ROLE doomed_role;
        <entry>pg_signal_backend</entry>
        <entry>Send signals to other backends (eg: cancel query, terminate).</entry>
       </row>
+      <row>
+       <entry>pg_read_server_files</entry>
+       <entry>Allow reading files from any location the database can access on the server with COPY and
+       other file-access functions.</entry>
+      </row>
+      <row>
+       <entry>pg_write_server_files</entry>
+       <entry>Allow writing to files in any location the database can access on the server with COPY and
+       other file-access functions.</entry>
+      </row>
+      <row>
+       <entry>pg_execute_server_program</entry>
+       <entry>Allow executing programs on the database server as the user the database runs as with
+       COPY.</entry>
+      </row>
       <row>
        <entry>pg_monitor</entry>
        <entry>Read/execute various monitoring views and functions.
@@ -545,6 +560,16 @@ DROP ROLE doomed_role;
     </tgroup>
    </table>
 
+  <para>
+  The <literal>pg_read_server_files</literal>, <literal>pg_write_server_files</literal> and
+  <literal>pg_execute_server_program</literal> 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.
+  </para>
+
   <para>
   The <literal>pg_monitor</literal>, <literal>pg_read_all_settings</literal>,
   <literal>pg_read_all_stats</literal> and <literal>pg_stat_scan_tables</literal>
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..86ccc97713 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 = 3436 ( "pg_read_server_files" f t f f f f f -1 _null_ _null_));
+#define DEFAULT_ROLE_READ_SERVER_FILES	3436
+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 521e46961603f5fbb08929611adb6b23202c92b4 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@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 |  51 +++++++
 contrib/adminpack/adminpack.c             | 245 +++++++++++++++++++++++++++---
 contrib/adminpack/adminpack.control       |   2 +-
 contrib/adminpack/expected/adminpack.out  |  19 ++-
 contrib/adminpack/sql/adminpack.sql       |  14 +-
 doc/src/sgml/adminpack.sgml               |  55 +------
 src/backend/utils/adt/genfile.c           |  53 ++++++-
 src/backend/utils/adt/misc.c              |  27 +++-
 src/include/catalog/pg_proc.h             |   8 +-
 10 files changed, 388 insertions(+), 88 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..22eeee2ffa
--- /dev/null
+++ b/contrib/adminpack/adminpack--1.0--1.1.sql
@@ -0,0 +1,51 @@
+/* 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;
+
+/* These functions are now in the backend and callers should update to use those */
+
+DROP FUNCTION pg_file_read(text, bigint, bigint);
+
+DROP FUNCTION pg_file_length(text);
+
+DROP FUNCTION pg_logfile_rotate();
diff --git a/contrib/adminpack/adminpack.c b/contrib/adminpack/adminpack.c
index 1785dee3a1..ecacae801a 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);
+
+static int64 pg_file_write_internal(text *file, text *data, bool replace);
+static bool pg_file_rename_internal(text *file1, text *file2, text *file3);
+static 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_write_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_WRITE_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;
+
+	count = pg_file_write_internal(file, data, replace);
+
+	PG_RETURN_INT64(count);
+}
 
-	if (!PG_GETARG_BOOL(2))
+/* ------------------------------------
+ * 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;
 
 	requireSuperuser();
 
 	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_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();
+
+	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..8747ac69a2 100644
--- a/contrib/adminpack/expected/adminpack.out
+++ b/contrib/adminpack/expected/adminpack.out
@@ -34,7 +34,12 @@ SELECT pg_read_file('test_file1');
  test1test1
 (1 row)
 
--- disallowed file paths
+-- disallowed file paths for non-superusers and users who are
+-- not members of pg_write_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 +52,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 +141,14 @@ 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
 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..1525f0a82b 100644
--- a/contrib/adminpack/sql/adminpack.sql
+++ b/contrib/adminpack/sql/adminpack.sql
@@ -12,12 +12,22 @@ SELECT pg_read_file('test_file1');
 SELECT pg_file_write('test_file1', 'test1', false);
 SELECT pg_read_file('test_file1');
 
--- disallowed file paths
+-- disallowed file paths for non-superusers and users who are
+-- not members of pg_write_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');
diff --git a/doc/src/sgml/adminpack.sgml b/doc/src/sgml/adminpack.sgml
index 1197eefbf3..2655417366 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">
@@ -113,50 +116,4 @@
   function.
  </para>
 
- <para>
-  The functions shown
-  in <xref linkend="functions-adminpack-deprecated-table"/> are deprecated
-  and should not be used in new applications; instead use those shown
-  in <xref linkend="functions-admin-signal-table"/>
-  and <xref linkend="functions-admin-genfile-table"/>.  These functions are
-  provided in <filename>adminpack</filename> only for compatibility with old
-  versions of <application>pgAdmin</application>.
- </para>
-
- <table id="functions-adminpack-deprecated-table">
-  <title>Deprecated <filename>adminpack</filename> Functions</title>
-  <tgroup cols="3">
-   <thead>
-    <row><entry>Name</entry> <entry>Return Type</entry> <entry>Description</entry>
-    </row>
-   </thead>
-
-   <tbody>
-    <row>
-     <entry><function>pg_catalog.pg_file_read(filename text, offset bigint, nbytes bigint)</function></entry>
-     <entry><type>text</type></entry>
-     <entry>
-      Alternate name for <function>pg_read_file()</function>
-     </entry>
-    </row>
-    <row>
-     <entry><function>pg_catalog.pg_file_length(filename text)</function></entry>
-     <entry><type>bigint</type></entry>
-     <entry>
-      Same as <structfield>size</structfield> column returned
-      by <function>pg_stat_file()</function>
-     </entry>
-    </row>
-    <row>
-     <entry><function>pg_catalog.pg_logfile_rotate()</function></entry>
-     <entry><type>integer</type></entry>
-     <entry>
-      Alternate name for <function>pg_rotate_logfile()</function>, but note that it
-      returns integer 0 or 1 rather than <type>boolean</type>
-     </entry>
-    </row>
-   </tbody>
-  </tgroup>
- </table>
-
 </sect1>
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 90d994c71a..c583bfa2f0 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3354,8 +3354,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_ ));
@@ -3367,8 +3369,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

#29Michael Paquier
michael@paquier.xyz
In reply to: Stephen Frost (#28)
Re: Add default role 'pg_access_server_files'

On Mon, Apr 02, 2018 at 05:09:21PM -0400, Stephen Frost wrote:

* Michael Paquier (michael@paquier.xyz) wrote:

No refactoring for pg_file_unlink and its v1.1?

I considered each function and thought about if it'd make sense to
refactor them or if they were simple enough that the additional function
wouldn't really be all that useful. I'm open to revisiting that, but
just want to make it clear that it was something I thought about and
considered. Since pg_file_unlink is basically two function calls, I
didn't think it worthwhile to refactor those into their own function.

I don't mind if this is done your way.

The argument checks are exactly the same for pg_file_rename and
pg_file_rename_v1_1. Why about just passing fcinfo around and simplify
the patch?

In general, I prefer to keep the PG_FUNCTION_ARGS abstraction when we
can. Unfortunately, that does fall apart when wrapping an SRF as in
pg_logdir_ls(), but with pg_file_rename we can maintain it and it's
really not that much code to do so. As with the refactoring of
pg_file_unlink, this is something which could really go either way.

Okay...

I'm not sure how useful it is to REVOKE the rights on the simple SQL
function; that would just mean that an admin has to go GRANT the rights
on that function as well as the three-argument version.

Indeed, I had a brain fade here.

The more I think about it, the more I like the approach of just dropping
these deprecated "alternative names for things in core" with the new
adminpack version. In terms of applications, as I understand it, they
aren't used in the latest version of pgAdmin3 and they also aren't used
with pgAdmin4, so I don't think we need to be worrying about supporting
them in v11.

+1 to simplify the code a bit.
--
Michael
#30Stephen Frost
sfrost@snowman.net
In reply to: Michael Paquier (#29)
Re: Add default role 'pg_access_server_files'

Michael,

* Michael Paquier (michael@paquier.xyz) wrote:

On Mon, Apr 02, 2018 at 05:09:21PM -0400, Stephen Frost wrote:

* Michael Paquier (michael@paquier.xyz) wrote:

No refactoring for pg_file_unlink and its v1.1?

I considered each function and thought about if it'd make sense to
refactor them or if they were simple enough that the additional function
wouldn't really be all that useful. I'm open to revisiting that, but
just want to make it clear that it was something I thought about and
considered. Since pg_file_unlink is basically two function calls, I
didn't think it worthwhile to refactor those into their own function.

I don't mind if this is done your way.

The argument checks are exactly the same for pg_file_rename and
pg_file_rename_v1_1. Why about just passing fcinfo around and simplify
the patch?

In general, I prefer to keep the PG_FUNCTION_ARGS abstraction when we
can. Unfortunately, that does fall apart when wrapping an SRF as in
pg_logdir_ls(), but with pg_file_rename we can maintain it and it's
really not that much code to do so. As with the refactoring of
pg_file_unlink, this is something which could really go either way.

Okay...

I'm not sure how useful it is to REVOKE the rights on the simple SQL
function; that would just mean that an admin has to go GRANT the rights
on that function as well as the three-argument version.

Indeed, I had a brain fade here.

The more I think about it, the more I like the approach of just dropping
these deprecated "alternative names for things in core" with the new
adminpack version. In terms of applications, as I understand it, they
aren't used in the latest version of pgAdmin3 and they also aren't used
with pgAdmin4, so I don't think we need to be worrying about supporting
them in v11.

+1 to simplify the code a bit.

Great, thanks. I'll be doing more review of it myself and see about
pushing it later this afternoon.

Stephen

#31Stephen Frost
sfrost@snowman.net
In reply to: Stephen Frost (#30)
Re: Add default role 'pg_access_server_files'

Greetings,

* Stephen Frost (sfrost@snowman.net) wrote:

Great, thanks. I'll be doing more review of it myself and see about
pushing it later this afternoon.

Took a bit longer as I wanted to check over a few more things, but I've
now pushed this. Thanks much for all of the help with review and
commentary, Michael and Magnus!

Thanks again!

Stephen