Re: Fwd: [Patch Review] TRUNCATE Permission

Hello all,

Robert Haas submitted the TRUNCATE permissions patch for the September
commit fest:
http://archives.postgresql.org/message-id/603c8f070807261444s133fb281sf34d069ab5b4c0b@mail.gmail.com

I had some extra time tonight, so I downloaded, installed and reviewed this
patch.
Here are my findings:

1. I found the patch style to be consistent with the surrounding code.
2. The patch provides documentation updates and regression test updates.
3. The patch applies (with some fuzz) to the latest GIT tree.

Three issues I found with the patch via code reading and verified via
testing:

1. File: src/backend/catalog/aclchk.c:
Function: pg_class_aclmask():

I believe the ACL_TRUNCATE trigger should be added to this check and mask.

/*
* Deny anyone permission to update a system catalog unless
* pg_authid.rolcatupdate is set. (This is to let superusers
protect
* themselves from themselves.) Also allow it if
allowSystemTableMods.
*
* As of 7.4 we have some updatable system views; those shouldn't be
* protected in this way. Assume the view rules can take care of
* themselves. ACL_USAGE is if we ever have system sequences.
*/
if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_USAGE)) &&
IsSystemClass(classForm) &&
classForm->relkind != RELKIND_VIEW &&
!has_rolcatupdate(roleid) &&
!allowSystemTableMods)
{
#ifdef ACLDEBUG
elog(DEBUG2, "permission denied for system catalog update");
#endif
mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_USAGE);
}

Here is where it is visible via the psql interface:

template1=# select rolname, rolcatupdate from pg_authid;
rolname | rolcatupdate
---------+--------------
rbrad | t
(1 row)

template1=# select has_table_privilege('pg_class', 'delete');
has_table_privilege
---------------------
t
(1 row)

template1=# select has_table_privilege('pg_class', 'truncate');
has_table_privilege
---------------------
t
(1 row)

template1=# update pg_authid set rolcatupdate = false;
UPDATE 1

template1=# select has_table_privilege('pg_class', 'delete');
has_table_privilege
---------------------
f
(1 row)

template1=# select has_table_privilege('pg_class', 'truncate');
has_table_privilege
---------------------
t
(1 row)

I do not believe this is a huge issue since truncate is prohibited on the
system catalogs
by the truncate_check_rel().

template1=# truncate pg_authid;
ERROR: permission denied: "pg_authid" is a system catalog

2. The src/test/regress/sql/privileges.sql regression test has tests for
the has_table_privilege() function. It looks like all the other
permissions
are tested in this function, but there is not a test for the TRUNCATE
privilege.

3. I believe you missed a documentation update in the ddl.sgml file:

There are several different privileges: <literal>SELECT</>,
<literal>INSERT</>, <literal>UPDATE</>, <literal>DELETE</>,
<literal>REFERENCES</>, <literal>TRIGGER</>,
<literal>CREATE</>, <literal>CONNECT</>, <literal>TEMPORARY</>,
<literal>EXECUTE</>, and <literal>USAGE</>.

I played around with the patch for an hour or so tonight and I did not
observer any other unusual behaviors.

Hopefully this review is useful. It is my first patch review for a
commit-fest.
I will update the wiki with a link to this email message for my review.

Thanks,

- Ryan

Hello all,

On Sun, Aug 31, 2008 at 11:55 PM, Ryan Bradetich <rbradetich(at)gmail(dot)com>wrote:

>
> I do not believe this is a huge issue since truncate is prohibited on the
> system catalogs
> by the truncate_check_rel().
>
> template1=# truncate pg_authid;
> ERROR: permission denied: "pg_authid" is a system catalog
>

I thought about this some more. I believe my suggestion was incorrect.
Since truncate_check_rel() prevents the use of the truncate command on
system catalogs, the TRUNCATE permission should always be stripped
from the system catalogs.

Here is the inconsistency I observed:

template1=# \z pg_catalog.pg_authid

Access privileges
Schema | Name | Type | Access privileges
------------+-----------+-------+---------------------
pg_catalog | pg_authid | table | rbrad=arwdDxt/rbrad
(1 row)

template1=# select rolname, rolcatupdate from pg_authid;
rolname | rolcatupdate
---------+--------------
rbrad | t
(1 row)

template1=# select has_table_privilege('pg_authid', 'truncate');
has_table_privilege
---------------------
t
(1 row)

template1=# truncate pg_authid;
ERROR: permission denied: "pg_authid" is a system catalog

The TRUNCATE fails even though \z and has_table_privilege() said I had
permissions.
Compare with the DELETE privilege:

template1=# select has_table_privilege('pg_authid', 'delete');
has_table_privilege
---------------------
t
(1 row)

template1=# delete from pg_authid;
DELETE 1

Thanks,

- Ryan

"Ryan Bradetich" <rbradetich(at)gmail(dot)com> writes:
>> I do not believe this is a huge issue since truncate is prohibited on the
>> system catalogs
>> by the truncate_check_rel().

Only when allowSystemTableMods is false. I think it would be a serious
mistake for your patch to treat the system catalogs differently from
other tables.

> Here is the inconsistency I observed:

It seems to me that you are assuming that lack of a TRUNCATE permission
bit is the only valid reason for a "permission denied" failure. This is
fairly obviously not so, since multiple permissions typically enter into
any command (consider schema-level permissions for instance).

regards, tom lane

I had intended to send this message to the pgsql-hackers mailing list as
well.

Thanks,

- Ryan

---------- Forwarded message ----------
From: Ryan Bradetich <rbradetich(at)gmail(dot)com>
Date: Mon, Sep 1, 2008 at 2:20 PM
Subject: Re: [HACKERS] [Patch Review] TRUNCATE Permission
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>

Hello Tom,

On Mon, Sep 1, 2008 at 1:00 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> "Ryan Bradetich" <rbradetich(at)gmail(dot)com> writes:
> >> I do not believe this is a huge issue since truncate is prohibited on
> the
> >> system catalogs
> >> by the truncate_check_rel().
>
> Only when allowSystemTableMods is false. I think it would be a serious
> mistake for your patch to treat the system catalogs differently from
> other tables.
>

Good Point. Looks like I still have more code reading to do :)

This is Robert Haas's patch for the September 2008 commit-fest.
I am just offering my review. Gives me a good excuse to dig into
the PostgreSQL code base. Hopefully this review is useful to the
person committing the patch.

> > Here is the inconsistency I observed:
>
> It seems to me that you are assuming that lack of a TRUNCATE permission
> bit is the only valid reason for a "permission denied" failure. This is
> fairly obviously not so, since multiple permissions typically enter into
> any command (consider schema-level permissions for instance).
>

I was just comparing the behavior between DELETE and TRUNCATE.
My last suggestion for the TRUNCATE permission always being removed
on the system tables is obviously bogus because of the allowSystemTableMods
issue you raised.

Does my first suggestion still make sense for removing the TRUNCATE in
pg_class_aclmask() when pg_Authid.rolcatupdate is not set?

Thanks,

- Ryan

"Ryan Bradetich" <rbradetich(at)gmail(dot)com> writes:
> On Mon, Sep 1, 2008 at 1:00 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> [ something about "your patch" ]

> This is Robert Haas's patch for the September 2008 commit-fest.
> I am just offering my review.

Sorry about that, I got confused by the reply-to-a-reply.

> Does my first suggestion still make sense for removing the TRUNCATE in
> pg_class_aclmask() when pg_Authid.rolcatupdate is not set?

Probably. AFAICS it should be treated exactly like ACL_DELETE, so
anyplace that acl-whacking code is doing something for ACL_DELETE and
the patch doesn't add in ACL_TRUNCATE, I'd be suspicious ...

regards, tom lane

Updated patch attached, based on comments from Ryan Bradetich and Tom
Lane, and sync'd to latest CVS version.

...Robert

On Mon, Sep 1, 2008 at 9:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Ryan Bradetich" <rbradetich(at)gmail(dot)com> writes:
>> On Mon, Sep 1, 2008 at 1:00 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> [ something about "your patch" ]
>
>> This is Robert Haas's patch for the September 2008 commit-fest.
>> I am just offering my review.
>
> Sorry about that, I got confused by the reply-to-a-reply.
>
>> Does my first suggestion still make sense for removing the TRUNCATE in
>> pg_class_aclmask() when pg_Authid.rolcatupdate is not set?
>
> Probably. AFAICS it should be treated exactly like ACL_DELETE, so
> anyplace that acl-whacking code is doing something for ACL_DELETE and
> the patch doesn't add in ACL_TRUNCATE, I'd be suspicious ...
>
> regards, tom lane
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Hello Robert,

On Fri, Sep 5, 2008 at 8:13 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Updated patch attached, based on comments from Ryan Bradetich and Tom
> Lane, and sync'd to latest CVS version.

Thanks for the update. I am out of town until tomorrow evening.
I will re-review this patch when I get back if it has not been
committed by then.

- Ryan

"Robert Haas" <robertmhaas(at)gmail(dot)com> writes:
> Updated patch attached, based on comments from Ryan Bradetich and Tom
> Lane, and sync'd to latest CVS version.

Applied with really pretty minor revisions --- this was a nice clean
patch. Changes I can recall making:

* You missed one or two documentation references to DELETE privilege.

* You modified the privileges test to create another userid, but forgot
to clean up afterwards.

* LOCK TABLE requires UPDATE or DELETE privilege for locks stronger
than AccessShareLock. I thought it would be inconsistent to not allow
TRUNCATE to satisfy this requirement too.

* Many of the information_schema views require some privilege on a table
to show details about the table. Again, it seemed inconsistent to not
allow TRUNCATE privilege to satisfy this requirement.

* A couple of the information_schema views show available privileges on
tables by name. It's a bit dubious whether we should show TRUNCATE in
them, since there is no such privilege bit in the SQL standard, but
after some reflection I concluded that functionality trumps a narrow
reading of the spec here. We can revisit that if anyone wants to argue
for the other way, though.

regards, tom lane

> Applied with really pretty minor revisions --- this was a nice clean
> patch. Changes I can recall making:

Woo-hoo, my first patch. Thanks for the cleanup.

...Robert

> * You missed one or two documentation references to DELETE privilege.
>
> * You modified the privileges test to create another userid, but forgot
> to clean up afterwards.
>
> * LOCK TABLE requires UPDATE or DELETE privilege for locks stronger
> than AccessShareLock. I thought it would be inconsistent to not allow
> TRUNCATE to satisfy this requirement too.
>
> * Many of the information_schema views require some privilege on a table
> to show details about the table. Again, it seemed inconsistent to not
> allow TRUNCATE privilege to satisfy this requirement.
>
> * A couple of the information_schema views show available privileges on
> tables by name. It's a bit dubious whether we should show TRUNCATE in
> them, since there is no such privilege bit in the SQL standard, but
> after some reflection I concluded that functionality trumps a narrow
> reading of the spec here. We can revisit that if anyone wants to argue
> for the other way, though.
>
> regards, tom lane
>