Add default roles for file/program access

sfrost · sfrost · commit 0fdc8495bff0 · 2018-04-06T14:47:10.000-04:00
This patch adds new default roles named 'pg_read_server_files', 'pg_write_server_files', 'pg_execute_server_program' which allow an administrator to GRANT to a non-superuser role the ability to access server-side files or run programs through PostgreSQL (as the user the database is running as). Having one of these roles allows a non-superuser to use server-side COPY to read, write, or with a program, and to use file_fdw (if installed by a superuser and GRANT'd USAGE on it) to read from files or run a program. The existing misc file functions are also changed to allow a user with the 'pg_read_server_files' default role to read any files on the filesystem, matching the privileges given to that role through COPY and file_fdw from above. Reviewed-By: Michael Paquier Discussion: https://postgr.es/m/20171231191939.GR2416%40tamriel.snowman.net
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
@@ -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
@@ -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
@@ -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
@@ -20119,10 +20119,21 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
     linkend="functions-admin-genfile-table"/> provide native access to
     files on the machine hosting the server. Only files within the
     database cluster directory and the <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 EXECUTE privilege on the
+    <function>pg_read_file()</function>, or related, functions allows them the
+    ability to read any file on the server which the database can read and
+    that those reads bypass all in-database privilege checks.  This means that,
+    among other things, a user with this access is able to read the contents of the
+    <literal>pg_authid</literal> table where authentication information is contained,
+    as well as read any file in the database.  Therefore, granting access to these
+    functions should be carefully considered.
    </para>
 
    <table id="functions-admin-genfile-table">
@@ -20140,7 +20151,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>
@@ -20171,7 +20182,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>
@@ -20180,7 +20191,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>
@@ -20189,7 +20200,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
@@ -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
@@ -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 and other functions which allow executing a server-side program.</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>
@@ -556,7 +581,8 @@ DROP ROLE doomed_role;
 
   <para>
   Care should be taken when granting these roles to ensure they are only used where
-  needed to perform the desired monitoring.
+  needed and with the understanding that these roles grant access to privileged
+  information.
   </para>
 
   <para>
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
@@ -23,6 +23,8 @@
 #include "access/sysattr.h"
 #include "access/xact.h"
 #include "access/xlog.h"
+#include "catalog/dependency.h"
+#include "catalog/pg_authid.h"
 #include "catalog/pg_type.h"
 #include "commands/copy.h"
 #include "commands/defrem.h"
@@ -769,8 +771,8 @@ CopyLoadRawBuf(CopyState cstate)
  * input/output stream. The latter could be either stdin/stdout or a
  * socket, depending on whether we're running under Postmaster control.
  *
- * Do not allow a Postgres user without superuser privilege to read from
- * or write to a file.
+ * Do not allow a Postgres user without the 'pg_access_server_files' role to
+ * read from or write to a file.
  *
  * Do not allow the copy if user doesn't have proper permission to access
  * the table or the specifically requested columns.
@@ -787,21 +789,37 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 	Oid			relid;
 	RawStmt    *query = NULL;
 
-	/* Disallow COPY to/from file or program except to superusers. */
-	if (!pipe && !superuser())
+	/*
+	 * Disallow COPY to/from file or program except to users with the
+	 * appropriate role.
+	 */
+	if (!pipe)
 	{
 		if (stmt->is_program)
-			ereport(ERROR,
-					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-					 errmsg("must be superuser to COPY to or from an external program"),
-					 errhint("Anyone can COPY to stdout or from stdin. "
-							 "psql's \\copy command also works for anyone.")));
+		{
+			if (!is_member_of_role(GetUserId(), DEFAULT_ROLE_EXECUTE_SERVER_PROGRAM))
+				ereport(ERROR,
+						(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+						 errmsg("must be superuser or a member of the pg_execute_server_program role to COPY to or from an external program"),
+						 errhint("Anyone can COPY to stdout or from stdin. "
+								 "psql's \\copy command also works for anyone.")));
+		}
 		else
-			ereport(ERROR,
-					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-					 errmsg("must be superuser to COPY to or from a file"),
-					 errhint("Anyone can COPY to stdout or from stdin. "
-							 "psql's \\copy command also works for anyone.")));
+		{
+			if (is_from && !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_SERVER_FILES))
+				ereport(ERROR,
+						(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+						 errmsg("must be superuser or a member of the pg_read_server_files role to COPY from a file"),
+						 errhint("Anyone can COPY to stdout or from stdin. "
+								 "psql's \\copy command also works for anyone.")));
+
+			if (!is_from && !is_member_of_role(GetUserId(), DEFAULT_ROLE_WRITE_SERVER_FILES))
+				ereport(ERROR,
+						(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+						 errmsg("must be superuser or a member of the pg_write_server_files role to COPY to a file"),
+						 errhint("Anyone can COPY to stdout or from stdin. "
+								 "psql's \\copy command also works for anyone.")));
+		}
 	}
 
 	if (stmt->relation)
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
@@ -22,6 +22,7 @@
 
 #include "access/htup_details.h"
 #include "access/xlog_internal.h"
+#include "catalog/pg_authid.h"
 #include "catalog/pg_type.h"
 #include "funcapi.h"
 #include "mb/pg_wchar.h"
@@ -45,6 +46,12 @@ typedef struct
  *
  * Filename may be absolute or relative to the DataDir, but we only allow
  * absolute paths that match DataDir or Log_directory.
+ *
+ * This does a privilege check against the 'pg_read_server_files' role, so
+ * this function is really only appropriate for callers who are only checking
+ * 'read' access.  Do not use this function if you are looking for a check
+ * for 'write' or 'program' access without updating it to access the type
+ * of check as an argument and checking the appropriate role membership.
  */
 static char *
 convert_and_check_filename(text *arg)
@@ -54,6 +61,15 @@ convert_and_check_filename(text *arg)
 	filename = text_to_cstring(arg);
 	canonicalize_path(filename);	/* filename can change length here */
 
+	/*
+	 * Members of the 'pg_read_server_files' role are allowed to access any
+	 * files on the server as the PG user, so no need to do any further checks
+	 * here.
+	 */
+	if (is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_SERVER_FILES))
+		return filename;
+
+	/* User isn't a member of the default role, so check if it's allowable */
 	if (is_absolute_path(filename))
 	{
 		/* Disallow '/a/b/data/..' */
diff --git a/src/include/catalog/pg_authid.h b/src/include/catalog/pg_authid.h
@@ -108,6 +108,12 @@ DATA(insert OID = 3375 ( "pg_read_all_stats" f t f f f f f -1 _null_ _null_));
 #define DEFAULT_ROLE_READ_ALL_STATS 3375
 DATA(insert OID = 3377 ( "pg_stat_scan_tables" f t f f f f f -1 _null_ _null_));
 #define DEFAULT_ROLE_STAT_SCAN_TABLES	3377
+DATA(insert OID = 4569 ( "pg_read_server_files" f t f f f f f -1 _null_ _null_));
+#define DEFAULT_ROLE_READ_SERVER_FILES	4569
+DATA(insert OID = 4570 ( "pg_write_server_files" f t f f f f f -1 _null_ _null_));
+#define DEFAULT_ROLE_WRITE_SERVER_FILES	4570
+DATA(insert OID = 4571 ( "pg_execute_server_program" f t f f f f f -1 _null_ _null_));
+#define DEFAULT_ROLE_EXECUTE_SERVER_PROGRAM	4571
 DATA(insert OID = 4200 ( "pg_signal_backend" f t f f f f f -1 _null_ _null_));
 #define DEFAULT_ROLE_SIGNAL_BACKENDID	4200
 
