[prev in list] [next in list] [prev in thread] [next in thread] 

List:       pgsql-bugs
Subject:    Re: Extension pg_trgm, permissions and pg_dump order
From:       Nathan Bossart <nathandbossart () gmail ! com>
Date:       2022-05-31 15:28:55
Message-ID: 20220531152855.GA2236210 () nathanxps13
[Download RAW message or body]

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

["v2-0001-Ensure-privilege-checks-in-CREATE-INDEX-run-as-ca.patch" (text/x-diff)]

From ca8818854c46277aee6d307692e7ca12febe9642 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Tue, 31 May 2022 07:59:05 -0700
Subject: [PATCH v2 1/1] Ensure privilege checks in CREATE INDEX run as calling
 user.

a117ceb, which ensures that the user ID is set to the relation owner before
running user code, inadvertently caused the call to ResolveOpClass() in
DefineIndex() to run as the relation owner instead of the calling user.  Since
the relation owner may not have the same lookup privileges as the caller, CREATE
INDEX commands that used to succeed might now fail.  In particular, pg_dump may
generate such failing commands.

This change introduces optional arguments to ComputeIndexAttrs() that
DefineIndex() will use to indicate the role to use for ResolveOpClass().
---
 src/backend/commands/indexcmds.c         | 32 +++++++++++++++++++++---
 src/test/regress/expected/privileges.out | 10 ++++++++
 src/test/regress/sql/privileges.sql      | 10 ++++++++
 3 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index eac13ac0b7..d5a05b1d78 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -80,7 +80,9 @@ static void ComputeIndexAttrs(IndexInfo *indexInfo,
 							  Oid relId,
 							  const char *accessMethodName, Oid accessMethodId,
 							  bool amcanorder,
-							  bool isconstraint);
+							  bool isconstraint,
+							  Oid opclass_lookup_userid,
+							  int opclass_lookup_secctx);
 static char *ChooseIndexName(const char *tabname, Oid namespaceId,
 							 List *colnames, List *exclusionOpNames,
 							 bool primary, bool isconstraint);
@@ -236,7 +238,7 @@ CheckIndexCompatible(Oid oldId,
 					  coloptions, attributeList,
 					  exclusionOpNames, relationId,
 					  accessMethodName, accessMethodId,
-					  amcanorder, isconstraint);
+					  amcanorder, isconstraint, InvalidOid, 0);
 
 
 	/* Get the soon-obsolete pg_index tuple. */
@@ -890,7 +892,8 @@ DefineIndex(Oid relationId,
 					  coloptions, allIndexParams,
 					  stmt->excludeOpNames, relationId,
 					  accessMethodName, accessMethodId,
-					  amcanorder, stmt->isconstraint);
+					  amcanorder, stmt->isconstraint, root_save_userid,
+					  root_save_sec_context);
 
 	/*
 	 * Extra checks when creating a PRIMARY KEY index.
@@ -1730,6 +1733,12 @@ CheckPredicate(Expr *predicate)
  * Compute per-index-column information, including indexed column numbers
  * or index expressions, opclasses and their options. Note, all output vectors
  * should be allocated for all columns, including "including" ones.
+ *
+ * opclass_lookup_userid and opclass_lookup_secctx indicate the role and
+ * security context to use when looking up the opclass to use.  The caller may
+ * have switched to the table owner, who may not have privileges to look up the
+ * specified opclass.  If opclass_lookup_userid is InvalidOid, the role and
+ * security context are not switched when looking up the opclass.
  */
 static void
 ComputeIndexAttrs(IndexInfo *indexInfo,
@@ -1743,12 +1752,16 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
 				  const char *accessMethodName,
 				  Oid accessMethodId,
 				  bool amcanorder,
-				  bool isconstraint)
+				  bool isconstraint,
+				  Oid opclass_lookup_userid,
+				  int opclass_lookup_secctx)
 {
 	ListCell   *nextExclOp;
 	ListCell   *lc;
 	int			attn;
 	int			nkeycols = indexInfo->ii_NumIndexKeyAttrs;
+	Oid			saved_userid;
+	int			saved_secctx;
 
 	/* Allocate space for exclusion operator info, if needed */
 	if (exclusionOpNames)
@@ -1922,6 +1935,14 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
 
 		collationOidP[attn] = attcollation;
 
+		/* If a user ID is provided, switch to it for opclass lookup. */
+		if (OidIsValid(opclass_lookup_userid))
+		{
+			GetUserIdAndSecContext(&saved_userid, &saved_secctx);
+			SetUserIdAndSecContext(opclass_lookup_userid,
+								   opclass_lookup_secctx);
+		}
+
 		/*
 		 * Identify the opclass to use.
 		 */
@@ -1930,6 +1951,9 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
 										 accessMethodName,
 										 accessMethodId);
 
+		if (OidIsValid(opclass_lookup_userid))
+			SetUserIdAndSecContext(saved_userid, saved_secctx);
+
 		/*
 		 * Identify the exclusion operator, if any.
 		 */
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 03df567d50..a353fe221e 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -2652,3 +2652,13 @@ SELECT COUNT(*) >= 0 AS ok FROM pg_shmem_allocations;
 RESET ROLE;
 -- clean up
 DROP ROLE regress_readallstats;
+-- privilege checks in CREATE INDEX run as calling user
+BEGIN;
+CREATE ROLE limitedrole;
+CREATE SCHEMA a;
+CREATE OPERATOR CLASS a.ops FOR TYPE int USING btree AS STORAGE int;
+CREATE TABLE x (y INT);
+ALTER TABLE x OWNER TO limitedrole;
+CREATE INDEX ON x USING btree(y a.ops); -- fails, but not due to permissions
+ERROR:  missing support function 1 for attribute 1 of index "x_y_idx"
+ROLLBACK;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 2a6ba38e52..38c94e0846 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -1706,3 +1706,13 @@ RESET ROLE;
 
 -- clean up
 DROP ROLE regress_readallstats;
+
+-- privilege checks in CREATE INDEX run as calling user
+BEGIN;
+CREATE ROLE limitedrole;
+CREATE SCHEMA a;
+CREATE OPERATOR CLASS a.ops FOR TYPE int USING btree AS STORAGE int;
+CREATE TABLE x (y INT);
+ALTER TABLE x OWNER TO limitedrole;
+CREATE INDEX ON x USING btree(y a.ops); -- fails, but not due to permissions
+ROLLBACK;
-- 
2.25.1



[prev in list] [next in list] [prev in thread] [next in thread]