Re: [HACKERS] Review for Add permission check on SELECT INTO
On Mon, Nov 21, 2011 at 6:38 AM, Albe Laurenz laurenz.a...@wien.gv.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review for Add permission check on SELECT INTO
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review for Add permission check on SELECT INTO
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.a...@wien.gv.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 kai...@kaigai.gr.jp pgsql-v9.2-add-select-into-checks.v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
