BUG #17062: Assert failed in RemoveRoleFromObjectPolicy() on DROP OWNED policy applied to duplicate role

PG Bug reporting form <noreply@postgresql.org> writes:

When executing the following query:
CREATE USER role1;
CREATE TABLE t1(id int);
CREATE POLICY p1 ON t1 TO role1,role1 USING (true);
DROP OWNED BY role1;

The server halts with the failed assertion:

Nice. Seems to be that way at least as far back as 9.6, too.

regards, tom lane

I wroteL

PG Bug reporting form <noreply@postgresql.org> writes:

When executing the following query:
CREATE USER role1;
CREATE TABLE t1(id int);
CREATE POLICY p1 ON t1 TO role1,role1 USING (true);
DROP OWNED BY role1;

The server halts with the failed assertion:

Nice. Seems to be that way at least as far back as 9.6, too.

So the proximate problem is RemoveRoleFromObjectPolicy's unfounded
assumption that there are no duplicate OIDs in a pg_policy.polroles
entry. But that function has got some other serious problems too:

* Locking. It acquires lock on the policy's relation only after
it looks up the pg_policy entry. By that point the entry could
be gone or modified.

* Why is it expensively reconstructing the dependencies of the
policy expressions? Those aren't going to be changed by this
operation. AFAICS it ought to be sufficient to remove and
rebuild the policy's shared dependencies.

I wonder whether other operations on policies share either
of these issues.

regards, tom lane

I wrote:

So the proximate problem is RemoveRoleFromObjectPolicy's unfounded
assumption that there are no duplicate OIDs in a pg_policy.polroles
entry. But that function has got some other serious problems too:

While I'm whining ... that function's permissions checks seem
completely out of line too. How is it that, if I have the right
to drop some role, I lose that right if the role is mentioned in
a policy of some relation I don't own? It feels like this function
was written by copy-and-pasting a whole bunch of irrelevant logic.

regards, tom lane

On Thu, Jun 17, 2021 at 05:51:18PM -0400, Tom Lane wrote:

While I'm whining ... that function's permissions checks seem
completely out of line too. How is it that, if I have the right
to drop some role, I lose that right if the role is mentioned in
a policy of some relation I don't own? It feels like this function
was written by copy-and-pasting a whole bunch of irrelevant logic.

Hm. Wouldn't it be better to do something similar to 21378e1f where
we ensure that there are no duplicated role OIDs in the catalogs to
begin with, letting the drop code as it is now? The array build
happens in policy_role_list_to_array() for CREATE/ALTER POLICY, so
that's more than just a CCI. And I think that making the DROP OWNED
BY code more reliable with drops of the same role makes the handling
of shdepend entries a bit shaky. Back-branches would need to handle a
multi-role logic, now nobody has complained either about this bug
issue for years.

* Locking. It acquires lock on the policy's relation only after
it looks up the pg_policy entry. By that point the entry could
be gone or modified.

Yeah. The policy could be gone, or its relations, so it is possible
to hit two elog() here. rename_policy() does not take a lock on the
policy's relation for one.

* Why is it expensively reconstructing the dependencies of the
policy expressions? Those aren't going to be changed by this
operation. AFAICS it ought to be sufficient to remove and
rebuild the policy's shared dependencies.

For the shdepend entries dropped and then recreated from scratch, I
agree that this is a waste. That would be simple enough to fix if we
make shdepDropDependency() available, and simpler to call it in loop
with the roles dropped.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Thu, Jun 17, 2021 at 05:51:18PM -0400, Tom Lane wrote:

While I'm whining ... that function's permissions checks seem
completely out of line too. How is it that, if I have the right
to drop some role, I lose that right if the role is mentioned in
a policy of some relation I don't own? It feels like this function
was written by copy-and-pasting a whole bunch of irrelevant logic.

Hm. Wouldn't it be better to do something similar to 21378e1f where
we ensure that there are no duplicated role OIDs in the catalogs to
begin with, letting the drop code as it is now?

Well, we'd have to rewrite RemoveRoleFromObjectPolicy in the back
branches in any case. Furthermore, I don't think it's a great
idea for this code to just die with an Assert failure if someone
has modified the polroles array by hand. I think we should just
make it clean out however many matches there are.

Stepping back a bit, I suspect that the weird permission rules
here stem from not wanting to allow a policy to just disappear
without the table owner's involvement. There might be a fair
amount of intellectual investment in the USING and WITH CHECK
expressions, so I can sympathize with that point; but I don't
sympathize with allowing it to block an otherwise-allowed role
drop. It seems like we could resolve this tension if we allowed
the polroles array to go to empty rather than requiring the policy
to be dropped when that would happen. The main thing blocking
that is the need to be able to represent that situation in
CREATE/ALTER POLICY. Maybe it'd work to allow "TO NONE" for
that case? (I hasten to add that I'm envisioning that as a future
feature, not something to back-patch. I think in the back branches
we should just silently drop the policy.)

As far as the locking concerns go, I think if we get rid of the
unnecessary reprocessing of the expression dependencies, we don't
have to open or lock the associated table at all. The operation
would reduce to one UPDATE on the pg_policy row (plus maybe some
deletions of pg_shdepend rows), and I think the existing handling
of tuple update conflicts would then be good enough to cope with
concurrent alter/drop of the policy.

regards, tom lane

Greetings,

On Fri, Jun 18, 2021 at 14:18 Tom Lane <tgl@sss.pgh.pa.us> wrote:

Michael Paquier <michael@paquier.xyz> writes:

On Thu, Jun 17, 2021 at 05:51:18PM -0400, Tom Lane wrote:

While I'm whining ... that function's permissions checks seem
completely out of line too. How is it that, if I have the right
to drop some role, I lose that right if the role is mentioned in
a policy of some relation I don't own? It feels like this function
was written by copy-and-pasting a whole bunch of irrelevant logic.

Hm. Wouldn't it be better to do something similar to 21378e1f where
we ensure that there are no duplicated role OIDs in the catalogs to
begin with, letting the drop code as it is now?

Well, we'd have to rewrite RemoveRoleFromObjectPolicy in the back
branches in any case. Furthermore, I don't think it's a great
idea for this code to just die with an Assert failure if someone
has modified the polroles array by hand. I think we should just
make it clean out however many matches there are.

Stepping back a bit, I suspect that the weird permission rules
here stem from not wanting to allow a policy to just disappear
without the table owner's involvement. There might be a fair
amount of intellectual investment in the USING and WITH CHECK
expressions, so I can sympathize with that point; but I don't
sympathize with allowing it to block an otherwise-allowed role
drop. It seems like we could resolve this tension if we allowed
the polroles array to go to empty rather than requiring the policy
to be dropped when that would happen. The main thing blocking
that is the need to be able to represent that situation in
CREATE/ALTER POLICY. Maybe it'd work to allow "TO NONE" for
that case? (I hasten to add that I'm envisioning that as a future
feature, not something to back-patch. I think in the back branches
we should just silently drop the policy.)

I haven’t had a chance to delve into this but as far as the question above
goes- short answer is yes, there was generally an idea that we don’t want
policies just disappearing. Also- we don’t allow a role to be dropped when
there are GRANT’d privileges, users have to go REVOKE any privileges that
reference the role.

As far as the locking concerns go, I think if we get rid of the

unnecessary reprocessing of the expression dependencies, we don't
have to open or lock the associated table at all. The operation
would reduce to one UPDATE on the pg_policy row (plus maybe some
deletions of pg_shdepend rows), and I think the existing handling
of tuple update conflicts would then be good enough to cope with
concurrent alter/drop of the policy.

This does sound appealing.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

I haven’t had a chance to delve into this but as far as the question above
goes- short answer is yes, there was generally an idea that we don’t want
policies just disappearing. Also- we don’t allow a role to be dropped when
there are GRANT’d privileges, users have to go REVOKE any privileges that
reference the role.

But shouldn't DROP OWNED BY clean those out for you? If you've got
the right to get rid of the role, ISTM that that should certainly
include the right to get rid of grants to it.

regards, tom lane

On 2021-Jun-18, Tom Lane wrote:

Stephen Frost <sfrost@snowman.net> writes:

I haven’t had a chance to delve into this but as far as the question above
goes- short answer is yes, there was generally an idea that we don’t want
policies just disappearing. Also- we don’t allow a role to be dropped when
there are GRANT’d privileges, users have to go REVOKE any privileges that
reference the role.

But shouldn't DROP OWNED BY clean those out for you? If you've got
the right to get rid of the role, ISTM that that should certainly
include the right to get rid of grants to it.

Yeah, that's the intention of DROP OWNED BY.

--
Álvaro Herrera 39°49'30"S 73°17'W
"Crear es tan difícil como ser libre" (Elsa Triolet)

Greetings,

On Fri, Jun 18, 2021 at 14:37 Tom Lane <tgl@sss.pgh.pa.us> wrote:

Stephen Frost <sfrost@snowman.net> writes:

I haven’t had a chance to delve into this but as far as the question

above

goes- short answer is yes, there was generally an idea that we don’t want
policies just disappearing. Also- we don’t allow a role to be dropped

when

there are GRANT’d privileges, users have to go REVOKE any privileges that
reference the role.

But shouldn't DROP OWNED BY clean those out for you? If you've got
the right to get rid of the role, ISTM that that should certainly
include the right to get rid of grants to it.

Ah, yes, I misunderstood what was being suggested … ideally it would just
remove the role from the set and not blow away the entire policy though,
but then that gets to the point about a NONE option as you suggested since
you certainly wouldn’t want that policy to suddenly be as if it was
declared for PUBLIC.

Hrmpf. Makes it a bit awkward as you wouldn’t know, afterwards, what role
that policy HAD been for though. Perhaps just letting it be removed in
such a case is the better option, if it’s the only role remaining. That
would be in line with the GRANT system- it’s not like you can review what
ACLs a role had been given after a DROP OWNED BY has been run.

Thanks,

Stephen

On 2021-Jun-18, Stephen Frost wrote:

But shouldn't DROP OWNED BY clean those out for you? If you've got
the right to get rid of the role, ISTM that that should certainly
include the right to get rid of grants to it.

Ah, yes, I misunderstood what was being suggested … ideally it would just
remove the role from the set and not blow away the entire policy though,
but then that gets to the point about a NONE option as you suggested since
you certainly wouldn’t want that policy to suddenly be as if it was
declared for PUBLIC.

Could you just set the policy to be granted to "only the bootstrap
superuser" in that case? I mean as an implementation path for back
branches; use NONE going forward. That would make the policy allow
nobody who can't already access the record, instead of falling back to
PUBLIC -- which I agree seems suboptimal security-wise.

Hrmpf. Makes it a bit awkward as you wouldn’t know, afterwards, what role
that policy HAD been for though. Perhaps just letting it be removed in
such a case is the better option, if it’s the only role remaining. That
would be in line with the GRANT system- it’s not like you can review what
ACLs a role had been given after a DROP OWNED BY has been run.

Yeah, I think if you really wanted to keep track of changes, you would
have an auditing system that records them. Pity you can't build one
with event triggers (because these don't work for global objects).

--
Álvaro Herrera 39°49'30"S 73°17'W
"No hay hombre que no aspire a la plenitud, es decir,
la suma de experiencias de que un hombre es capaz"

Here's a stop-the-bleeding patch that just gets rid of the assertion
failure and the tuple-updated-by-self problem. I think this is
reasonable to slip in for beta2, although the other issues we
mentioned seem to require more thought.

(For ease of review, I've not re-pgindented.)

regards, tom lane

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

Could you just set the policy to be granted to "only the bootstrap
superuser" in that case? I mean as an implementation path for back
branches; use NONE going forward. That would make the policy allow
nobody who can't already access the record, instead of falling back to
PUBLIC -- which I agree seems suboptimal security-wise.

That doesn't seem like a great solution --- it would produce very
confusing output from pg_dump for instance. In fact, I think it
breaks pg_dump for cases where the target DB has a different
bootstrap superuser name.

regards, tom lane

Here's a draft patch that gets rid of all the seems-to-me-unnecessary
stuff in RemoveRoleFromObjectPolicy(). As I said before, the permission
check seems misguided, the rebuild of non-shared dependencies is a waste
of cycles, and the table locking is neither necessary nor well designed.
In this version, if somebody manages to commit a concurrent table drop,
policy change, etc, you just get a "tuple concurrently deleted" or
equivalent failure. That's much like most other DDL-conflict cases.

(I did not look to see if there's any documentation mentioning the
existing permission check. If there's not, I don't think we need any
doc changes, since this seems to me to be strictly less surprising
than the old behavior.)

There remains the question of whether we could preserve the policy
in the edge case of deleting the last role by letting its polroles go
to empty. But that's clearly not going to be a back-patchable change,
and I'm not terribly interested in working on it personally.

regards, tom lane

On Sun, Jun 20, 2021 at 04:15:08PM -0400, Tom Lane wrote:

Here's a draft patch that gets rid of all the seems-to-me-unnecessary
stuff in RemoveRoleFromObjectPolicy(). As I said before, the permission
check seems misguided, the rebuild of non-shared dependencies is a waste
of cycles, and the table locking is neither necessary nor well designed.
In this version, if somebody manages to commit a concurrent table drop,
policy change, etc, you just get a "tuple concurrently deleted" or
equivalent failure. That's much like most other DDL-conflict cases.

Hm. This version still removes all the shared dependencies and
rebuilds them from scratch for each role. Is that something you
intended to change?

(I did not look to see if there's any documentation mentioning the
existing permission check. If there's not, I don't think we need any
doc changes, since this seems to me to be strictly less surprising
than the old behavior.)

Don't see any.

There remains the question of whether we could preserve the policy
in the edge case of deleting the last role by letting its polroles go
to empty. But that's clearly not going to be a back-patchable change,
and I'm not terribly interested in working on it personally.

What would be the advantage of keeping the policy around anyway? The
code behaves like that since its introduction. If you decide to
change that, note that the current code would fail to empty
pg_policy.polroles as the array gets manipulated only when num_roles >
0. So this could leave around policies with roles part of it even
after some DROP OWNED query.

- if (!allowSystemTableMods && IsSystemRelation(rel))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system catalog",
- RelationGetRelationName(rel))));
-
- if (!allowSystemTableMods && IsSystemRelation(rel))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied: \"%s\" is a system
- catalog",
- RelationGetRelationName(rel))));
Right. This looks like a copy-paste coming from
RangeVarCallbackForPolicy() and this removes the need to lock the
relation of a policy.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Sun, Jun 20, 2021 at 04:15:08PM -0400, Tom Lane wrote:

Here's a draft patch that gets rid of all the seems-to-me-unnecessary
stuff in RemoveRoleFromObjectPolicy(). As I said before, the permission
check seems misguided, the rebuild of non-shared dependencies is a waste
of cycles, and the table locking is neither necessary nor well designed.
In this version, if somebody manages to commit a concurrent table drop,
policy change, etc, you just get a "tuple concurrently deleted" or
equivalent failure. That's much like most other DDL-conflict cases.

Hm. This version still removes all the shared dependencies and
rebuilds them from scratch for each role. Is that something you
intended to change?

I hadn't intended to mess with that, since the main point of this
patch was just to get rid of unnecessary code. It could be done as a
follow-on micro-optimization, if you like. You'd need to be sure
that the new coding gets rid of duplicate pg_shdepend entries if there
are any.

(I did not look to see if there's any documentation mentioning the
existing permission check. If there's not, I don't think we need any
doc changes, since this seems to me to be strictly less surprising
than the old behavior.)

Don't see any.

Thanks for looking!

There remains the question of whether we could preserve the policy
in the edge case of deleting the last role by letting its polroles go
to empty. But that's clearly not going to be a back-patchable change,
and I'm not terribly interested in working on it personally.

What would be the advantage of keeping the policy around anyway? The
code behaves like that since its introduction.

Just that if you wanted to add some new roles to the same policy,
you wouldn't have to remember the rest of the policy's details.

If you decide to
change that, note that the current code would fail to empty
pg_policy.polroles as the array gets manipulated only when num_roles >
0.

Sure, you'd get rid of that if-test, and for that matter the
function's return convention.

regards, tom lane

On Tue, Jun 22, 2021 at 11:10:42AM -0400, Tom Lane wrote:

I hadn't intended to mess with that, since the main point of this
patch was just to get rid of unnecessary code. It could be done as a
follow-on micro-optimization, if you like. You'd need to be sure
that the new coding gets rid of duplicate pg_shdepend entries if there
are any.

Okay. There is no urgency in that. You mentioned it, so I was just
wondering about it.

Just that if you wanted to add some new roles to the same policy,
you wouldn't have to remember the rest of the policy's details.

At the same time, nobody has complained about the current behavior
either, and what HEAD does now is the same behavior as one would have
for any objects using SHARED_DEPENDENCY_OWNER which is what DROP OWNED
BY ensures. So the existing forced deletion seems better kept as-is.
--
Michael