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

Robert,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Wed, Jan 25, 2017 at 10:31 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > Frankly, I get quite tired of the argument essentially being made here
> > that because pg_ls_dir() wouldn't grant someone superuser rights, that
> > we should remove superuser checks from everything. As long as you are
> > presenting it like that, I'm going to be quite dead-set against any of
> > it.
>
> I'm not presenting it like that, and I think I've been quite clear
> about that. I'm making two separate arguments which you keep
> conflating.

The only function you mentioned in the last email was pg_ls_dir(), this
one looks better in that you're at least calling out the other changes
you wish to make explicitly with reasoning behind them.

> The first is that restricting the ability to GRANT access
> to a function, even a function that allows escalation to superuser
> privileges, doesn't improve security, because the superuser can still
> grant those privileges with more work.

Having various inconsistent and unclear ways to grant access to a
privileged user is making security worse and that is very much part of
my concern. I don't agree, regardless of how many times you claim it,
that because the superuser could still grant superuser to someone (or
could grant the individual privileges with more work) keeps things
status-quo regarding security, or somehow that not making the changes
you are proposing reduces existing security.

I've certainly learned and come to accept that security, particularly
extremely privileged security, is much better when it's kept simple and
having multiple ways for someone to become a superuser, even if we admit
that there are and document the different ways, certainly isn't simpler
and doesn't make me feel like we're improving security.

> The second is that if we were to
> accept as official policy the idea that functions should have built-in
> superuser() checks when and only when they allow escalation to
> superuser, then those checks should be removed from those functions
> which do not allow such an escalation. That argument favors removing
> at least some of the superuser checks as inconsistent with policy. If
> there's a policy -- even one that I don't like -- it should be
> enforced consistently.

This I tend to agree with and I do believe there are may be additional
superuser() checks which could be removed. In my initial work, I
focused on just the back-end, with some subsequent work on a particular
contrib module which I was asked to look at. There are likely further
contrib modules which could also be changed, as long as those changes
are reviewed and done carefully, with consideration that we don't want
the admin to end up granting away superuser rights when they really only
intended to grant some specific subset of privileges.

> I audited the core backend for hard-coded superuser() checks that made
> it categorically impossible to call a certain SQL function. I looked
> only for functions where the check was precisely if (!superuser())
> ereport(ERROR, ...), so things that were contingent on rolreplication
> or anything other than superuser-ness are not included in this
> analysis. I found a total of five such functions, of which I can
> think of possible escalation-to-superuser risk for two. I did not
> examine contrib although that might be worth doing at some point.

> 1. pg_ls_dir. I cannot see how this can ever be used to get superuser
> privileges.

> 2. pg_stat_file. I cannot see how this can ever be used to get
> superuser privileges.

I appreciate your efforts, but, for my part, I still don't agree with
removing the checks from functions which are, essentially, providing
filesystem-level access to non-superusers. The right way to support
this would be to work through the issues that were already brought up
when Adam and I proposed similar capabilities on this very list,
complete with patches, three years ago:

https://www.postgresql.org/message-id/CAKRt6CRCQ1jmh3o8gkBBHxWqBJz4StnYUQjO0W8Kauru_SfPKA%40mail.gmail.com

As it relates specifically to *monitoring* tools, I continue to be the
opinion that we should provide better ways for those tools to get at
the information they need.

> 3. pg_read_binary_file. This could potentially let you get superuser
> or other privileges. Most obviously, if you read and parse the
> contents of pg_authid, you might be able to find passwords and then
> break into things. There might be other methods as well.
>
> 4. pg_read_file. Essentially the same risks, although reading data
> files will be a little harder with this function because any NUL byte
> is going to make the read error out. But given time and determination
> you can probably still ascertain the contents of every byte in the
> database, so the risks are about the same.

Yes, these are clearly functions which can, in most if not all cases, be
used to get superuser-level access, and I don't believe we do anyone a
service by making it less obvious that it's possible to become a
superuser using them.

> 5. pg_import_system_collations. There doesn't seem to be any obvious
> way to use this to get superuser privileges, but there might be some
> subtle risk I'm missing.

I haven't looked particularly carefully at it myself.

Why aren't you including the other functions mentioned? Not including
them, even if they're in contrib, would still end up with things in an
inconssitent state, and it hardly seems like it'd be much effort to go
through the rest of them with equal care.

> Attached is a patch that removes the hard-coded superuser checks from
> all five of these functions, based on the first argument above.

Perhaps unsuprisingly, but you've still not convinced me, so I don't
agree with this change.

> Currently, I count three votes in favor of this approach and one
> opposed. If anyone else wants to weigh in, please do. It would be
> helpful if anyone weighing in can be clear about whether (a) they are
> in favor of the patch as proposed, or (b) they are not in favor of the
> patch as proposed but could support a narrower patch that removed the
> checks only from functions with no known escalate-to-superuser risks,
> or (c) they oppose all change. It would also be helpful if the
> reasons why each person takes the position that they do could be
> mentioned.

I agree that it'd be nice if others would weigh in on this.

Thanks!

Stephen