running logical replication as the subscription owner

I'm definitely in favor of making it easier to use logical replication
in a safe manner. In Citus we need to logically replicate and we're
currently using quite some nasty and undocumented hacks to do so:
We're creating a subscription per table owner, where each subscription
is owned by a temporary user that has the same permissions as the
table owner. These temporary users were originally superusers, because
otherwise we cannot make them subscription owners, but once assigning
a subscription to them we take away the superuser permissions from
them[1]https://github.com/citusdata/citus/blob/main/src/backend/distributed/replication/multi_logical_replication.c#L1487-L1573. And we also need to hook into ALTER/DELETE subscription
commands to make sure that these temporary owners cannot edit their
own subscription[2]https://github.com/citusdata/citus/blob/main/src/backend/distributed/commands/utility_hook.c#L455-L487.

Getting this right was not easy. And even it has the serious downside
that we need multiple subscriptions/replication slots which causes
extra complexity in various ways and it eats much more aggressively
into the replication slot limits than we'd like. Having one
subscription that could apply into tables that were owned by multiple
users in a safe way would make this sooo much easier.

[1]: https://github.com/citusdata/citus/blob/main/src/backend/distributed/replication/multi_logical_replication.c#L1487-L1573
[2]: https://github.com/citusdata/citus/blob/main/src/backend/distributed/commands/utility_hook.c#L455-L487

On Fri, Mar 3, 2023 at 6:57 PM Jelte Fennema <postgres@jeltef.nl> wrote:

I'm definitely in favor of making it easier to use logical replication
in a safe manner.

Cool.

In Citus we need to logically replicate and we're
currently using quite some nasty and undocumented hacks to do so:
We're creating a subscription per table owner, where each subscription
is owned by a temporary user that has the same permissions as the
table owner. These temporary users were originally superusers, because
otherwise we cannot make them subscription owners, but once assigning
a subscription to them we take away the superuser permissions from
them[1]. And we also need to hook into ALTER/DELETE subscription
commands to make sure that these temporary owners cannot edit their
own subscription[2].

Getting this right was not easy. And even it has the serious downside
that we need multiple subscriptions/replication slots which causes
extra complexity in various ways and it eats much more aggressively
into the replication slot limits than we'd like. Having one
subscription that could apply into tables that were owned by multiple
users in a safe way would make this sooo much easier.

Yeah. As Andres pointed out somewhere or other, that also means you're
decoding the WAL once per user instead of just once. I'm surprised
that hasn't been cost-prohibitive.

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

Yeah. As Andres pointed out somewhere or other, that also means you're
decoding the WAL once per user instead of just once. I'm surprised
that hasn't been cost-prohibitive.

We'd definitely prefer to have one subscription and do the decoding
only once. But we haven't run into big perf issues with the current
setup so far. We use it for non-blocking copying of shards (regular PG
tables under the hood). Most of the time is usually spent in the
initial copy phase, not the catchup. And also in practice our users
often only have one table owning user (and more than 5 table owning
users is extremely rare).

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

On Fri, Mar 24, 2023 at 3:59 AM Jeff Davis <pgsql@j-davis.com> wrote:

That's a little confusing, why not just always use the
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.

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.

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.

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?

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.

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

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.

I personally cannot think of a reasonable example that would be
broken, but I agree the code is simple enough. I do think that if
there is an actual reason to do this, we'd probably want it in other
places where SECURITY_RESTRICTED_OPERATION is enforced too.

I think there's some important tests missing related to this:
1. Ensuring that SECURITY_RESTRICTED_OPERATION things are enforced
when the user **does not** have SET ROLE permissions to the
subscription owner, e.g. don't allow SET ROLE from a trigger.
2. Ensuring that SECURITY_RESTRICTED_OPERATION things are not enforced
when the user **does** have SET ROLE permissions to the subscription
owner, e.g. allows SET ROLE from trigger.

Finally a small typo in the one of the comments:

+ * If we created a new GUC nest level, also role back any changes that were

s/role/roll/

On Fri, Mar 24, 2023 at 12:17 PM Jelte Fennema <postgres@jeltef.nl> wrote:

I personally cannot think of a reasonable example that would be
broken, but I agree the code is simple enough. I do think that if
there is an actual reason to do this, we'd probably want it in other
places where SECURITY_RESTRICTED_OPERATION is enforced too.

I don't think it makes sense for this patch to run around and try to
adjust all of those other pages. We have to pick between doing it this
way (and thus being inconsistent with what we do elsewhere) or taking
that logic out (and taking our chances that something will break for
some users). I'm OK with either of those, but I'm not OK with going
and changing the way this works in all of the other cases first and
only then coming back to this problem. This problem is WAY more
important than fiddling with the details of how
SECURITY_RESTRICTED_OPERATION is applied.

I think there's some important tests missing related to this:
1. Ensuring that SECURITY_RESTRICTED_OPERATION things are enforced
when the user **does not** have SET ROLE permissions to the
subscription owner, e.g. don't allow SET ROLE from a trigger.
2. Ensuring that SECURITY_RESTRICTED_OPERATION things are not enforced
when the user **does** have SET ROLE permissions to the subscription
owner, e.g. allows SET ROLE from trigger.

Yeah, if we stick with the current approach we should probably add
tests for that stuff.

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

On Mar 24, 2023, at 7:00 AM, Robert Haas <robertmhaas@gmail.com> wrote:

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.

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

I don't think it makes sense for this patch to run around and try to
adjust all of those other pages. We have to pick between doing it this
way (and thus being inconsistent with what we do elsewhere) or taking
that logic out (and taking our chances that something will break for
some users). I'm OK with either of those, but I'm not OK with going
and changing the way this works in all of the other cases first and
only then coming back to this problem. This problem is WAY more
important than fiddling with the details of how
SECURITY_RESTRICTED_OPERATION is applied.

Yes, I totally agree. I now realise that wasn't clear at all from the
wording in my previous email. I'm fine with both behaviours. I mainly meant
that if we actually think the new behaviour is better (which honestly I'm
not convinced of yet), then some follow up patch would probably be good. I
definitely don't want to block this patch on any of that though. Both
behaviors would be vastly better than the current one in my opinion. So if
others wanted the behaviour in your patch, I'm completely fine with that.

Yeah, if we stick with the current approach we should probably add
tests for that stuff.

Even if we don't, we should still have tests showing that the security
restrictions that we intend to put in place actually do their job.

On Fri, Mar 24, 2023 at 12:58 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

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?

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.

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

On Fri, Mar 24, 2023 at 2:14 PM Jelte Fennema <postgres@jeltef.nl> wrote:

Yes, I totally agree. I now realise that wasn't clear at all from the wording in my previous email. I'm fine with both behaviours. I mainly meant that if we actually think the new behaviour is better (which honestly I'm not convinced of yet), then some follow up patch would probably be good. I definitely don't want to block this patch on any of that though. Both behaviors would be vastly better than the current one in my opinion. So if others wanted the behaviour in your patch, I'm completely fine with that.

Makes sense. I hope a few more people will comment on what they think
we should do here, especially Andres and Noah.

Yeah, if we stick with the current approach we should probably add
tests for that stuff.

Even if we don't, we should still have tests showing that the security restrictions that we intend to put in place actually do their job.

Yeah, I just don't want to write the tests and then decide to change
the behavior and then have to write them over again. It's not so much
fun that I'm yearning to do it twice.

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

On Mar 24, 2023, at 11:35 AM, Robert Haas <robertmhaas@gmail.com> wrote:

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.

I assume you mean this bit:

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.

I don't find that terribly convincing. First, there's no reason a subscription owner should be an ordinary user able to volitionally do anything. The subscription owner should just be a role that the subscription runs under, as a means of superuser dropping privileges before applying changes. So the only real problem would be that the changes coming from the publisher might, upon application, hack the table owner. But if that's the case, the table owner's vulnerability on the subscription-database side is equal to their vulnerability on the publication-database side (assuming equal schemas on both). Flagging this vulnerability as being logical replication related seems a category error. Instead, it's a schema vulnerability.

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 don't understand the direction this patch is going. I'm emphatically not objecting to it, merely expressing my confusion about it.

I had imagined the solution to the replication security problem was to stop running the replication as superuser, and instead as a trivial user. Imagine that superuser creates roles "deadhead_bob" and "deadhead_alice" which cannot log in, are not members of any groups nor have any other roles as members of themselves, and have no privileges beyond begin able to replicate into bob's and alice's tables, respectively. The superuser sets up the subscriptions disabled, transfers ownership to deadhead_bob and deadhead_alice, and only then enables the subscriptions.

Since deadhead_bob and deadhead_alice cannot log in, and nobody can set role to them, I don't see what the vulnerability is. Sure, maybe alice can attack deadhead_alice, or bob can attack deadhead_bob, but that's more of a privilege deescalation than a privilege escalation, so where's the risk? That's not a rhetorical question. Is there a risk here? Or are we just concerned that most users will set up replication with superuser or some other high-privilege user, and we're trying to protect them from the consequences of that choice?

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

On Fri, 2023-03-24 at 10:00 -0400, Robert Haas wrote:

On Fri, Mar 24, 2023 at 3:59 AM Jeff Davis <pgsql@j-davis.com> wrote:

That's a little confusing, why not just always use the
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.

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?

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.

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?

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

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.

Regards,
Jeff Davis

On Fri, 2023-03-24 at 14:35 -0400, Robert Haas wrote:

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.

That's certainly a loss. It makes a lot of sense to want to limit a
subscription to only write to certain tables.

If you want to filter by role, you can do that today by granting the
role, or some role that has the necessary privileges.

It takes me a while to re-learn the problems of poorly-written trigger
functions, malicious trigger functions, search paths, etc., each time I
start working in this area again. Can you include an example of such a
trigger function that we're worried about? Can the subscription owner
change the search path in the subscription process, and if so, why?

The doc here:

https://www.postgresql.org/docs/devel/sql-createfunction.html#SQL-CREATEFUNCTION-SECURITY

mentions search_path, but other hazards don't really seem applicable.
(Is the trigger creating new roles?)

Regards,
Jeff Davis

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.

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.

On Fri, Mar 24, 2023 at 10:00:36AM -0400, Robert Haas wrote:

On Fri, Mar 24, 2023 at 3:59 AM Jeff Davis <pgsql@j-davis.com> wrote:

That's a little confusing, why not just always use the
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

SECURITY_RESTRICTED_OPERATION blocks deferred triggers and CREATE TEMP TABLE.
If you create a DEFERRABLE INITIALLY DEFERRED fk constraint and replicate to
the constraint's table in a SECURITY_RESTRICTED_OPERATION, I expect an error.

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

Hard to know. It's the sort of thing that I model as creating months of work
for epsilon users to adapt their applications. Epsilon could be zero. Most
users don't notice.

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.

Firing a trigger in an index expression or materialized view query is not
reasonable, so today's uses of SECURITY_RESTRICTED_OPERATION would not benefit
from this approach. Being able to create temp tables in those places has some
value, but I would allow temp tables in SECURITY_RESTRICTED_OPERATION instead
of proliferating the check on the ability to SET ROLE. (One might allow temp
tables by introducing NewTempSchemaNestLevel(), called whenever we call
NewGUCNestLevel(). The transaction would then proceed as though it has no
temp schema, allocating an additional schema if creating a temp object.)

On Fri, Mar 24, 2023 at 4:11 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

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.

I don't find that terribly convincing. First, there's no reason a subscription owner should be an ordinary user able to volitionally do anything. The subscription owner should just be a role that the subscription runs under, as a means of superuser dropping privileges before applying changes. So the only real problem would be that the changes coming from the publisher might, upon application, hack the table owner. But if that's the case, the table owner's vulnerability on the subscription-database side is equal to their vulnerability on the publication-database side (assuming equal schemas on both). Flagging this vulnerability as being logical replication related seems a category error. Instead, it's a schema vulnerability.

I think there are actually a number of reasons why the subscription
owner should be a regular user account rather than a special
low-privilege account. First, it's only barely possible to do anything
else. As of today, you can't create a subscription owned by a
non-superuser except by making the subscription first and then
removing superuser from the account. You can't even do this:

rhaas=# alter subscription s1 owner to alice;
ERROR: permission denied to change owner of subscription "s1"
HINT: The owner of a subscription must be a superuser.

That hint is actually a lie because of the loophole mentioned above,
but even if making the subscription owner a low-privilege account were
the right model (which I don't believe) we've got error messages in
the current source code saying that you're not allowed to even do it.

Second, even if this kind of setup were fully supported and all the
stuff worked as you expect, it's not very convenient. It requires you
to create this extra dummy account that doesn't otherwise need to
exist. I don't see a good reason to impose that requirement on
everybody. Given that subscriptions initially could only be owned by
superusers, and that's still mostly true, it seems to me that the
feature was intended to be used with the superuser as the subscription
owner, and I think that's what most people must be doing now and will
probably want to continue to do, and we should try to make it safe
instead of back-pedaling and saying, hey, do it this totally other way
instead.

More discussion of problems below.

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 don't understand the direction this patch is going. I'm emphatically not objecting to it, merely expressing my confusion about it.

I had imagined the solution to the replication security problem was to stop running the replication as superuser, and instead as a trivial user. Imagine that superuser creates roles "deadhead_bob" and "deadhead_alice" which cannot log in, are not members of any groups nor have any other roles as members of themselves, and have no privileges beyond begin able to replicate into bob's and alice's tables, respectively. The superuser sets up the subscriptions disabled, transfers ownership to deadhead_bob and deadhead_alice, and only then enables the subscriptions.

Since deadhead_bob and deadhead_alice cannot log in, and nobody can set role to them, I don't see what the vulnerability is. Sure, maybe alice can attack deadhead_alice, or bob can attack deadhead_bob, but that's more of a privilege deescalation than a privilege escalation, so where's the risk? That's not a rhetorical question. Is there a risk here? Or are we just concerned that most users will set up replication with superuser or some other high-privilege user, and we're trying to protect them from the consequences of that choice?

Having a separate subscription for each different table owner is
extremely undesirable from a performance perspective, because it means
that the WAL on the origin server has to be decoded once per table
owner. That's not very much fun even if the number of table owners is
only two, as in your example, and there could be many more than two
users who own tables. In addition, it breaks the transactional
consistency of replication. If there's any single transaction that
modifies both a table owned by alice and a table owned by bob, we lose
transactional consistency on the subscriber side unless both of those
transactions are replicated in a single transaction, which means they
also need to be part of a single subscription.

I admit that hacking into deadhead_alice or deadhead_bob is probably
only minimally interesting. There are, perhaps, things you could do
with that, like creating objects owned by that user, and maybe your
own account is subject to some kind of restrictions separate from
what's in place on the deadhead account, but most likely in most
circumstances you can't get very far by breaking into a deadhead
account whose only purpose is to replicate changes on tables you
already own. However, because of the problems with having a
subscription per table owner, you're probably really going to need a
shared deadhead account that is used to replicate across all users who
own tables, and now breaking into that account gets a lot more
interesting. Two users that aren't supposed to be able to talk to each
other could use the shared deadhead account to pass data back and
forth, e.g. to exfiltrate data from a top secret account to one with a
lower security classification. Or maybe a malicious user can try to
steal data as it's being replicated, or just mess with the system.
They can create objects owned by the deadhead account in any
publicly-writable schemas, and they can mess with the GUC settings
that apply to the deadhead account, at a minimum.

I basically don't think there's such a thing as an account that is so
low-privilege that we don't care about someone hacking into it. Now,
you can go way down the rabbit hole here and say "well, what if we
invented a SUPER low privilege account that ever create or own objects
and can't use even those privileges that are granted to public and
<insert other restrictions here>." And I admit that could be done, but
that sounds like a lot of work and I see no point. If we just make it
safe for superusers to own subscriptions, then the job's done. And I
see nothing preventing us from doing that. That's the point of this
patch, in fact.

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

On Fri, Mar 24, 2023 at 8:02 PM Jeff Davis <pgsql@j-davis.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

On Sat, Mar 25, 2023 at 12:01 PM Noah Misch <noah@leadboat.com> wrote:

(One might allow temp
tables by introducing NewTempSchemaNestLevel(), called whenever we call
NewGUCNestLevel(). The transaction would then proceed as though it has no
temp schema, allocating an additional schema if creating a temp object.)

Neat idea. That would require some adjustments, I believe, because of
the way that temp schemas are named. But it sounds doable.

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