Re: [HACKERS] pg_ls_dir & friends still have a hard-coded superuser check

On Wed, Jan 25, 2017 at 3:17 PM, Stephen Frost <sfrost@snowman.net> wrote:
> Your email is 'pg_ls_dir & friends', which I took to imply at *least*
> pg_read_file() and pg_read_binary_file(), and it's not unreasonable to
> think you may have meant everything in adminpack, which also includes
> pg_file_write(), pg_file_rename() and pg_file_unlink().

Well, I was talking about genfile.c, which doesn't contain those
functions, but for the record I'm in favor of extirpating every single
hard-coded superuser check from the backend without exception.
Preventing people from calling functions by denying the ability to
meaningfully GRANT EXECUTE on those functions doesn't actually stop
them from delegating those functions to non-superusers.  It either (a)
causes them to give superuser privileges to accounts that otherwise
would have had lesser privileges or (b) forces them to use wrapper
functions to grant access rather than doing it directly or (c) some
other dirty hack.  If they pick (a), security is worse; if they pick
(b) or (c), you haven't prevented them from doing what they wanted to
do anyway.  You've just made it annoying and inconvenient.

Both EnterpriseDB's PEM and check_postgres.pl currently have a bunch
of things that don't work unless you run as superuser.  I think we
should be trying to provide ways of reducing those lists.  If I can't
get agreement on method (a), I'm going to go look for ways of doing
(b) or (c), which is more work and uglier but if I can't get consensus
here then oh well.

>> I don't really think it's necessary to outline the use case more than
>> I have already.  It's perfectly reasonable to want a monitoring tool
>> to have access to pg_ls_dir() - for example, you could use that to
>> monitor for relation files orphaned by a previous crash.
>
> Sure.  What I believe would be better would be a function in PG that
> allows you to know about all of the relation files which were orphaned
> from a previous crash, and perhaps even one to go clean all of those up.
> I certainly would have no issue with both of those being available to a
> non-superuser.
>
> Sure, you could do the same with pg_ls_dir(), various complex bits of
> SQL to figure out what's been orphaned, and pg_file_unlink() to handle
> the nasty part of unlinking the files, but you could also use
> pg_ls_dir() to look at the files in PG's home directory, or
> pg_file_unlink() to remove the PG user's .bashrc.
>
> Does the monitoring tool need to be able to see the files in PG's root
> directory, or to be able to  unlink the PG user's .bashrc?  No.

I do not accept your authority to determine what monitoring tools need
to or should do.  Monitoring tools that use pg_ls_dir are facts on the
ground, and your disapprobation doesn't change that at all.  It just
obstructs moving to a saner permissions framework.  I agree that
pg_file_unlink() is an unlikely need for a monitoring tool, but (1)
conflating pg_ls_dir with pg_file_unlink is a bit unfair and (2)
there's no actual point whatsoever in trying to restrict to whom the
superuser can give permissions, because the superuser has numerous
ways of working around any such restriction.

> I don't think it's nannyism to keep things like pg_read_file and
> pg_file_write as superuser-only.

I entirely disagree.  It is for the DBA to decide to whom and for what
purpose he wishes to give permissions.  And if somebody writes a tool
-- monitoring or otherwise -- that needs those permissions, the DBA
should be empowered to give those permissions and no more, and should
be empowered to do that in a nice, clean way without a lot of hassle.
The idea that you or I would try to decide which functions a user is
allowed to want to grant access to and which ones they are not allowed
to want to grant access to seems laughable to me.  We're not smart
enough to foresee every possible use case, and we shouldn't try.  Even
for a function that theoretically allows a privilege escalation to
superuser (like pg_write_file), it's got to be better to give a user
account permission to use that one function than to just give up and
give that account superuser privileges, because escalating privileges
with that blunt instrument would take more work than a casual hacker
would be willing to put in.  For a function like the one that this
thread actually started out being about, like pg_ls_dir, it's vastly
better.  You mention that you're not sure that pg_ls_dir() would allow
a privilege escalation to superuser, but I think we both know that
there is probably no such escalation.  Why you'd rather have people
running check_postgres.pl's wal_files check under an actual superuser
account rather than an account that only has rights to do pg_ls_dir is
beyond me.

Your response will no doubt be that there should otherwise be some
other way to write that check that doesn't use pg_ls_dir.  Maybe.  But
the thing is that pg_ls_dir is a general tool.  You can use it to do a
lot of things, and you can write a monitoring probe that does those
things without having to wait for a new release of PostgreSQL.  So
your argument boils down to saying that when somebody wants to monitor
something that could be checked using pg_ls_dir(), they shouldn't do
write a query using pg_ls_dir().  Instead, they should submit a new
patch to core that adds a special-purpose function implementing the
exact functionality they need, get consensus on it, get somebody to
commit it, wait for a new PostgreSQL release to come out, wait for the
users who might want the probe to upgrade to that new release, and
THEN they can do the monitoring they want to do.  Now, I say that's
ridiculous.  What's actually going to happen is that the tool author
is going to say "this probe requires superuser" and move on.  The fact
that check_postgres.pl has done exactly that for about 5 different
probes -- not all because of pg_ls_dir, but one of them is for that
reason -- suggests that this will in fact be a typical reaction
whenever a fine-grained privilege is not available to be granted.  We
can decide to alleviate that pain or we can decide against it, but
telling people "don't do that" is not going to work.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company