Re: Delegating superuser tasks to new security roles (Was: Granting control of SUSET gucs to non-superusers) - Mailing list pgsql-hackers

On Thu, Jul 22, 2021 at 1:29 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> Certainly not.  What I meant on May 28 by "eventually" was that the patch set (posted May 25 and named "v3") had not
yetimplemented such security, as I was fishing for comments from the community about whether the basic division of
superuserinto these new roles was the right division.  Having gotten little feedback on that, on June 29 I posted
anotherpatch set (confusingly also named "v3", my apologies) in which patch 0001 had expanded to include new security
restrictions.

Oh.

> Prior to this patch, the logical replication workers run under the userid of the owner of the subscription.  This is
unchangedafter the patch.  The real difference is that prior to the patch, only superusers can own subscriptions, so
checkingpermissions on tables during replication would be silly (though not harmful).  The worker is assured of passing
allsuch permission checks by virtue of being a superuser.  After the patch, since subscription owners need not be
superusers,the permission checks are no longer silly.  There is no assurance that they have permission to apply changes
toa table, so naturally that has to be checked, and it is. 

Aren't you supposing that the set of superusers never changes? Unless
we have some code for this that we don't have elsewhere, a superuser
could create a subscription and then be de-superuser'd, or the
subscription's owner could be altered.

> > I
> > also think it should be accompanied not only by new test cases (which
> > you seem to have added, though I have not reviewed them in detail) but
> > also documentation changes (which seem to be missing, since the doc
> > changes are all about the new predefined role). This is a really
> > significant behavior change to logical replication IMV and shouldn't
> > just be slipped into some other patch.
>
> I'm not sure what is meant by "slipped into some other patch", but I *think* you mean that the documentation changes
shouldnot be in a separate patch from the behavioral changes.  I agree with that.  I'll add documentation of the
changesto logical replication in the same patch as the changes themselves. 

I just meant that I think the behavioral change to logical replication
is significant in its own right and should be a separate patch.
Perhaps it's not as significant as I thought, but I still think it
should be made separately and likely documented as an incompatibility
with previous releases, unless I'm still confused.

> > It also seems based on Noah's comments and your response that there
> > might be some other issue here, and I haven't understood what that is,
> > but I think that should also be fixed separately, and first.
> > Considering all this, I would suggest not having this be patch #1 in
> > your series; make something come first that doesn't have
> > prerequisites.
>
> The issue that gets thrown around in the email archive is that "arbitrary code" can be made to run on the subscriber
side. As I understand the problem, this is because trigger functions can be created on tables with arbitrary code in
them,and that code will be executed under the userid of the user who causes the trigger to fire during an
insert/update/deleterather than as the user who created the trigger.  This of course is not peculiar to logical
replication;it is how triggers work generally.  What is peculiar is that a non-superuser who can create tables,
triggers,publications and subscriptions can get the logical replication worker to perform inserts/updates/deletes on
thosetables, thereby firing those triggers, and executing the trigger code as superuser.  That is ordinarily not
somethingthat a user can do simply by creating a table with a trigger, since there would be no mechanism to force the
superuserto perform operations on the table. 
>
> After patch 0001 (which will be split in the next patch set, but hasn't been split yet) the user who creates the
subscriptionis also the user whose permissions are checked when operating on the table and executing the trigger.  This
closesthe security hole, so far as I am aware.  I would very much like more eyeballs on this patch, and if anybody sees
whythis is an insufficient solution, please speak up.  But it's not as if I punted the security issue down the road to
someill-defined future patch.  On the contrary, this patch both creates the ability to delegate subscription creation
authorityto a non-superuser and closes the security hole which that would otherwise entail, or at least, that is the
intent.

OK. I thought Noah must be talking about some other problem, because
on May 28th you wrote "Oh, I agree that this patch set does not go the
extra step to make it safe" and I failed to understand that you
thought you'd addressed this in v4.

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