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

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> Your proposed policy is essentially that functions should have
> built-in superuser checks if having access to that function is
> sufficient to escalate your account to full superuser privileges.

Yes, I do.

> 1. There's no consensus on any such policy.

Evidently.  I will say that it was brought up on a thread with quite a
bit of general discussion and resulted in a committed patch.  If you
want my 2c on it, there was at least a consensus on it at that time,
even if one no longer exists.  I certainly don't recall anyone
clamouring at the time that we should re-evaluate how my assessment was
done or the choices made regarding the functions which you now, 2 years
later, wish to change.

> 2. If there were such a policy it would favor, not oppose, changing
> pg_ls_dir(), because you can't escalate to superuser given access to
> pg_ls_dir().  Yet you are also opposed to changing pg_ls_dir() for
> reasons that boil down to a personal preference on your part for
> people not using it to build monitoring scripts.

If your argument was specifically regarding pg_ls_dir() and pg_ls_dir()
only then I would have misgivings about it because it's a piss-poor
solution to the problem that you've actually presented and I am
concerned that such a "design" will lead to later implication that it's
the "right" thing to do and that we shouldn't try to do better.

In other words, the exact argument we always tend to have when we're
talking about new features and their design- is it the right design,
etc.

> 3. Such a policy can only be enforced to the extent that we can
> accurately predict which functions can be used to escalate to
> superuser, which is not necessarily obvious in every case.  Under your
> proposed policy, if a given function turns out to be more dangerous
> than we'd previously thought, we'd have to stick the superuser check
> back in for the next release.  And if it turns out to be less
> dangerous than we thought, we'd take the check out.  That would be
> silly.

If the proposed function was able to be used to escalate a user to
superuser status then I'd think we'd want to do a security release to
fix whatever that hole is, since it's almost certainly not the intended
use of the function and the lack of proper checking is a security risk.

If we stop doing that, where do we draw the line?  Is it ok for
pg_termiante_backend() to be used to gain superuser-level access through
some kind of race condition?  What about pg_start_backup()?  Which
functions are "OK" to be allowed to make users superuser, and which are
not, when we don't have superuser() checks in any of them?  Further, how
is the user supposed to know?  Are we going to start sticking labels on
functions which say "WARNING: This can be used to become a superuser!"
I certainly hope not.

> 4. Such a policy is useless from a security perspective because you
> can't actually prevent superusers from delegating access to those
> functions.  You can force them to use wrapper functions but that
> doesn't eo ipso improve security.  It might make security better or
> worse depending on how well the functions are written, and it seems
> extremely optimistic to suppose that everyone who writes a security
> definer wrapper function will actually do anything more than expose
> the underlying function as-is (and maybe forget to schema-qualify
> something).

This is not about preventing superusers from delegating access to
whichever users they wish to, where it makes sense to do so.

> 5. If you're worried about people granting access to functions that
> allow escalation to the superuser account, what you really ought to do
> is put some effort into documenting which functions have such hazards
> and for what general reasons.

I did, by making the ones that I don't believe can be used to gain
superuser level access GRANT'able to non-superusers.  The ones which I
didn't feel comfortable with changing in that way were left with the
superuser() checks in them.

Maybe I got that wrong, but that was certainly the idea, as discussed on
the thread which I linked you to.

> I'd be willing to agree to write documentation along the lines
> suggested in (5) as a condition of removing the remaining superuser
> checks if you'd be willing to review it and suggest a place to put it.

I'm really not anxious to have a bunch of such warnings throughout the
docs.  Nor am I thrilled about the idea of having to argue about what
can and what can't- would you say pg_read_file() is superuser-equiv?  I
would, because in a great many cases, for all practical purposes, it is,
because a ton of users run with md5 and the auth info could be pulled
from pg_authid directly with pg_read_file().  The lack of any way to
contain the access that such a function would provide would also mean
that users would end up making a similar choice- GRANT access to this
function that an attacker could use to gain superuser() rights, or not?

That's not that far different from the choice of giving a user superuser
rights, and if the DBA has already said that they won't run their
monitoring solution with superuser rights, why would they say yes to
running it with access to a function that'll let the monitoring user
become a superuser?

> But I have a feeling compromise may not be possible here.  To me, the
> hand-wringing about the evils of pg_ls_dir() on this thread contrasts
> rather starkly with the complete lack of discussion about whether the
> patch removing superuser checks from pgstattuple was opening up any
> security vulnerabilities.  And given that the aforesaid patch lets a
> user who has EXECUTE privileges on pgstattuple run that function even
> on relations for which they have no other privileges, such as say
> pg_authid, it hardly seems self-evident that there are no leaks there.

Perhaps you might review what is returned by pgstattuple.  I don't
disagree that perhaps we should add a permissions check to those
functions because it makes sense to, but getting to superuser based on
the knowledge of how big a table is, how many tuples are in it, and
similar information, strikes me as not quite the same as letting users
use pg_file_write(), which, despite all of your complaints regarding my
stance on pg_ls_dir(), is one of the functions you'd like to change.

Were we to require some kind of per-table permission check on
pgstattuple, I'd prefer that it be something different from SELECT,
considering you can't actually get any of the table's DATA using
pgstattuple(), requiring SELECT rights on the table would end up
increasing a user's access to that table, possibly beyond what the DBA
wishes, understandably.

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.

Thanks!

Stephen