running logical replication as the subscription owner - Mailing list pgsql-hackers

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