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
