Re: Extension pg_trgm, permissions and pg_dump order

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(dot)Faerber(at)stmuk(dot)bayern(dot)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.