Обсуждение: "has_column_privilege()" issue with attnums and non-existent columns

Greetings

Consider a table set up as follows:

    CREATE TABLE foo (id INT, val TEXT);
    ALTER TABLE foo DROP COLUMN val;

When specifying the name of a dropped or non-existent column, the function
"has_column_privilege()" works as expected:

    postgres=# SELECT has_column_privilege('foo', 'val', 'SELECT');
    ERROR:  column "val" of relation "foo" does not exist

    postgres=# SELECT has_column_privilege('foo', 'bar', 'SELECT');
    ERROR:  column "bar" of relation "foo" does not exist

However when specifying a dropped or non-existent attnum, it returns
"TRUE", which is unexpected:

    postgres=# SELECT has_column_privilege('foo', 2::int2, 'SELECT');
     has_column_privilege
    ----------------------
     t
    (1 row)

    postgres=# SELECT has_column_privilege('foo', 999::int2, 'SELECT');
     has_column_privilege
    ----------------------
     t
    (1 row)

The comment on the relevant code section in "src/backend/utils/adt/acl.c"
(related to "column_privilege_check()") indicate that NULL is the intended
return value in these cases:

     Likewise, the variants that take an integer attnum
     return NULL (rather than throwing an error) if there is no such
     pg_attribute entry.  All variants return NULL if an attisdropped
     column is selected.

The unexpected "TRUE" value is a result of "column_privilege_check()" returning
TRUE if the user has table-level privileges.  This returns a valid result with
the function variants where the column name is specified, as the calling
function will have already performed a check of the column through its call to
"convert_column_name()".  However when the attnum is specified, the status of
the column never gets checked.

Attached patch resolves this by having "column_privilege_check()" callers
indicate whether they have checked the column. If this is the case, and if the
user as table-level privileges, "column_privilege_check()" can return as before
with no further lookups needed.

If the user has table-level privileges but the column was not checked (i.e
attnum provided), the column lookup is performed.

The regression tests did contain checks for dropped/non-existent attnums,
however one group was acting on a table where the user had no table-level
privilege, giving a fall-through to the column-level check; the other group
contained a check which was just plain wrong.

This issue appears to have been present since "has_column_privilege()" was
introduced in PostreSQL 8.4 (commit 7449427a1e6a099bc7e76164cb99a01d5e87237b),
so falls into the "obscure, extreme corner-case" category, OTOH seems worth
backpatching to supported versions.

The second patch adds a bunch of missing static prototypes to "acl.c",
on general
principles.

I'll add this to the next commitfest.


Regards

Ian Barwick


-- 
EnterpriseDB: https://www.enterprisedb.com

On 2021-01-12 06:53, Ian Lawrence Barwick wrote:
>      postgres=# SELECT has_column_privilege('foo', 999::int2, 'SELECT');
>       has_column_privilege
>      ----------------------
>       t
>      (1 row)
> 
> The comment on the relevant code section in "src/backend/utils/adt/acl.c"
> (related to "column_privilege_check()") indicate that NULL is the intended
> return value in these cases:
> 
>       Likewise, the variants that take an integer attnum
>       return NULL (rather than throwing an error) if there is no such
>       pg_attribute entry.  All variants return NULL if an attisdropped
>       column is selected.
> 
> The unexpected "TRUE" value is a result of "column_privilege_check()" returning
> TRUE if the user has table-level privileges.  This returns a valid result with
> the function variants where the column name is specified, as the calling
> function will have already performed a check of the column through its call to
> "convert_column_name()".  However when the attnum is specified, the status of
> the column never gets checked.

I'm not convinced the current behavior is wrong.  Is there some 
practical use case that is affected by this behavior?

> The second patch adds a bunch of missing static prototypes to "acl.c",
> on general
> principles.

Why is this necessary?

On 1/29/21 12:13 AM, Ian Lawrence Barwick wrote:
> 2021年1月28日(木) 17:18 Peter Eisentraut:
>     I'm not convinced the current behavior is wrong.  Is there some
>     practical use case that is affected by this behavior?
>
>  
> I was poking around at the function with a view to using it for something and was
> curious what it did with bad input.
>
> Providing the column name of a dropped column:
>
>     Me: "Hey Postgres, do I have privileges on the dropped column 'bar' of my
> table 'foo'?"
>     Pg: "That column doesn't even exist - here, have an error instead."
>     Me: "Hey Postgres, does some other less-privileged user have privileges on the
>          dropped column 'bar' of my table 'foo'?
>     Pg: "That column doesn't even exist - here, have an error instead."
>
> Providing the attnum of a dropped column:
>
>     Me: "Hey Postgres, here's the attnum of the dropped column 'bar', does some
>          other less-privileged user have privileges on that column?"
>     Pg: "That column doesn't even exist - here, have a NULL".
>     Me: "Hey Postgres, here's the attnum of the dropped column 'bar' on this table
>          I own, do I have privileges on that column?"
>     Pg: "Yup. And feel free to throw any other smallint at me, I'll pretend that
>          represents a column too even if it never existed.".
>
> Looking at the code, particularly the cited comment, it does seems the intent was
> to return NULL in all cases where an invalid attnum was provided, but that gets
> short-circuited by the assumption table owner = has privilege on any column.

Nicely illustrated :-)

> Not the most urgent or exciting of issues, but seems inconsistent to me.

+1

Joe


--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On 3/16/21 2:45 PM, Joe Conway wrote:
> Ian, or anyone else, any comments/complaints on my changes? If not I will commit
> and push that version sooner rather than later.

Any thoughts on back-patching this?

On one hand, in my view it is clearly a bug. On the other hand, no one has 
complained before and this does change user visible behavior.

I guess I am leaning toward not back-patching...

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Joe Conway <mail@joeconway.com> writes:
> On 3/16/21 2:45 PM, Joe Conway wrote:
>> Ian, or anyone else, any comments/complaints on my changes? If not I will commit
>> and push that version sooner rather than later.

I took a quick look, and I'm afraid I don't believe this:

!      * We have to check for dropped attribute first, and we rely on the
!      * syscache not to notice a concurrent drop before pg_attribute_aclcheck
!      * fetches the row.

What the existing code is assuming is that if we do a successful
SearchSysCache check, then an immediately following pg_xxx_aclcheck call
that needs to examine that same row will also find that row in the cache,
because there will be no need for any actual database traffic to do that.

What you've done here is changed that pattern to be

    SearchSysCache(row1)

    SearchSysCache(row2)

    aclcheck on row1

    aclcheck on row2

But that does NOT offer any such guarantee, because the row2 cache lookup
could incur a database access, which might notice that row1 has been
deleted.

I think we may have to adjust the acl.c APIs, or maybe better provide new
entry points, so that we can have variants of pg_xxx_aclcheck that won't
throw a hard error upon not finding the row.  We cheesily tried to avoid
adjusting those APIs to support the semantics we need here, and we now see
that it didn't really work.

            regards, tom lane

On 3/21/21 12:27 PM, Tom Lane wrote:
> I think we may have to adjust the acl.c APIs, or maybe better provide new
> entry points, so that we can have variants of pg_xxx_aclcheck that won't
> throw a hard error upon not finding the row.  We cheesily tried to avoid
> adjusting those APIs to support the semantics we need here, and we now see
> that it didn't really work.

Ok, I took a shot at that; see attached.

Questions:

1. I confined the changes to just pg_class_aclcheck/mask
    and pg_attribute_aclcheck/mask -- did you intend
    that we do this same change across the board? Or
    perhaps do the rest of them once we open up pg15
    development?

2. This seems more invasive than something we would want
    to back patch -- agreed?

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Joe Conway <mail@joeconway.com> writes:
> On 3/21/21 12:27 PM, Tom Lane wrote:
>> I think we may have to adjust the acl.c APIs, or maybe better provide new
>> entry points, so that we can have variants of pg_xxx_aclcheck that won't
>> throw a hard error upon not finding the row.  We cheesily tried to avoid
>> adjusting those APIs to support the semantics we need here, and we now see
>> that it didn't really work.

> Ok, I took a shot at that; see attached.

Looks generally reasonable by eyeball.  The lack of any documentation
comment for the new functions makes me itch, although the ones that
are there are hardly better.

> 1. I confined the changes to just pg_class_aclcheck/mask
>     and pg_attribute_aclcheck/mask -- did you intend
>     that we do this same change across the board? Or
>     perhaps do the rest of them once we open up pg15
>     development?

In principle, it might be nice to fix all of those functions in acl.c
to be implemented similarly --- you could get rid of the initial
SearchSysCacheExists calls in the ones that are trying not to throw
error for is-missing cases.  In practice, as long as there's no
reachable bug for the other cases, there are probably better things
to spend time on.

> 2. This seems more invasive than something we would want
>     to back patch -- agreed?

You could make an argument either way, but given the limited number
of complaints about this, I'd lean to no back-patch.

            regards, tom lane

On 3/30/21 3:37 PM, Joe Conway wrote:
> On 3/21/21 12:27 PM, Tom Lane wrote:
>> I think we may have to adjust the acl.c APIs, or maybe better provide new
>> entry points, so that we can have variants of pg_xxx_aclcheck that won't
>> throw a hard error upon not finding the row.  We cheesily tried to avoid
>> adjusting those APIs to support the semantics we need here, and we now see
>> that it didn't really work.
> 
> Ok, I took a shot at that; see attached.

Heh, I missed the forest for the trees it seems.

That version undid the changes fixing what Ian was originally complaining about.

New version attached that preserves the fixed behavior.

> Questions:
> 1. I confined the changes to just pg_class_aclcheck/mask
>      and pg_attribute_aclcheck/mask -- did you intend
>      that we do this same change across the board? Or
>      perhaps do the rest of them once we open up pg15
>      development?
> 
> 2. This seems more invasive than something we would want
>      to back patch -- agreed?

The questions remain...

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Joe Conway <mail@joeconway.com> writes:
> Heh, I missed the forest for the trees it seems.
> That version undid the changes fixing what Ian was originally complaining about.

Duh, right.  It would be a good idea for there to be a code comment
explaining this, because it's *far* from obvious.  Say like

    * Check for column-level privileges first.  This serves in
    * part as a check on whether the column even exists, so we
    * need to do it before checking table-level privilege.

My gripe about providing API-spec comments for the new aclchk.c
entry points still stands.  Other than that, I think it's good
to go.

            regards, tom lane

On 3/30/21 6:22 PM, Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
>> Heh, I missed the forest for the trees it seems.
>> That version undid the changes fixing what Ian was originally complaining about.
> 
> Duh, right.  It would be a good idea for there to be a code comment
> explaining this, because it's *far* from obvious.  Say like
> 
>     * Check for column-level privileges first.  This serves in
>     * part as a check on whether the column even exists, so we
>     * need to do it before checking table-level privilege.

Will do.

> My gripe about providing API-spec comments for the new aclchk.c
> entry points still stands.  Other than that, I think it's good
> to go.

Yeah, I was planning to put something akin to this in all four spots:
8<-------------------
/*
  * Exported routine for checking a user's access privileges to a table
  *
  * Does the bulk of the work for pg_class_aclcheck(), and allows other
  * callers to avoid the missing relation ERROR when is_missing is non-NULL.
  */
AclResult
pg_class_aclcheck_ext(Oid table_oid, Oid roleid,
                      AclMode mode, bool *is_missing)
...
8<-------------------

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On 3/30/21 8:17 PM, Joe Conway wrote:
> On 3/30/21 6:22 PM, Tom Lane wrote:
>> Joe Conway <mail@joeconway.com> writes:
>>> Heh, I missed the forest for the trees it seems.
>>> That version undid the changes fixing what Ian was originally complaining about.
>> 
>> Duh, right.  It would be a good idea for there to be a code comment
>> explaining this, because it's *far* from obvious.  Say like
>> 
>>     * Check for column-level privileges first.  This serves in
>>     * part as a check on whether the column even exists, so we
>>     * need to do it before checking table-level privilege.
> 
> Will do.
> 
>> My gripe about providing API-spec comments for the new aclchk.c
>> entry points still stands.  Other than that, I think it's good
>> to go.
> 
> Yeah, I was planning to put something akin to this in all four spots:
> 8<-------------------
> /*
>    * Exported routine for checking a user's access privileges to a table
>    *
>    * Does the bulk of the work for pg_class_aclcheck(), and allows other
>    * callers to avoid the missing relation ERROR when is_missing is non-NULL.
>    */
> AclResult
> pg_class_aclcheck_ext(Oid table_oid, Oid roleid,
>                       AclMode mode, bool *is_missing)
> ...
> 8<-------------------


Pushed that way.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development