Re: running logical replication as the subscription owner

On Fri, 2023-03-03 at 11:02 -0500, Robert Haas wrote:
> 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.

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

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

+1

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

+1

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

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

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

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

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

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

That's an interesting case.

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

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

Regards,
Jeff Davis