Re: running logical replication as the subscription owner

On Fri, Mar 24, 2023 at 8:02 PM Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> Without a reasonable example, we should probably be on some kind of
> path to disallowing crazy stuff in triggers that poses only risks and
> no benefits. Not the job of this patch, but perhaps it can be seen as a
> step in that direction?

Possibly, but it's a little harder to say what's crazy in a trigger
than in some other contexts. I feel like it would be fine to say that
your index expression should probably not be doing "ALTER USER
somebody SUPERUSER" really ever, but it's not quite so clear in case
of a trigger. And the stuff that's prevented by
SECURITY_RESTRICTED_OPERATION isn't that sort of thing, but has rather
to do with stuff that messes with the session state, maybe leaving
booby-traps behind for the next user. For instance, if user A runs
some code as user B and user B closes a cursor opened by A and opens a
new one with the same name, that has a rather good chance of making A
do something they didn't intend to do. SECURITY_RESTRICTED_OPERATION
is aimed at preventing that sort of attack.

> > I am not thrilled with this either, but if you can arrange to run
> > code
> > as a certain user without the ability to SET ROLE to that user then
> > there is, by definition, a potential privilege escalation.
>
> I don't think that's "by definition".
>
> The code is being executed with the privileges of the person who wrote
> it. I don't see anything inherently insecure about that. There could be
> incidental or practical risks, but it's a pretty reasonable thing to do
> at a high level.

Not really. My home directory on my laptop is full of Perl and sh
scripts that I wouldn't want someone else to execute as me. They don't
have any defenses against malicious use because I don't expect anyone
else has access to run them, especially as me. If someone got access
to run them as me, they'd compromise my laptop account in no time at
all. I don't see any reason to believe the situation is any different
inside of a database. People have no reason to harden code unless
someone else is going to have access to run it.

> > I don't
> > think we can just dismiss that as a non-issue. If the ability of one
> > user to potentially become some other user weren't a problem, we
> > wouldn't need any patch at all. Imagine for example that the table
> > owner has a trigger which doesn't sanitize search_path. The
> > subscription owner can potentially leverage that to get the table
> > owner's privileges.
>
> Can you explain? Couldn't we control the subscription process's search
> path?

There's no place in the code right now where when we switch user
identities we also change the search_path. There is nothing to prevent
us from writing such code, but we have no place from which to obtain a
search_path that will cause the called code to behave properly. We
don't have access to the search_path that would prevail at the time
the target user logged in, and even if we did, we don't know that that
search_path is secure. We do know that an empty search_path is secure,
but it's probably also going to cause any code we run to fail, unless
that code schema-qualifies all references outside of pg_catalog, or
unless it sets search_path itself. search_path also isn't necessarily
the only problem. As a hypothetical example, suppose I create a table
with one text column, revoke all public access to that table, and then
create an on-insert trigger that executes as an SQL command any text
value inserted into the table. This is perfectly secure as long as I'm
the only one who can access the table, but if someone else gets access
to insert things into that table using my credentials then they can
easily break into my account. Real examples aren't necessarily that
dramatically bad, but that doesn't mean they don't exist.

> The benefit of delegating to a non-superuser is to contain the risk if
> that account is compromised. Allowing SET ROLE on tons of accounts
> diminishes that benefit, a lot.

Well, I continue to feel that if you can compromise the subscription
owner's account, we haven't really fixed anything yet. I mean, it used
to be that autovacuum could compromise the superuser's account, and we
fixed that. If we find more ways for that same thing to happen, we
would presumably fix those too. We would never accept a situation
where autovacuum can compromise the superuser's account. And we
shouldn't accept a situation where either the table owner can
compromise the subscription owner's account, either. And similarly
nobody ever proposed that that issue should be fixed by running the
autovacuum worker process as some kind of low-privileged user that we
created specially for that purpose. We just ... fixed it so that no
compromise was possible. And I think that's also the right approach
here.

I do agree with you that allowing SET ROLE on tons of accounts is not
great, though. I don't really think it matters very much today,
because basically all subscriptions today are owned by superusers and
can do everything anyway. But if you imagine that a lot of
subscriptions are going to be owned by less-privileged users, then
it's a lot less nice. I think it has the strength of at least being
honest about what the problem is. It makes it clear right on the tin
that the subscription owner is going to be able to get into the table
owner's accounts, in a way that people shouldn't really miss noticing.
And if they notice it, then they're at least aware of it, which is
something. It would be better still if we could prevent the
subscription owner from hacking into the table owner's account. Then,
we could very sensibly remove the SET ROLE requirement and check
something weaker instead, and that would be fantastic.

Since I didn't see how to engineer that, I did this.

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