[PATCH v2] use has_privs_for_role for predefined roles

On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle
<joshua.brindle@crunchydata.com> wrote:

Attached is an updated version of the patch to replace
is_member_of_role with has_privs_for_role for predefined roles. It
does not remove is_member_of_role from acl.h but it does add a warning
not to use that function for privilege checking.

Please consider this for the upcoming commitfest.

I am not sure I understand what the advantage of this is supposed to be.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Oct 27, 2021 at 5:16 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle
<joshua.brindle@crunchydata.com> wrote:

Attached is an updated version of the patch to replace
is_member_of_role with has_privs_for_role for predefined roles. It
does not remove is_member_of_role from acl.h but it does add a warning
not to use that function for privilege checking.

Please consider this for the upcoming commitfest.

I am not sure I understand what the advantage of this is supposed to be.

Pre-defined roles currently do not operate the same way other roles do
with respect to inheritance. The patchfile has an explanation and
examples, I wasn't sure if that needed to be repeated in the email or
not.

On Wed, Oct 27, 2021 at 5:20 PM Joshua Brindle
<joshua.brindle@crunchydata.com> wrote:

On Wed, Oct 27, 2021 at 5:16 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle
<joshua.brindle@crunchydata.com> wrote:

Attached is an updated version of the patch to replace
is_member_of_role with has_privs_for_role for predefined roles. It
does not remove is_member_of_role from acl.h but it does add a warning
not to use that function for privilege checking.

Please consider this for the upcoming commitfest.

I am not sure I understand what the advantage of this is supposed to be.

Pre-defined roles currently do not operate the same way other roles do
with respect to inheritance. The patchfile has an explanation and
examples, I wasn't sure if that needed to be repeated in the email or
not.

And FWIW several predefined role patches on the list currently
correctly use has_privs_for_role rather than is_memver_of_role so this
brings the older roles to be consistent with the newer ones being
proposed.

On 10/27/21, 2:19 PM, "Robert Haas" <robertmhaas@gmail.com> wrote:

I am not sure I understand what the advantage of this is supposed to be.

There are a couple of other related threads with some additional
context:

/messages/by-id/20211026224731.115337-1-joshua.brindle@crunchydata.com
/messages/by-id/CAGB+Vh4enxvLBM_BJweWEO12Q0ySLMBWK9iOLaM7e=V1Y0YadA@mail.gmail.com

Nathan

Greetings,

* Joshua Brindle (joshua.brindle@crunchydata.com) wrote:

On Wed, Oct 27, 2021 at 5:20 PM Joshua Brindle <joshua.brindle@crunchydata.com> wrote:

On Wed, Oct 27, 2021 at 5:16 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle
<joshua.brindle@crunchydata.com> wrote:

Attached is an updated version of the patch to replace
is_member_of_role with has_privs_for_role for predefined roles. It
does not remove is_member_of_role from acl.h but it does add a warning
not to use that function for privilege checking.

Please consider this for the upcoming commitfest.

I am not sure I understand what the advantage of this is supposed to be.

Pre-defined roles currently do not operate the same way other roles do
with respect to inheritance. The patchfile has an explanation and
examples, I wasn't sure if that needed to be repeated in the email or
not.

And FWIW several predefined role patches on the list currently
correctly use has_privs_for_role rather than is_memver_of_role so this
brings the older roles to be consistent with the newer ones being
proposed.

Right, we really should be consistent here and we're not and that's not
a good thing. Further, if you're logged in as an unprivileged role and
expect to not see things that you shouldn't, then it's not good for that
to end up happening because you've been GRANT'd a more privileged, but
noinherit, role that was given some predefined roles. This doesn't end
up being an actual security issue as you could just SET ROLE to that
more privileged role, so it's not that you couldn't see that data, just
that you should have had to SET ROLE to do so.

Reviewing the actual patch, it generally looks good to me except that
you missed updating this comment:

Superusers or members of pg_read_all_stats members are allowed

Thanks,

Stephen

On Mon, Nov 8, 2021 at 3:44 PM Stephen Frost <sfrost@snowman.net> wrote:

Greetings,

* Joshua Brindle (joshua.brindle@crunchydata.com) wrote:

On Wed, Oct 27, 2021 at 5:20 PM Joshua Brindle <joshua.brindle@crunchydata.com> wrote:

On Wed, Oct 27, 2021 at 5:16 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle
<joshua.brindle@crunchydata.com> wrote:

Attached is an updated version of the patch to replace
is_member_of_role with has_privs_for_role for predefined roles. It
does not remove is_member_of_role from acl.h but it does add a warning
not to use that function for privilege checking.

Please consider this for the upcoming commitfest.

I am not sure I understand what the advantage of this is supposed to be.

Pre-defined roles currently do not operate the same way other roles do
with respect to inheritance. The patchfile has an explanation and
examples, I wasn't sure if that needed to be repeated in the email or
not.

And FWIW several predefined role patches on the list currently
correctly use has_privs_for_role rather than is_memver_of_role so this
brings the older roles to be consistent with the newer ones being
proposed.

Right, we really should be consistent here and we're not and that's not
a good thing. Further, if you're logged in as an unprivileged role and
expect to not see things that you shouldn't, then it's not good for that
to end up happening because you've been GRANT'd a more privileged, but
noinherit, role that was given some predefined roles. This doesn't end
up being an actual security issue as you could just SET ROLE to that
more privileged role, so it's not that you couldn't see that data, just
that you should have had to SET ROLE to do so.

Reviewing the actual patch, it generally looks good to me except that
you missed updating this comment:

Superusers or members of pg_read_all_stats members are allowed

Thanks for the review, attached is an update with that comment fixed
and also sgml documentation changes that I missed earlier.

On 11/8/21, 2:19 PM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:

Thanks for the review, attached is an update with that comment fixed
and also sgml documentation changes that I missed earlier.

I think there are a number of documentation changes that are still
missing. I did a quick scan and saw the "is member of" language in
func.sgml, monitoring.sgml, pgbuffercache.sgml, pgfreespacemap.sgml,
pgrowlocks.sgml, pgstatstatements.sgml, and pgvisibility.sgml.

<para>
 By default, the <structname>pg_shmem_allocations</structname> view can be
-   read only by superusers or members of the <literal>pg_read_all_stats</literal>
-   role.
+   read only by superusers or roles with privilges of the
+   <literal>pg_read_all_stats</literal> role.
</para>
</sect1>

nitpick: "privileges" is misspelled.

Nathan

On Wed, Nov 10, 2021 at 12:45 PM Bossart, Nathan <bossartn@amazon.com> wrote:

On 11/8/21, 2:19 PM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:

Thanks for the review, attached is an update with that comment fixed
and also sgml documentation changes that I missed earlier.

I think there are a number of documentation changes that are still
missing. I did a quick scan and saw the "is member of" language in
func.sgml, monitoring.sgml, pgbuffercache.sgml, pgfreespacemap.sgml,
pgrowlocks.sgml, pgstatstatements.sgml, and pgvisibility.sgml.

All of these and also adminpack.sgml updated. I think that is all of
them but docs broken across lines and irregular wording makes it
difficult.

<para>
By default, the <structname>pg_shmem_allocations</structname> view can be
-   read only by superusers or members of the <literal>pg_read_all_stats</literal>
-   role.
+   read only by superusers or roles with privilges of the
+   <literal>pg_read_all_stats</literal> role.
</para>
</sect1>

nitpick: "privileges" is misspelled.

Fixed, thanks for reviewing.

On 11/12/21, 12:34 PM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:

All of these and also adminpack.sgml updated. I think that is all of
them but docs broken across lines and irregular wording makes it
difficult.

LGTM. I've marked this as ready-for-committer.

Nathan

"Bossart, Nathan" <bossartn@amazon.com> writes:

On 11/12/21, 12:34 PM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:

All of these and also adminpack.sgml updated. I think that is all of
them but docs broken across lines and irregular wording makes it
difficult.

LGTM. I've marked this as ready-for-committer.

This needs another rebase --- it's trying to adjust
file_fdw/output/file_fdw.source, which no longer exists
(fix the regular file_fdw.out, instead).

regards, tom lane

On Tue, Jan 4, 2022 at 3:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Bossart, Nathan" <bossartn@amazon.com> writes:

On 11/12/21, 12:34 PM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:

All of these and also adminpack.sgml updated. I think that is all of
them but docs broken across lines and irregular wording makes it
difficult.

LGTM. I've marked this as ready-for-committer.

This needs another rebase --- it's trying to adjust
file_fdw/output/file_fdw.source, which no longer exists
(fix the regular file_fdw.out, instead).

regards, tom lane

Attached, thanks

On 1/4/22 16:51, Joshua Brindle wrote:

On Tue, Jan 4, 2022 at 3:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Bossart, Nathan" <bossartn@amazon.com> writes:

On 11/12/21, 12:34 PM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:

All of these and also adminpack.sgml updated. I think that is all of
them but docs broken across lines and irregular wording makes it
difficult.

LGTM. I've marked this as ready-for-committer.

This needs another rebase --- it's trying to adjust
file_fdw/output/file_fdw.source, which no longer exists
(fix the regular file_fdw.out, instead).

regards, tom lane

Attached, thanks

I'd like to pick this patch up and see it through to commit/push.
Presumably that will include back-patching to all supported pg versions.

Before I go through the effort to back-patch, does anyone want to argue
that this should *not* be back-patched?

Thanks,

Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Joe Conway <mail@joeconway.com> writes:

I'd like to pick this patch up and see it through to commit/push.
Presumably that will include back-patching to all supported pg versions.
Before I go through the effort to back-patch, does anyone want to argue
that this should *not* be back-patched?

Hm, I'm -0.5 or so. I think changing security-related behaviors
in a stable branch is a hard sell unless you are closing a security
hole. This is a fine improvement for HEAD but I'm inclined to
leave the back branches alone.

regards, tom lane

On Sun, Feb 6, 2022 at 12:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Joe Conway <mail@joeconway.com> writes:

I'd like to pick this patch up and see it through to commit/push.
Presumably that will include back-patching to all supported pg versions.
Before I go through the effort to back-patch, does anyone want to argue
that this should *not* be back-patched?

Hm, I'm -0.5 or so. I think changing security-related behaviors
in a stable branch is a hard sell unless you are closing a security
hole. This is a fine improvement for HEAD but I'm inclined to
leave the back branches alone.

I think the threshold to back-patch a clear behavior change is pretty
high, so I do not think it should be back-patched.

I am also not convinced that a sufficient argument has been made for
changing this in master. This isn't the only thread where NOINHERIT
has come up lately, and I have to admit that I'm not a fan. Let's
suppose that I have two users, let's say sunita and sri. In the real
world, Sunita is Sri's manager and needs to be able to perform actions
as Sri when Sri is out of the office, but it isn't desirable for
Sunita to have Sri's privileges in all situations all the time. So we
mark role sunita as NOINHERIT and grant sri to sunita. Then it turns
out that Sunita also needs to be COPY TO/FROM PROGRAM, so we give her
pg_execute_server_program. Now, if she can't exercise this privilege
without setting role to the prefined role, that's bad, isn't it? I
mean, we want her to be able to copy between *her* tables and various
shell commands, not the tables owned by pg_execute_server_program, of
which there are presumably none.

It seems to me that the INHERIT role flag isn't very well-considered.
Inheritance, or the lack of it, ought to be decided separately for
each inherited role. However, that would be a major architectural
change. But in the absence of that, it seems clearly better for
predefined roles to disregard INHERIT and just always grant the rights
they are intended to give. Because if we don't do that, then we end up
with people having to SET ROLE to the predefined role and perform
actions directly as that role, which seems like it can't be what we
want. I almost feel like we ought to be looking for ways of preventing
people from doing SET ROLE to a predefined role altogether, not
encouraging them to do it.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2/7/22 10:35, Robert Haas wrote:

On Sun, Feb 6, 2022 at 12:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Joe Conway <mail@joeconway.com> writes:

I'd like to pick this patch up and see it through to commit/push.
Presumably that will include back-patching to all supported pg versions.
Before I go through the effort to back-patch, does anyone want to argue
that this should *not* be back-patched?

Hm, I'm -0.5 or so. I think changing security-related behaviors
in a stable branch is a hard sell unless you are closing a security
hole. This is a fine improvement for HEAD but I'm inclined to
leave the back branches alone.

I think the threshold to back-patch a clear behavior change is pretty
high, so I do not think it should be back-patched.

ok

I am also not convinced that a sufficient argument has been made for
changing this in master. This isn't the only thread where NOINHERIT
has come up lately, and I have to admit that I'm not a fan. Let's
suppose that I have two users, let's say sunita and sri. In the real
world, Sunita is Sri's manager and needs to be able to perform actions
as Sri when Sri is out of the office, but it isn't desirable for
Sunita to have Sri's privileges in all situations all the time. So we
mark role sunita as NOINHERIT and grant sri to sunita. Then it turns
out that Sunita also needs to be COPY TO/FROM PROGRAM, so we give her
pg_execute_server_program. Now, if she can't exercise this privilege
without setting role to the prefined role, that's bad, isn't it? I
mean, we want her to be able to copy between *her* tables and various
shell commands, not the tables owned by pg_execute_server_program, of
which there are presumably none.

Easily worked around with one additional level of role:

8<---------------------------------------
nmx=# create user sunita;
CREATE ROLE
nmx=# create user sri superuser;
CREATE ROLE
nmx=# create user sri_alt noinherit;
CREATE ROLE
nmx=# grant sri to sri_alt;
GRANT ROLE
nmx=# grant sri_alt to sunita;
GRANT ROLE
nmx=# grant pg_execute_server_program to sunita;
GRANT ROLE
nmx=# set session authorization sri;
SET
nmx=# create table foo(id int);
CREATE TABLE
nmx=# insert into foo values(42);
INSERT 0 1
nmx=# set session authorization sunita;
SET
nmx=> select * from foo;
ERROR: permission denied for table foo
nmx=> set role sri;
SET
nmx=# select * from foo;
id
----
42
(1 row)

nmx=# reset role;
RESET
nmx=> select current_user;
current_user
--------------
sunita
(1 row)

nmx=> create temp table sfoo(f1 text);
CREATE TABLE
nmx=> copy sfoo(f1) from program 'id';
COPY 1
nmx=> select f1 from sfoo;
f1

----------------------------------------------------------------------------------------
uid=1001(postgres) gid=1001(postgres)
groups=1001(postgres),108(ssl-cert),1002(pgconf)
(1 row)
8<---------------------------------------

It seems to me that the INHERIT role flag isn't very well-considered.
Inheritance, or the lack of it, ought to be decided separately for
each inherited role. However, that would be a major architectural
change.

Agreed -- that would be useful.

But in the absence of that, it seems clearly better for predefined
roles to disregard INHERIT and just always grant the rights they are
intended to give. Because if we don't do that, then we end up with
people having to SET ROLE to the predefined role and perform actions
directly as that role, which seems like it can't be what we want. I
almost feel like we ought to be looking for ways of preventing people
from doing SET ROLE to a predefined role altogether, not encouraging
them to do it.

I disagree with this though.

It is confusing and IMHO dangerous that the predefined roles currently
work differently than regular roles eith respect to privilege inheritance.

In fact, I would extend that argument to the pseudo-role PUBLIC.

Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Mon, Feb 7, 2022 at 11:13 AM Joe Conway <mail@joeconway.com> wrote:

Easily worked around with one additional level of role:

Interesting.

But in the absence of that, it seems clearly better for predefined
roles to disregard INHERIT and just always grant the rights they are
intended to give. Because if we don't do that, then we end up with
people having to SET ROLE to the predefined role and perform actions
directly as that role, which seems like it can't be what we want. I
almost feel like we ought to be looking for ways of preventing people
from doing SET ROLE to a predefined role altogether, not encouraging
them to do it.

I disagree with this though.

It is confusing and IMHO dangerous that the predefined roles currently
work differently than regular roles eith respect to privilege inheritance.

I feel like that's kind of a conclusory statement, as opposed to
making an argument. I mean that this tells me something about how you
feel, but it doesn't really help me understand why you feel that way.

I suppose one argument in favor of your position is that if it
happened to be sri who was granted a predefined role, sunita would
inherit the rest of sr's privileges only with SET ROLE, but the
predefined role either way (IIUC, which I might not). If that's so,
then I guess I see the point, but I'm still sort of inclined to think
we're just trading one set of problems in for a different set. I just
have such a hard time imaging anyone using NOINHERIT in anger and
being happy with the result....

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Feb 7, 2022 at 12:09 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Feb 7, 2022 at 11:13 AM Joe Conway <mail@joeconway.com> wrote:

Easily worked around with one additional level of role:

Interesting.

But in the absence of that, it seems clearly better for predefined
roles to disregard INHERIT and just always grant the rights they are
intended to give. Because if we don't do that, then we end up with
people having to SET ROLE to the predefined role and perform actions
directly as that role, which seems like it can't be what we want. I
almost feel like we ought to be looking for ways of preventing people
from doing SET ROLE to a predefined role altogether, not encouraging
them to do it.

I disagree with this though.

It is confusing and IMHO dangerous that the predefined roles currently
work differently than regular roles eith respect to privilege inheritance.

I feel like that's kind of a conclusory statement, as opposed to
making an argument. I mean that this tells me something about how you
feel, but it doesn't really help me understand why you feel that way.

I suppose one argument in favor of your position is that if it
happened to be sri who was granted a predefined role, sunita would
inherit the rest of sr's privileges only with SET ROLE, but the
predefined role either way (IIUC, which I might not). If that's so,
then I guess I see the point, but I'm still sort of inclined to think
we're just trading one set of problems in for a different set. I just
have such a hard time imaging anyone using NOINHERIT in anger and
being happy with the result....

IMO this is inarguably a plain bug. The inheritance system works one
way for pre-defined roles and another way for other roles - and the
difference isn't even documented.

The question is whether there is a security issue warranting back
patching, which is a bit of a tossup I think. According to git history
it's always worked this way, and the possible breakage of pre-existing
clusters seems maybe not worth it.

On 2/7/22 12:09, Robert Haas wrote:

On Mon, Feb 7, 2022 at 11:13 AM Joe Conway <mail@joeconway.com> wrote:

It is confusing and IMHO dangerous that the predefined roles currently
work differently than regular roles eith respect to privilege inheritance.

I feel like that's kind of a conclusory statement, as opposed to
making an argument. I mean that this tells me something about how you
feel, but it doesn't really help me understand why you feel that way.

The argument is that we call these things "predefined roles", but they
do not behave the same way normal "roles" behave.

Someone not intimately familiar with that fact could easily make bad
assumptions, and therefore wind up with misconfigured security settings.
In other words, it violates the principle of least astonishment (POLA).

As Joshua said nearby, it simply jumps out at me as a bug.

-------
After more thought, perhaps the real problem is that these things should
not have been called "predefined roles" at all. I know, the horse has
already left the barn on that, but in any case...

They are (to me at least) similar in concept to Linux capabilities in
that they allow roles other than superusers to do a certain subset of
the things historically reserved for superusers through a special
mechanism (hardcoded) rather than through the normal privilege system
(GRANTS/ACLs).

As an example, the predefined role pg_read_all_settings allows a
non-superuser to read GUC normally reserved for superuser access only.

If I create a new user "bob" with the default INHERIT attribute, and I
grant postgres to bob, bob must SET ROLE to postgres in order to access
the capability to read superuser settings.

This is similar to bob's access to the default superuser privilege to
read data in someone else's table (must SET ROLE to access that capability).

But it is different from bob's access to inherited privileges which are
GRANTed:
8<--------------------------
psql nmx
psql (15devel)
Type "help" for help.

nmx=# create user bob;
CREATE ROLE

nmx=# grant postgres to bob;
GRANT ROLE

nmx=# \q
8<--------------------------
-and-
8<--------------------------
psql -U bob nmx
psql (15devel)
Type "help" for help.

nmx=> select current_user;
current_user
--------------
bob
(1 row)

nmx=> show stats_temp_directory;
ERROR: must be superuser or have privileges of pg_read_all_settings to
examine "stats_temp_directory"
nmx=> set role postgres;
SET
nmx=# show stats_temp_directory;
stats_temp_directory
----------------------
pg_stat_tmp
(1 row)

nmx=# select current_user;
current_user
--------------
postgres
(1 row)

nmx=# select * from foo;
id
----
42
(1 row)

nmx=# reset role;
RESET
nmx=> select current_user;
current_user
--------------
bob
(1 row)

nmx=> select * from foo;
ERROR: permission denied for table foo
nmx=> set role postgres;
SET
nmx=# grant select on table foo to postgres;
GRANT
nmx=# reset role;
RESET
nmx=> select current_user;
current_user
--------------
bob
(1 row)

nmx=> select * from foo;
id
----
42
(1 row)
8<--------------------------

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Tue, Feb 8, 2022 at 6:59 AM Joe Conway <mail@joeconway.com> wrote:

This is similar to bob's access to the default superuser privilege to
read data in someone else's table (must SET ROLE to access that capability).

But it is different from bob's access to inherited privileges which are
GRANTed:

Yeah. I think right here you've put your finger on what's been bugging
me about this: it's similar to one thing, and it's different from
another. To you and Joshua and Stephen, it seems 100% obvious that
these roles should work like grants of other roles. But I think of
them as capabilities derived from the superuser account, and so I'm
sort of tempted to think that they should work the way the superuser
bit does. And that's why I don't think the fact that they work the
other way is "just a bug" -- it's one of two possible ways that
someone could think that it ought to work based on how other things in
the system actually do work.

I'm not hard stuck on the idea that the current behavior is right, but
I don't think that we can really say that we've made things fully
consistent unless we make things like SUPERUSER and BYPASSRLS work the
same way that you want to make predefined roles work. And probably do
something about the INHERIT flag too because the current situation
seems like a hot mess.

--
Robert Haas
EDB: http://www.enterprisedb.com