Thread: Re: Extension pg_trgm, permissions and pg_dump order

[ redirecting to -bugs ]

=?iso-8859-1?Q?F=E4rber=2C_Franz-Josef_=28StMUK=29?= <Franz-Josef.Faerber@stmuk.bayern.de> writes:
> My minimal example goes like this: On the fresh container, execute

> ```sql
> CREATE ROLE limitedrole;
> CREATE SCHEMA ext_trgm;
> CREATE EXTENSION pg_trgm SCHEMA ext_trgm;
> GRANT USAGE ON SCHEMA ext_trgm TO limitedrole;

> SET ROLE limitedrole;
> CREATE TABLE x(y text);
> CREATE INDEX ON x USING gist(y ext_trgm.gist_trgm_ops);
> ```

> Dump the database with `pg_dump > /tmp/x`, then do
> ```sql
> DROP SCHEMA ext_trgm CASCADE; DROP TABLE x;
> ```
> (or alternatively create a fresh database and do a ` CREATE ROLE limitedrole;`)

> Then try to restore the dump with `cat /tmp/x | psql`.

> On version 14.2, this succeeds.
> On version 14.3, this fails with "ERROR:  permission denied for schema ext_trgm".

I think what is happening here is that parse analysis of the index
expression is now being done as the owner of the table (already assigned
as limitedrole), as a consequence of the patch for CVE-2022-1552.

So basically, that patch has completely broken pg_dump's scheme for
when it can issue GRANTs.  I'm not sure there is a simple fix :-(.
We could issue the GRANTs earlier but that is going to break some
other use-cases, if memory serves.

            regards, tom lane

Thanks for the report.

On Wed, May 25, 2022 at 01:02:35PM -0400, Tom Lane wrote:
> =?iso-8859-1?Q?F=E4rber=2C_Franz-Josef_=28StMUK=29?= <Franz-Josef.Faerber@stmuk.bayern.de> writes:
> > My minimal example goes like this: On the fresh container, execute
> 
> > ```sql
> > CREATE ROLE limitedrole;
> > CREATE SCHEMA ext_trgm;
> > CREATE EXTENSION pg_trgm SCHEMA ext_trgm;
> > GRANT USAGE ON SCHEMA ext_trgm TO limitedrole;
> 
> > SET ROLE limitedrole;
> > CREATE TABLE x(y text);
> > CREATE INDEX ON x USING gist(y ext_trgm.gist_trgm_ops);
> > ```
> 
> > Dump the database with `pg_dump > /tmp/x`, then do
> > ```sql
> > DROP SCHEMA ext_trgm CASCADE; DROP TABLE x;
> > ```
> > (or alternatively create a fresh database and do a ` CREATE ROLE limitedrole;`)
> 
> > Then try to restore the dump with `cat /tmp/x | psql`.

More-minimally, one can run this without involving pg_dump:

BEGIN;
CREATE ROLE limitedrole;
CREATE SCHEMA ext_trgm;
CREATE EXTENSION pg_trgm SCHEMA ext_trgm;
CREATE TABLE x(y text);
ALTER TABLE x OWNER TO limitedrole;
CREATE INDEX ON x USING gist(y ext_trgm.gist_trgm_ops);
ROLLBACK;

> > On version 14.2, this succeeds.
> > On version 14.3, this fails with "ERROR:  permission denied for schema ext_trgm".
> 
> I think what is happening here is that parse analysis of the index
> expression is now being done as the owner of the table (already assigned
> as limitedrole), as a consequence of the patch for CVE-2022-1552.

Yep.

> So basically, that patch has completely broken pg_dump's scheme for
> when it can issue GRANTs.  I'm not sure there is a simple fix :-(.

Not too simple, no.  The parts of DefineIndex() that execute user code
(e.g. DefineIndex->ComputeIndexAttrs->CheckMutability) are interspersed with
the parts that do permissions checks, like the one yielding $SUBJECT at
DefineIndex->ComputeIndexAttrs->ResolveOpClass->LookupExplicitNamespace.  My
first candidate is to undo the userid switch before the ResolveOpClass() call
and restore it after.  My second candidate is to pass down the userid we want
used for this sort of permissions check.  Depending on the full list of call
stacks reaching permissions checks, this could get hairy.

> We could issue the GRANTs earlier but that is going to break some
> other use-cases, if memory serves.

It does.  Early GRANT is still the right thing, but there's more to build
before one can do that without breaking things that manage to work today.

On Thu, May 26, 2022 at 1:50 AM Noah Misch <noah@leadboat.com> wrote:
> > I think what is happening here is that parse analysis of the index
> > expression is now being done as the owner of the table (already assigned
> > as limitedrole), as a consequence of the patch for CVE-2022-1552.
> > So basically, that patch has completely broken pg_dump's scheme for
> > when it can issue GRANTs.  I'm not sure there is a simple fix :-(.
>
> Not too simple, no.  The parts of DefineIndex() that execute user code
> (e.g. DefineIndex->ComputeIndexAttrs->CheckMutability) are interspersed with
> the parts that do permissions checks, like the one yielding $SUBJECT at
> DefineIndex->ComputeIndexAttrs->ResolveOpClass->LookupExplicitNamespace.  My
> first candidate is to undo the userid switch before the ResolveOpClass() call
> and restore it after.  My second candidate is to pass down the userid we want
> used for this sort of permissions check.  Depending on the full list of call
> stacks reaching permissions checks, this could get hairy.

Why isn't the current behavior - i.e. failing with a permissions error
- correct? I mean I realize it's wrong in the sense that you can't
restore a dump you just took, but what about from a security
perspective? I'm not sure there's an actual problem, because it's the
superuser who is creating a new index here, and the superuser can do
as they wish, but the point of CVE-2022-1552 seems to be that
unprivileged users could induce the superuser to do things they
couldn't do themselves, and the consequence of preventing that seems
to be that things done as the superuser might sometimes fail because
the unprivileged user wouldn't have been able to do them, which is
what's happening here.

Now, I don't think it's an intrinsic problem if we have different
rules for different parts of the operation, and I guess that is what
you are proposing here, but it doesn't seem very clear which parts of
the operation ought to be subject to which rules, and why. Like,
what's the distinction, exactly, between "parts that do permissions
checks" and "parts that user code"? Under what circumstances, exactly,
do we need to use the table owner's permissions, and under what
circumstances the permissions of the user performing the operation?

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

Robert Haas <robertmhaas@gmail.com> writes:
> Why isn't the current behavior - i.e. failing with a permissions error
> - correct? I mean I realize it's wrong in the sense that you can't
> restore a dump you just took, but what about from a security
> perspective?

From the security perspective it may be just fine.  Nonetheless,
we need to un-break pg_dump; it's not optional for that to work.

            regards, tom lane

On Fri, May 27, 2022 at 2:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > Why isn't the current behavior - i.e. failing with a permissions error
> > - correct? I mean I realize it's wrong in the sense that you can't
> > restore a dump you just took, but what about from a security
> > perspective?
>
> From the security perspective it may be just fine.  Nonetheless,
> we need to un-break pg_dump; it's not optional for that to work.

That's pretty obvious. What's not obvious - at least to me - is
whether the kinds of changes that Noah is proposing to make here have
the effect of putting back the security vulnerability that the
original commit was intended to fix. If we decide that reverting and
living with the vulnerability is the only option, so be it. But we
shouldn't *accidentally* destroy the security that the commit that
caused the problem was trying to create.

To put that another way, there should be some kind of understandable
theory behind whatever the code does. I get nervous when we start
talking about making this bit of the code work this way and this other
bit work that way. If we don't know why we're doing that, other than
"to make it work," then I think we can't have much confidence in the
result ... even if it does, in fact, "make it work."

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, May 27, 2022 at 2:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> From the security perspective it may be just fine.  Nonetheless,
>> we need to un-break pg_dump; it's not optional for that to work.

> That's pretty obvious. What's not obvious - at least to me - is
> whether the kinds of changes that Noah is proposing to make here have
> the effect of putting back the security vulnerability that the
> original commit was intended to fix.

Yeah, that is the problematic part of this.  I'm not sure about that
either, and we'll need to look at it closely.

            regards, tom lane

On Wed, May 25, 2022 at 10:50:47PM -0700, Noah Misch wrote:
> BEGIN;
> CREATE ROLE limitedrole;
> CREATE SCHEMA ext_trgm;
> CREATE EXTENSION pg_trgm SCHEMA ext_trgm;
> CREATE TABLE x(y text);
> ALTER TABLE x OWNER TO limitedrole;
> CREATE INDEX ON x USING gist(y ext_trgm.gist_trgm_ops);
> ROLLBACK;

> Not too simple, no.  The parts of DefineIndex() that execute user code
> (e.g. DefineIndex->ComputeIndexAttrs->CheckMutability) are interspersed with
> the parts that do permissions checks, like the one yielding $SUBJECT at
> DefineIndex->ComputeIndexAttrs->ResolveOpClass->LookupExplicitNamespace.  My
> first candidate is to undo the userid switch before the ResolveOpClass() call
> and restore it after.  My second candidate is to pass down the userid we want
> used for this sort of permissions check.  Depending on the full list of call
> stacks reaching permissions checks, this could get hairy.

While I'd value the opportunity to work on this, there only a 50% chance I
could get it done by 2022-08-01.  I've set aside four hours on 2022-06-12 to
see how far I get.  For a 95% chance, the date would be 2023-02-01.  (I've
already canceled a 2022-07 vacation for the thing taking my time instead;
there's nothing clearly left to cut.)  If anyone would like it done faster
than that, I welcome that person taking over.

On Fri, May 27, 2022 at 09:51:22PM -0700, Noah Misch wrote:
> On Wed, May 25, 2022 at 10:50:47PM -0700, Noah Misch wrote:
>> BEGIN;
>> CREATE ROLE limitedrole;
>> CREATE SCHEMA ext_trgm;
>> CREATE EXTENSION pg_trgm SCHEMA ext_trgm;
>> CREATE TABLE x(y text);
>> ALTER TABLE x OWNER TO limitedrole;
>> CREATE INDEX ON x USING gist(y ext_trgm.gist_trgm_ops);
>> ROLLBACK;
> 
>> Not too simple, no.  The parts of DefineIndex() that execute user code
>> (e.g. DefineIndex->ComputeIndexAttrs->CheckMutability) are interspersed with
>> the parts that do permissions checks, like the one yielding $SUBJECT at
>> DefineIndex->ComputeIndexAttrs->ResolveOpClass->LookupExplicitNamespace.  My
>> first candidate is to undo the userid switch before the ResolveOpClass() call
>> and restore it after.  My second candidate is to pass down the userid we want
>> used for this sort of permissions check.  Depending on the full list of call
>> stacks reaching permissions checks, this could get hairy.
> 
> While I'd value the opportunity to work on this, there only a 50% chance I
> could get it done by 2022-08-01.  I've set aside four hours on 2022-06-12 to
> see how far I get.  For a 95% chance, the date would be 2023-02-01.  (I've
> already canceled a 2022-07 vacation for the thing taking my time instead;
> there's nothing clearly left to cut.)  If anyone would like it done faster
> than that, I welcome that person taking over.

I'd like to help.  I started looking at the problem and hacked together a
proof-of-concept based on your first candidate that seems to fix the
reported issue.  However, from the upthread discussion, it is not clear
whether there is agreement on the approach.  IIUC there are still many
other code paths that would require a similar treatment, so perhaps
identifying all of those would be a good next step.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Sat, May 28, 2022 at 08:30:33PM -0700, Nathan Bossart wrote:
> On Fri, May 27, 2022 at 09:51:22PM -0700, Noah Misch wrote:
> > On Wed, May 25, 2022 at 10:50:47PM -0700, Noah Misch wrote:
> >> BEGIN;
> >> CREATE ROLE limitedrole;
> >> CREATE SCHEMA ext_trgm;
> >> CREATE EXTENSION pg_trgm SCHEMA ext_trgm;
> >> CREATE TABLE x(y text);
> >> ALTER TABLE x OWNER TO limitedrole;
> >> CREATE INDEX ON x USING gist(y ext_trgm.gist_trgm_ops);
> >> ROLLBACK;
> > 
> >> Not too simple, no.  The parts of DefineIndex() that execute user code
> >> (e.g. DefineIndex->ComputeIndexAttrs->CheckMutability) are interspersed with
> >> the parts that do permissions checks, like the one yielding $SUBJECT at
> >> DefineIndex->ComputeIndexAttrs->ResolveOpClass->LookupExplicitNamespace.  My
> >> first candidate is to undo the userid switch before the ResolveOpClass() call
> >> and restore it after.  My second candidate is to pass down the userid we want
> >> used for this sort of permissions check.  Depending on the full list of call
> >> stacks reaching permissions checks, this could get hairy.
> > 
> > While I'd value the opportunity to work on this, there only a 50% chance I
> > could get it done by 2022-08-01.  I've set aside four hours on 2022-06-12 to
> > see how far I get.  For a 95% chance, the date would be 2023-02-01.  (I've
> > already canceled a 2022-07 vacation for the thing taking my time instead;
> > there's nothing clearly left to cut.)  If anyone would like it done faster
> > than that, I welcome that person taking over.
> 
> I'd like to help.

Thanks.

> I started looking at the problem and hacked together a
> proof-of-concept based on your first candidate that seems to fix the
> reported issue.  However, from the upthread discussion, it is not clear
> whether there is agreement on the approach.  IIUC there are still many
> other code paths that would require a similar treatment, so perhaps
> identifying all of those would be a good next step.

Agreed.  To identify them, perhaps put an ereport(..., errbacktrace()) in
aclmask(), then write some index-creating DDL that refers to the
largest-possible number of objects.

On Sat, May 28, 2022 at 09:34:03PM -0700, Noah Misch wrote:
> On Sat, May 28, 2022 at 08:30:33PM -0700, Nathan Bossart wrote:
>> I started looking at the problem and hacked together a
>> proof-of-concept based on your first candidate that seems to fix the
>> reported issue.  However, from the upthread discussion, it is not clear
>> whether there is agreement on the approach.  IIUC there are still many
>> other code paths that would require a similar treatment, so perhaps
>> identifying all of those would be a good next step.
>
> Agreed.  To identify them, perhaps put an ereport(..., errbacktrace()) in
> aclmask(), then write some index-creating DDL that refers to the
> largest-possible number of objects.

While we've reached an agreement on the thread related to the
corruption caused by the incorrect snapshots for concurrent index
builds, this thread seems to be stalling a bit and we should try to
move on before getting a release out.  Is somebody looking at what we
have here?
--
Michael

On Tue, May 31, 2022 at 01:30:11PM +0900, Michael Paquier wrote:
> On Sat, May 28, 2022 at 09:34:03PM -0700, Noah Misch wrote:
>> On Sat, May 28, 2022 at 08:30:33PM -0700, Nathan Bossart wrote:
>>> I started looking at the problem and hacked together a
>>> proof-of-concept based on your first candidate that seems to fix the
>>> reported issue.  However, from the upthread discussion, it is not clear
>>> whether there is agreement on the approach.  IIUC there are still many
>>> other code paths that would require a similar treatment, so perhaps
>>> identifying all of those would be a good next step.
>> 
>> Agreed.  To identify them, perhaps put an ereport(..., errbacktrace()) in
>> aclmask(), then write some index-creating DDL that refers to the
>> largest-possible number of objects.
> 
> While we've reached an agreement on the thread related to the
> corruption caused by the incorrect snapshots for concurrent index
> builds, this thread seems to be stalling a bit and we should try to
> move on before getting a release out.  Is somebody looking at what we
> have here?

I've spent some time looking at all the code impacted by a117ceb and
0abc1a0, and I've yet to identify any additional problems besides the one
with ResolveOpClass().  However, I'm far from confident in this analysis,
and I still need to try out the ereport() approach that Noah suggested.  I
would welcome any assistance identifying other problem areas.

For now, I've attached a slightly polished patch to fix the reported issue.
It's still rather hacky, and it does nothing to reduce the complexity of
the current approach of weaving between user IDs, but perhaps it can serve
as a stopgap solution.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, May 31, 2022 at 08:28:55AM -0700, Nathan Bossart wrote:
> I've spent some time looking at all the code impacted by a117ceb and
> 0abc1a0, and I've yet to identify any additional problems besides the one
> with ResolveOpClass().  However, I'm far from confident in this analysis,
> and I still need to try out the ereport() approach that Noah suggested.  I
> would welcome any assistance identifying other problem areas.

Ah, I think I am missing something.  For example, the code that identifies
the exclusion operator (immediately following the call to ResolveOpClass())
is likely subject to a similar problem.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Jun 03, 2022 at 11:17:24AM -0700, Nathan Bossart wrote:
> On Tue, May 31, 2022 at 08:28:55AM -0700, Nathan Bossart wrote:
> > I've spent some time looking at all the code impacted by a117ceb and
> > 0abc1a0, and I've yet to identify any additional problems besides the one
> > with ResolveOpClass().  However, I'm far from confident in this analysis,
> > and I still need to try out the ereport() approach that Noah suggested.  I
> > would welcome any assistance identifying other problem areas.
> 
> Ah, I think I am missing something.  For example, the code that identifies
> the exclusion operator (immediately following the call to ResolveOpClass())
> is likely subject to a similar problem.

That would not surprise me.  Do you have what you need to continue?

On Sun, Jun 05, 2022 at 11:30:56PM -0700, Noah Misch wrote:
> On Fri, Jun 03, 2022 at 11:17:24AM -0700, Nathan Bossart wrote:
> > On Tue, May 31, 2022 at 08:28:55AM -0700, Nathan Bossart wrote:
> > > I've spent some time looking at all the code impacted by a117ceb and
> > > 0abc1a0, and I've yet to identify any additional problems besides the one
> > > with ResolveOpClass().  However, I'm far from confident in this analysis,
> > > and I still need to try out the ereport() approach that Noah suggested.  I
> > > would welcome any assistance identifying other problem areas.
> > 
> > Ah, I think I am missing something.  For example, the code that identifies
> > the exclusion operator (immediately following the call to ResolveOpClass())
> > is likely subject to a similar problem.
> 
> That would not surprise me.  Do you have what you need to continue?

I'll use that window on Sunday to review what you have.  If you have work in
progress, please get it to the mailing list by then.

On Wed, Jun 08, 2022 at 10:17:08PM -0700, Noah Misch wrote:
> On Sun, Jun 05, 2022 at 11:30:56PM -0700, Noah Misch wrote:
> > On Fri, Jun 03, 2022 at 11:17:24AM -0700, Nathan Bossart wrote:
> > > On Tue, May 31, 2022 at 08:28:55AM -0700, Nathan Bossart wrote:
> > > > I've spent some time looking at all the code impacted by a117ceb and
> > > > 0abc1a0, and I've yet to identify any additional problems besides the one
> > > > with ResolveOpClass().  However, I'm far from confident in this analysis,
> > > > and I still need to try out the ereport() approach that Noah suggested.  I
> > > > would welcome any assistance identifying other problem areas.
> > > 
> > > Ah, I think I am missing something.  For example, the code that identifies
> > > the exclusion operator (immediately following the call to ResolveOpClass())
> > > is likely subject to a similar problem.
> > 
> > That would not surprise me.  Do you have what you need to continue?
> 
> I'll use that window on Sunday to review what you have.  If you have work in
> progress, please get it to the mailing list by then.

I have the patch mostly done.  I will send something late on Wednesday.

On Sun, Jun 12, 2022 at 08:45:51PM -0700, Noah Misch wrote:
> On Wed, Jun 08, 2022 at 10:17:08PM -0700, Noah Misch wrote:
>> I'll use that window on Sunday to review what you have.  If you have work in
>> progress, please get it to the mailing list by then.
> 
> I have the patch mostly done.  I will send something late on Wednesday.

I apologize for taking so long to reply.  I intend to review your patch.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Sun, Jun 12, 2022 at 08:45:51PM -0700, Noah Misch wrote:
> On Wed, Jun 08, 2022 at 10:17:08PM -0700, Noah Misch wrote:
> > On Sun, Jun 05, 2022 at 11:30:56PM -0700, Noah Misch wrote:
> > > On Fri, Jun 03, 2022 at 11:17:24AM -0700, Nathan Bossart wrote:
> > > > On Tue, May 31, 2022 at 08:28:55AM -0700, Nathan Bossart wrote:
> > > > > I've spent some time looking at all the code impacted by a117ceb and
> > > > > 0abc1a0, and I've yet to identify any additional problems besides the one
> > > > > with ResolveOpClass().  However, I'm far from confident in this analysis,
> > > > > and I still need to try out the ereport() approach that Noah suggested.  I
> > > > > would welcome any assistance identifying other problem areas.
> > > > 
> > > > Ah, I think I am missing something.  For example, the code that identifies
> > > > the exclusion operator (immediately following the call to ResolveOpClass())
> > > > is likely subject to a similar problem.
> > > 
> > > That would not surprise me.  Do you have what you need to continue?
> > 
> > I'll use that window on Sunday to review what you have.  If you have work in
> > progress, please get it to the mailing list by then.
> 
> I have the patch mostly done.  I will send something late on Wednesday.

I also found post-202205 CREATE INDEX reaching an improper "permission denied"
for the namespace containing a collation.  The attached version fixes and
tests all three improper denials.  Also, I added discards of GUC changes
before switching to the original userid.  Without those, an index expression
changing search_path affects the collation lookup.  Unfortunately, this is
making ComputeIndexAttrs() look like an unplanned mess.  I considered two
alternatives:

1. Move all "List *names -> oid" translation activities from
   ComputeIndexAttrs() to transformIndexStmt().  I ruled this out for a
   back-patched fix, because such a change would tend to break compatibility
   with the PGXN modules that call transformIndexStmt() or DefineIndex().  It
   might be a good long-term direction, though the note in the header of
   transformIndexStmt() gives another obstacle.

2. Move all "List *names -> oid" translation activities from
   ComputeIndexAttrs() to DefineIndex(), before table_open().  There's nothing
   ruling out this option, but I ran out of time to try it.  It would be weird
   to have per-attribute logic in both DefineIndex() and ComputeIndexAttrs(),
   so I'm skeptical about this helping.

For partitioned tables, it would add good defense-in-depth to perform the
"List *names -> oid" translations once per SQL statement, not once per
partition as has been the case.  (1) would move in that direction.

Thanks,
nm

On Wed, Jun 15, 2022 at 10:42:18PM -0700, Noah Misch wrote:
> -REGRESS = citext citext_utf8
> +REGRESS = create_index_acl citext citext_utf8

It's a little unfortunate that these tests are located within the citext
module, but I can't claim to have a better idea.

> +         * Identify the opclass to use.  Use of ddl_userid is necessary due to
> +         * ACL checks therein.  This is safe despite opclasses containing
> +         * opaque expressions (specifically, functions), because only
> +         * superusers can define opclasses.

It's not clear to me why the fact that only superusers can define opclasses
makes this safe.

Overall, the patch seems reasonable to me.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Jun 21, 2022 at 10:56:16AM -0700, Nathan Bossart wrote:
> On Wed, Jun 15, 2022 at 10:42:18PM -0700, Noah Misch wrote:
> > -REGRESS = citext citext_utf8
> > +REGRESS = create_index_acl citext citext_utf8
> 
> It's a little unfortunate that these tests are located within the citext
> module, but I can't claim to have a better idea.

A little, yes.  I did consider creating an operator class in the main
regression suite, then testing that.  If I had used that approach, it probably
would have been a my_text that behaves as much like text as possible.  There's
little difference in value between a contrib/ test and an equivalent
src/test/regress/ test, so I picked this for the increase in realism and the
decrease in test code bulk.

> > +         * Identify the opclass to use.  Use of ddl_userid is necessary due to
> > +         * ACL checks therein.  This is safe despite opclasses containing
> > +         * opaque expressions (specifically, functions), because only
> > +         * superusers can define opclasses.
> 
> It's not clear to me why the fact that only superusers can define opclasses
> makes this safe.

        classOidP[attn] = ResolveOpClass(attribute->opclass,
                                         atttype,
                                         accessMethodName,
                                         accessMethodId);

To write the comment, I pondered how those four arguments could conceivably
lead ResolveOpClass() to locate Trojan code.  Since only superusers can define
opclasses, we can assume the catalog entries of an opclass do not point to
Trojan code.  (The superuser could just do the mischief directly, rather than
going to extra trouble to set a trap for later.)  If you see a hole in that
thinking, please do share.

> Overall, the patch seems reasonable to me.

Thanks for reviewing.  I'll push it sometime in the next seven days.

On Tue, Jun 21, 2022 at 08:37:04PM -0700, Noah Misch wrote:
> On Tue, Jun 21, 2022 at 10:56:16AM -0700, Nathan Bossart wrote:
>> On Wed, Jun 15, 2022 at 10:42:18PM -0700, Noah Misch wrote:
>> > +         * Identify the opclass to use.  Use of ddl_userid is necessary due to
>> > +         * ACL checks therein.  This is safe despite opclasses containing
>> > +         * opaque expressions (specifically, functions), because only
>> > +         * superusers can define opclasses.
>> 
>> It's not clear to me why the fact that only superusers can define opclasses
>> makes this safe.
> 
>         classOidP[attn] = ResolveOpClass(attribute->opclass,
>                                          atttype,
>                                          accessMethodName,
>                                          accessMethodId);
> 
> To write the comment, I pondered how those four arguments could conceivably
> lead ResolveOpClass() to locate Trojan code.  Since only superusers can define
> opclasses, we can assume the catalog entries of an opclass do not point to
> Trojan code.  (The superuser could just do the mischief directly, rather than
> going to extra trouble to set a trap for later.)  If you see a hole in that
> thinking, please do share.

Thanks for clarifying.  That makes sense to me.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com