Re: Review for "Add permission check on SELECT INTO"

The patch is in context diff format and applies cleanly.

The functionality is needed because it keeps users from circumventing
privilege restrictions, as they can currently do in this case.

There is no documentation, which I think is OK since it changes
behaviour to work as documented.

The patch compiles without warning.

It contains a test case, but that test case does not test
the feature at all!

The expected (and encountered) result is:

CREATE SCHEMA selinto_schema;
CREATE USER selinto_user;
ALTER DEFAULT PRIVILEGES IN SCHEMA selinto_schema
REVOKE INSERT ON TABLES FROM selinto_user;
SET SESSION AUTHORIZATION selinto_user;
SELECT * INTO TABLE selinto_schema.tmp1
FROM onek WHERE onek.unique1 < 2; -- Error
ERROR: permission denied for relation onek
RESET SESSION AUTHORIZATION;
DROP SCHEMA selinto_schema CASCADE;
DROP USER selinto_user;

This does not fail because "selinto_user" lacks INSERT permission
on "selinto_schema.tmp1" (he doesn't!), but because he lacks
SELECT permission on "onek" (as the error message indicates).
Setting default privileges on a schema can never revoke
default privileges.

The patch works as advertised in that it causes SELECT ... INTO
and CREATE TABLE ... AS to fail, however the error message is
misleading. Here a (correct) test case:

CREATE ROLE laurenz LOGIN;
CREATE TABLE public.test(id, val) AS VALUES (1, 'eins'), (2, 'zwei');
GRANT SELECT ON public.test TO laurenz;
ALTER DEFAULT PRIVILEGES FOR ROLE laurenz REVOKE INSERT ON TABLES FROM
laurenz;
SET ROLE laurenz;
CREATE TABLE public.copy1(a, b) AS SELECT * FROM public.test;
ERROR: whole-row update is not implemented
CREATE TABLE public.copy1(a) AS SELECT id FROM public.test;
ERROR: whole-row update is not implemented
SELECT * INTO public.copy2 FROM public.test;
ERROR: whole-row update is not implemented
RESET ROLE;
ALTER DEFAULT PRIVILEGES FOR ROLE laurenz GRANT ALL ON TABLES TO
laurenz;
DROP TABLE test;
DROP ROLE laurenz;

The correct error message would be:
ERROR: permission denied for relation copy1

It seems like a wrong code path is taken in ExecCheckRTEPerms,
maybe there's something wrong with rte->modifiedCols.

I'll mark the patch as "Waiting on Author" until these problems are
addressed.

Yours,
Laurenz Albe

Thanks for your reviewing.

The reason of this strange message was bug is the patch.

> CREATE TABLE public.copy1(a, b) AS SELECT * FROM public.test;
> ERROR:  whole-row update is not implemented

When it constructs a fake RangeTblEntry, it marked modifiedCols for
attribute 0 to (tupdesc->natts - 1), although it should be 1 to natts.
InvalidAttrNumber equals 0, so ExecCheckRTPerms got confused.

The attached patch is a revised version.
It fixed up this bug, and revised test cases to ensure permission
check error shall be raised due to the new table.

+SET SESSION AUTHORIZATION selinto_user;
+SELECT * INTO TABLE selinto_schema.tmp1
+ FROM pg_class WHERE relname like '%a%'; -- Error
+ERROR: permission denied for relation tmp1

Thanks,

2011/11/18 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>:
> The patch is in context diff format and applies cleanly.
>
> The functionality is needed because it keeps users from circumventing
> privilege restrictions, as they can currently do in this case.
>
> There is no documentation, which I think is OK since it changes
> behaviour to work as documented.
>
> The patch compiles without warning.
>
> It contains a test case, but that test case does not test
> the feature at all!
>
> The expected (and encountered) result is:
>
> CREATE SCHEMA selinto_schema;
> CREATE USER selinto_user;
> ALTER DEFAULT PRIVILEGES IN SCHEMA selinto_schema
>         REVOKE INSERT ON TABLES FROM selinto_user;
> SET SESSION AUTHORIZATION selinto_user;
> SELECT * INTO TABLE selinto_schema.tmp1
>         FROM onek WHERE onek.unique1 < 2;     -- Error
> ERROR:  permission denied for relation onek
> RESET SESSION AUTHORIZATION;
> DROP SCHEMA selinto_schema CASCADE;
> DROP USER selinto_user;
>
> This does not fail because "selinto_user" lacks INSERT permission
> on "selinto_schema.tmp1" (he doesn't!), but because he lacks
> SELECT permission on "onek" (as the error message indicates).
> Setting default privileges on a schema can never revoke
> default privileges.
>
>
> The patch works as advertised in that it causes SELECT ... INTO
> and CREATE TABLE ... AS to fail, however the error message is
> misleading. Here a (correct) test case:
>
> CREATE ROLE laurenz LOGIN;
> CREATE TABLE public.test(id, val) AS VALUES (1, 'eins'), (2, 'zwei');
> GRANT SELECT ON public.test TO laurenz;
> ALTER DEFAULT PRIVILEGES FOR ROLE laurenz REVOKE INSERT ON TABLES FROM
> laurenz;
> SET ROLE laurenz;
> CREATE TABLE public.copy1(a, b) AS SELECT * FROM public.test;
> ERROR:  whole-row update is not implemented
> CREATE TABLE public.copy1(a) AS SELECT id FROM public.test;
> ERROR:  whole-row update is not implemented
> SELECT * INTO public.copy2 FROM public.test;
> ERROR:  whole-row update is not implemented
> RESET ROLE;
> ALTER DEFAULT PRIVILEGES FOR ROLE laurenz GRANT ALL ON TABLES TO
> laurenz;
> DROP TABLE test;
> DROP ROLE laurenz;
>
> The correct error message would be:
> ERROR:  permission denied for relation copy1
>
> It seems like a wrong code path is taken in ExecCheckRTEPerms,
> maybe there's something wrong with rte->modifiedCols.
>
>
> I'll mark the patch as "Waiting on Author" until these problems are
> addressed.
>
> Yours,
> Laurenz Albe
>
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Kohei KaiGai wrote:
> The attached patch is a revised version.
> It fixed up this bug, and revised test cases to ensure permission
> check error shall be raised due to the new table.

Thanks.
The second patch seems fine to me, I'll mark it "ready for committer".

Yours,
Laurenz Albe

On Mon, Nov 21, 2011 at 6:38 AM, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:
> Kohei KaiGai wrote:
>> The attached patch is a revised version.
>> It fixed up this bug, and revised test cases to ensure permission
>> check error shall be raised due to the new table.
>
> Thanks.
> The second patch seems fine to me, I'll mark it "ready for committer".

Committed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company