Re: Missing warning on revokes with grant options

On Wed, May 17, 2023 at 11:48 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
>    The thread for the aforementioned change [0] mentions the standard quite a
>    bit, which might explain the current behavior.

I went through that thread and the quoted parts of the SQL standard. It
seems clear that if a user tries to REVOKE some privilege and they
don't have a grant option on that privilege, then a warning should be
issued. There was some additional discussion on when there should be
an error vs a warning, but I don't think it's that relevant to this
discussion. However, I was not able to find any discussion about the
restriction that a revoker can only revoke privileges that they granted
themselves.

The restriction was added to PostgreSQL at the same time as GRANT
OPTIONs were introduced. The commit [0] and mailing thread [1] don't
provide much details on this specific restriction.

The SQL99 standard for REVOKE is very dense and I may have
misunderstood parts, but here's my interpretation of how this
restriction might come from the standard and what it says about issuing
a warning (section 12.6).

Let's start with the Syntax Rules:

    1) Let O be the object identified by the <object name> contained in <privileges>

In my example O is the table t.

    3) Let U be the current user identifier and R be the current role name.
    4) Case:
      a) If GRANTED BY <grantor> is not specified, then
        Case:
          i) If U is not the null value, then let A be U.
          ii) Otherwise, let A be R.

In my example A is the role r1.

    9) Case:
      a) If the <revoke statement> is a <revoke privileges statement>, then for every <grantee>
         specified, a set of privilege descriptors is identified. A privilege descriptor P is said to be
         identified if it belongs to the set of privilege descriptors that defined, for any <action>
         explicitly or implicitly in <privileges>, that <action> on O, or any of the objects in S, granted
         by A to <grantee>

In my example, <grantee> is the role r1, <privileges> is the list of
privileges that only contain SELECT, <action> is SELECT. Therefore the
set of identified privilege descriptors would be a single privilege
descriptor on table t where the privileges contain SELECT, the grantor
is r1, and the grantee is r1. Such a privilege does not exist, so the
identified privilege set is empty.

Now onto the General Rules:

    1) Case:
      a) If the <revoke statement> is a <revoke privilege statement>, then
        Case:
          i) If neither WITH HIERARCHY OPTION nor GRANT OPTION FOR is specified, then:
            2) The identified privilege descriptors are destroyed.

In my example, the identified set of privileges is empty, so no
privileges are destroyed (which I'm interpreting to mean the same thing
as revoked).

    18) If the <revoke statement> is a <revoke privileges statement>, then:
      a) For every combination of <grantee> and <action> on O specified in <privileges>, if there
         is no corresponding privilege descriptor in the set of identified privilege descriptors, then a
         completion condition is raised: warning — privilege not revoked.

In my example the identified privileges set is empty, therefore it
cannot contain a corresponding privilege descriptor, therefore we
should be issuing a warning.

So I think our current behavior is not in spec. Would you agree with
this evaluation or do you think I've misunderstood something?

>    > I wasn't able to locate where the check for
>    >> A user can only revoke privileges that were granted directly by that
>    >> user.
>    > is in the code, but we should probably just add a warning there.
>
>    І'm not certain, but I suspect the calls to aclupdate() in
>    merge_acl_with_grant() take care of this because the grantors will never
>    match.

I looked into this function and that is correct. We fail to find a
match for the revoked privilege here:

/*
* Search the ACL for an existing entry for this grantee and grantor. If
* one exists, just modify the entry in-place (well, in the same position,
* since we actually return a copy); otherwise, insert the new entry at
* the end.
*/

for (dst = 0; dst < num; ++dst)
{
if (aclitem_match(mod_aip, old_aip + dst))
{
/* found a match, so modify existing item */
new_acl = allocacl(num);
new_aip = ACL_DAT(new_acl);
memcpy(new_acl, old_acl, ACL_SIZE(old_acl));
break;
}
}

Seeing that there was no match, we add a new empty privilege to the end
of the existing ACL list here:

if (dst == num)
{
/* need to append a new item */
new_acl = allocacl(num + 1);
new_aip = ACL_DAT(new_acl);
memcpy(new_aip, old_aip, num * sizeof(AclItem));

/* initialize the new entry with no permissions */
new_aip[dst].ai_grantee = mod_aip->ai_grantee;
new_aip[dst].ai_grantor = mod_aip->ai_grantor;
ACLITEM_SET_PRIVS_GOPTIONS(new_aip[dst],
  ACL_NO_RIGHTS, ACL_NO_RIGHTS);
num++; /* set num to the size of new_acl */
}

We then try and revoke the specified privileges from the new empty
privilege, leaving it empty (modechg will equal ACL_MODECHG_DEL here):

old_rights = ACLITEM_GET_RIGHTS(new_aip[dst]);
old_goptions = ACLITEM_GET_GOPTIONS(new_aip[dst]);

/* apply the specified permissions change */
switch (modechg)
{
case ACL_MODECHG_ADD:
ACLITEM_SET_RIGHTS(new_aip[dst],
  old_rights | ACLITEM_GET_RIGHTS(*mod_aip));
break;
case ACL_MODECHG_DEL:
ACLITEM_SET_RIGHTS(new_aip[dst],
  old_rights & ~ACLITEM_GET_RIGHTS(*mod_aip));
break;
case ACL_MODECHG_EQL:
ACLITEM_SET_RIGHTS(new_aip[dst],
  ACLITEM_GET_RIGHTS(*mod_aip));
break;
}

Then since the new privilege remains empty, we remove it from the ACL
list:

new_rights = ACLITEM_GET_RIGHTS(new_aip[dst]);
new_goptions = ACLITEM_GET_GOPTIONS(new_aip[dst]);

/*
* If the adjusted entry has no permissions, delete it from the list.
*/
if (new_rights == ACL_NO_RIGHTS)
{
memmove(new_aip + dst,
new_aip + dst + 1,
(num - dst - 1) * sizeof(AclItem));
/* Adjust array size to be 'num - 1' items */
ARR_DIMS(new_acl)[0] = num - 1;
SET_VARSIZE(new_acl, ACL_N_SIZE(num - 1));
}

Thanks,
Joe Koshakow

[0] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ef7422510e93266e5aa9bb926d6747d5f2ae21f4
[1] https://www.postgresql.org/message-id/Pine.LNX.4.44.0301191916160.789-100000%40localhost.localdomain