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

On Mon, Mar 27, 2023 at 3:04 PM Jeff Davis <pgsql@j-davis.com> wrote:
> On Mon, 2023-03-27 at 12:53 -0400, Robert Haas wrote:
> > 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.
>
> I am confused.
>
> If the trigger function requires schema "xyz" to be in the search_path,
> and the function itself doesn't set it, how will it ever get set in the
> subscription process? Won't such a function be simply broken for all
> logical subscriptions (in current code or with any of the proposals
> active right now)?
>
> And if the trigger function requires object "abc" (regardless of
> schema) to be somehow accessible without qualification, and if it
> doesn't set the search path itself, and it's not in pg_catalog; then
> again I think that function would be broken today.

No, not really. It's pretty common for a lot of things to be in the
public schema, and the public schema is likely to be in the search
path of every user involved.

> It feels like we are reaching to say that a trigger function might be
> broken based on some contrived cases, but we can already contrive cases
> that are broken for logical replication today. It might not be exactly
> the same set, but unless I'm missing something it would be a very
> similar set.

No, it's not a contrived case, and it's not the same set of cases, not
even close. Running functions with a different search path than the
author intended is a really common cause of all kinds of things
breaking. See for example commit
582edc369cdbd348d68441fc50fa26a84afd0c1a, which certainly did break
things for some users.

> Are you suggesting that we require the subscription owner to have SET
> ROLE on everybody so that everyone is equally insecure and there's no
> way to ever fix it even if you don't have any triggers on your tables
> at all?

I certainly am not. I don't even know how to respond to this. I want
to make it secure, not insecure. But we don't gain any security by
pretending that certain exploits don't exist or aren't problems when
they do and are. Quite the contrary.

> And all of this is to protect a user that writes a trigger function
> that executes arbitrary SQL based on the input data?

Or is insecure in any other way, and there are quite a few ways. If
you don't think that this is a problem in reality, I really don't know
how to carry this conversation forward. The idea that the average
trigger function is safe if it can be unexpectedly called by someone
other than the table owner with arguments and GUC settings chosen by
the caller doesn't seem remotely correct to me. It matches no part of
my experience with user-defined functions, either written by me or by
EDB customers. Every database I've ever seen that used triggers at all
would be vulnerable to the subscription owner compromising the table
owner's account.

> > Well, I continue to feel that if you can compromise the subscription
> > owner's account, we haven't really fixed anything yet.
>
> I'm trying to reconcile the following two points:
>
>   A. That you are proposing a patch to allow non-superuser
> subscriptions; and
>   B. That you are arguing that the subscription owner basically needs
> to be superuser and we should just accept that.
>
> Granted, there's some nuance here and I won't say that those two are
> mutually exclusive. But I think it would be helpful to explain how
> those two ideas fit together.

I do think that what this patch does is tantamount to B, because you
can have SET ROLE to some users without having SET ROLE to all users.
That's a big part of the point of the CREATEROLE and
createrole_self_grant work.

> But I think you're saying something slightly different about the
> subscription process compromising the table owner, and assuming that
> problem exists, your patch doesn't do anything to stop it. In fact,
> your patch requires the subscription owner can SET ROLE to each of the
> table owners, guaranteeing that the table owners can never do anything
> at all to protect themselves from the subscription owner.

Yeah, that's true, and like I said, if there's a way to avoid that,
great. But wishing it were so is not that way.

Let's back up a minute here. Suppose someone were to request a new
command, ALTER TABLE <name> DO NOT LET THE SUPERUSER ACCESS THIS. We
would reject that proposal. The reason we would reject it is because
it wouldn't actually work as documented. We know that the superuser
has the power to access that account and reverse that command, either
by SET ROLE or by changing the account password or by changing
pg_hba.conf or by shelling out to the filesystem and doing whatever.
The feature purports to do something that it actually cannot do. No
one can defend themselves against the superuser. Not even another
superuser can defend themselves against a superuser. Pretending
otherwise would be confusing and have no security benefits.

Now let's think about this case. Can a table owner defend themselves
against a subscription owner who wants to usurp their privileges? If
they cannot, then I think that what the patch does is correct for the
same reason that I think we would correctly reject the hypothetical
command proposed above. If they can, then the patch has got the wrong
idea, perhaps. But what is the actual answer to the question? My
answer is that it's *often* impossible but not *always* impossible. If
the table owner has no executable code at all attached to the table --
not just triggers, but also expression indexes and default expressions
and so forth -- then they can. If they do have those things then in
theory they might be able to protect themselves, but in practice they
are unlikely to be careful enough. I judge that practically every
installation where table owners use triggers would be easily
compromised. Only the most security-conscious of table owners are
going to get this right.

So that's a grey area, at least IMHO. The patch could be revised in
some way, and the permissions requirements downgraded. However, if we
do that, I think we're going to have to document that although in
theory table owners can make themselves against subscription owners,
in practice they probably won't be. That approach has some advantages,
and I don't think it's insane. However, I am not convinced that it is
the best idea, either, and I had the impression based on
pgsql-security discussion that Andres and Noah thought this way was
better. I might have misinterpreted their position, and they might
have changed their minds, and they might have been wrong. But that's
how we got here.

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