Re: fixing CREATEROLE

On Wed, Nov 23, 2022 at 1:11 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I haven't thought about these issues hard enough to form an overall
> opinion (though I agree that making CREATEROLE less tantamount
> to superuser would be an improvement). However, I share Mark's
> discomfort about making these commands act differently depending on
> context. We learned long ago that allowing GUCs to affect query
> semantics was a bad idea. Basing query semantics on properties
> of the issuing role (beyond success-or-permissions-failure) doesn't
> seem a whole lot different from that. It still means that
> applications can't just issue command X and expect Y to happen;
> they have to inquire about context in order to find out that Z might
> happen instead. That's bad in any case, but it seems especially bad
> for security-critical behaviors.

I'm not sure that this behavior qualifies as security-critical. If
INHERITCREATEDROLES and SETCREATEDROLES are both true, then the grant
has INHERIT TRUE and SET TRUE and there are no more rights to be
gained. If not, the createrole user can do something like GRANT
new_role TO my_own_account WITH INHERIT TRUE, SET TRUE. Even if we
somehow disallowed that, they could gain access to the privilege of
the created role in a bunch of other ways, such as granting the rights
to someone else, or just changing the password and using the new
password to log into the account.

When I started working in this area, I thought non-inherited grants
were pretty useless, because you can so easily work around it.
However, other people did not agree. From what I can gather, I think
the reason why people like non-inherited grants is that they prevent
mistakes. A user who has ADMIN OPTION on another role but does not
inherit its privileges can break into that account and do whatever
they want, but they cannot ACCIDENTALLY perform an operation that
makes use of that user's privileges. They will have to SET ROLE, or
GRANT themselves something, and those actions can be logged and
audited if desired. Because of the potential for that sort of logging
and auditing, you can certainly make an argument that this is a
security-critical behavior, but it's not that clear cut, because it's
making assumptions about the behavior of other software, and of human
beings. Strictly speaking, looking just at PostgreSQL, these options
don't affect security.

On the more general question of configurability, I tend to agree that
it's not great to have the behavior of commands depend too much on
context, especially for security-critical things. A particularly toxic
example IMHO is search_path, which I think is an absolute disaster in
terms of security that I believe we will never be able to fully fix.
Yet there are plenty of examples of configurability that no one finds
problematic. No one agitates against the idea that a database can have
a default tablespace, or that you can ALTER USER or ALTER DATABASE to
configure an setting on a user-specific or database-specific setting,
even a security-critical one like search_path, or one that affects
query behavior like work_mem. No one is outraged that a data type has
a default btree operator class that is used for indexes unless you
specify another one explicitly. What people mostly complain about IME
is stuff like standard_conforming_strings, or bytea_output, or
datestyle. Often, when proposal come up on pgsql-hackers and get shot
down on these grounds, the issue is that they would essentially make
it impossible to write SQL that will run portably on PostgreSQL.
Instead, you'd need to write your application to issue different SQL
depending on the value of settings on the local system. That's un-fun
at best, and impossible at worst, as in the case of extension scripts,
whose content has to be static when you ship the thing.

But it's not exactly clear to me what the broader organizing principle
is here, or ought to be. I think it would be ridiculous to propose --
and I assume that you are not proposing -- that no command should have
behavior that in any way depends on what SQL commands have been
executed previously. Taken to a silly extreme, that would imply that
CREATE TABLE ought to be removed, because the behavior of SELECT *
FROM something will otherwise depend on whether someone has previously
issued CREATE TABLE something. Obviously that is a stupid argument.
But on the other hand, it would also be ridiculous to propose the
reverse, that it's fine to add arbitrary settings that affect the
behavior of any command whatsoever in arbitrary ways. Simon's proposal
to add a GUC that would make vacuum request a background vacuum rather
than performing one in the foreground is a good example of a proposal
that did not sit well with either of us.

But I don't know on what basis exactly we put a proposal like this in
one category rather than the other. I'm not sure I can really
articulate the general principle in a sensible way. For me, this
clearly falls into the "good" category: it's configuration that you
put into the database that makes things happen the way you want, not a
behavior-changing setting that comes along and ruins somebody's day.
But if someone else feels otherwise, I'm not sure I can defend that
view in a really rigorous way, because I'm not really sure what the
litmus test is, or should be. I think the best that I can do is to say
that if we *don't* add those options but *do* adopt the rest of the
patch set, we will have to make a decision about what behavior
everyone is going to get, and no matter what we decide, some people
are not going to be really unhappy with the result. I would like to
find a way to avoid that.

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