8000 CREATE INDEX: use the original userid for more ACL checks. · postgres/postgres@00377b9 · GitHub
[go: up one dir, main page]

Skip to content

Commit 00377b9

Browse files
committed
CREATE INDEX: use the original userid for more ACL checks.
Commit a117ceb used the original userid for ACL checks located directly in DefineIndex(), but it still adopted the table owner userid for more ACL checks than intended. That broke dump/reload of indexes that refer to an operator class, collation, or exclusion operator in a schema other than "public" or "pg_catalog". Back-patch to v10 (all supported versions), like the earlier commit. Nathan Bossart and Noah Misch Discussion: https://postgr.es/m/f8a4105f076544c180a87ef0c4822352@stmuk.bayern.de
1 parent 2f2e24d commit 00377b9

File tree

4 files changed

+256
-16
lines changed
  • contrib/citext
  • src/backend/commands
  • 4 files changed

    +256
    -16
    lines changed

    contrib/citext/Makefile

    Lines changed: 1 addition & 1 deletion
    Original file line numberDiff line numberDiff line change
    @@ -11,7 +11,7 @@ DATA = citext--1.4.sql \
    1111
    citext--1.0--1.1.sql
    1212
    PGFILEDESC = "citext - case-insensitive character string data type"
    1313

    14-
    REGRESS = citext citext_utf8
    14+
    REGRESS = create_index_acl citext citext_utf8
    1515

    1616
    ifdef USE_PGXS
    1717
    PG_CONFIG = pg_config
    Lines changed: 86 additions & 0 deletions
    Original file line numberDiff line numberDiff line change
    @@ -0,0 +1,86 @@
    1+
    -- Each DefineIndex() ACL check uses either the original userid or the table
    2+
    -- owner userid; see its header comment. Here, confirm that DefineIndex()
    3+
    -- uses its original userid where necessary. The test works by creating
    4+
    -- indexes that refer to as many sorts of objects as possible, with the table
    5+
    -- owner having as few applicable privileges as possible. (The privileges.sql
    6+
    -- regress_sro_user tests look for the opposite defect; they confirm that
    7+
    -- DefineIndex() uses the table owner userid where necessary.)
    8+
    SET allow_in_place_tablespaces = true;
    9+
    CREATE TABLESPACE regress_create_idx_tblspace LOCATION '';
    10+
    RESET allow_in_place_tablespaces;
    11+
    BEGIN;
    12+
    CREATE ROLE regress_minimal;
    13+
    CREATE SCHEMA s;
    14+
    CREATE EXTENSION citext SCHEMA s;
    15+
    -- Revoke all conceivably-relevant ACLs within the extension. The system
    16+
    -- doesn't check all these ACLs, but this will provide some coverage if that
    17+
    -- ever changes.
    18+
    REVOKE ALL ON TYPE s.citext FROM PUBLIC;
    19+
    REVOKE ALL ON FUNCTION s.citext_pattern_lt FROM PUBLIC;
    20+
    REVOKE ALL ON FUNCTION s.citext_pattern_le FROM PUBLIC;
    21+
    REVOKE ALL ON FUNCTION s.citext_eq FROM PUBLIC;
    22+
    REVOKE ALL ON FUNCTION s.citext_pattern_ge FROM PUBLIC;
    23+
    REVOKE ALL ON FUNCTION s.citext_pattern_gt FROM PUBLIC;
    24+
    REVOKE ALL ON FUNCTION s.citext_pattern_cmp FROM PUBLIC;
    25+
    -- Functions sufficient for making an index column that has the side effect of
    26+
    -- changing search_path at expression planning time.
    27+
    CREATE FUNCTION public.setter() RETURNS bool VOLATILE
    28+
    LANGUAGE SQL AS $$SET search_path = s; SELECT true$$;
    29+
    CREATE FUNCTION s.const() RETURNS bool IMMUTABLE
    30+
    LANGUAGE SQL AS $$SELECT public.setter()$$;
    31+
    CREATE FUNCTION s.index_this_expr(s.citext, bool) RETURNS s.citext IMMUTABLE
    32+
    LANGUAGE SQL AS $$SELECT $1$$;
    33+
    REVOKE ALL ON FUNCTION public.setter FROM PUBLIC;
    34+
    REVOKE ALL ON FUNCTION s.const FROM PUBLIC;
    35+
    REVOKE ALL ON FUNCTION s.index_this_expr FROM PUBLIC;
    36+
    -- Even for an empty table, expression planning calls s.const & public.setter.
    37+
    GRANT EXECUTE ON FUNCTION public.setter TO regress_minimal;
    38+
    GRANT EXECUTE ON FUNCTION s.const TO regress_minimal;
    39+
    -- Function for index predicate.
    40+
    CREATE FUNCTION s.index_row_if(s.citext) RETURNS bool IMMUTABLE
    41+
    LANGUAGE SQL AS $$SELECT $1 IS NOT NULL$$;
    42+
    REVOKE ALL ON FUNCTION s.index_row_if FROM PUBLIC;
    43+
    -- Even for an empty table, CREATE INDEX checks ii_Predicate permissions.
    44+
    GRANT EXECUTE ON FUNCTION s.index_row_if TO regress_minimal;
    45+
    -- Non-extension, non-function objects.
    46+
    CREATE COLLATION s.coll (LOCALE="C");
    47+
    CREATE TABLE s.x (y s.citext);
    48+
    ALTER TABLE s.x OWNER TO regress_minimal;
    49+
    -- Empty-table DefineIndex()
    50+
    CREATE UNIQUE INDEX u0rows ON s.x USING btree
    51+
    ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_pattern_ops)
    52+
    TABLESPACE regress_create_idx_tblspace
    53+
    WHERE s.index_row_if(y);
    54+
    ALTER TABLE s.x ADD CONSTRAINT e0rows EXCLUDE USING btree
    55+
    ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=)
    56+
    USING INDEX TABLESPACE regress_create_idx_tblspace
    57+
    WHERE (s.index_row_if(y));
    58+
    -- Make the table nonempty.
    59+
    INSERT INTO s.x VALUES ('foo'), ('bar');
    60+
    -- If the INSERT runs the planner on index expressions, a search_path change
    61+
    -- survives. As of 2022-06, the INSERT reuses a cached plan. It does so even
    62+
    -- under debug_discard_caches, since each index is new-in-transaction. If
    63+
    -- future work changes a cache lifecycle, this RESET may become necessary.
    64+
    RESET search_path;
    65+
    -- For a nonempty table, owner needs permissions throughout ii_Expressions.
    66+
    GRANT EXECUTE ON FUNCTION s.index_this_expr TO regress_minimal;
    67+
    CREATE UNIQUE INDEX u2rows ON s.x USING btree
    68+
    ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_pattern_ops)
    69+
    TABLESPACE regress_create_idx_tblspace
    70+
    WHERE s.index_row_if(y);
    71+
    ALTER TABLE s.x ADD CONSTRAINT e2rows EXCLUDE USING btree
    72+
    ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=)
    73+
    USING INDEX TABLESPACE regress_create_idx_tblspace
    74+
    WHERE (s.index_row_if(y));
    75+
    -- Shall not find s.coll via search_path, despite the s.const->public.setter
    76+
    -- call having set search_path=s during expression planning. Suppress the
    77+
    -- message itself, which depends on the database encoding.
    78+
    \set VERBOSITY sqlstate
    79+
    ALTER TABLE s.x ADD CONSTRAINT underqualified EXCLUDE USING btree
    80+
    ((s.index_this_expr(y, s.const())) COLLATE coll WITH s.=)
    81+
    USING INDEX TABLESPACE regress_create_idx_tblspace
    82+
    WHERE (s.index_row_if(y));
    83+
    ERROR: 42704
    84+
    \set VERBOSITY default
    85+
    ROLLBACK;
    86+
    DROP TABLESPACE regress_create_idx_tblspace;
    Lines changed: 88 additions & 0 deletions
    Original file line numberDiff line numberDiff line change
    @@ -0,0 +1,88 @@
    1+
    -- Each DefineIndex() ACL check uses either the original userid or the table
    2+
    -- owner userid; see its header comment. Here, confirm that DefineIndex()
    3+
    -- uses its original userid where necessary. The test works by creating
    4+
    -- indexes that refer to as many sorts of objects as possible, with the table
    5+
    -- owner having as few applicable privileges as possible. (The privileges.sql
    6+
    -- regress_sro_user tests look for the opposite defect; they confirm that
    7+
    -- DefineIndex() uses the table owner userid where necessary.)
    8+
    9+
    SET allow_in_place_tablespaces = true;
    10+
    CREATE TABLESPACE regress_create_idx_tblspace LOCATION '';
    11+
    RESET allow_in_place_tablespaces;
    12+
    13+
    BEGIN;
    14+
    CREATE ROLE regress_minimal;
    15+
    CREATE SCHEMA s;
    16+
    CREATE EXTENSION citext SCHEMA s;
    17+
    -- Revoke all conceivably-relevant ACLs within the extension. The system
    18+
    -- doesn't check all these ACLs, but this will provide some coverage if that
    19+
    -- ever changes.
    20+
    REVOKE ALL ON TYPE s.citext FROM PUBLIC;
    21+
    REVOKE ALL ON FUNCTION s.citext_pattern_lt FROM PUBLIC;
    22+
    REVOKE ALL ON FUNCTION s.citext_pattern_le FROM PUBLIC;
    23+
    REVOKE ALL ON FUNCTION s.citext_eq FROM PUBLIC;
    24+
    REVOKE ALL ON FUNCTION s.citext_pattern_ge FROM PUBLIC;
    25+
    REVOKE ALL ON FUNCTION s.citext_pattern_gt FROM PUBLIC;
    26+
    REVOKE ALL ON FUNCTION s.citext_pattern_cmp FROM PUBLIC;
    27+
    -- Functions sufficient for making an index column that has the side effect of
    28+
    -- changing search_path at expression planning time.
    29+
    CREATE FUNCTION public.setter() RETURNS bool VOLATILE
    30+
    LANGUAGE SQL AS $$SET search_path = s; SELECT true$$;
    31+
    CREATE FUNCTION s.const() RETURNS bool IMMUTABLE
    32+
    LANGUAGE SQL AS $$SELECT public.setter()$$;
    33+
    CREATE FUNCTION s.index_this_expr(s.citext, bool) RETURNS s.citext IMMUTABLE
    34+
    LANGUAGE SQL AS $$SELECT $1$$;
    35+
    REVOKE ALL ON FUNCTION public.setter FROM PUBLIC;
    36+
    REVOKE ALL ON FUNCTION s.const FROM PUBLIC;
    37+
    REVOKE ALL ON FUNCTION s.index_this_expr FROM PUBLIC;
    38+
    -- Even for an empty table, expression planning calls s.const & public.setter.
    39+
    GRANT EXECUTE ON FUNCTION public.setter TO regress_minimal;
    40+
    GRANT EXECUTE ON FUNCTION s.const TO regress_minimal;
    41+
    -- Function for index predicate.
    42+
    CREATE FUNCTION s.index_row_if(s.citext) RETURNS bool IMMUTABLE
    43+
    LANGUAGE SQL AS $$SELECT $1 IS NOT NULL$$;
    44+
    REVOKE ALL ON FUNCTION s.index_row_if FROM PUBLIC;
    45+
    -- Even for an empty table, CREATE INDEX checks ii_Predicate permissions.
    46+
    GRANT EXECUTE ON FUNCTION s.index_row_if TO regress_minimal;
    47+
    -- Non-extension, non-function objects.
    48+
    CREATE COLLATION s.coll (LOCALE="C");
    49+
    CREATE TABLE s.x (y s.citext);
    50+
    ALTER TABLE s.x OWNER TO regress_minimal;
    51+
    -- Empty-table DefineIndex()
    52+
    CREATE UNIQUE INDEX u0rows ON s.x USING btree
    53+
    ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_pattern_ops)
    54+
    TABLESPACE regress_create_idx_tblspace
    55+
    WHERE s.index_row_if(y);
    56+
    ALTER TABLE s.x ADD CONSTRAINT e0rows EXCLUDE USING btree
    57+
    ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=)
    58+
    USING INDEX TABLESPACE regress_create_idx_tblspace
    59+
    WHERE (s.index_row_if(y));
    60+
    -- Make the table nonempty.
    61+
    INSERT INTO s.x VALUES ('foo'), ('bar');
    62+
    -- If the INSERT runs the planner on index expressions, a search_path change
    63+
    -- survives. As of 2022-06, the INSERT reuses a cached plan. It does so even
    64+
    -- under debug_discard_caches, since each index is new-in-transaction. If
    65+
    -- future work changes a cache lifecycle, this RESET may become necessary.
    66+
    RESET search_path;
    67+
    -- For a nonempty table, owner needs permissions throughout ii_Expressions.
    68+
    GRANT EXECUTE ON FUNCTION s.index_this_expr TO regress_minimal;
    69+
    CREATE UNIQUE INDEX u2rows ON s.x USING btree
    70+
    ((s.index_this_expr(y, s.const())) COLLATE s.coll s.citext_pattern_ops)
    71+
    TABLESPACE regress_create_idx_tblspace
    72+
    WHERE s.index_row_if(y);
    73+
    ALTER TABLE s.x ADD CONSTRAINT e2rows EXCLUDE USING btree
    74+
    ((s.index_this_expr(y, s.const())) COLLATE s.coll WITH s.=)
    75+
    USING INDEX TABLESPACE regress_create_idx_tblspace
    76+
    WHERE (s.index_row_if(y));
    77+
    -- Shall not find s.coll via search_path, despite the s.const->public.setter
    78+
    -- call having set search_path=s during expression planning. Suppress the
    79+
    -- message itself, which depends on the database encoding.
    80+
    \set VERBOSITY sqlstate
    81+
    ALTER TABLE s.x ADD CONSTRAINT underqualified EXCLUDE USING btree
    82+
    ((s.index_this_expr(y, s.const())) COLLATE coll WITH s.=)
    83+
    USING INDEX TABLESPACE regress_create_idx_tblspace
    84+
    WHERE (s.index_row_if(y));
    85+
    \set VERBOSITY default
    86+
    ROLLBACK;
    87+
    88+
    DROP TABLESPACE regress_create_idx_tblspace;

    src/backend/commands/indexcmds.c

    Lines changed: 81 additions & 15 deletions
    Original file line numberDiff line numberDiff line change
    @@ -80,7 +80,10 @@ static void ComputeIndexAttrs(IndexInfo *indexInfo,
    8080
    Oid relId,
    8181
    const char *accessMethodName, Oid accessMethodId,
    8282
    bool amcanorder,
    83-
    bool isconstraint);
    83+
    bool isconstraint,
    84+
    Oid ddl_userid,
    85+
    int ddl_sec_context,
    86+
    int *ddl_save_nestlevel);
    8487
    static char *ChooseIndexName(const char *tabname, Oid namespaceId,
    8588
    List *colnames, List *exclusionOpNames,
    8689
    bool primary, bool isconstraint);
    @@ -220,9 +223,8 @@ CheckIndexCompatible(Oid oldId,
    220223
    * Compute the operator classes, collations, and exclusion operators for
    221224
    * the new index, so we can test whether it's compatible with the existing
    222225
    * one. Note that ComputeIndexAttrs might fail here, but that's OK:
    223-
    * DefineIndex would have called this function with the same arguments
    224-
    * later on, and it would have failed then anyway. Our attributeList
    225-
    * contains only key attributes, thus we're filling ii_NumIndexAttrs and
    226+
    * DefineIndex would have failed later. Our attributeList contains only
    227+
    * key attributes, thus we're filling ii_NumIndexAttrs and
    226228
    * ii_NumIndexKeyAttrs with same value.
    227229
    */
    228230
    indexInfo = makeIndexInfo(numberOfAttributes, numberOfAttributes,
    @@ -236,7 +238,7 @@ CheckIndexCompatible(Oid oldId,
    236238
    coloptions, attributeList,
    237239
    exclusionOpNames, relationId,
    238240
    accessMethodName, accessMethodId,
    239-
    amcanorder, isconstraint);
    241+
    amcanorder, isconstraint, InvalidOid, 0, NULL);
    240242

    241243

    242244
    /* Get the soon-obsolete pg_index tuple. */
    @@ -482,6 +484,19 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress)
    482484
    * DefineIndex
    483485
    * Creates a new index.
    484486
    *
    487+
    * This function manages the current userid according to the needs of pg_dump.
    488+
    * Recreating old-database catalog entries in new-database is fine, regardless
    489+
    * of which users would have permission to recreate those entries now. That's
    490+
    * just preservation of state. Running opaque expressions, like calling a
    491+
    * function named in a catalog entry or evaluating a pg_node_tree in a catalog
    492+
    * entry, as anyone other than the object owner, is not fine. To adhere to
    493+
    * those principles and to remain fail-safe, use the table owner userid for
    494+
    * most ACL checks. Use the original userid for ACL checks reached without
    495+
    * traversing opaque expressions. (pg_dump can predict such ACL checks from
    496+
    * catalogs.) Overall, this is a mess. Future DDL development should
    497+
    * consider offering one DDL command for catalog setup and a separate DDL
    498+
    * command for steps that run opaque expressions.
    499+
    *
    485500
    * 'relationId': the OID of the heap relation on which the index is to be
    486501
    * created
    487502
    * 'stmt': IndexStmt describing the properties of the new index.
    @@ -890,7 +905,8 @@ DefineIndex(Oid relationId,
    890905
    coloptions, allIndexParams,
    891906
    stmt->excludeOpNames, relationId,
    892907
    accessMethodName, accessMethodId,
    893-
    amcanorder, stmt->isconstraint);
    908+
    amcanorder, stmt->isconstraint, root_save_userid,
    909+
    root_save_sec_context, &root_save_nestlevel);
    894910

    895911
    /*
    896912
    * Extra checks when creating a PRIMARY KEY index.
    @@ -1170,11 +1186,8 @@ DefineIndex(Oid relationId,
    11701186

    11711187
    /*
    11721188
    * Roll back any GUC changes executed by index functions, and keep
    1173-
    * subsequent changes local to this command. It's barely possible that
    1174-
    * some index function changed a behavior-affecting GUC, e.g. xmloption,
    1175-
    * that affects subsequent steps. This improves bug-compatibility with
    1176-
    * older PostgreSQL versions. They did the AtEOXact_GUC() here for the
    1177-
    * purpose of clearing the above default_tablespace change.
    1189+
    * subsequent changes local to this command. This is essential if some
    1190+
    * index function changed a behavior-affecting GUC, e.g. search_path.
    11781191
    */
    11791192
    AtEOXact_GUC(false, root_save_nestlevel);
    11801193
    root_save_nestlevel = NewGUCNestLevel();
    @@ -1730,6 +1743,10 @@ CheckPredicate(Expr *predicate)
    17301743
    * Compute per-index-column information, including indexed column numbers
    17311744
    * or index expressions, opclasses and their options. Note, all output vectors
    17321745
    * should be allocated for all columns, including "including" ones.
    1746+
    *
    1747+
    * If the caller switched to the table owner, ddl_userid is the role for ACL
    1748+
    * checks reached without traversing opaque expressions. Otherwise, it's
    1749+
    * InvalidOid, and other ddl_* arguments are undefined.
    17331750
    */
    17341751
    static void
    17351752
    ComputeIndexAttrs(IndexInfo *indexInfo,
    @@ -1743,12 +1760,17 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
    17431760
    const char *accessMethodName,
    17441761
    Oid accessMethodId,
    17451762
    bool amcanorder,
    1746-
    bool isconstraint)
    1763+
    bool isconstraint,
    1764+
    Oid ddl_userid,
    1765+
    int ddl_sec_context,
    1766+
    int *ddl_save_nestlevel)
    17471767
    {
    17481768
    ListCell *nextExclOp;
    17491769
    ListCell *lc;
    17501770
    int attn;
    17511771
    int nkeycols = indexInfo->ii_NumIndexKeyAttrs;
    1772+
    Oid save_userid;
    1773+
    int save_sec_context;
    17521774

    17531775
    /* Allocate space for exclusion operator info, if needed */
    17541776
    if (exclusionOpNames)
    @@ -1762,6 +1784,9 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
    17621784
    else
    17631785
    nextExclOp = NULL;
    17641786

    1787+
    if (OidIsValid(ddl_userid))
    1788+
    GetUserIdAndSecContext(&save_userid, &save_sec_context);
    1789+
    17651790
    /*
    17661791
    * process attributeList
    17671792
    */
    @@ -1892,10 +1917,24 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
    18921917
    }
    18931918

    18941919
    /*
    1895-
    * Apply collation override if any
    1920+
    * Apply collation override if any. Use of ddl_userid is necessary
    1921+
    * due to ACL checks therein, and it's safe because collations don't
    1922+
    * contain opaque expressions (or non-opaque expressions).
    18961923
    */
    18971924
    if (attribute->collation)
    1925+
    {
    1926+
    if (OidIsValid(ddl_userid))
    1927+
    {
    1928+
    AtEOXact_GUC(false, *ddl_save_nestlevel);
    1929+
    SetUserIdAndSecContext(ddl_userid, ddl_sec_context);
    1930+
    }
    18981931
    attcollation = get_collation_oid(attribute->collation, false);
    1932+
    if (OidIsValid(ddl_userid))
    1933+
    {
    1934+
    SetUserIdAndSecContext(save_userid, save_sec_context);
    1935+
    *ddl_save_nestlevel = NewGUCNestLevel();
    1936+
    }
    1937+
    }
    18991938

    19001939
    /*
    19011940
    * Check we have a collation iff it's a collatable type. The only
    @@ -1923,12 +1962,25 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
    19231962
    collationOidP[attn] = attcollation;
    19241963

    19251964
    /*
    1926-
    * Identify the opclass to use.
    1965+
    * Identify the opclass to use. Use of ddl_userid is necessary due to
    1966+
    * ACL checks therein. This is safe despite opclasses containing
    1967+
    * opaque expressions (specifically, functions), because only
    1968+
    * superusers can define opclasses.
    19271969
    */
    1970+
    if (OidIsValid(ddl_userid))
    1971+
    {
    1972+
    AtEOXact_GUC(false, *ddl_save_nestlevel);
    1973+
    SetUserIdAndSecContext(ddl_userid, ddl_sec_context);
    1974+
    }
    19281975
    classOidP[attn] = ResolveOpClass(attribute->opclass,
    19291976
    atttype,
    19301977
    accessMethodName,
    19311978
    accessMethodId);
    1979+
    if (OidIsValid(ddl_userid))
    1980+
    {
    1981+
    SetUserIdAndSecContext(save_userid, save_sec_context);
    1982+
    *ddl_save_nestlevel = NewGUCNestLevel();
    1983+
    }
    19321984

    19331985
    /*
    19341986
    * Identify the exclusion operator, if any.
    @@ -1942,9 +1994,23 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
    19421994

    19431995
    /*
    19441996
    * Find the operator --- it must accept the column datatype
    1945-
    * without runtime coercion (but binary compatibility is OK)
    1997+
    * without runtime coercion (but binary compatibility is OK).
    1998+
    * Operators contain opaque expressions (specifically, functions).
    1999+
    * compatible_oper_opid() boils down to oper() and
    2000+
    * IsBinaryCoercible(). PostgreSQL would have security problems
    2001+
    * elsewhere if oper() started calling opaque expressions.
    19462002
    */
    2003+
    if (OidIsValid(ddl_userid))
    2004+
    {
    2005+
    AtEOXact_GUC(false, *ddl_save_nestlevel);
    2006+
    SetUserIdAndSecContext(ddl_userid, ddl_sec_context);
    2007+
    }
    19472008
    opid = compatible_oper_opid(opname, atttype, atttype, false);
    2009+
    if (OidIsValid(ddl_userid))
    2010+
    {
    2011+
    SetUserIdAndSecContext(save_userid, save_sec_context);
    2012+
    *ddl_save_nestlevel = NewGUCNestLevel();
    2013+
    }
    19482014

    19492015
    /*
    19502016
    * Only allow commutative operators to be used in exclusion

    0 commit comments

    Comments
     (0)
    0