pg_auth_members.grantor is bunk

On Fri, Mar 4, 2022 at 4:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> If we are not tracking the grantors of role authorizations,
> then we are doing it wrong and we ought to fix that.

We are definitely doing it wrong. It's not that we aren't doing it at
all, but we are doing it incorrectly. If user foo executes "GRANT foo
TO bar GRANTED BY quux", pg_auth_members.grantor gets the OID of role
"quux". Without the "GRANTED BY" clause, it gets the OID of role
"foo". But no dependency is created; therefore, the OID of that column
can point to a role that no longer exists, or potentially to a role
that did exist at one point, was dropped, and was later replaced by
some other role that happens to get the same OID. pg_dump handles this
by dumping "GRANTED BY whoever" if pg_auth_members.grantor is a
still-extant role and omitting the clause if not. This would be a
security vulnerability if there were any logic in the backend that
actually did anything with pg_auth_members.grantor, because if the
original grantor is removed and replaced by another role with the same
OID, a dump/restore could change the notional grantor. Since there is
no such logic, I don't think it's insecure, but it's still really
lame.

So let's talk about how we could fix this. In a vacuum I'd say this is
just a feature that never got finished and we should rip the whole
thing out. That is, remove pg_auth_members.grantor entirely and at
most keep some do-nothing syntax around for backward compatibility.
However, what Tom is saying in the text quoted above is that we ought
to have something that actually works, which is more challenging.
Apparently, the desired behavior here is for this to work like grants
on non-role objects, where executing "GRANT SELECT ON TABLE s1 TO foo"
under two different user accounts bar and baz that both have
permissions to grant that privilege creates two independent grants
that can be independently revoked. To get there, we'll have to change
a good few things -- not only will we need a dependency to prevent a
grantor from being dropped without revoking the grant, but we need to
change the primary key of pg_auth_members from (roleid, member) to
(roleid, member, grantor). Then we'll also have to change the behavior
of the GRANT and REVOKE commands at the SQL level, and also the
behavior of pg_dump, which will need to dump and restore all grants.

I'm open to other proposals, but my thought is that it might be
simplest to try to clean this up in two steps. In step one, the only
goal would be to make pg_auth_members.grantor reliably sane. In other
words, we'd add a dependency on the grantor when a role is granted to
another role. You could still only have one grant of role A to role B,
but the notional grantor C would always be a user that actually
exists. I suspect it would be a really good idea to also patch pg_dump
to not ever dump the grantor when working from an older release,
because the information is not necessarily reliable and I fear that
propagating it forward could lead to broken stuff or maybe even
security hazards as noted above. Then, in step two, we change things
around to allow multiple grants of the same role to the same other
role, one per grantor. Now you've achieved parity between the behavior
we have for roles and the behavior we have for permissions on other
kinds of SQL objects.

There may be other improvements we want to make in this area -
previous discussions have suggested various ideas - but it seems to me
that making the behavior sane and consistent with other types of
objects would be a good start. That way, if we decide we do want to
change anything else, we will be starting from a firm foundation,
rather than building on sand.

Thoughts?

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