pg_ls_dir & friends still have a hard-coded superuser check

I tend to agree. But in the past when this came up people pointed out
you could equally do things this way and still grant all the access
you wanted using SECURITY DEFINER. Arguably that's a better approach
because then instead of auditing the entire monitor script you only
need to audit this one wrapper function, pg_ls_monitor_dir() which
just calls pg_ls_dir() on this one directory.

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

On Wed, Jan 25, 2017 at 11:30 AM, Greg Stark <stark@mit.edu> wrote:

I tend to agree. But in the past when this came up people pointed out
you could equally do things this way and still grant all the access
you wanted using SECURITY DEFINER. Arguably that's a better approach
because then instead of auditing the entire monitor script you only
need to audit this one wrapper function, pg_ls_monitor_dir() which
just calls pg_ls_dir() on this one directory.

I agree that can be done, but it's nicer if you can use the same SQL
all the time. With that solution, you need one SQL query to run when
you've got superuser privileges and a different SQL query to run when
you are running without superuser privileges but somebody's run the
create-security-definer-wrappers-for-me script. That's a deployment
nuisance if you want to support both configurations.

Also, the same argument could be made about removing the built-in
superuser check from ANY function, and we've already rejected that
argument for a bunch of other functions. If we say that argument is
valid for some functions but not others, then we've got to decide for
which ones it's valid and for which ones it isn't, and consensus will
not be forthcoming. I take the position that hard-coded superuser
checks stink in general, and I'm grateful to Stephen for his work
making dump/restore work properly on system catalog permissions so
that we can support better alternatives. I'm not asking for anything
more than that we apply that same policy here as we have in other
cases.

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

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:

The use case I have in mind is
a monitoring tool that needs access to one more of those functions --
in keeping with the principle of least privilege, it's much better to
give the monitoring user only the privileges which it actually needs
than to make it a superuser.

That isn't what you're doing with those functions though, you're giving
the monitoring tool superuser-level rights but trying to pretend like
you're not.

That's not really how good security works.

I am entirely in agreement with providing a way to give monitoring tools
more information, but that should be done through proper design and
consideration of just what info they actually need (as well as what a
useful format for it is).

On my plate for a long time has been to add a function to return how
much WAL is remaining in pg_wal for a monitoring system to be able to
report on. That could be done with something like pg_ls_dir, but that's
a rather hokey way to get it, and it'd be a lot nicer to just have a
function which returns it, or maybe one that returns the oldest WAL
position available on the system and what the current position is, which
I think we might actually have.

In other words, please actually outline a use-case, and let's design a
proper solution.

Thanks!

Stephen

Greg,

* Greg Stark (stark@mit.edu) wrote:

I tend to agree. But in the past when this came up people pointed out
you could equally do things this way and still grant all the access
you wanted using SECURITY DEFINER. Arguably that's a better approach
because then instead of auditing the entire monitor script you only
need to audit this one wrapper function, pg_ls_monitor_dir() which
just calls pg_ls_dir() on this one directory.

I'm not a fan of SECURITY DEFINER functions for this sort of thing and
don't like the suggestion of simply wrapping functions that provide
superuser-level access in a security definer function and then saying
that giving someone access to that function isn't giving them superuser,
because that's just false.

Thanks!

Stephen

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:

Also, the same argument could be made about removing the built-in
superuser check from ANY function, and we've already rejected that
argument for a bunch of other functions. If we say that argument is
valid for some functions but not others, then we've got to decide for
which ones it's valid and for which ones it isn't, and consensus will
not be forthcoming. I take the position that hard-coded superuser
checks stink in general, and I'm grateful to Stephen for his work
making dump/restore work properly on system catalog permissions so
that we can support better alternatives. I'm not asking for anything
more than that we apply that same policy here as we have in other
cases.

I went over *every* superuser check in the system when I did that work,
wrote up a long email about why I made the decisions that I did, posted
it here, had follow-on discussions, all of which lead to the patch which
ended up going in.

I am not anxious to revisit that decision and certainly not based on
an argument that, so far, boils down to "I think a monitoring system
might be able to use this function that allows it to read pg_authid
directly, so we should drop the superuser() check in it."

Thanks!

Stephen

On Wed, Jan 25, 2017 at 2:08 PM, Stephen Frost <sfrost@snowman.net> wrote:

That isn't what you're doing with those functions though, you're giving
the monitoring tool superuser-level rights but trying to pretend like
you're not.

Uh, how so? None of those functions can be used to escalate to
superuser privileges. I am trying to give SOME superuser privileges
and not others. That IS how good security works.

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. Also, as
mentioned above, I don't think this should have to be litigated for
every single function individually. If it's a good idea for a
non-superuser to be able to pg_start_backup() and pg_stop_backup(),
the person doing that has access to read every byte of data in the
filesystem; if they don't, there's no point in giving them access to
run those functions. Access to just pg_ls_dir(), for example, can't
be any more dangerous than that. Indeed, I'd argue that it's a heck
of a lot LESS dangerous.

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

On Wed, Jan 25, 2017 at 2:13 PM, Stephen Frost <sfrost@snowman.net> wrote:

I went over *every* superuser check in the system when I did that work,
wrote up a long email about why I made the decisions that I did, posted
it here, had follow-on discussions, all of which lead to the patch which
ended up going in.

Link to that email? I went back and looked at that thread and didn't
see anything that looked like a general policy statement to me. But I
may have missed it.

I am not anxious to revisit that decision and certainly not based on
an argument that, so far, boils down to "I think a monitoring system
might be able to use this function that allows it to read pg_authid
directly, so we should drop the superuser() check in it."

Well, I'm not eager to revisit all the decisions you'd like to
overturn either, but we'll just both have to cope. I assume we're
both coming at these issues with the intention of making PostgreSQL
better; the fact that we don't always agree on everything is probably
inevitable.

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

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Wed, Jan 25, 2017 at 2:13 PM, Stephen Frost <sfrost@snowman.net> wrote:

I went over *every* superuser check in the system when I did that work,
wrote up a long email about why I made the decisions that I did, posted
it here, had follow-on discussions, all of which lead to the patch which
ended up going in.

Link to that email? I went back and looked at that thread and didn't
see anything that looked like a general policy statement to me. But I
may have missed it.

Not sure which thread you were looking at, but this one:

/messages/by-id/20141015052259.GG28859@tamriel.snowman.net

Has a review of all superuser checks in the backend, as noted in the
first paragraph ("shown below in a review of the existing superuser
checks in the backend").

Later on in that thread, at least in:
/messages/by-id/20160106161302.GP3685@tamriel.snowman.net

In an email to you and Noah:
----------------
The general approach which I've been using for the default roles is that
they should grant rights which aren't able to be used to trivially make
oneself a superuser.
----------------

My recollection is saying that about 10 times during that period of
time, though perhaps I am exaggurating due to it being a rather painful
process to get through.

I assume we're
both coming at these issues with the intention of making PostgreSQL
better;

Always.

the fact that we don't always agree on everything is probably
inevitable.

Agreed.

Thanks!

Stephen

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Wed, Jan 25, 2017 at 2:08 PM, Stephen Frost <sfrost@snowman.net> wrote:

That isn't what you're doing with those functions though, you're giving
the monitoring tool superuser-level rights but trying to pretend like
you're not.

Uh, how so? None of those functions can be used to escalate to
superuser privileges. I am trying to give SOME superuser privileges
and not others. That IS how good security works.

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

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.

Also, as
mentioned above, I don't think this should have to be litigated for
every single function individually. If it's a good idea for a
non-superuser to be able to pg_start_backup() and pg_stop_backup(),
the person doing that has access to read every byte of data in the
filesystem; if they don't, there's no point in giving them access to
run those functions. Access to just pg_ls_dir(), for example, can't
be any more dangerous than that. Indeed, I'd argue that it's a heck
of a lot LESS dangerous.

It wasn't litigated for every single function. A reivew of all cases
was performed and a very lengthy discussion held about how to give
non-superusers access to the functions which made sense was done, with
the resulting work finally being accepted and included into 9.6.

Further, as discussed and explained on the thread I just linked you to,
the assumption that a user who can call pg_start/stop_backup() has
access to read every byte on the filesystem isn't correct.

Am I sure that someone who has pg_ls_dir() could get superuser access on
the box? No, but I am sure that, in my opinion, it's frankly a pretty
dirty hack to use pg_ls_dir() in a monitoring tool and we should provide
better ways to monitor PG to our users.

Also, am I sure that we shouldn't ever give a user the ability to
arbitrairly list directories on the filesystem? No, but this isn't a
good justification for that change. If we *are* going to go down the
road of giving users filesystem-like access of any kind then I would
*much* rather look at the approach that I outlined before and that other
database systems have, which is to have a way to say "user X can read,
write, list, do whatever, with files in directory /blah/blah". Perhaps
with sub-directory abilities too. Nannyism, if we had that ability,
would be to say "you can set DIRECTORY to anything but PGDATA." or "you
can't set DIRECTORY to be /".

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

Thanks!

Stephen

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

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

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:

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.

Then it was correct for me to assume that's what you meant, and means my
reaction and response are on-point.

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.

The notion that security is 'worse' under (a) is flawed- it's no
different. With regard to 'b', if their wrapper function is
sufficiently careful to ensure that the caller isn't able to do anything
which would increase the caller's level to that of superuser, then
security is improved. If the wrapper simply turns around can calls the
underlying function, then it's no different from '(a)'.

I am suggesting that we shouldn't make it look like there are
distinctions when there is actually no difference. That is a good thing
for our users because it keeps them informed about what they're actually
doing when they grant access.

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've commented on here and spoken numerous times about exactly that goal
of reducing the checks in check_postgres.pl which require superuser.
You're not actually doing that and nothing you've outlined in here so
far makes me believe you see how having pg_write_file() access is
equivilant to giving someone superuser, and that concerns me.

As someone who has contributed code and committed code back to
check_postgres.pl, I would be against making changes there which
install security definer functions to give the monitoring user
superuser-level access, and I believe Greg S-M would feel the same way
considering that he and I have discussed exactly that in the past. I
don't mean to speak for him and perhaps his opinion has changed, but it
seems unlikely to me.

If the DBA wants to give the monitoring user superuser-level access to
run the superuser-requiring checks that check_postgres.pl has, they're
welcome to do so, but they'll be making an informed decision that they
have weighed the risk of their monitoring user being compromised against
the value of that additional monitoring, which is an entirely
appropriate and reasonable decision for them to make.

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.

Allowing GRANT to be used to give access to pg_write_file and friends is
not a 'permissions framework'. Further, I am not claiming authority
over what monitoring tools should need to do or not do, and a great many
people run their monitoring tools as superuser. I am not trying to take
away their ability to do so.

The way to make progress here is not, however, to just decide that all
those superuser() checks we put in place were silly and should be
removed, it's to provide better ways to monitor PG which provide exactly
the monitoring information needed in a useful and sensible way.

I understand the allure of just removing a few lines of code to make
things "easier" or "faster" or "better", but I don't think it's a good
idea to remove these superuser checks, nor do I think it's a good idea
to remove our WAL CRCs even if it'd help some of my clients, nor do I
think it's a good idea to have checksums disabled by default, or to rip
out all of WAL as being "unnecessary" because we have ZFS and it should
handle all things data and we could just fsync all of the heap files on
commit and be done with it.

Admittedly, you're not argueing for half of what I just mentioned, but
I'm not arguing that DBAs shouldn't be able to have their monitoring
users be superusers either, I'm pointing out that removing those checks
makes any user who is GRANT access to the functions the equivilant of a
superuser, potentially without the DBA realizing it, and that is an
all-together bad thing. If a DBA sees the reasoning behind not making a
monitoring user a superuser, we shouldn't be trying to pull the wool
over their eyes or sprinkling a little GRANT dust over in the corner
that turns the monitoring user into an account with superuser-level
access, we should accept that their decision is to not have the
monitoring user have superuser rights and ensure that the database
permission system doesn't give it to them.

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 DBA is well within their rights and abilities to do so with a simple
comamnd: ALTER ROLE. I am not suggesting that we take their right or
ability to do so away from them.

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.

I don't want people to run the wal_files check as a superuser, as I've
said before, even on this thread, there should be a better answer. I do
not view removing the superuser() check, while appealing and appearing
to be simple, to be that 'better answer'.

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.

With pg_write_file(), pg_read_file(), and friends, there's a whole lot
of stuff we could punt on and say "user, you figure out how to make
these work for you, we don't feel like doing anything more."

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

No, I'm saying that we should provide something better than pg_ls_dir()
for them to use.

If they want to use pg_ls_dir(), then they can go for it, but I
really do *not* think the answer to "what's the best way to see how much
WAL space PG is using?" is "oh, just use pg_ls_dir()." Consdier if we
had such a neat feature today to return the number of WAL files-
assuming we named it correctly, we wouldn't have people complaining
about the fact that we're about to change pg_xlog to pg_wal. Generally
speaking, I'd really like external tools, monitoring systems, etc, to
know less, not more, about our on-disk data layout and structures, where
that's possible.

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.

I sure do think that we should add new monitoring capabilities to PG and
that we absolutely should *not* start telling people who want to
implement new monitoring features that "oh, well, users can just use
pg_read_binary_file() directly to get that info from pg_controldata, why
would we need to write a function for that?" From my perspective, at
least, that's about the same level as what you're asking to do with
pg_ls_dir(). If your goal is to give users the ability to query how
many WAL files are in pg_wal, I don't doubt that you could get it
implemented and into PG10 with a lot more ease than arguing about
pg_ls_dir() with me, or anyone else who wants to chime in on this, and
users would have it at just the same time as they would a hack to
pg_ls_dir(), and they wouldn't be giving their monitoring user the
ability to look at what files the DBA has his home directory on the
database server.

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.

Sure, and that's a fair response.

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.

I am all in favor of alleviating that pain, by providing a proper
solution which is actually well thought out and designed to answer the
question.

Removing the check from pg_ls_dir() isn't any of that.

Thanks!

Stephen

Hi,

On 2017-01-25 16:52:38 -0500, Stephen Frost wrote:

* Robert Haas (robertmhaas@gmail.com) wrote:

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.

The notion that security is 'worse' under (a) is flawed- it's no
different.

Huh? Obviously that's nonesense, given the pg_ls_dir example.

With regard to 'b', if their wrapper function is
sufficiently careful to ensure that the caller isn't able to do anything
which would increase the caller's level to that of superuser, then
security is improved.

Given how complex "sufficiently careful" is for security definer UDFs,
in comparison to estimating the security of granting to a function like
pg_ls_dir (or pretty much any other that doesn't call out to SQL level
stuff like operators, output functions, etc), I don't understand this.

If the wrapper simply turns around can calls the underlying function,
then it's no different from '(a)'.

Except for stuff like search path.

I am suggesting that we shouldn't make it look like there are
distinctions when there is actually no difference. That is a good thing
for our users because it keeps them informed about what they're actually
doing when they grant access.

This position doesn't make a lick of sense to me. There's simply no
benefit at all in requiring to create wrapper functions, over allowing
to grant to non-superuser. Both is possible, secdef is a lot harder to
get right. And you already heavily started down the path of removing
superuser() type checks - you're just arguing to make it more or less
randomly less consistent.

I've commented on here and spoken numerous times about exactly that goal
of reducing the checks in check_postgres.pl which require superuser.
You're not actually doing that and nothing you've outlined in here so
far makes me believe you see how having pg_write_file() access is
equivilant to giving someone superuser, and that concerns me.

That's the users responsibility, and Robert didnt' really suggest
granting pg_write_file() permissions, so this seems to be a straw man.

Andres

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

Andres,

* Andres Freund (andres@anarazel.de) wrote:

On 2017-01-25 16:52:38 -0500, Stephen Frost wrote:

* Robert Haas (robertmhaas@gmail.com) wrote:

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.

The notion that security is 'worse' under (a) is flawed- it's no
different.

Huh? Obviously that's nonesense, given the pg_ls_dir example.

Robert's made it clear that he'd like to have a blanket rule that we
don't have superuser checks in these code paths if they can be GRANT'd
at the database level, which goes beyond pg_ls_dir.

If the question was only about pg_ls_dir, then I still wouldn't be for
it, because, as the bits you didn't quote discussed, it encourages users
and 3rd party tool authors to base more things off of pg_ls_dir to
look into the way PG stores data on disk, and affords more access than
the monitoring user has any need for, none of which are good, imv. It
also discourages people from implementing proper solutions which you can
'just use pg_ls_dir()', which I also don't agree with.

If you really want to do an ls, go talk to the OS. ACLs are possible to
provide that with more granularity than what would be available through
pg_ls_dir(). We aren't in the "give a user the ability to do an ls"
business, frankly.

With regard to 'b', if their wrapper function is
sufficiently careful to ensure that the caller isn't able to do anything
which would increase the caller's level to that of superuser, then
security is improved.

Given how complex "sufficiently careful" is for security definer UDFs,
in comparison to estimating the security of granting to a function like
pg_ls_dir (or pretty much any other that doesn't call out to SQL level
stuff like operators, output functions, etc), I don't understand this.

If you're implying that security definer UDFs are hard to write and get
correct, then I agree with you there. I was affording the benefit of
the doubt to that proposed approach.

If the wrapper simply turns around can calls the underlying function,
then it's no different from '(a)'.

Except for stuff like search path.

If you consider '(a)' to be the same as superuser, which I was
postulating, then this doesn't strike me as making terribly much
difference.

I am suggesting that we shouldn't make it look like there are
distinctions when there is actually no difference. That is a good thing
for our users because it keeps them informed about what they're actually
doing when they grant access.

This position doesn't make a lick of sense to me. There's simply no
benefit at all in requiring to create wrapper functions, over allowing
to grant to non-superuser. Both is possible, secdef is a lot harder to
get right. And you already heavily started down the path of removing
superuser() type checks - you're just arguing to make it more or less
randomly less consistent.

I find this bizarre considering I went through a detailed effort to go
look at every superuser check in the system and discussed, on this list,
the reasoning behind each and every one of them. I do generally
consider arbitrary access to syscalls via the database to be a privilege
which really only the superuser should have.

I've commented on here and spoken numerous times about exactly that goal
of reducing the checks in check_postgres.pl which require superuser.
You're not actually doing that and nothing you've outlined in here so
far makes me believe you see how having pg_write_file() access is
equivilant to giving someone superuser, and that concerns me.

That's the users responsibility, and Robert didnt' really suggest
granting pg_write_file() permissions, so this seems to be a straw man.

He was not arguing for only pg_ls_dir(), but for checks in all
"friends", which he later clarified to include pg_write_file().

Thanks!

Stephen

On 2017-01-25 18:04:09 -0500, Stephen Frost wrote:

Andres,

* Andres Freund (andres@anarazel.de) wrote:

On 2017-01-25 16:52:38 -0500, Stephen Frost wrote:

* Robert Haas (robertmhaas@gmail.com) wrote:

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.

The notion that security is 'worse' under (a) is flawed- it's no
different.

Huh? Obviously that's nonesense, given the pg_ls_dir example.

Robert's made it clear that he'd like to have a blanket rule that we
don't have superuser checks in these code paths if they can be GRANT'd
at the database level, which goes beyond pg_ls_dir.

That seems right to me. I don't see much benefit for the superuser()
style checks, with a few exceptions. Granting by default is obviously
an entirely different question.

If the question was only about pg_ls_dir, then I still wouldn't be for
it, because, as the bits you didn't quote discussed, it encourages users
and 3rd party tool authors to base more things off of pg_ls_dir to
look into the way PG stores data on disk, and affords more access than
the monitoring user has any need for, none of which are good, imv. It
also discourages people from implementing proper solutions which you can
'just use pg_ls_dir()', which I also don't agree with.

In other words, you're trying to force people to do stuff your preferred
way, instead of allowing them to get things done is a reasonable manner.

If you really want to do an ls, go talk to the OS. ACLs are possible to
provide that with more granularity than what would be available through
pg_ls_dir(). We aren't in the "give a user the ability to do an ls"
business, frankly.

Wut.

I am suggesting that we shouldn't make it look like there are
distinctions when there is actually no difference. That is a good thing
for our users because it keeps them informed about what they're actually
doing when they grant access.

This position doesn't make a lick of sense to me. There's simply no
benefit at all in requiring to create wrapper functions, over allowing
to grant to non-superuser. Both is possible, secdef is a lot harder to
get right. And you already heavily started down the path of removing
superuser() type checks - you're just arguing to make it more or less
randomly less consistent.

I find this bizarre considering I went through a detailed effort to go
look at every superuser check in the system and discussed, on this list,
the reasoning behind each and every one of them. I do generally
consider arbitrary access to syscalls via the database to be a privilege
which really only the superuser should have.

Just because you argued doesn't mean I agree.

Greetings,

Andres Freund

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

Andres,

* Andres Freund (andres@anarazel.de) wrote:

On 2017-01-25 18:04:09 -0500, Stephen Frost wrote:

Robert's made it clear that he'd like to have a blanket rule that we
don't have superuser checks in these code paths if they can be GRANT'd
at the database level, which goes beyond pg_ls_dir.

That seems right to me. I don't see much benefit for the superuser()
style checks, with a few exceptions. Granting by default is obviously
an entirely different question.

Well, for my part at least, I disagree. Superuser is a very different
animal, imv, than privileges which can be GRANT'd, and I feel that's an
altogether good thing.

In other words, you're trying to force people to do stuff your preferred
way, instead of allowing them to get things done is a reasonable manner.

Apparently we disagree about what is a 'reasonable manner'.

Thanks!

Stephen

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

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

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

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

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:

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

/messages/by-id/CAKRt6CRCQ1jmh3o8gkBBHxWqBJz4StnYUQjO0W8Kauru_SfPKA@mail.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