Subject: running logical replication as the subscription owner

From: Robert Haas <[email-masked]>
Date: Fri, 3 Mar 2023 11:02:30 -0500

Hi,

Here's a patch against master for $SUBJECT. It lacks documentation
changes and might have bugs, so please review if you're concerned
about this issue.

To recap, under CVE-2020-14349, Noah documented that untrusted users
shouldn't own tables into which the system is performing logical
replication. Otherwise, the users could hook up triggers or default
expressions or whatever to those tables and they would execute with
the subscription owner's privileges, which would allow the table
owners to escalate to superuser. However, I'm unsatisfied with just
documenting the hazard, because I feel like almost everyone who uses
logical replication wants to do the exact thing that this
documentation says you shouldn't do. If you don't use logical
replication or run everything as the superuser or just don't care
about security, well then you don't have any problem here, but
otherwise you probably do.

The proposed fix is to perform logical replication actions (SELECT,
INSERT, UPDATE, DELETE, and TRUNCATE) as the user who owns the table
rather than as the owner of the subscription. The session still runs
as the subscription owner, but the active user ID is switched to the
table owner for the duration of each operation. To prevent table
owners from doing tricky things to attack the subscription owner, we
impose SECURITY_RESTRICTED_OPERATION while running as the table owner.
To avoid inconveniencing users when this restriction adds no
meaningful security, we refrain from imposing
SECURITY_RESTRICTED_OPERATION when the table owner can SET ROLE to the
subscription owner anyway. Such a user need not use logical
replication to break into the subscription owner's account: they have
access to it anyway.

There is also a possibility of an attack in the other direction. Maybe
the subscription owner would like to obtain the table owner's
permissions, or at the very least, use logical replication as a
vehicle to perform operations they can't perform directly. A malicious
subscription owner could hook up logical replication to a table into
which the table owner doesn't want replication to occur. To block such
attacks, the patch requires that the subscription owner have the
ability to SET ROLE to each table owner. If the subscription owner is
a superuser, which is usual, this will be automatic. Otherwise, the
superuser will need to grant to the subscription owner the roles that
own relevant tables. This can usefully serve as a kind of access
control to make sure that the subscription doesn't touch any tables
other than the ones it's supposed to be touching: just make those
tables owned by a different user and don't grant them to the
subscription owner. Previously, we provided no way at all of
controlling the tables that replication can target.

This fix interacts in an interesting way with Mark Dilger's work,
committed by Jeff Davis, to make logical replication respect table
permissions. I initially thought that with this change, that commit
would end up just being reverted, with the permissions scheme
described above replacing the existing one. However, I then realized
that it's still good to perform those checks. Normally, a table owner
can do any DML operation on a table they own, so those checks will
never fail. However, if a table owner has revoked their ability to,
say, INSERT into one of their own tables, then logical replication
shouldn't bypass that and perform the INSERT anyway. So I now think
that the checks added by that commit complement the ones added by this
proposed patch, rather than competing with them.

It is unclear to me whether we should try to back-port this. It's
definitely a behavior change, and changing the behavior in the back
branches is not a nice thing to do. On the other hand, at least in my
opinion, the security consequences of the status quo are pretty dire.
I tend to suspect that a really high percentage of people who are
using logical replication at all are vulnerable to this, and lots of
people having a way to escalate to superuser isn't good.

Comments?

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

---

From: Jeff Davis <[email-masked]>
Date: Fri, 24 Mar 2023 00:59:55 -0700

> changes and might have bugs, so please review if you're concerned

I think the subject has a typo, you mean "as the table owner", right?

> However, I'm unsatisfied with just
> documentation says you shouldn't do.

+1

> impose SECURITY_RESTRICTED_OPERATION while running as the table

+1

> subscription owner anyway.

That's a little confusing, why not just always use the
SECURITY_RESTRICTED_OPERATION? Is there a use case I'm missing?

> ability to SET ROLE to each table owner.

It would be really nice if this could be done with some kind of
ordinary privilege rather than requiring SET ROLE. A user might expect
that INSERT/UPDATE/DELETE/TRUNCATE privileges are enough. Or
pg_write_all_data.

I can see theoretically that a table owner might write some dangerous
code and attach it to their table. But I don't quite understand why
they would do that. If the code was vulnerable to attack, would that
mean that they couldn't even insert into their own table safely?

Requiring SET ROLE seems like it makes the subscription role into
something very close to a superuser. And that takes away some of the
benefits of delegating to non-superusers.

>  If the subscription owner is

We check for ordinary access privileges, which I think is your next
point, so the above paragraph confuses me a bit.

> proposed patch, rather than competing with them.

That's an interesting case.

> people having a way to escalate to superuser isn't good.

It's worth considering given that most subscription owners are
superusers anyway. What plausible cases might it break?

Regards,
	Jeff Davis

---

From: Robert Haas <[email-masked]>
Date: Fri, 24 Mar 2023 10:00:36 -0400

> SECURITY_RESTRICTED_OPERATION? Is there a use case I'm missing?

Some concern was expressed -- not sure exactly where the email is
exactly, and it might've been on pgsql-security -- that doing that
categorically might break things that are currently working. The
people who were concerned included Andres and I forget who else. My
gut reaction was the same as yours, just do it always and don't worry
about it. But if people think that users are likely to run afoul of
the SECURITY_RESTRICTED_OPERATION restrictions in practice, then this
is better, and the implementation complexity isn't high. We could even
think of extending this kind of logic to other places where
SECURITY_RESTRICTED_OPERATION is enforced, if desired.

> benefits of delegating to non-superusers.

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

More generally, Stephen Frost has elsewhere argued that we should want
the subscription owner to be a very low-privilege user, so that if
their privileges get stolen, it's no big deal. I disagree with that. I
think it's always a problem if one user can get unauthorized access to
another user's account, regardless of exactly what those accounts can
do. I think our goal should be to make it safe for the subscription
owner to be a very high-privilege user, because you're going to need
to be a very high-privilege user to set up replication. And if you do
have that level of privilege, it's more convenient and simpler if you
can just own the subscription yourself, rather than having to make a
dummy account to own it. To put that another way, I think that what
people are going to want to do in a lot of cases is have the superuser
own the subscription, so I think we need to make that case safe,
whatever it takes. In cases where the subscription owner isn't the
superuser, I think the next most likely possibility is that the
subscription owner is some kind of almost-super-user, like a
CREATEROLE user or someone running with rds_superuser or similar on
some PG fork. So that needs to be safe too, and I think this does
that. Having the subscription owner be some random user that doesn't
have a lot of privileges doesn't seem particularly useful to me. If it
were unproblematic to allow that, sure, but considering how easy it
would be for that low-privilege user to steal table owner privileges,
I don't think it makes sense.

> superusers anyway. What plausible cases might it break?

AFAIU, the main concern is about the SECURITY_RESTRICTED_OPERATION
flag interacting badly with things people are already doing. Other
problems seem possible, e.g. if you're doing something that gets the
current user name and does something with it, the answer's going to
change, and you might like the new answer more or less than the old
one. It's a little hard to predict who will be inconvenienced in what
ways when you change behavior, but problems are certainly possible.

---

From: Mark Dilger <[email-masked]>
Date: Fri, 24 Mar 2023 09:58:57 -0700

> On Mar 24, 2023, at 7:00 AM, Robert Haas <[email-masked]> wrote:
> 
> whatever it takes.

I also think the subscription owner should be a low-privileged user, owing to the risk of the publisher injecting malicious content into the publication.  I think you are focused on all the bad actors on the subscription-side database and what they can do to each other.  That's also valid, but I get the impression that you're losing sight of the risk posed by malicious publishers.  Or maybe you aren't, and can explain?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Robert Haas <[email-masked]>
Date: Fri, 24 Mar 2023 14:35:19 -0400

> I also think the subscription owner should be a low-privileged user, owing to the risk of the publisher injecting malicious content into the publication.  I think you are focused on all the bad act…

You have a point.

As things stand today, if you think somebody's going to send you
changes to tables other than the ones you intend to replicate, you
could handle that by making sure that the user that owns the
subscription only has permission to write to the tables that are
expected to receive replicated data. It's a bit awkward to set up
because you have to initially make the subscription owner a superuser
and then later remove the superuser bit, so I think this is another
argument for the pg_create_subscription patch posted elsewhere, but if
you're a superuser yourself, you can do it. However, with this patch,
that wouldn't work any more, because the permissions checks don't
happen until after we've switched to the target role. You could
alternatively set up a user to own the subscription who has the
ability to SET ROLE to some users and not others, but that only lets
you restrict replication based on which user owns the tables, rather
than which specific tables are getting data replicated into them. That
actually wouldn't work today, and with the patch it would start
working, so basically the effect of the patch on the problem that you
mention would be to remove the ability to filter by specific table and
add the ability to filter by owning role.

I don't know how bad that sounds to you, and if it does sound bad, I
don't immediately see how to mitigate it. As I said to Jeff, if you
can replicate into a table that has a casually-written SECURITY
INVOKER trigger on it, you can probably hack into the table owner's
account. So I think that if we allow user A to replicate into user B's
table with fewer privileges than A-can-set-role-to-B, we're building a
privilege-escalation attack into the system. But if we do require
A-can-set-role-to-B, then things change as described above.

I suppose in theory we could check both A-can-set-role-to-B and
A-can-modify-this-table-as-A, but that feels pretty unprincipled. I
can't particularly see why A should need permission to perform an
action that is actually going to be performed as B; what makes sense
is to check that A can become B, and that B has permission to perform
the action, which is the state that this patch would create. I guess
there are other things we could do, too, like add replication
restriction or filtering capabilities specifically to address this
problem, or devise some other kind of new kind of permission system
around this, but I don't have a specific idea what that would look
like.

---

From: Jelte Fennema <[email-masked]>
Date: Sat, 25 Mar 2023 10:24:32 +0100

> happen until after we've switched to the target role.

For my purposes I always trust the publisher, what I don't trust is
the table owners. But I can indeed imagine scenarios where that's the
other way around, and indeed you can protect against that currently,
but not with your new patch. That seems fairly easily solvable though.

+   if (!member_can_set_role(context->save_userid, userid))
+       ereport(ERROR,
+               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                errmsg("role \"%s\" cannot SET ROLE to \"%s\"",
+                       GetUserNameFromId(context->save_userid, false),
+                       GetUserNameFromId(userid, false))));

If we don't throw an error here, but instead simply return, then the
current behaviour is preserved and people can manually configure
permissions to protect against an untrusted publisher. This would
still mean that the table owners can escalate privileges to the
subscription owner, but if that subscription owner actually has fewer
privileges than the table owner then you don't have that issue.