Re: "has_column_privilege()" issue with attnums and non-existent columns

On 3/3/21 9:43 AM, Joe Conway wrote:
> On 3/3/21 8:50 AM, David Steele wrote:
>> On 1/29/21 4:56 AM, Joe Conway wrote:
>>> 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
>>
>> Peter, did Ian's explanation answer your concerns?
>>
>> Joe (or Peter), any interest in reviewing/committing this patch?
>
> Sure, I'll take a look

Based on Tom's commit here:

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=3d0f68dd3

it appears to me that the intent is to return NULL.

However I was not to happy with the way the submitted patch added an argument to
column_privilege_check(). It seems to me that it is better to just reorder the
checks in column_privilege_check() a bit, as in the attached.

I wasn't entirely sure it was ok to split up the check for dropped attribute and
pg_attribute_aclcheck like I did, but when I tested the race condition (by
pausing at pg_attribute_aclcheck and dropping the column in another session)
everything seemed to work fine.

Any comments?

Thanks,

Joe

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