fix(security): validate table ownership before acquiring relation locks

CHANGELOG.md

@@ -54,6 +54,20 @@ pinned at `1.0-dev` or `1.0-alpha`, each true until the next version shipped.
 
 ### Fixed
 
+- Five entry points requested a relation lock before checking that the caller
+  owns the table. `pgcolumnar.add_projection` takes a `ShareLock`, and
+  `drop_projection`, `recluster`, `compact_rewrite` and `compact` take a
+  `ShareUpdateExclusiveLock`; each opened the relation first and validated
+  ownership after. An unprivileged caller could therefore queue in the lock
+  manager on a table it has no rights to, blocking readers and writers until it
+  was refused. Measured against a held `AccessExclusiveLock`: the call reached a
+  four second lock timeout before the ownership error, where it now returns the
+  ownership error immediately. Ownership is now checked before the lock, which
+  is the ordering `pgcolumnar.vacuum`, `vacuum_sorted` and `cluster` already
+  used. `drop_projection` also reported whether an arbitrary relation was
+  columnar to a caller who does not own it; it now reports only that they are
+  not the owner, matching the four entry points beside it. (#749)
+
 - The columnar scan resolved `pgcolumnar.zone_map` once per chunk group per
   predicate column instead of once per scan. Every group a predicate could
   exclude ran a relation open with a lock and two catalog name lookups, then

src/columnar.h

@@ -445,6 +445,7 @@ extern bool pgcolumnar_enable_end_truncation;
 
 /* error unless the current user owns the relation (maintenance/DDL gate) */
 extern void PgColumnarRequireTableOwner(Relation rel);
+extern void PgColumnarRequireTableOwnerByOid(Oid relid);
 extern void PgColumnarRequireNoRowSecurity(Oid relid);
 
 /*

src/columnar_projection.c

@@ -187,14 +187,15 @@ pgcolumnar_add_projection(PG_FUNCTION_ARGS)
 				(errcode(ERRCODE_NAME_TOO_LONG),
 				 errmsg("projection name \"%s\" is too long", projname)));
 
+	PgColumnarRequireTableOwnerByOid(relid);
+
 	/*
 	 * ShareLock: block concurrent INSERT/UPDATE/DELETE (RowExclusiveLock) while
 	 * we back-fill the projection from existing rows, so no concurrently written
 	 * row is missed -- the same lock non-concurrent CREATE INDEX takes. Reads are
 	 * unaffected. (A CONCURRENTLY variant is future work.)
 	 */
 	rel = table_open(relid, ShareLock);
-	PgColumnarRequireTableOwner(rel);
 	storageId = PgColumnarStorageId(rel);
 
 	existing = PgColumnarListProjections(storageId);
@@ -291,14 +292,23 @@ pgcolumnar_drop_projection(PG_FUNCTION_ARGS)
 	relid = PG_GETARG_OID(0);
 	projname = text_to_cstring(PG_GETARG_TEXT_PP(1));
 
+	/*
+	 * Ownership FIRST, then the relation type. Both precede table_open so a
+	 * non-owner never reaches the lock manager, and doing them in this order
+	 * means a non-owner is told only that they are not the owner: asking about
+	 * an arbitrary relation must not report back whether it is columnar. The
+	 * sibling entry points in columnar_vacuum.c and columnar_visibilitymap.c
+	 * are ordered the same way.
+	 */
+	PgColumnarRequireTableOwnerByOid(relid);
+
 	if (!PgColumnarIsColumnarRelation(relid))
 		ereport(ERROR,
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 				 errmsg("\"%s\" is not a columnar table",
 						get_rel_name(relid))));
 
 	rel = table_open(relid, ShareUpdateExclusiveLock);
-	PgColumnarRequireTableOwner(rel);
 	storageId = PgColumnarStorageId(rel);
 	existing = PgColumnarListProjections(storageId);
 

src/columnar_vacuum.c

@@ -191,7 +191,7 @@ PgColumnarRequireTableOwner(Relation rel)
  * not own until its own lock request was resolved, so it has to be turned away
  * before the lock is requested rather than after it is held.
  */
-static void
+void
 PgColumnarRequireTableOwnerByOid(Oid relid)
 {
 	if (!COLUMNAR_TABLE_OWNERCHECK(relid))
@@ -776,6 +776,8 @@ pgcolumnar_recluster(PG_FUNCTION_ARGS)
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 				 errmsg("Z-order clustering supports at most 8 columns")));
 
+	PgColumnarRequireTableOwnerByOid(relid);
+
 	/* the lazy lock: concurrent reads and writes during the recluster */
 	rel = table_open(relid, ShareUpdateExclusiveLock);
 
@@ -788,8 +790,6 @@ pgcolumnar_recluster(PG_FUNCTION_ARGS)
 						RelationGetRelationName(rel))));
 	}
 
-	PgColumnarRequireTableOwner(rel);
-
 	tupdesc = RelationGetDescr(rel);
 	atts = palloc(ncols * sizeof(AttrNumber));
 	for (i = 0; i < ncols; i++)
@@ -882,6 +882,8 @@ pgcolumnar_compact_rewrite(PG_FUNCTION_ARGS)
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 				 errmsg("min_deleted_fraction must be between 0 and 1")));
 
+	PgColumnarRequireTableOwnerByOid(relid);
+
 	rel = table_open(relid, ShareUpdateExclusiveLock);
 
 	if (!PgColumnarIsColumnarRelation(relid))
@@ -893,8 +895,6 @@ pgcolumnar_compact_rewrite(PG_FUNCTION_ARGS)
 						RelationGetRelationName(rel))));
 	}
 
-	PgColumnarRequireTableOwner(rel);
-
 	/* dev/test: hold SUEL so the daemon's yield is observable (#415) */
 	pgcolumnar_maintenance_hold();
 
@@ -1859,6 +1859,8 @@ pgcolumnar_compact(PG_FUNCTION_ARGS)
 				(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
 				 errmsg("table name cannot be null")));
 
+	PgColumnarRequireTableOwnerByOid(relid);
+
 	/* the lazy lock: concurrent reads and writes are allowed during compaction */
 	rel = table_open(relid, ShareUpdateExclusiveLock);
 
@@ -1871,8 +1873,6 @@ pgcolumnar_compact(PG_FUNCTION_ARGS)
 						RelationGetRelationName(rel))));
 	}
 
-	PgColumnarRequireTableOwner(rel);
-
 	/* self-heal a truncate crash-residual so the no-overlap assert holds eagerly,
 	 * even though compact does not reuse (see PgColumnarReconcileFreeList) */
 	PgColumnarReconcileFreeList(rel);

test/vacuum_lock_privilege.sh

@@ -81,6 +81,46 @@ check "vacuum_sorted refuses a non-owner before taking the exclusive lock" \
 check "cluster refuses a non-owner before taking the exclusive lock" \
 	"$(outcome_under_contention "SELECT pgcolumnar.cluster('victim'::regclass, 'id');")" "owner"
 
+# ---- #749: the same ordering for the five maintenance/projection entries -----
+#
+# add_projection takes ShareLock and the other four take
+# ShareUpdateExclusiveLock, all of which conflict with the AccessExclusiveLock
+# held above, so an unfixed caller queues exactly as vacuum did. Asserting only
+# the SQLSTATE would NOT be a removal proof: on unfixed main these calls are
+# also eventually refused, just after the lock request. Contention is what
+# separates the two orderings, which is the argument this file already makes.
+check "add_projection refuses a non-owner before taking its lock (#749)" \
+	"$(outcome_under_contention "SELECT pgcolumnar.add_projection('victim'::regclass, 'p1', ARRAY['id']);")" "owner"
+check "drop_projection refuses a non-owner before taking its lock (#749)" \
+	"$(outcome_under_contention "SELECT pgcolumnar.drop_projection('victim'::regclass, 'p1');")" "owner"
+check "recluster refuses a non-owner before taking its lock (#749)" \
+	"$(outcome_under_contention "SELECT pgcolumnar.recluster('victim'::regclass, 'id');")" "owner"
+check "compact_rewrite refuses a non-owner before taking its lock (#749)" \
+	"$(outcome_under_contention "SELECT pgcolumnar.compact_rewrite('victim'::regclass, 0.2);")" "owner"
+check "compact refuses a non-owner before taking its lock (#749)" \
+	"$(outcome_under_contention "SELECT pgcolumnar.compact('victim'::regclass);")" "owner"
+
+# A non-owner asking about a relation they do not own must learn only that they
+# are not the owner. drop_projection checked the relation TYPE first, so it
+# reported "is not a columnar table" for an arbitrary heap relation to a caller
+# with no rights to it, while the four entries beside it did not (#749 review).
+psql_run "CREATE TABLE heapvictim (id int);" >/dev/null 2>&1
+psql_run "REVOKE ALL ON heapvictim FROM PUBLIC;" >/dev/null 2>&1
+notowner_heap() {	# sql -> owner | typedisclosed | other
+	local out
+	out="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U t_unpriv \
+		-d "$PGC_DB" -Atq -v ON_ERROR_STOP=0 -c "$1" 2>&1)"
+	case "$out" in
+		*"must be owner"*|*"permission denied for"*) echo owner ;;
+		*"is not a columnar table"*) echo typedisclosed ;;
+		*) echo "other:${out:0:40}" ;;
+	esac
+}
+check "drop_projection tells a non-owner nothing about the relation type (#749)" \
+	"$(notowner_heap "SELECT pgcolumnar.drop_projection('heapvictim'::regclass, 'p1');")" "owner"
+check "compact does the same, which is the behaviour drop_projection now matches" \
+	"$(notowner_heap "SELECT pgcolumnar.compact('heapvictim'::regclass);")" "owner"
+
 # The caller must never have entered the lock queue: no ungranted request from it.
 check "the refused caller left no lock request queued on victim" \
 	"$(q "SELECT count(*) FROM pg_locks l JOIN pg_class c ON c.oid=l.relation JOIN pg_stat_activity a ON a.pid=l.pid WHERE c.relname='victim' AND a.usename='t_unpriv' AND NOT l.granted;")" "0"
@@ -93,4 +133,12 @@ wait "$holder_pid" 2>/dev/null || true
 check "the owner can still vacuum the table" \
 	"$(as_super -c "SELECT pgcolumnar.vacuum('victim'::regclass);" | grep -c 'ERROR')" "0"
 
+# The positive controls for the #749 arms. Without these, a guard that refused
+# every caller would satisfy every deny arm above.
+check "the owner can still compact the table (#749 control)" \
+	"$(as_super -c "SELECT pgcolumnar.compact('victim'::regclass);" | grep -c 'ERROR')" "0"
+check "the owner can still add and drop a projection (#749 control)" \
+	"$(as_super -c "SELECT pgcolumnar.add_projection('victim'::regclass, 'p1', ARRAY['id']);" \
+	           -c "SELECT pgcolumnar.drop_projection('victim'::regclass, 'p1');" | grep -c 'ERROR')" "0"
+
 pgc_summary