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
withmy 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
mysuperuser to be able to perform this task, but it seems that for security reasons we presently don't allow access to
absolutepath names (except in the data dir and log dir) - not even for a superuser. Is that correct? In that case the
securityimplications 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
granularityof 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
