Re: WITH CHECK and Column-Level Privileges

On 30 September 2014 16:52, Stephen Frost <sfrost@snowman.net> wrote:
> * Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
>> On 29 September 2014 16:46, Stephen Frost <sfrost@snowman.net> wrote:
>> > * Robert Haas (robertmhaas@gmail.com) wrote:
>> >> Well, I think that's an acceptable approach from the point of view of
>> >> fixing the security exposure, but it's far from ideal.  Good error
>> >> messages are important for usability.  I can live with this as a
>> >> short-term fix, but in the long run I strongly believe we should try
>> >> to do better.
>>
>> Yes agreed, error messages are important, and longer term it would be
>> better to report on the specific columns the user has access to (for
>> all constraints), rather than the all-or-nothing approach of the
>> current patch.
>
> If the user only has column-level privileges on the table then I'm not
> really sure how useful the detail will be.
>

One of the main things that detail is useful for is identifying the
failing row in a multi-row update. In most real-world cases, I would
expect the column-level privileges to include the table's PK, so the
detail would meet that requirement. In fact the column-level
privileges would pretty much have to include sufficient columns to
usefully identify rows, otherwise updates wouldn't be practical.

>> However, for WCOs, I don't think the executor has the
>> information it needs to work out how to do that because it doesn't
>> even know which view the user updated, because it's not necessarily
>> the same one as failed the WCO.
>
> That's certainly an issue also.
>
>> > It certainly wouldn't be hard to add the same check around the WITH
>> > OPTION case that's around my proposed solution for the other issues-
>> > just check for SELECT rights on the underlying table.
>>
>> I guess that would work well for RLS, since the user would typically
>> have SELECT rights on the table they're updating, but I'm not
>> convinced it would do much good for auto-updatable views.
>
> I've been focusing on the 9.4 and back-branches concern, but as for RLS,
> if we're going to try and include the detail in this case then I'd
> suggest we do so only if the user has SELECT rights and RLS is disabled
> on the relation.

Huh? If RLS is disabled, isn't the check option also disabled?
 Otherwise, we'd have to check that the row being
> returned actually passes the SELECT policy.  I'm already not really
> thrilled that we are changing error message results based on user
> permissions, and that seems like it'd be worse.
>

That's one of the things I never liked about allowing different RLS
policies for different commands --- the idea that the user can UPDATE
a row that they can't even SELECT just doesn't make sense to me.

>> > Another question
>> > is if we could/should limit this to the UPDATE case.  With the INSERT
>> > case, any columns not provided by the user would be filled out by
>> > defaults, which can likely be seen in the catalog, or the functions in
>> > the catalog for the defaults or for any triggers might be able to be
>> > run by the user executing the INSERT anyway to see what would have been
>> > used in the resulting row.  I'm not completely convinced there's no risk
>> > there though..
>> >
>>
>> I think it's conceivable that a trigger could populate a column hidden
>> to the user with some confidential information, possibly pulled from
>> another table that the user doesn't have access to, so the fix has to
>> apply to INSERTs as well as UPDATEs.
>
> What do you think about returning just what the user provided in the
> first place in both of these cases..?  I'm not quite sure what that
> would look like in the UPDATE case but for INSERT (and COPY) it would be
> the subset of columns being inserted and the values originally provided.
> That may not be what the actual conflict is due to, as there could be
> before triggers changing things in the middle, or the conflict could be
> on default values, but at least the input row could be identified and
> there wouldn't be this information leak risk.  Not sure how difficult
> that would be to implement though.
>

I could see that working for INSERTs, but for UPDATEs I don't think
that would be very useful in practice, because the columns most likely
to be useful for identifying the failing row (e.g., key columns) are
also the columns least likely to have been updated.

Regards,
Dean