Subject: Re: Clarification on Role Access Rights to Table Indexes

From: Robert Haas <[email-masked]>
Date: Tue, 18 Feb 2025 11:21:33 -0500

On Tue, Feb 18, 2025 at 11:02 AM Tom Lane <[email-masked]> wrote:
> Is that a +1 for the specific design of "check SELECT on the index's
> table", or just a +1 for changing something here?

That is a +1 for the specific design of "check SELECT on the index's
table". I don't want to be closed-minded: if you have some strong
reason for believing that's the wrong thing to do, I'm all ears.
However, I'm presently of the view that it is exactly the right thing
to do, to the point where I don't currently understand why there's
anything to think about here.

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

---

From: Tom Lane <[email-masked]>
Date: Tue, 18 Feb 2025 11:30:02 -0500

> anything to think about here.

I have no objection to it, but I wasn't as entirely convinced
as you are that it's the only plausible answer.

One specific thing I'm slightly worried about is that a naive
implementation would probably cause this function to lock the
table after the index, risking deadlock against queries that
take the locks in the more conventional order.  I don't recall
what if anything we've done about that in other places
(-ENOCAFFEINE).

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Tue, 18 Feb 2025 13:16:24 -0500

> as you are that it's the only plausible answer.

Hmm, OK.

> what if anything we've done about that in other places

Yeah, that seems like a good thing to worry about from an
implementation point of view but it doesn't seem like a reason to
question the basic design choice. In general, if you can use a table,
you also get to use its indexes, so that interpretation seems natural
to me here, also. Now, if somebody finds a problem with requiring only
SELECT permission, I could see changing the requirements for both
tables and indexes, but I find it harder to imagine that we'd want
those things to work differently from each other. Of course I'm
willing to be convinced that there's a good reason for them to be
different; I just can't currently imagine what it might be.

---

From: Ayush Vatsa <[email-masked]>
Date: Wed, 19 Feb 2025 15:53:48 +0530

Hello Everyone,
It seems there's a general consensus that we should maintain a
original design to support pg_prewarm, with a minor adjustment:
when querying indexes, we should verify the privileges of the parent table.

I’ve attached a patch for this, which includes some test cases as well.
Let me know if it needs any changes.

Regards,
Ayush Vatsa
SDE AWS

---

From: Ayush Vatsa <[email-masked]>
Date: Wed, 19 Feb 2025 16:01:32 +0530

Added the CF entry for the same -
https://commitfest.postgresql.org/patch/5583/

Regards,
Ayush Vatsa
SDE AWS

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 4 Mar 2025 14:01:28 -0600

On Wed, Feb 19, 2025 at 03:53:48PM +0530, Ayush Vatsa wrote:
> I´ve attached a patch for this, which includes some test cases as well.

+        tableOid = IndexGetRelation(relOid, false);
+        aclresult = pg_class_aclcheck(tableOid, GetUserId(), ACL_SELECT);

I'm wondering whether setting missing_ok to true is correct here.  IIUC we
should have an AccessShareLock on the index, but I don't know if that's
enough protection.  The only other similar coding pattern I'm aware of is
RangeVarCallbackForReindexIndex(), which sets missing_ok to false and
attempts to gracefully handle a missing table.  Of course, maybe that's
wrong, too.

Perhaps it's all close enough in practice.  If we get it wrong, you might
get a slightly less helpful error message when the table is concurrently
dropped, which isn't so bad.

-- 
nathan

---

From: Ayush Vatsa <[email-masked]>
Date: Sat, 8 Mar 2025 20:34:40 +0530

> enough protection.

Since we are already opening the relation with rel = relation_open(relOid,
AccessShareLock);,
if relOid does not exist, it will throw an error. If it does exist, we
acquire an AccessShareLock,
preventing it from being dropped.

By the time we reach IndexGetRelation(), we can be confident that relOid
exists and is
protected by the lock. Given this, it makes sense to keep missing_ok = false
here.

Let me know if you agree or if you see any scenario where
missing_ok = true would be preferable—I can update the condition
accordingly.

Thanks!
Ayush Vatsa
SDE AWS

---

From: Nathan Bossart <[email-masked]>
Date: Sat, 8 Mar 2025 15:08:02 -0600

On Sat, Mar 08, 2025 at 08:34:40PM +0530, Ayush Vatsa wrote:
> missing_ok = true would be preferable-I can update the condition
> accordingly.

Right, we will have a lock on the index, but my concern is that we won't
have a lock on its table.  I was specifically concerned that a concurrent
DROP TABLE could cause IndexGetRelation() to fail, i.e., emit a gross
"cache lookup failed" error.  From a quick test and skim of the relevant
code, I think your patch is fine, though.  IndexGetRelation() retrieves the
table OID from pg_index, so the OID should definitely be valid.  And IIUC
DROP TABLE first acquires a lock on the table and its dependent objects
(e.g., indexes) before any actual deletions, so AFAICT there's no problem
with using it in pg_class_aclcheck() and get_rel_name(), either.

-- 
nathan

---

From: Ayush Vatsa <[email-masked]>
Date: Sun, 9 Mar 2025 03:01:41 +0530

> From a quick test and skim of the relevant
> code, I think your patch is fine, though
Thanks for reviewing.

> And IIUC
True, I have also verified that from [1], hence I think we are safe here.
Maybe we can move ahead with the patch if we can see no other concerns.

[1]
https://github.com/postgres/postgres/blob/master/src/backend/catalog/dependency.c#L398-L430

Thanks,
Ayush Vatsa
SDE AWS

---

From: Nathan Bossart <[email-masked]>
Date: Sat, 8 Mar 2025 15:57:25 -0600

> Maybe we can move ahead with the patch if we can see no other concerns.

I think we should allow some time in case others want to review the patch.
I do see a concern upthread about increased deadlock risk [0], but your
patch doesn't lock the table, but unless I'm wrong [1] (which is always
possible), it doesn't need to lock it.

Anyway, here is a tidied up patch.

[0] https://postgr.es/m/1246906.1739896202%40sss.pgh.pa.us
[1] https://postgr.es/m/Z8yxsm9ZWVkHlPbV%40nathan

-- 
nathan

---

From: Tom Lane <[email-masked]>
Date: Sat, 08 Mar 2025 17:17:40 -0500

> possible), it doesn't need to lock it.

It bothers me a bit that this proposes to do something as complicated
as pg_class_aclcheck on a table we have no lock on.  As you say, the
lock we hold on the index would prevent DROP TABLE, but that doesn't
mean we won't have any issues with other DDL on the table.  Still,
taking a lock would be bad because of the deadlock hazard, and I
think the potential for conflicts with concurrent DDL is nonzero in
a lot of other places.  So I don't have any concrete reason to object.

ReindexIndex() faces this same problem and solves it with some
very complex code that manages to get the table's lock first.
But I see that it's also doing pg_class_aclcheck on a table
it hasn't locked yet, so I don't think that adopting its approach
would do anything useful for us here.

			regards, tom lane

---

From: Nathan Bossart <[email-masked]>
Date: Sat, 8 Mar 2025 19:35:18 -0600

> would do anything useful for us here.

I noticed that amcheck's bt_index_check_internal() handles this problem,
and I think that approach could be adapted here:

	relkind = get_rel_relkind(relOid);
	if (relkind == RELKIND_INDEX || relkind == RELKIND_PARTITIONED_INDEX)
	{
		permOid = IndexGetRelation(relOid, true);
		if (OidIsValid(permOid))
			LockRelationOid(permOid, AccessShareLock);
		else
			fail = true;
	}
	else
		permOid = relOid;

	rel = relation_open(relOid, AccessShareLock);
	if (fail ||
		(permOid != relOid && permOid != IndexGetRelation(relOid, false)))
		ereport(ERROR,
				(errcode(ERRCODE_UNDEFINED_TABLE),
				 errmsg("could not find parent table of index \"%s\"",
						RelationGetRelationName(rel))));

	aclresult = pg_class_aclcheck(permOid, GetUserId(), ACL_SELECT);
	if (aclresult != ACLCHECK_OK)
		aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));

	if (permOid != relOid)
		UnlockRelationOid(permOid, AccessShareLock);

stats_lock_check_privileges() does something similar, but it's not as
cautious about the "heapid != IndexGetRelation(indrelid, false)" race
condition.  Maybe RangeVarCallbackForReindexIndex() should be smarter about
this, too.  That being said, this is a fair amount of complexity to handle
something that is in theory extremely rare...

-- 
nathan

---

From: Tom Lane <[email-masked]>
Date: Sun, 09 Mar 2025 11:48:05 -0400

> ...
> condition.

Egad, we've already got three inconsistent implementations of this
functionality?  I think the first step must be to unify them into
a common implementation, if at all possible.

			regards, tom lane

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 10 Mar 2025 10:15:19 -0500

> a common implementation, if at all possible.

Agreed.  I worry that trying to unify each bespoke implementation into a
single function might result in an unwieldy mess, but I'll give it a
shot...

-- 
nathan

---

From: vignesh C <[email-masked]>
Date: Sun, 16 Mar 2025 18:30:23 +0530

> Anyway, here is a tidied up patch.

I noticed that Tom Lane's comment from [1] is not addressed. I'm
changing the commitfest entry status to Waiting on Author, Please
address them and update the status to Needs Review.
[1] - https://www.postgresql.org/message-id/279947.1741535285%40sss.pgh.pa.us

Regards,
Vignesh

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 24 Sep 2025 10:58:25 -0500

> single function might result in an unwieldy mess, but I'll give it a

I tried to unify these, but each one seems to be just different enough to
make it not worth the trouble.  Instead, I took a look at each
implementation:

* amcheck's amcheck_lock_relation_and_check() seems correct to me.

* stats_lock_check_privileges() appears to be missing the second
IndexGetRelation() check after locking the table and index, so I added
that in 0001.  Since this code is new to v18, I proposed to back-patch 0001
there.

* RangeVarCallbackForReindexIndex() was checking privileges on the table
before locking it, so I reversed it in 0002.  Interestingly, this caused
test errors because LockRelationOid() checks for invalidation messages, so
the pg_class_aclcheck() call started failing with unhelpful errors due to
concurrently dropped relations.  To deal with that, I switched to
pg_class_aclcheck_ext() so that we can handle missing relations.
Furthermore, I noticed that this callback seems to assume that as long as
the index does not change between calls, its table won't, either.  That's
probably always true in practice, but even if it's completely true, I see
no reason to rely on it.  So, I simplified the code to unconditionally
unlock any previously-locked table and to lock whatever IndexGetRelation()
returns.  This could probably be back-patched, but in the absence of any
reports or any reproducible bugs, I don't think we should.

* 0003 fixes pg_prewarm's privilege checks by following a similar pattern.
This probably ought to get back-patched to all supported versions.

-- 
nathan

---

From: Tom Lane <[email-masked]>
Date: Wed, 24 Sep 2025 12:13:34 -0400

Nathan Bossart <[email-masked]> writes:
> before locking it, so I reversed it in 0002.

Don't we do that intentionally, to make sure someone can't cause DOS
on a table they have no privileges on?

			regards, tom lane

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 24 Sep 2025 11:52:09 -0500

> on a table they have no privileges on?

Ah, right.  I switched it back in v4.

-- 
nathan

---

From: Jeff Davis <[email-masked]>
Date: Mon, 13 Oct 2025 11:16:36 -0700

> on a table they have no privileges on?

Is this only a problem for strong locks (ShareLock or greater)?

Strong locks are a problem when you have a pattern like a long running
query that holds an AccessShareLock, and then an unprivileged user
requests an AccessExclusiveLock, forcing other queries to queue up
behind it, and the queue doesn't clear until the long running query
finishes.

But weaker locks don't seem to have that problem, right?

Regards,
	Jeff Davis

---

From: Tom Lane <[email-masked]>
Date: Mon, 13 Oct 2025 17:21:07 -0400

> But weaker locks don't seem to have that problem, right?

I don't think so.  Even AccessShareLock is enough to block another
session trying to acquire AccessExclusiveLock, and then not only
have you DoS'd that session, but everything else trying to access
the table will queue up behind the AccessExclusiveLock request.
So it's only not-a-problem if nothing anywhere in the system wants
non-sharable locks.

			regards, tom lane

---

From: Jeff Davis <[email-masked]>
Date: Mon, 13 Oct 2025 15:11:38 -0700

> So it's only not-a-problem if nothing anywhere in the system wants

I tried imagining how that could be a problem, but couldn't come up
with anything. If the privilege check is right after the lock, then
either:

(a) The malicious AccessShareLock is granted, then is quickly released
when the privilege check fails and the transaction aborts; or

(b) The malicious AccessShareLock is queued behind a legitimate
AccessExclusiveLock, in which case every other lock would be queued up
as well. As soon as the AccessExclusiveLock is released, the
AccessShareLock would be granted, but quickly released when the
privilege check fails.

For it to be a problem, the malicious lock needs to be strong enough to
conflict with a lock level weaker than itself, i.e. ShareLock or
stronger.

I'm not sure we save anything by being lazier for weaker lock levels,
so perhaps the point is irrelevant. But if I'm missing something,
please let me know.

Regards,
	Jeff Davis

---

From: Tom Lane <[email-masked]>
Date: Tue, 14 Oct 2025 12:30:26 -0400

> conflict with a lock level weaker than itself, i.e. ShareLock or

Robert might remember better, but I think the assumption behind
the current design of RangeVarGetRelidExtended is that it's not
okay to take a lock in the first place if you don't have the
privilege to do so.  Your analysis here supposes that it's okay
to take a lock without privileges so long as you can't block someone
else for very long, where "very long" is not tightly defined but
hopefully isn't controllable by the malicious user.  So that's
moving the goalposts somewhat, but you might get people to sign
onto it with more careful analysis of the worst-case delay.
(The thing I'd worry about is whether it's possible to block
execution of the privilege check, or even just make it slow.)

Given that definition, I think you're right that it's possible to
identify cases where lock-then-check can't cause meaningful DoS.
RangeVarGetRelidExtended has to cope with the general case, so
that's not an argument for simplifying it, but we might not need
equivalent complexity everywhere.

			regards, tom lane