Re: Missing warning on revokes with grant options - Mailing list pgsql-hackers

On Thu, May 18, 2023 at 7:17 PM Joseph Koshakow <koshy44@gmail.com> wrote:
>
>    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));
>    }

Sorry about the unformatted code, here's the entire quoted section
again with proper formatting:

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