Re: [HACKERS] pg_ls_dir & friends still have a hard-coded superusercheck - Mailing list pgsql-hackers

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Fri, Jan 27, 2017 at 11:34 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > * Robert Haas (robertmhaas@gmail.com) wrote:
> >> OK, fair enough.  get_raw_page() is clearly not something that we
> >> really want everybody to have access to by default, but if it were up
> >> to me, I'd change the permissions check inside the function to do a
> >> check for select privileges on the relation, and then I'd change the
> >> SQL script to revoke access from everybody but the superuser.
> >
> > The way to do properly do this would not be to have some conflation of
> > the right to execute get_raw_page() combined with a typically granted
> > right like 'SELECT'.  Instead, it would be to have an explicit RAW_PAGE
> > or similar permission which could be GRANT'd to a user for a particular
> > relation, and then we could change the superuser() check into a check
> > against that right, and call it done.
>
> That's moving the goalposts a very large distance.
>
> >> Actually, I think that's Stephen should have done something similar
> >> for pgstattuple in commit fd321a1dfd64d30bf1652ea6b39b654304f68ae4, ...
> >
> > Requiring pgstattuple() to check if a user has any rights on a table
> > means that an existing non-superuser monitoring tool that's calling
> > pgstattuple() for summary information would be required to have SELECT
> > rights across, most likely, all tables, even though the monitoring
> > user's got no need for that level of access.  Now, we could argue that
> > having access to just one column would be enough for that user, but
> > that's still more access than the monitoring user needs, and there's
> > still the issue of new tables (and default privileges don't currently
> > support column-level privileges anyway).
>
> ...whereas here you don't want to move the goalposts at all.  I can't
> understand this.  When I want the superuser to have the option to hand
> out pg_ls_dir() access for all directories pg_ls_dir() can access, you
> complain that's too broad.  Yet your own previous commit, which allows
> pgstattuple() access to be granted, makes no provision for any
> granularity of access control at all.  And I think there is an
> excellent argument - which I have made - that pgstattuple() is more
> likely to expose sensitive information than pg_ls_dir().

You're completely ignoring the use-cases for which these are being done.

I've outlined the precise use-case for pgstattuple()'s usage across the
entire database for which the admin has granted the EXECUTE access in.
I've not yet seen a use-case for access to pg_ls_dir() across all
directories pg_ls_dir() can access.  My recollection is that you brought
up pg_wal, but that's hardly every directory, and some hypothetical tool
which could intelligently figure out what orphaned files exist.  For the
former, I would recommend a function for exactly that (or perhaps
something better which provides more than just a count of files), for
the latter, that's something that I would be very worried that someone
trying to implement outside of core would get wrong or which could be
version-dependent.  We've already got some code in core to find files
left over from a crash and deal with them and perhaps that could be
expanded to deal with other cases.

> Even if we someday have the capability for people to grant pg_ls_dir()
> access on a directory-by-directory basis, I am sure there will still
> be a way for them to grant access on all the directories to which
> pg_ls_dir() can access today; after all, that's just two directories,
> and their subdirectories.  At most somebody would have to make two
> GRANTs.  Removing the hard-coded superuser() checks allows that use
> case now, and doesn't prohibit you from implementing the other thing
> later when and if and when we reach agreement on it.

With an appropriate use-case for it, I wouldn't be against it.  I linked
to both use-cases and a provided patch for finer-grained access to
pg_ls_dir() and friends three years ago, which got shot down.  I'm not
against it if the community wishes to reconsider that decision and
support having filesystem-like access where there's an appropriate
use-case for it, and with fine-grained access control provided to meet
that use-case.

Further, as it relates to goal-posts, if your goal is to let an admin
grant out the ability to see how many files are in pg_wal, you're
spending a great deal more effort than that would require.  I wouldn't
object (and would actually appreciate) a function which is able to do
exactly that, and I'd go work with Greg S-M right away to make sure that
check_postgres.pl knows about that function and uses it in PG10 and
above.

I do object to someone coming along and saying "let's rip out all the
superuser() checks" and it would seem that I'm not alone.

Thanks!

Stephen