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

On Wed, Jan 25, 2017 at 8:22 PM, Stephen Frost <sfrost@snowman.net> wrote:
> Apparently we disagree about what is a 'reasonable manner'.

Yes.  I think that a "reasonable manner" should mean "what the DBA
thinks is reasonable", whereas you apparently think it should mean
"what the DBA thinks is reasonable, but only if the core developers
and in particular Stephen Frost also think it's reasonable".

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.
But:

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

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.

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.

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).

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.  That would have a much better chance of
preventing people from delegating access to dangerous functions
inadvertently than the current method, which relies on people knowing
(without documentation) that you've attempted to leave hard-coded
superuser() checks in some functions but not others for reasons that
sometimes but not always include privilege escalation, correctly
distinguishing which such cases involve privilege escalation as
opposed to other arbitrary criteria, and deciding neither to create
secdef wrappers for anything that has a built-in check for reasons of
privilege isolation nor to give up on privilege isolation and hand out
superuser to people who need pg_ls_dir().  While such a clever chain
of deductive reasoning cannot be ruled out, it would be astonishing if
it happened very often.

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.
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.

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