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

On Wed, Jan 25, 2017 at 10:31 PM, Stephen Frost <sfrost@snowman.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 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.  That argument favors removing
all superuser checks as pointless.  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.

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.

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.

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.

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

Thanks,

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers