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
> https://www.postgresql.org/message-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