Обсуждение: Re: pgsql: Add new GUC createrole_self_grant.

On Tue, Jan 10, 2023 at 8:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> [ squint ... ]  Are you sure it's not a security *hazard*, though?

I think you have to squint pretty hard to find a security hazard here.
The effect of this GUC is to control the set of privileges that a
CREATEROLE user automatically grants to themselves. But granting
yourself privileges does not normally lead to any sort of security
privilege. It's not completely impossible, I believe. For example,
suppose that I, as a CREATEROLE user who is not a superuser, execute
"CREATE ROLE shifty". I then set up a schema that shifty can access
and I cannot. I then put that schema into my search_path despite the
fact that I haven't given myself access to it. Now, depending on the
value of this setting, I might either implicitly inherit shifty's
privileges, or I might not. So, if I was expecting that I wouldn't,
and I do, then I have now created a situation where if I do more dumb
things I could execute some shifty code that lets that shifty user
take over my account.

But, you know, if I'm that dumb, I could also hit myself in the head
with a hammer and the shifty guy could use the fact that I'm
unconscious to fish the sticky note out of my wallet where,
presumably, I keep my database password.

The bigger point here, I think, is that this GUC only controls default
privileges -- and we already have a system for default privileges that
allows any user to give away privileges on virtually any object that
they create to anyone. Nothing about that system is superuser-only.
This system is far more restricted in its scope. It only allows you to
give privileges to yourself, not anyone else, and only if you're a
CREATEROLE user who is not a SUPERUSER. It seems a bit crazy to say
that it's not a hazard for Alice to automatically grant every
permission in the book to Emil every time she creates a table or
schema or type or sequence or a function, but it is a hazard if Bob
can grant INHERIT and SET to himself on roles that he creates.

That said, in my original design, this was controlled via a different
mechanism which was superuser-only. I was informed that made no sense,
so I changed it. Now here we are.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jan 10, 2023 at 8:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> [ squint ... ]  Are you sure it's not a security *hazard*, though?

> I think you have to squint pretty hard to find a security hazard here.

Maybe, but I'd be sad if somebody manages to find one after this is
out in the wild.

> That said, in my original design, this was controlled via a different
> mechanism which was superuser-only. I was informed that made no sense,
> so I changed it. Now here we are.

Yeah.  I concur that a SUSET GUC isn't much fun for a non-superuser
CREATEROLE holder who might wish to adjust the default behavior they get.
I also concur that it seems a bit far-fetched that a CREATEROLE holder
might create a SECURITY DEFINER function that would do something that
would be affected by this setting.  Still, we have no field experience
with how these mechanisms will actually be used, so I'm worried.

The scenario I'm worried about could be closed, mostly, if we were willing
to invent an intermediate GUC privilege level "can be set interactively
but only by CREATEROLE holders" ("PGC_CRSET"?).  But that's an awful lot
of infrastructure to add for one GUC.  Are there any other GUCs where
that'd be a more useful choice than any we have now?

            regards, tom lane

On Tue, Jan 10, 2023 at 9:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah.  I concur that a SUSET GUC isn't much fun for a non-superuser
> CREATEROLE holder who might wish to adjust the default behavior they get.
> I also concur that it seems a bit far-fetched that a CREATEROLE holder
> might create a SECURITY DEFINER function that would do something that
> would be affected by this setting.  Still, we have no field experience
> with how these mechanisms will actually be used, so I'm worried.

All right. I'm not that worried because I think any problems that crop
up probably won't be that bad, primarily due to the extremely
restricted set of circumstances in which the GUC operates -- but
that's a judgement call, and reasonable people can think differently.

> The scenario I'm worried about could be closed, mostly, if we were willing
> to invent an intermediate GUC privilege level "can be set interactively
> but only by CREATEROLE holders" ("PGC_CRSET"?).  But that's an awful lot
> of infrastructure to add for one GUC.  Are there any other GUCs where
> that'd be a more useful choice than any we have now?

I don't quite understand what that would do. If a non-CREATEROLE user
sets the GUC, absolutely nothing happens, because the code that is
controlled by the GUC cannot be reached without CREATEROLE privileges.

Of course, if it's possible for a non-CREATEROLE user to set the value
that a CREATEROLE user experiences, that'd be more of a problem --
though still insufficient to create a security vulnerability in and of
itself -- but if user A can change the GUC settings that user B
experiences, why screw around with this when you could just set
search_path?

To answer your question directly, though, I don't know of any other
setting where that would be a useful level. Up until this morning,
CREATEROLE was not usable for any serious purpose because we've been
shipping something that was broken by design for years, so it's
probably fortunate that not much depends on it.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jan 10, 2023 at 9:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The scenario I'm worried about could be closed, mostly, if we were willing
>> to invent an intermediate GUC privilege level "can be set interactively
>> but only by CREATEROLE holders" ("PGC_CRSET"?).

> Of course, if it's possible for a non-CREATEROLE user to set the value
> that a CREATEROLE user experiences, that'd be more of a problem --

That's exactly the case I'm worried about, and it's completely reachable
if a CREATEROLE user makes a SECURITY DEFINER function that executes
an affected GRANT and is callable by an unprivileged user.  Now, that
probably isn't terribly likely, and it's unclear that there'd be any
serious security consequence even if the GRANT did do something
different than the author of the SECURITY DEFINER function was expecting.
Nonetheless, I'm feeling itchy about this chain of assumptions.

> To answer your question directly, though, I don't know of any other
> setting where that would be a useful level.

Yeah, I didn't think of one either.  Also, even if we invented PGC_CRSET,
it can't stop one CREATEROLE user from attacking another one, assuming
that there is some interesting attack that can be constructed here.
I think the whole point of your recent patches is to not assume that
CREATEROLE users are mutually trusting, so that's bad.

Bottom line is that a GUC doesn't feel like the right mechanism to use.
What do you think about inventing a role property, or a grantable role
that controls this behavior?

            regards, tom lane

On Tue, Jan 10, 2023 at 10:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Of course, if it's possible for a non-CREATEROLE user to set the value
> > that a CREATEROLE user experiences, that'd be more of a problem --
>
> That's exactly the case I'm worried about, and it's completely reachable
> if a CREATEROLE user makes a SECURITY DEFINER function that executes
> an affected GRANT and is callable by an unprivileged user.  Now, that
> probably isn't terribly likely, and it's unclear that there'd be any
> serious security consequence even if the GRANT did do something
> different than the author of the SECURITY DEFINER function was expecting.
> Nonetheless, I'm feeling itchy about this chain of assumptions.

If you want to make safe a SECURITY DEFINER function written using sql
or plpgsql, you either have to schema-qualify every single reference
or, more realistically, attach a SET clause to the function to set the
search_path to a sane value during the time that the function is
executing. The problem here can be handled the same way, except that
it's needed in a vastly more limited set of circumstances: you have to
be calling a SECURITY DEFINER function that will execute CREATE ROLE
as a non-superuser (and that user then needs to be sensitive to the
value of this GUC in some security-relevant way). It might be good to
document this -- I just noticed that the CREATE FUNCTION page has a
section on "Writing SECURITY DEFINER Functions Safely" which talks
about dealing with the search_path issues, and it seems like it would
be worth adding a sentence or two there to talk about this.

It might also be a good idea to make the section of the page that
explains the meaning of the SECURITY INVOKER and SECURITY DEFINER
clauses cross-link to the section on writing such functions safely,
because that section is way down at the bottom of the page and seems
easy to miss.

But I'm not convinced that we need more than documentation changes here.

> Bottom line is that a GUC doesn't feel like the right mechanism to use.
> What do you think about inventing a role property, or a grantable role
> that controls this behavior?

Well, I originally had a pair of role properties called
INHERITCREATEDROLES and SETCREATEDROLES, but nobody liked that,
including you. One reason was probably that the names are long and
ugly, but this is also unlike existing role properties, which
typically can only be changed by a superuser, or by a CREATEROLE user
with ADMIN OPTION on the target role. Since you don't have admin
option on yourself, that would mean you couldn't change this property
for yourself, which I wasn't too exercised about, but everyone else
who commented disliked it. We could go back to having those properties
and have the rules for changing them be different than the roles for
changing other role properties, I suppose.

But I don't think that's better. Some of the existing role properties
(SUPERUSER, REPLICATION, BYPASSRLS, CREATEROLE, CREATEDB) are
capabilities, and others (LOGIN, CONNECTION LIMIT, VALID UNTIL) are
limits established by the system administrator. This is neither: it's
a default. So if we put it into the role property system, then we're
saying this one particular default is a role property, whereas pretty
much all of the others are GUCs. Now, admittedly, I did have it as a
role property originally, but that was before we decided that it made
sense to give the CREATEROLE user, rather than the superuser, control
over the value of the property. And I think we decided that for good
reason, because the CREATEROLE user can always turn "no" into "yes" by
granting the created role back to themselves with additional options.
They can't necessarily turn "yes" into "no," because we could create
the implicit grant with one or both of those options turned on and the
CREATEROLE user can't undo that. But nobody thought that was a useful
thing to enforce, and neither do I. Given that shift in thinking, I
have a hard time believing that it's a good idea to shove this option
into a system that is meant for capabilities or limits and has no
existing precedent for anything that behaves like a setting or user
preference (except perhaps ALTER USER ... PASSWORD '...' but calling
that a preference might be a stretch).

If we used grantable roles, I suppose the design there would to invent
two new predefined role and grant them to the CREATEROLE user, or not,
to affect whether and how subsequent implicit self-grants by that user
would be performed. But that again takes the decision out of the hands
of the CREATEROLE user, and it again puts a user preference into a
system that we currently use only for capabilities.

Stepping back a second, I think that your underlying concern here is
that the entire GUC system is shaky from a security perspective.
Trying to write SECURITY DEFINER functions or procedures in sql or
plpgsql is not unlike trying to write a safe setuid shell script.
Among the problems with trying to do that is that the caller might
establish surprising values for PATH, IFS, or other environment
variables before calling your script, and if you're not careful,
you'll end up doing whatever the caller wants instead of what you
intended to be doing. I think it's really justifiable to be worried
about the same kinds of problems within PostgreSQL, but I don't think
that the right solution is to have new settings opt out of the GUC
mechanism on a retail basis. We need a solution that's going to work
for every GUC we have, in a consistent and understandable way, and if
we can't have that then we at least need a solution for search_path.
If we come up with such a solution, it seems likely that also adopting
that solution for createrole_self_grant would be a good idea. But if
we don't, I have trouble believing that doing something only for
createrole_self_grant is really going to improve security.

In fact, it might make it harder to fix the real problems in this
area. If we have all of our settings in one system, then any solution
we devise can apply to all of them equally. If we start storing some
of them in other places, it's potentially more separate things that
have to be fixed. I don't want to make overly strong statements here
because I don't think we really know what any of the fixes to these
problems are. If we can find some place to put this where it fits
nicely and that place isn't the GUC system, well and good: I don't
mind writing a patch to do it. But what I don't want to do is do
contortions to avoid relying on GUCs because we don't trust the GUC
mechanism in general. I don't think that kind of thing will end well.

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

Robert Haas <robertmhaas@gmail.com> writes:
> If you want to make safe a SECURITY DEFINER function written using sql
> or plpgsql, you either have to schema-qualify every single reference
> or, more realistically, attach a SET clause to the function to set the
> search_path to a sane value during the time that the function is
> executing. The problem here can be handled the same way, except that
> it's needed in a vastly more limited set of circumstances: you have to
> be calling a SECURITY DEFINER function that will execute CREATE ROLE
> as a non-superuser (and that user then needs to be sensitive to the
> value of this GUC in some security-relevant way). It might be good to
> document this -- I just noticed that the CREATE FUNCTION page has a
> section on "Writing SECURITY DEFINER Functions Safely" which talks
> about dealing with the search_path issues, and it seems like it would
> be worth adding a sentence or two there to talk about this.

OK, I'd be satisfied with that.

            regards, tom lane

On Wed, Jan 11, 2023 at 4:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > If you want to make safe a SECURITY DEFINER function written using sql
> > or plpgsql, you either have to schema-qualify every single reference
> > or, more realistically, attach a SET clause to the function to set the
> > search_path to a sane value during the time that the function is
> > executing. The problem here can be handled the same way, except that
> > it's needed in a vastly more limited set of circumstances: you have to
> > be calling a SECURITY DEFINER function that will execute CREATE ROLE
> > as a non-superuser (and that user then needs to be sensitive to the
> > value of this GUC in some security-relevant way). It might be good to
> > document this -- I just noticed that the CREATE FUNCTION page has a
> > section on "Writing SECURITY DEFINER Functions Safely" which talks
> > about dealing with the search_path issues, and it seems like it would
> > be worth adding a sentence or two there to talk about this.
>
> OK, I'd be satisfied with that.

OK, I'll draft a patch tomorrow.

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

On Wed, Jan 11, 2023 at 7:53 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
> Justed wanted to chime in and say Robert has eloquently put into words much of what I have been thinking here, and
thatI concur that guiding the DBA to use care with the power they have been provided is a sane position to take.
 
>
> +1, and thank you.

Thanks!

Here's a patch. In it I make three changes, only one of which is
directly relevant to the topic at hand:

1. Add a sentence to the documentation on writing SECURITY FUNCTIONS
safely concerning createrole_self_grant.
2. Add a sentence to the documentation on SECURITY DEFINER referring
to the section about writing such functions safely.
3. Remove a note discussing the fact that pre-8.3 versions did not
have SET clauses for functions.

I can separate this into multiple patches if desired. And of course
you, Tom, or others may have suggestions on which of these changes
should be included at all or how to word them better.

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

Hi,

On 2023-01-12 18:15:50 -0700, David G. Johnston wrote:
> On Thu, Jan 12, 2023 at 8:11 AM Robert Haas <robertmhaas@gmail.com> wrote:
> 
> > On Wed, Jan 11, 2023 at 7:53 PM David G. Johnston
> > <david.g.johnston@gmail.com> wrote:
> > > Justed wanted to chime in and say Robert has eloquently put into words
> > much of what I have been thinking here, and that I concur that guiding the
> > DBA to use care with the power they have been provided is a sane position
> > to take.
> > >
> > > +1, and thank you.
> >
> > Thanks!
> >
> > Here's a patch. In it I make three changes, only one of which is
> > directly relevant to the topic at hand:
> >
> > 1. Add a sentence to the documentation on writing SECURITY FUNCTIONS
> > safely concerning createrole_self_grant.
> > 2. Add a sentence to the documentation on SECURITY DEFINER referring
> > to the section about writing such functions safely.
> > 3. Remove a note discussing the fact that pre-8.3 versions did not
> > have SET clauses for functions.
> >
> > I can separate this into multiple patches if desired. And of course
> > you, Tom, or others may have suggestions on which of these changes
> > should be included at all or how to word them better.
> >
> >
> I'm still not really feeling how security definer is the problematic option
> here.  Security invoker scares me much more.

I don't really see what that has to do with the topic at hand, unless you want
to suggest removing the entire section about how to write secure security
definer functions?


> postgres=# create role unpriv login;
> CREATE ROLE
> postgres=# create role creator createrole login;
> CREATE ROLE
> postgres=# grant pg_read_all_data to creator with admin option;
> postgres=#
> create procedure makeunprivpowerful() LANGUAGE sql AS $$
> grant pg_read_all_data to unpriv;
> $$;
> CREATE PROCEDURE
> postgres=# alter procedure makeunprivpowerful() owner to unpriv;
> ALTER PROCEDURE
> 
> postgres=# \c postgres unpriv
> You are now connected to database "postgres" as user "unpriv".
> postgres=> call makeunprivpowerful();
> ERROR:  must have admin option on role "pg_read_all_data"
> CONTEXT:  SQL function "makeunprivpowerful" statement 1
> 
> postgres=# \c postgres creator
> You are now connected to database "postgres" as user "creator".
> postgres=> call makeunprivpowerful();
> CALL
> 
> Personally I think the best advice for a CREATEROLE granted user is to
> never call routines they themselves don't own; or at least only those have
> reviewed thoroughly and understand the consequences of.  Regardless of
> security definer/invoker.
> 
> In short, a low-privilege user can basically run anything without much fear
> because they can't do harm anyway.  Thus security invoker applies to them,
> and having security definer gives them the ability to do some things
> without needing to have permanent higher privileges.  These are useful,
> security related attributes with respect to them.
> 
> For a high-privilege I would argue that neither security invoker nor
> definer are relevant considerations from a security point-of-view.  If you
> have high-privilege you must know what it is you are executing before you
> execute it and anything bad it causes you to do using your privileges, or
> higher if you run a security definer function blindly, is an administrative
> failure, not a security hole.

The point of the security definer section is to explain how to safely write
security definer functions that you grant to less privileged users. It's not
about whether it's safe to call a security invoker / definer function -
indeed, if you don't trust the function author / owner, it's never safe to
call the function.


Greetings,

Andres Freund

On Fri, Jan 13, 2023 at 8:29 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>> The point of the security definer section is to explain how to safely write
>> security definer functions that you grant to less privileged users
>
> Yeah, we are really good at "how".
>
> +    If the security definer function intends to create roles, and if it
> +    is running as a non-superuser, <varname>createrole_self_grant</varname>
> +    should also be set to a known value using the <literal>SET</literal>
> +    clause.
>
> I'd like to know "why".  Without knowing why we are adding this I can't give it a +1.  I want the patch to include
thewhy.
 

What don't you understand about the "why"? If your security-definer
function relies on some GUC having some particular value for some
security-critical purpose, and the caller can substitute some other
value, they might be able to create a security compromise. Since this
GUC has some connection to security, there is at least some distant
possibility of that happening. Reasonable people can, perhaps, differ
about how likely that is, but I don't really see what's confusing
about the theory. As a general statement about the human condition, if
you know that someone may be untrustworthy, you should be careful
about letting them influence your decisions. If you aren't, something
bad may happen to you.

The whole thing about SECURITY INVOKER functions is really a separate
issue. You can tell people how to write SECURITY DEFINER functions
more safely, and we do. You cannot tell them how to write SECURITY
INVOKER functions more safely, because the direction of the attack is
reversed. In the case of SECURITY DEFINER, the caller of the function
can attack the owner of the function. In the case of a SECURITY
INVOKER function, the owner of the function can attack the caller of
the function. We can't document how to write security invoker
functions safely because the author of the function is the one
potentially making an attack, and therefore would do the opposite of
whatever advice we gave. We *could* add whole new sections to the
documentation telling people to be careful about calling security
invoker functions, and that's a fine thing to discuss, but what I'm
doing here is following up an already-committed patch by adjusting
parts of the existing documentation to account for the changes.
Inventing whole new sections of the documentation would be a job for a
new patch on a new thread, not a follow-up patch on an existing
thread.

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

On Sat, Jan 14, 2023 at 8:12 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
> OK, given all of that, I suggest reworking the first paragraph of security definer functions safely to something like
thefollowing: 
>
> (Replace: Because a SECURITY DEFINER function is executed with the privileges of the user that owns it, care is
neededto ensure that the function cannot be misused. For security, search_path should be set to exclude any schemas
writableby untrusted users.) with: 
>
> The execution of a SECURITY DEFINER function has two interacting behaviors that make writing and administering such
functionsrequire extra care.  While the privileges that come into play during execution are those of the function
owner,the execution environment is inherited from the calling context.  Therefore, any settings that the function
reliesupon must be specified in the SET clause of the CREATE command (or within the body of the function). 
>
> Of particular importance is the search_path setting.  The search_path should be set to the bare minimum required for
thefunction to operate and, more importantly, not include any schemas writable by untrusted users. 
>
> <existing wording>
> This prevents malicious users [...]
> (existing example)
> [...] the function could be subverted by creating a temporary table named pwds.
> </existing wording>

I find this wording less clear than what we have now. And I reiterate
that the purpose of the patch under discussion is to add a mention of
the new GUC to an existing section, not to rewrite that section -- or
any other section -- of the documentation.

> <added note="specifically by this patch">
> Another setting of note (at least in the case that the function owner is not a superuser) is createrole_self_grant.
Whilethe function owner has their own pg_db_role_setting preference for this setting, when wrapping execution of CREATE
ROLEwithin a function, particularly to be executed by others, it is the executor's setting that would be in effect, not
theowner's. 
> </added>

I think these sentences are really contorted, and they are also
factually incorrect. For this setting to matter, you need (1) the
function to be running CREATEROLE and (2) the owner of that function
to not be a superuser. You've put those facts at opposite end of the
sentence. You've also brought pg_db_role_setting into it, which
doesn't matter here because it's applied at login, and a security
definer function doesn't log in as the user to which it switches.
There really is no such thing as "the owner's" setting. There may be a
setting which is applied to the owner's session if the owner logs in,
but there's no default value for all code run as the owner -- perhaps
there should be, but that's not how it works. I don't think we have
much precedent for using the word "executor" to mean "the user who
called a function" as opposed to "the code that executed a planned
query".

I don't really think there's too much wrong with what I wrote in the
patch as proposed, and I would like to get it committed and move on
without getting drawn into a wide-ranging discussion of every way in
which we might be able to improve the surrounding structure.

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

I don't really think there's too much wrong with what I wrote in the
patch as proposed, and I would like to get it committed and move on
without getting drawn into a wide-ranging discussion of every way in
which we might be able to improve the surrounding structure.

On Mon, Jan 16, 2023 at 10:33 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:
> I’m moving on as well.  Go with what you have.  I have my personal understanding clarified at this point.  If the
docsneed more work people will ask questions to help guide such work. 

Yeah, I hope so.

It's becoming increasingly clear to me that we haven't put enough
effort into clarifying what I will broadly call "trust issues" in the
documentation. It's bad if you call untrusted code that runs as you,
and it's bad if code that runs as you gets called by untrusted people
for whose antics you are not sufficiently prepared, and there are a
lot of ways those things things can happen: direction function calls,
operators, triggers, row-level security, views, index or materialized
view rebuilds, etc. I think it would be good to have a general
treatment of those issues in the documentation written by a
security-conscious hacker or hackers who are really familiar both with
the behavior of the system and also able to make the security
consequences understandable to people who are not so deeply invested
in PostgreSQL. I don't want to do that on this thread, but to the
extent that you're arguing that the current treatment is inadequate,
I'm fully in agreement with that.

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