Thread: more RLS oversights
I happened to notice this morning while hacking that the "hasRowSecurity" fields added to PlannerGlobal and PlannedStmt have not been given proper nodefuncs.c support. Both need to be added to outfuncs.c, and the latter to copyfuncs.c. The latter omission may well be a security bug, although I haven't attempted to verify that, but fortunately this isn't released yet. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert,
* Robert Haas (robertmhaas@gmail.com) wrote:
> I happened to notice this morning while hacking that the
> "hasRowSecurity" fields added to PlannerGlobal and PlannedStmt have
> not been given proper nodefuncs.c support. Both need to be added to
> outfuncs.c, and the latter to copyfuncs.c. The latter omission may
> well be a security bug, although I haven't attempted to verify that,
> but fortunately this isn't released yet.
I saw this and will address it. Would be great if you wouldn't mind
CC'ing me directly on anything RLS-related, same as you CC'd me on the
column-privilege backpatch. I expect I'll probably notice anyway, but
I'll see them faster when I'm CC'd.
I agree that it's great that we're catching issues prior to when the
feature is released and look forward to anything else you (or anyone
else!) finds.
Thanks!
Stephen
Robert, all,
* Stephen Frost (sfrost@snowman.net) wrote:
> * Robert Haas (robertmhaas@gmail.com) wrote:
> > I happened to notice this morning while hacking that the
> > "hasRowSecurity" fields added to PlannerGlobal and PlannedStmt have
> > not been given proper nodefuncs.c support. Both need to be added to
> > outfuncs.c, and the latter to copyfuncs.c. The latter omission may
> > well be a security bug, although I haven't attempted to verify that,
> > but fortunately this isn't released yet.
>
> I saw this and will address it. Would be great if you wouldn't mind
> CC'ing me directly on anything RLS-related, same as you CC'd me on the
> column-privilege backpatch. I expect I'll probably notice anyway, but
> I'll see them faster when I'm CC'd.
>
> I agree that it's great that we're catching issues prior to when the
> feature is released and look forward to anything else you (or anyone
> else!) finds.
I've pushed a fix for this. Please let me know if you see any other
issues or run into any problems.
Thanks!
Stephen
On Wed, Feb 25, 2015 at 11:37:24PM -0500, Stephen Frost wrote:
> * Stephen Frost (sfrost@snowman.net) wrote:
> > I agree that it's great that we're catching issues prior to when the
> > feature is released and look forward to anything else you (or anyone
> > else!) finds.
>
> I've pushed a fix for this. Please let me know if you see any other
> issues or run into any problems.
(1) CreatePolicy() and AlterPolicy() omit to call assign_expr_collations() on
the node trees. Test case:
begin;
set row_security = force;
create table t (c) as values ('bar'::text);
create policy p on t using (c < ('foo'::text COLLATE "C"));
alter table t enable row level security;
table pg_policy; -- note ":inputcollid 0"
select * from t; -- ERROR: could not determine which collation ...
rollback;
(2) CreatePolicy() and AlterPolicy() omit to create a pg_shdepend entry for
each role in the TO clause. Test case:
begin;
create role alice;
create table t (c) as values ('bar'::text);
grant select on table t to alice;
create policy p on t to alice using (true);
select refclassid::regclass, refobjid, refobjsubid, deptype from pg_depend where classid = 'pg_policy'::regclass;
select refclassid::regclass, refobjid, deptype from pg_shdepend where classid = 'pg_policy'::regclass;
savepoint q; drop role alice; rollback to q;
revoke all on table t from alice;
\d t
drop role alice;
\d t
rollback;
> +static void
> +dumpPolicy(Archive *fout, PolicyInfo *polinfo)
...
> + if (polinfo->polqual != NULL)
> + appendPQExpBuffer(query, " USING %s", polinfo->polqual);
(3) The USING clause needs parentheses; a dump+reload failed like so:
pg_restore: [archiver (db)] could not execute query: ERROR: syntax error at or near "CASE"
LINE 2: CASE ^ Command was: CREATE POLICY "p2" ON "category" FOR ALL TO PUBLIC USING
CASE WHEN ("current_user"() = 'rls_regress_user1'::"name") THE...
Add the same parentheses to psql \d output also, keeping that output similar
to the SQL syntax.
(3a) I found this by hacking the rowsecurity.sql regression test to not drop
its objects, then running the pg_upgrade test suite. New features that affect
pg_dump should leave objects in the regression database to test the pg_dump
support via that suite.
(4) When DefineQueryRewrite() is about to convert a table to a view, it checks
the table for features unavailable to views. For example, it rejects tables
having triggers. It omits to reject tables having relrowsecurity or a
pg_policy record. Test case:
begin;
set row_security = force;
create table t (c) as select * from generate_series(1,5);
create policy p on t using (c % 2 = 1);
alter table t enable row level security;
table t;
truncate t;
create rule "_RETURN" as on select to t do instead select * from generate_series(1,5) t0(c);
table t;
select polrelid::regclass from pg_policy;
select relrowsecurity from pg_class where oid = 't'::regclass;
rollback;
> + <para>
> + Referential integrity checks, such as unique or primary key constraints
> + and foreign key references, will bypass row security to ensure that
> + data integrity is maintained. Care must be taken when developing
> + schemas and row level policies to avoid a "covert channel" leak of
> + information through these referential integrity checks.
...
> + /*
> + * Row-level security should be disabled in the case where a foreign-key
> + * relation is queried to check existence of tuples that references the
> + * primary-key being modified.
> + */
> + temp_sec_context = save_sec_context | SECURITY_LOCAL_USERID_CHANGE;
> + if (qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK
> + || qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK_FROM_PK
> + || qkey->constr_queryno == RI_PLAN_RESTRICT_DEL_CHECKREF
> + || qkey->constr_queryno == RI_PLAN_RESTRICT_UPD_CHECKREF)
> + temp_sec_context |= SECURITY_ROW_LEVEL_DISABLED;
(5) This code does not use SECURITY_ROW_LEVEL_DISABLED for foreign keys with
CASCADE, SET NULL or SET DEFAULT actions. The associated documentation says
nothing about this presumably-important distinction.
Is SECURITY_ROW_LEVEL_DISABLED mode even needed? We switch users to the owner
of the FROM-clause table before running an RI query. That means use of this
mode can only matter under row_security=force, right? I would rest easier if
this mode went away, because it is a security landmine. If user code managed
to run in this mode, it would bypass every policy in the database. (I find no
such vulnerability today, because we use the mode only for parse analysis of
ri_triggers.c queries.)
(6) AlterPolicy() calls InvokeObjectPostAlterHook(PolicyRelationId, ...), but
CreatePolicy() and DropPolicy() lack their respective hook invocations.
(7) Using an aggregate function in a policy predicate elicits an inapposite
error message due to use of EXPR_KIND_WHERE for parse analysis. Need a new
ParseExprKind. Test case:
begin;
set row_security = force;
create table t (c) as values ('bar'::text);
-- ERROR: aggregate functions are not allowed in WHERE
create policy p on t using (max(c));
rollback;
Noah, * Noah Misch (noah@leadboat.com) wrote: > On Wed, Feb 25, 2015 at 11:37:24PM -0500, Stephen Frost wrote: > > * Stephen Frost (sfrost@snowman.net) wrote: > > > I agree that it's great that we're catching issues prior to when the > > > feature is released and look forward to anything else you (or anyone > > > else!) finds. > > > > I've pushed a fix for this. Please let me know if you see any other > > issues or run into any problems. > > (1) CreatePolicy() and AlterPolicy() omit to call assign_expr_collations() on > the node trees. Test case: Will fix. > (2) CreatePolicy() and AlterPolicy() omit to create a pg_shdepend entry for > each role in the TO clause. Test case: Will fix. > > +static void > > +dumpPolicy(Archive *fout, PolicyInfo *polinfo) > ... > > + if (polinfo->polqual != NULL) > > + appendPQExpBuffer(query, " USING %s", polinfo->polqual); > > (3) The USING clause needs parentheses; a dump+reload failed like so: Will fix. > Add the same parentheses to psql \d output also, keeping that output similar > to the SQL syntax. Yup. > (3a) I found this by hacking the rowsecurity.sql regression test to not drop > its objects, then running the pg_upgrade test suite. New features that affect > pg_dump should leave objects in the regression database to test the pg_dump > support via that suite. Will fix. > (4) When DefineQueryRewrite() is about to convert a table to a view, it checks > the table for features unavailable to views. For example, it rejects tables > having triggers. It omits to reject tables having relrowsecurity or a > pg_policy record. Test case: Will fix. > > + <para> > > + Referential integrity checks, such as unique or primary key constraints > > + and foreign key references, will bypass row security to ensure that > > + data integrity is maintained. Care must be taken when developing > > + schemas and row level policies to avoid a "covert channel" leak of > > + information through these referential integrity checks. > ... > > + /* > > + * Row-level security should be disabled in the case where a foreign-key > > + * relation is queried to check existence of tuples that references the > > + * primary-key being modified. > > + */ > > + temp_sec_context = save_sec_context | SECURITY_LOCAL_USERID_CHANGE; > > + if (qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK > > + || qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK_FROM_PK > > + || qkey->constr_queryno == RI_PLAN_RESTRICT_DEL_CHECKREF > > + || qkey->constr_queryno == RI_PLAN_RESTRICT_UPD_CHECKREF) > > + temp_sec_context |= SECURITY_ROW_LEVEL_DISABLED; > > (5) This code does not use SECURITY_ROW_LEVEL_DISABLED for foreign keys with > CASCADE, SET NULL or SET DEFAULT actions. The associated documentation says > nothing about this presumably-important distinction. Agreed, will fix. > Is SECURITY_ROW_LEVEL_DISABLED mode even needed? We switch users to the owner > of the FROM-clause table before running an RI query. That means use of this > mode can only matter under row_security=force, right? I would rest easier if > this mode went away, because it is a security landmine. If user code managed > to run in this mode, it would bypass every policy in the database. (I find no > such vulnerability today, because we use the mode only for parse analysis of > ri_triggers.c queries.) That's a very interesting point.. At first blush, I agree, it shouldn't be necessary. I'll play with it and see if I can get everything to work properly without it. > (6) AlterPolicy() calls InvokeObjectPostAlterHook(PolicyRelationId, ...), but > CreatePolicy() and DropPolicy() lack their respective hook invocations. Will fix. > (7) Using an aggregate function in a policy predicate elicits an inapposite > error message due to use of EXPR_KIND_WHERE for parse analysis. Need a new > ParseExprKind. Test case: Will fix. Thanks a lot for your review! Much appreciated. Stephen
On Fri, Jul 10, 2015 at 03:08:53PM -0700, Joe Conway wrote: > On 07/03/2015 10:03 AM, Noah Misch wrote: > > (1) CreatePolicy() and AlterPolicy() omit to call assign_expr_collations() on > > the node trees. > The attached fixes this issue for me, but I am unsure whether we really > need/want the regression test. Given the recent push to increase test > coverage maybe so. I wouldn't remove the test from your patch.
On 07/03/2015 10:03 AM, Noah Misch wrote:
> (1) CreatePolicy() and AlterPolicy() omit to call assign_expr_collations() on
> the node trees. Test case:
>
> begin;
> set row_security = force;
> create table t (c) as values ('bar'::text);
> create policy p on t using (c < ('foo'::text COLLATE "C"));
> alter table t enable row level security;
> table pg_policy; -- note ":inputcollid 0"
> select * from t; -- ERROR: could not determine which collation ...
> rollback;
The attached fixes this issue for me, but I am unsure whether we really
need/want the regression test. Given the recent push to increase test
coverage maybe so. Any objections?
Joe
Attachment
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 07/10/2015 06:15 PM, Noah Misch wrote: > On Fri, Jul 10, 2015 at 03:08:53PM -0700, Joe Conway wrote: >> On 07/03/2015 10:03 AM, Noah Misch wrote: >>> (1) CreatePolicy() and AlterPolicy() omit to call >>> assign_expr_collations() on the node trees. > >> The attached fixes this issue for me, but I am unsure whether we >> really need/want the regression test. Given the recent push to >> increase test coverage maybe so. > > I wouldn't remove the test from your patch. > Ok -- pushed. - -- Joe Conway -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJVoYj2AAoJEDfy90M199hleBcP/3anI8IIEkFyPiUDX0QvzfEM EOBm+AdolwgAvscU6RaDbrVXJlE32YbSWsXhtXOA5jJvhY80ln3YO+ko1ONWV3dW iYbvSO+zQalHDqmID2bqbnY/k+7GTGWPCTsSLMsmUK7P0QCVP2f02lCukqr4yWpH tIVbOfb0A1+Mrb9dxta43Bj32maBBiEpWIwaebotik6BmfwHNeeaZ082PUJQvaqS wtshrlctAaCsCyjQnNiPvtD9mw0rlSWOhNDc7R8KGflWnwXmBlyu7jD4aFHKcPZO v1ErqG2Z0allm3p6snpFbiunQssVpHgF7V8FcWxIReu73lV25ig3Ix56MoUXHIOq a2Y5iAfRw106V1GA6ARW0kjCaE0DrRcfA6/Um8LeEhw44cvUBZkhXx/ozt0t62pz 6mvhKN4UjmO/XfbA9GEN7b9kDz+LZtMFQ1PqcH7mK3OYKgGfYTAdJOA7qwHuWMBC MGVHP5WEUCJEToTNyzVe0matOH8+IHS4LQ9qAtUFVCmhh27FK0m8kjoZAmT/xDk5 uNcMG9mvBOTZe5EmVdC1gywDOsRntzAgWM1SFBK2v0YEgj3YarKll839Jm+dNHGZ nxjniR/XJkNxISrTN6Qq797nhYsmpqRg+d7ZVk0GxmhfNNp/f2SFRVB9/9ovrk4x //7pllazs48qu6e/3eYK =EEZ7 -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 07/03/2015 10:03 AM, Noah Misch wrote: > (2) CreatePolicy() and AlterPolicy() omit to create a pg_shdepend > entry for each role in the TO clause. Test case: Please see the attached patch. Note that I used SHARED_DEPENDENCY_ACL for this. It seems appropriate, but possibly we should invent a new shared dependency type for this use case? Comments? Thanks, Joe -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJVtSlAAAoJEDfy90M199hlrkIP/0BqMj30JpJVj3H5pIopU0RZ pesBzFzzsWmt2QmasX9hajRfo6eXQAWPmKQmw/NDTJvHHNACxLdo0MHE7A9y7No7 aZIFTm0KhnG4jpzxfzpGqQ4ron5Vsc2TlNQgCBYCtbEEtuD0mV2qmcuTkqGte7iB 7iqneRBhmTYBy63X7S0ir4AhDi+JJg8P4F7YuU/XMcha5v5CpNJAToPpW7mCoZ8O 9w/RbXCHso7p1DIxfx4tfxVwLQ7j7G2j0rXbuA2e6OKfwZWWrk5E8Wnvc3xy3yCY J37fcjOxFdhU/j1j+Tr90LYOSuRu5fQ4z6PmmsPkBM3T0Vllz/nNP64aVKbmHzga szrIBERRs2icKa4OI08qZF42ObIEv0/t/NuyrXpegY6awRNNNsjyZvwM+51zdjw1 fuWh+2rObzh3RDH8lPB0N0rVfDMM+wU85Vaekckv/7UQ/pbWcwsYD/tykA1engQ8 Ww8kBAEct4dWdqppTqvLLxLgo4d+vuiS1mJ7SRY2aZFRX8QQ8TfB3eObETUsU4/X 9UWyMn+E0Au9ICX+wfD4whaBKst8EuO36qOZx0oZt+++EMOlzypgkCCxDtZO0KWd KZHbtmJXHFVH+ihbEGXAKMIx+gLqSDG3IKy9MIJxpB3JY3XSdBNqobL2hiQy36/w svK7i+mEYmUCQ6pB1j8c =P1AS -----END PGP SIGNATURE-----
Attachment
Joe Conway wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 07/03/2015 10:03 AM, Noah Misch wrote:
> > (2) CreatePolicy() and AlterPolicy() omit to create a pg_shdepend
> > entry for each role in the TO clause. Test case:
>
> Please see the attached patch. Note that I used SHARED_DEPENDENCY_ACL
> for this. It seems appropriate, but possibly we should invent a new
> shared dependency type for this use case? Comments?
Hmm, these are not ACL objects, so conceptually it seems cleaner to use
a different symbol for this. I think the catalog state and the error
messages would be a bit confusing otherwise.
> if (spec->roletype == ROLESPEC_PUBLIC)
> {
> ! Datum *tmp_role_oids;
> !
> ! if (*num_roles != 1)
> ereport(WARNING,
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> errmsg("ignoring roles specified other than public"),
> errhint("All roles are members of the public role.")));
> ! *num_roles = 1;
> ! tmp_role_oids = (Datum *) palloc(*num_roles * sizeof(Datum));
> ! tmp_role_oids[0] = ObjectIdGetDatum(ACL_ID_PUBLIC);
Isn't this leaking the previously allocated array? Not sure it's all
that critical, but still. (I don't think you really need to call palloc
at all here.)
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 07/27/2015 01:13 PM, Alvaro Herrera wrote: > Hmm, these are not ACL objects, so conceptually it seems cleaner to > use a different symbol for this. I think the catalog state and the > error messages would be a bit confusing otherwise. Ok -- done > Isn't this leaking the previously allocated array? Not sure it's > all that critical, but still. (I don't think you really need to > call palloc at all here.) Agreed -- I think the attached is a bit cleaner. Any other comments or concerns? - -- Joe Conway -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJVts3+AAoJEDfy90M199hlzTIQAJ6LEOrEEhHEjsoVvCT6TUAu pMQl/LWmb0s3/vF9o5VFTpVd21k0LxcggD+DdbxSagiw1WpLK5x67C9Lj5uuFn/d 3a95nFnQje3ciQJaAKS4XcGyx8+6rHPZqfBRC1rARbLuDHrwpKqmbKwvpQCud4xN kD7OolYk5Gd3cId0xH0XBHuwLVJg4Bt/MAexrcHn+kXuQoF8V6iOjnmBI/BHvTQy w4j4ov7DpWSAR1ZiCTCkF2ZuNd9TJ8FmAEtSDVrlWMxB9J+9PU5oTfD50jp62BTz wycANVYmbpCyZ7DAAiqopt3IQFIiF/1bYwzWH3/M8RRMKJF8HNyE8KBPDyC/doO5 0c0poCugJI2wOhumLGJShizgSAS85vqijh2Uxpp2yQxdStMnADykzT4Nb5EnEJVE i7OKy6w+2xyilfFGEWhHfS7uo5Y0zBorhpjgT9fRaqPBGoK4jYwZoO8w97SUd9aS kNXOCfmFxvcDFSZIYZP77pOeJZR2dLCbr+X9bF1Fz5S4FVkgCXVOp9rmsqzgxoDh lcpkDh9zPPezdczRkfq/qCf0lmzGg8apdqdr7+g8DxU+01OvPspEdpovQQN0HXlM m5Kbny4KCWhAgBTyAsOFTEy6lK7u4KsHV1cee3bG+ev65czbQ14gGRMJc/Lhm6Gg Apcn/UcF14vLWxYVo6lS =pS6L -----END PGP SIGNATURE-----
Attachment
On 07/03/2015 10:03 AM, Noah Misch wrote: >> +static void >> +dumpPolicy(Archive *fout, PolicyInfo *polinfo) > ... >> + if (polinfo->polqual != NULL) >> + appendPQExpBuffer(query, " USING %s", polinfo->polqual); > > (3) The USING clause needs parentheses; a dump+reload failed like so: Also needed for WITH CHECK. Fix pushed to 9.5 and HEAD. Joe
On 07/03/2015 10:03 AM, Noah Misch wrote: > (4) When DefineQueryRewrite() is about to convert a table to a view, it checks > the table for features unavailable to views. For example, it rejects tables > having triggers. It omits to reject tables having relrowsecurity or a > pg_policy record. Test case: Please see the attached patch for this issue. Comments? Thanks, Joe
Attachment
* Joe Conway (joe.conway@crunchydata.com) wrote:
> On 07/03/2015 10:03 AM, Noah Misch wrote:
> > (4) When DefineQueryRewrite() is about to convert a table to a view, it checks
> > the table for features unavailable to views. For example, it rejects tables
> > having triggers. It omits to reject tables having relrowsecurity or a
> > pg_policy record. Test case:
>
> Please see the attached patch for this issue. Comments?
Looks good to me.
Thanks!
Stephen
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 07/27/2015 05:34 PM, Joe Conway wrote: > On 07/27/2015 01:13 PM, Alvaro Herrera wrote: >> Hmm, these are not ACL objects, so conceptually it seems cleaner >> to use a different symbol for this. I think the catalog state >> and the error messages would be a bit confusing otherwise. > > Ok -- done > >> Isn't this leaking the previously allocated array? Not sure >> it's all that critical, but still. (I don't think you really >> need to call palloc at all here.) > > Agreed -- I think the attached is a bit cleaner. > > Any other comments or concerns? Pushed to HEAD and 9.5 Joe -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJVuAp9AAoJEDfy90M199hlokgP/2RVYjPiwM0EcE9pAdGP3Uqa qkCqj5cPq/5SdLH553h/Xd6056qwchoMJYQo9RZzzVR4wS4HPyeZ00e33RrJZbtm MGvjat1NJWEyl03y1AwAy9c9yHCRJa5vQlKtm0v2xWDG3xXqwejz/SK2ijw2IWVW W1/7OnObbquHa7a+IsO1ndXoI0jECzOFDXY6YzFVJuPAf3asOHT44lJbHkfUq3Kv k5eK14Xrb3kcYqhBQg+eG50lr1aRDj8yDlYQuoGmw/dg/X0TyAo2v+AOaFrN8rXD igsYaMDafsXY55izkiRuNfcYZAnHC7hNVt+ffV+wLycCTiaEEflp+BCxLTYmh53T xDB39Lr0Sz7AP77l0M9hbMr3Ao3GA1KFOc9OfRN11eSo5Y6uDtpMeOIFBAke6hxl DanWPc/YmXzacga99xzOQglDZkDWTohWsEDwniRJmi7UC0Z/gwZ2P60OnwE1lqbd eOmUu0JZCwklInWoDo9XmWdfp9+OrviGNm0vhQbplhEm/LC9PqBB/DOy44QjSv84 jfY/iMPn8uvGqWiQ/65za1O/1QsRukgp5PVnj7TyNojSskSuAYOF5BcFVIdB7krj ZKHChreUMVw1nH8py4HkdPOXTHmAItV9/9T2c/UUuJWAECiLy+tIY/if+Tzi+Zn6 nRm99YM401PAsRKLyn0m =gYmH -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 07/28/2015 11:50 AM, Stephen Frost wrote: > * Joe Conway (joe.conway@crunchydata.com) wrote: >> On 07/03/2015 10:03 AM, Noah Misch wrote: >>> (4) When DefineQueryRewrite() is about to convert a table to a >>> view, it checks the table for features unavailable to views. >>> For example, it rejects tables having triggers. It omits to >>> reject tables having relrowsecurity or a pg_policy record. >>> Test case: >> >> Please see the attached patch for this issue. Comments? > > Looks good to me. Thanks -- pushed to HEAD and 9.5 Joe -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJVuA+UAAoJEDfy90M199hlrtYQAJGVucRtarWTQoP39of8IYXE OoAi+q6CKEJWEJdVHmnnCwMc6y0lx0+Qugpb5SAVqMG/0oYvZ/bnvvN5OHMxujCI bw7A/eJ62BILAt07Oczs1gtt+k4PT1001c4jFjgSC6AV51cZ9UM+d0b4C2YhKcm7 B4tycmC/yGxa5UFT6vdOhneILzBeOMLxgMXKouoD7Gnf4UKh1KUWxxWKU9q0Kl7d mvRJke1wj/+WfquO09g33+1jHCsnbV4fcQ1q8RQbFPItpqcp4alsuuIYzLiPRbbs VtZYurFLpjKdJg8OuHw9IYNbDjPZoEuv5eV16aSjOJxnngcXROxwXOwZMVaCuQMq 1D5FoUunJqsqlxVKggJJLfBt9uM/gafjmnRHGBZftfX/A3ZM6c6YewA68Nxo+rTE xtgA+n1lzWsahje0n2KSFUNRSCqdWcnLQ/HtnVcNjGdFdCxeUDQ5kUAE1hFCMCXe 5eqAvohQQ25RiurZ1rI1IfUoeAPRDp3nvgcMMM7FQMmv6oKNBLr2RivVz0ZE17vd Htz0y0cPx8mJgEHKMJrV/yF9odECTxevBzkO5rASLCLnGHEYp8WZfqWO/s/HoujS KU99lfzOfnyBIyl2zIGSmkmCvUIqaP1cUP5xMHIedNhjDRy/Rt6IxwA9qEtgUokI sC6BWHpxd19RAh5NLXCK =NFOR -----END PGP SIGNATURE-----
On 07/03/2015 10:03 AM, Noah Misch wrote: > (6) AlterPolicy() calls InvokeObjectPostAlterHook(PolicyRelationId, ...), but > CreatePolicy() and DropPolicy() lack their respective hook invocations. Patch attached. Actually AlterPolicy() was also missing its hook -- the existing InvokeObjectPostAlterHook() was only in rename_policy(). I'm not 100% sure about the hook placement -- would appreciate if someone could confirm I got it correct. Joe
Attachment
On 07/03/2015 10:03 AM, Noah Misch wrote: > (7) Using an aggregate function in a policy predicate elicits an inapposite > error message due to use of EXPR_KIND_WHERE for parse analysis. Need a new > ParseExprKind. Test case: Patch attached. Comments? Joe
Attachment
On 29 July 2015 at 02:36, Joe Conway <joe.conway@crunchydata.com> wrote: > On 07/03/2015 10:03 AM, Noah Misch wrote: >> (6) AlterPolicy() calls InvokeObjectPostAlterHook(PolicyRelationId, ...), but >> CreatePolicy() and DropPolicy() lack their respective hook invocations. > > Patch attached. Actually AlterPolicy() was also missing its hook -- the > existing InvokeObjectPostAlterHook() was only in rename_policy(). > > I'm not 100% sure about the hook placement -- would appreciate if > someone could confirm I got it correct. > The CreatePolicy() and AlterPolicy() changes look OK to me, but the RemovePolicyById() change looks to be unnecessary --- RemovePolicyById() is called only from doDeletion(), which in turned is called only from deleteOneObject(), which already invokes the drop hooks. So ISTM that RemovePolicyById() doesn't need to do anything, right? Regards, Dean
On 29 July 2015 at 05:02, Joe Conway <joe.conway@crunchydata.com> wrote: > On 07/03/2015 10:03 AM, Noah Misch wrote: >> (7) Using an aggregate function in a policy predicate elicits an inapposite >> error message due to use of EXPR_KIND_WHERE for parse analysis. Need a new >> ParseExprKind. Test case: > > Patch attached. Comments? > I don't think there is any point in adding the new function transformPolicyClause(), which is identical to transformWhereClause(). You can just use transformWhereClause() with EXPR_KIND_POLICY. It's already used for lots of other expression kinds. There are a couple of places in test_rls_hooks.c that also need updating. I think that check_agglevels_and_constraints() and transformWindowFuncCall() could be made to emit more targeted error messages for EXPR_KIND_POLICY, for example "aggregate functions are not allowed in policy USING and WITH CHECK expressions". Regards, Dean
On 07/29/2015 01:01 AM, Dean Rasheed wrote: > The CreatePolicy() and AlterPolicy() changes look OK to me, but the > RemovePolicyById() change looks to be unnecessary --- > RemovePolicyById() is called only from doDeletion(), which in turned > is called only from deleteOneObject(), which already invokes the drop > hooks. So ISTM that RemovePolicyById() doesn't need to do anything, > right? Seems correct. Will remove that change and commit it that way. Joe
On 07/29/2015 02:41 AM, Dean Rasheed wrote: > I don't think there is any point in adding the new function > transformPolicyClause(), which is identical to transformWhereClause(). > You can just use transformWhereClause() with EXPR_KIND_POLICY. It's > already used for lots of other expression kinds. Yeah, I was debating that. Although I do think the name transformWhereClause() is unfortunate. Maybe it ought to be renamed transformBooleanExpr() or something similar? > There are a couple of places in test_rls_hooks.c that also need updating. Right -- thanks > I think that check_agglevels_and_constraints() and > transformWindowFuncCall() could be made to emit more targeted error > messages for EXPR_KIND_POLICY, for example "aggregate functions are > not allowed in policy USING and WITH CHECK expressions". ok Thanks for the review. Joe
On 29 July 2015 at 16:52, Joe Conway <joe.conway@crunchydata.com> wrote: > On 07/29/2015 02:41 AM, Dean Rasheed wrote: >> I don't think there is any point in adding the new function >> transformPolicyClause(), which is identical to transformWhereClause(). >> You can just use transformWhereClause() with EXPR_KIND_POLICY. It's >> already used for lots of other expression kinds. > > Yeah, I was debating that. Although I do think the name > transformWhereClause() is unfortunate. Maybe it ought to be renamed > transformBooleanExpr() or something similar? > I expect it's probably being used by other people's code though, so renaming it might cause pain for quite a few people. Regards, Dean
On 07/29/2015 08:46 AM, Joe Conway wrote: > On 07/29/2015 01:01 AM, Dean Rasheed wrote: >> The CreatePolicy() and AlterPolicy() changes look OK to me, but the >> RemovePolicyById() change looks to be unnecessary --- >> RemovePolicyById() is called only from doDeletion(), which in turned >> is called only from deleteOneObject(), which already invokes the drop >> hooks. So ISTM that RemovePolicyById() doesn't need to do anything, >> right? > > Seems correct. Will remove that change and commit it that way. Pushed to HEAD and 9.5. Joe
On 07/29/2015 02:41 AM, Dean Rasheed wrote: > I don't think there is any point in adding the new function > transformPolicyClause(), which is identical to transformWhereClause(). > You can just use transformWhereClause() with EXPR_KIND_POLICY. It's > already used for lots of other expression kinds. Ok -- I went back to using transformWhereClause. I'd still prefer to change the name -- more than half the uses of the function are for other than EXPR_KIND_WHERE -- but I don't feel that strongly about it. > There are a couple of places in test_rls_hooks.c that also need updating. done > I think that check_agglevels_and_constraints() and > transformWindowFuncCall() could be made to emit more targeted error > messages for EXPR_KIND_POLICY, for example "aggregate functions are > not allowed in policy USING and WITH CHECK expressions". done Unless there are other comments/objections, will commit this later today. Joe
Attachment
On 29 July 2015 at 20:36, Joe Conway <joe.conway@crunchydata.com> wrote: > On 07/29/2015 02:41 AM, Dean Rasheed wrote: >> I don't think there is any point in adding the new function >> transformPolicyClause(), which is identical to transformWhereClause(). >> You can just use transformWhereClause() with EXPR_KIND_POLICY. It's >> already used for lots of other expression kinds. > > > Ok -- I went back to using transformWhereClause. I'd still prefer to > change the name -- more than half the uses of the function are for other > than EXPR_KIND_WHERE -- but I don't feel that strongly about it. > > >> There are a couple of places in test_rls_hooks.c that also need updating. > > done > >> I think that check_agglevels_and_constraints() and >> transformWindowFuncCall() could be made to emit more targeted error >> messages for EXPR_KIND_POLICY, for example "aggregate functions are >> not allowed in policy USING and WITH CHECK expressions". > > done > > Unless there are other comments/objections, will commit this later today. > In the final error s/aggregate/window/. Other than that it looks good to me. Regards, Dean
Joe Conway wrote:
> On 07/29/2015 02:41 AM, Dean Rasheed wrote:
> > I don't think there is any point in adding the new function
> > transformPolicyClause(), which is identical to transformWhereClause().
> > You can just use transformWhereClause() with EXPR_KIND_POLICY. It's
> > already used for lots of other expression kinds.
>
> Ok -- I went back to using transformWhereClause. I'd still prefer to
> change the name -- more than half the uses of the function are for other
> than EXPR_KIND_WHERE -- but I don't feel that strongly about it.
Currently the comment about it says "for WHERE and allied", maybe this
should be a bit more explicit. I think it was originally for things
like WHERE and HAVING, and usage slowly extended beyond that. Does it
make sense to consider the USING clause in CREATE/ALTER POLICY as an
"allied" clause of WHERE? I don't particularly think so. I'm just
talking about a small rewording of the comment.
> > I think that check_agglevels_and_constraints() and
> > transformWindowFuncCall() could be made to emit more targeted error
> > messages for EXPR_KIND_POLICY, for example "aggregate functions are
> > not allowed in policy USING and WITH CHECK expressions".
>
> done
> + case EXPR_KIND_POLICY:
> + if (isAgg)
> + err = _("aggregate functions are not allowed in POLICY USING and WITH CHECK expressions");
> + else
> + err = _("grouping operations are not allowed in POLICY USING and WITH CHECK expressions");
I think this reads a bit funny. What's a "POLICY USING" clause? I
expect that translators will treat the two words POLICY USING as a
single token, and the result is not going to make any sense.
Maybe "in a policy's USING and WITH CHECK expressions", or perhaps "in
policies's USING and WITH CHECK exprs", not sure.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Jul 29, 2015 at 3:58 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > I think this reads a bit funny. What's a "POLICY USING" clause? I > expect that translators will treat the two words POLICY USING as a > single token, and the result is not going to make any sense. > > Maybe "in a policy's USING and WITH CHECK expressions", or perhaps "in > policies's USING and WITH CHECK exprs", not sure. Yeah, I don't see why we would capitalize POLICY there. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 07/29/2015 01:26 PM, Robert Haas wrote: > On Wed, Jul 29, 2015 at 3:58 PM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: >> I think this reads a bit funny. What's a "POLICY USING" clause? I >> expect that translators will treat the two words POLICY USING as a >> single token, and the result is not going to make any sense. >> >> Maybe "in a policy's USING and WITH CHECK expressions", or perhaps "in >> policies's USING and WITH CHECK exprs", not sure. > > Yeah, I don't see why we would capitalize POLICY there. The equivalent message for functions is: ".. are not allowed in functions in FROM" So how does this sound: "... are not allowed in policies in USING and WITH CHECK expressions" or perhaps more simply: "... are not allowed in policies in USING and WITH CHECK" Joe
On Wed, Jul 29, 2015 at 4:57 PM, Joe Conway <joe.conway@crunchydata.com> wrote: > On 07/29/2015 01:26 PM, Robert Haas wrote: >> On Wed, Jul 29, 2015 at 3:58 PM, Alvaro Herrera >> <alvherre@2ndquadrant.com> wrote: >>> I think this reads a bit funny. What's a "POLICY USING" clause? I >>> expect that translators will treat the two words POLICY USING as a >>> single token, and the result is not going to make any sense. >>> >>> Maybe "in a policy's USING and WITH CHECK expressions", or perhaps "in >>> policies's USING and WITH CHECK exprs", not sure. >> >> Yeah, I don't see why we would capitalize POLICY there. > > The equivalent message for functions is: > ".. are not allowed in functions in FROM" > > So how does this sound: > "... are not allowed in policies in USING and WITH CHECK expressions" > or perhaps more simply: > "... are not allowed in policies in USING and WITH CHECK" Awkward. The "in policies in" phrasing is just hard to read. Why not just "in policy expressions"? There's no third kind that does allow these. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas wrote: > On Wed, Jul 29, 2015 at 4:57 PM, Joe Conway <joe.conway@crunchydata.com> wrote: > > The equivalent message for functions is: > > ".. are not allowed in functions in FROM" > > > > So how does this sound: > > "... are not allowed in policies in USING and WITH CHECK expressions" > > or perhaps more simply: > > "... are not allowed in policies in USING and WITH CHECK" > > Awkward. The "in policies in" phrasing is just hard to read. Yeah. Besides, it's not really the same thing. > Why not just "in policy expressions"? There's no third kind that does > allow these. WFM -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 07/29/2015 02:04 PM, Alvaro Herrera wrote: >> Why not just "in policy expressions"? There's no third kind that does >> allow these. > > WFM Sold! Will do it that way. Joe
On 07/29/2015 02:56 PM, Joe Conway wrote: > On 07/29/2015 02:04 PM, Alvaro Herrera wrote: >>> Why not just "in policy expressions"? There's no third kind that does >>> allow these. >> >> WFM > > Sold! Will do it that way. Committed/pushed to HEAD and 9.5. Joe
Noah, First off, thanks again for your review and comments on RLS. Hopefully this addresses the last remaining open item from that review. * Noah Misch (noah@leadboat.com) wrote: > On Wed, Feb 25, 2015 at 11:37:24PM -0500, Stephen Frost wrote: > > + <para> > > + Referential integrity checks, such as unique or primary key constraints > > + and foreign key references, will bypass row security to ensure that > > + data integrity is maintained. Care must be taken when developing > > + schemas and row level policies to avoid a "covert channel" leak of > > + information through these referential integrity checks. > ... > > + /* > > + * Row-level security should be disabled in the case where a foreign-key > > + * relation is queried to check existence of tuples that references the > > + * primary-key being modified. > > + */ > > + temp_sec_context = save_sec_context | SECURITY_LOCAL_USERID_CHANGE; > > + if (qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK > > + || qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK_FROM_PK > > + || qkey->constr_queryno == RI_PLAN_RESTRICT_DEL_CHECKREF > > + || qkey->constr_queryno == RI_PLAN_RESTRICT_UPD_CHECKREF) > > + temp_sec_context |= SECURITY_ROW_LEVEL_DISABLED; > > (5) This code does not use SECURITY_ROW_LEVEL_DISABLED for foreign keys with > CASCADE, SET NULL or SET DEFAULT actions. The associated documentation says > nothing about this presumably-important distinction. I believe the original intent was to only include the queries which were against the primary table, but reviewing what ends up happening, that clearly doesn't actually make sense. As you note below, this is only to address the 'row_security = force' mode, which I suspect is why it wasn't picked up in earlier testing. > Is SECURITY_ROW_LEVEL_DISABLED mode even needed? We switch users to the owner > of the FROM-clause table before running an RI query. That means use of this > mode can only matter under row_security=force, right? I would rest easier if > this mode went away, because it is a security landmine. If user code managed > to run in this mode, it would bypass every policy in the database. (I find no > such vulnerability today, because we use the mode only for parse analysis of > ri_triggers.c queries.) You're right, the reason for it to exist was to address the 'row_security = force' case. I spent a bit of time considering that and have come up with the attached for consideration (which needs additional work before being committed, but should get the idea across clearly). This removes the notion of SECURITY_ROW_LEVEL_DISABLED entirely and instead ignores 'row_security = force' if InLocalUserIdChange() is true. Further, this changes row_security to have GUC_NOT_WHILE_SEC_REST set. I didn't set GUC_NO_RESET_ALL for it though as the default is for row_security to be 'on'. The biggest drawback that I can see to this is that users won't be able to set the row_security GUC inside of a security definer function. I can imagine use-cases where someone might want to change it in a security definer function but it doesn't seem like they're valuable enough to counter your argument (which I agree with) that SECURITY_ROW_LEVEL_DISABLED is a security landmine. Another approach which I considered was to add a new 'RLS_IGNORE_FORCE' flag, which would, again, ignore 'row_security=force' when set (meaning owners would bypass RLS regardless of the actual row_security setting). I didn't like adding that to sec_context though and it wasn't clear where a good place would be. Further, it seems like it'd be nice to have a generic flag that says "we're running an internal referential integrity operation, don't get in the way", though that could also be a risky flag to have. Such an approach would allow us to differentiate RI queries from operations run under a security definer function though. Thoughts? Thanks! Stephen
Attachment
On Thu, Sep 10, 2015 at 03:23:13PM -0400, Stephen Frost wrote: > First off, thanks again for your review and comments on RLS. Hopefully > this addresses the last remaining open item from that review. Item (3a), too, remains open. > * Noah Misch (noah@leadboat.com) wrote: > > On Wed, Feb 25, 2015 at 11:37:24PM -0500, Stephen Frost wrote: > > > + /* > > > + * Row-level security should be disabled in the case where a foreign-key > > > + * relation is queried to check existence of tuples that references the > > > + * primary-key being modified. > > > + */ > > > + temp_sec_context = save_sec_context | SECURITY_LOCAL_USERID_CHANGE; > > > + if (qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK > > > + || qkey->constr_queryno == RI_PLAN_CHECK_LOOKUPPK_FROM_PK > > > + || qkey->constr_queryno == RI_PLAN_RESTRICT_DEL_CHECKREF > > > + || qkey->constr_queryno == RI_PLAN_RESTRICT_UPD_CHECKREF) > > > + temp_sec_context |= SECURITY_ROW_LEVEL_DISABLED; > > Is SECURITY_ROW_LEVEL_DISABLED mode even needed? We switch users to the owner > > of the FROM-clause table before running an RI query. That means use of this > > mode can only matter under row_security=force, right? I would rest easier if > > this mode went away, because it is a security landmine. If user code managed > > to run in this mode, it would bypass every policy in the database. (I find no > > such vulnerability today, because we use the mode only for parse analysis of > > ri_triggers.c queries.) > > You're right, the reason for it to exist was to address the > 'row_security = force' case. I spent a bit of time considering that and > have come up with the attached for consideration (which needs additional > work before being committed, but should get the idea across clearly). > > This removes the notion of SECURITY_ROW_LEVEL_DISABLED entirely and > instead ignores 'row_security = force' if InLocalUserIdChange() is true. > The biggest drawback that I can see to this is that users won't be able > to set the row_security GUC inside of a security definer function. I > can imagine use-cases where someone might want to change it in a > security definer function but it doesn't seem like they're valuable > enough to counter your argument (which I agree with) that > SECURITY_ROW_LEVEL_DISABLED is a security landmine. SECURITY DEFINER execution blocks SET ROLE, SET SESSION AUTHORIZATION, and sometimes "GRANT role1 TO role2". Otherwise, it works like regular execution. Adding exceptions, particularly silent behavior changes as opposed to hard errors, is a big deal. When I wrote the 2015-07-03 review, I had in mind to just let policies run normally during referential integrity checks. Table owners, which can already break referential integrity with triggers and rules, would be responsible for crafting policies accordingly. Pondering it afresh this week, I see now that row_security=force itself is the problem. It's a new instance of the maligned "behavior-changing GUC". Function authors will not consistently test the row_security=force case, and others can run the functions under row_security=force and get novel results. This poses a reliability and security threat. While row_security=force is handy for testing, visiting a second role for testing purposes is a fine replacement. Let's remove "force", making row_security a config_bool. If someone later desires to revive the capability, a DDL-specified property of each policy would be free from these problems. On a related topic, check_enable_rls() has two kinds of RLS bypass semantics. Under row_security=off|on, superusers bypass every policy, and table owners bypass policies of their own tables. Under row_security=off only, roles with BYPASSRLS privilege additionally bypass every policy; BYPASSRLS doesn't affect semantics under row_security=on. Is that distinction a good thing? Making BYPASSRLS GUC-independent, like superuser bypass, would simplify things for the user and would make row_security no longer a behavior-changing GUC. row_security would come to mean simply "if a policy would otherwise attach to one of my queries, raise an error instead." Again, if you have cause to toggle BYPASSRLS, use multiple roles. Implementing that with GUCs creates harder problems. In summary, I propose to remove row_security=force and make BYPASSRLS effective under any setting of the row_security GUC. Some past, brief discussion leading to the current semantics: http://www.postgresql.org/message-id/CA+TgmoYA=uixXmN390SFgfQgVmLL-As5bJaL0oM7yrpPVwNPxQ@mail.gmail.com http://www.postgresql.org/message-id/CAKRt6CQnghzWUGwb5Pkwg5gfXwd+-joy8MmMEnqh+O6vpLYzfA@mail.gmail.com http://www.postgresql.org/message-id/20150729130927.GS3587@tamriel.snowman.net
On Mon, Sep 14, 2015 at 3:29 AM, Noah Misch <noah@leadboat.com> wrote: > SECURITY DEFINER execution blocks SET ROLE, SET SESSION AUTHORIZATION, and > sometimes "GRANT role1 TO role2". Otherwise, it works like regular execution. > Adding exceptions, particularly silent behavior changes as opposed to hard > errors, is a big deal. Yeah, that does seem possibly surprising... > Pondering it afresh this week, I see now that row_security=force itself is the > problem. It's a new instance of the maligned "behavior-changing GUC". > Function authors will not consistently test the row_security=force case, and > others can run the functions under row_security=force and get novel results. > This poses a reliability and security threat. While row_security=force is > handy for testing, visiting a second role for testing purposes is a fine > replacement. Let's remove "force", making row_security a config_bool. If > someone later desires to revive the capability, a DDL-specified property of > each policy would be free from these problems. ...but I'm not sure I like this, either. Without row_security=force, it's hard for a table owner to test their policy, unless they have the ability to assume some other user ID, which some won't. If someone puts in place a policy which they believe secures their data properly but which actually does not, and they are unable to test it properly for lack of this setting, that too will be a security hole. We will be able to attribute it to user error rather than product defect, but that will be cold comfort to the person whose sensitive data has been leaked. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Sep 14, 2015 at 07:30:33AM -0400, Robert Haas wrote: > On Mon, Sep 14, 2015 at 3:29 AM, Noah Misch <noah@leadboat.com> wrote: > > Pondering it afresh this week, I see now that row_security=force itself is the > > problem. It's a new instance of the maligned "behavior-changing GUC". > > Function authors will not consistently test the row_security=force case, and > > others can run the functions under row_security=force and get novel results. > > This poses a reliability and security threat. While row_security=force is > > handy for testing, visiting a second role for testing purposes is a fine > > replacement. Let's remove "force", making row_security a config_bool. If > > someone later desires to revive the capability, a DDL-specified property of > > each policy would be free from these problems. [A variation on that idea is "ALTER TABLE foo FORCE ROW LEVEL SECURITY", affecting all policies of one table rather than individual policies.] > ...but I'm not sure I like this, either. Without row_security=force, > it's hard for a table owner to test their policy, unless they have the > ability to assume some other user ID, which some won't. If someone > puts in place a policy which they believe secures their data properly > but which actually does not, and they are unable to test it properly > for lack of this setting, that too will be a security hole. We will > be able to attribute it to user error rather than product defect, but > that will be cold comfort to the person whose sensitive data has been > leaked. The testing capability is nice, all else being equal. Your scenario entails a data custodian wishing to test with row_security=force but willing to entrust sensitive data to an untested policy. It also requires a DBA unwilling to furnish test accounts to custodians of sensitive data. With or without row_security=force, such a team is on the outer perimeter of the audience able to benefit from RLS. Nonetheless, I'd welcome a replacement test aid.
Noah Misch <noah@leadboat.com> writes:
> On Mon, Sep 14, 2015 at 07:30:33AM -0400, Robert Haas wrote:
>> ...but I'm not sure I like this, either. Without row_security=force,
>> it's hard for a table owner to test their policy, unless they have the
>> ability to assume some other user ID, which some won't. If someone
>> puts in place a policy which they believe secures their data properly
>> but which actually does not, and they are unable to test it properly
>> for lack of this setting, that too will be a security hole. We will
>> be able to attribute it to user error rather than product defect, but
>> that will be cold comfort to the person whose sensitive data has been
>> leaked.
> The testing capability is nice, all else being equal. Your scenario entails a
> data custodian wishing to test with row_security=force but willing to entrust
> sensitive data to an untested policy. It also requires a DBA unwilling to
> furnish test accounts to custodians of sensitive data. With or without
> row_security=force, such a team is on the outer perimeter of the audience able
> to benefit from RLS. Nonetheless, I'd welcome a replacement test aid.
I have to say I'm with Noah on this. I do not think we should create
potential security holes to help users with uncooperative DBAs. Those
users have got problems far worse than whether they can test their RLS
policies; or in other words, what in God's name are you doing storing
sensitive data in a system with a hostile DBA?
regards, tom lane
On Tue, Sep 15, 2015 at 1:00 AM, Noah Misch <noah@leadboat.com> wrote: >> ...but I'm not sure I like this, either. Without row_security=force, >> it's hard for a table owner to test their policy, unless they have the >> ability to assume some other user ID, which some won't. If someone >> puts in place a policy which they believe secures their data properly >> but which actually does not, and they are unable to test it properly >> for lack of this setting, that too will be a security hole. We will >> be able to attribute it to user error rather than product defect, but >> that will be cold comfort to the person whose sensitive data has been >> leaked. > > The testing capability is nice, all else being equal. Your scenario entails a > data custodian wishing to test with row_security=force but willing to entrust > sensitive data to an untested policy. That's not really accurate. You can test the policy first and only afterwords GRANT access to others. > It also requires a DBA unwilling to > furnish test accounts to custodians of sensitive data. With or without > row_security=force, such a team is on the outer perimeter of the audience able > to benefit from RLS. Nonetheless, I'd welcome a replacement test aid. I can't argue with that, I suppose, but I think row_security=force is a pretty useful convenience. If we must remove it, so be it, but I'd be a little sad. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Sep 15, 2015 at 1:00 AM, Noah Misch <noah@leadboat.com> wrote:
>> It also requires a DBA unwilling to
>> furnish test accounts to custodians of sensitive data. With or without
>> row_security=force, such a team is on the outer perimeter of the audience able
>> to benefit from RLS. Nonetheless, I'd welcome a replacement test aid.
> I can't argue with that, I suppose, but I think row_security=force is
> a pretty useful convenience. If we must remove it, so be it, but I'd
> be a little sad.
Keep in mind that if you have an uncooperative DBA on your production
system, you can always test your policy to your heart's content on a
playpen installation. In fact, most people would consider that good
engineering practice anyway, rather than pushing untested code directly
into production.
regards, tom lane
On 09/15/2015 10:58 AM, Robert Haas wrote: > I can't argue with that, I suppose, but I think row_security=force is > a pretty useful convenience. If we must remove it, so be it, but I'd > be a little sad. There are use cases where row_security=force will be set in production environments, not only in testing. I would be very strongly opposed to removing the ability to force RLS from being applied to owners and superusers, and in fact think we should figure out how to make changing row_security, once it is set, more difficult. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On 09/15/2015 11:10 AM, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Tue, Sep 15, 2015 at 1:00 AM, Noah Misch <noah@leadboat.com> wrote: >>> It also requires a DBA unwilling to >>> furnish test accounts to custodians of sensitive data. With or without >>> row_security=force, such a team is on the outer perimeter of the audience able >>> to benefit from RLS. Nonetheless, I'd welcome a replacement test aid. > >> I can't argue with that, I suppose, but I think row_security=force is >> a pretty useful convenience. If we must remove it, so be it, but I'd >> be a little sad. > > Keep in mind that if you have an uncooperative DBA on your production > system, you can always test your policy to your heart's content on a > playpen installation. In fact, most people would consider that good > engineering practice anyway, rather than pushing untested code directly > into production. That's exactly right. We should provide flexibility for testing in test environments, and also the ability to lock things down tight in production. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Joe Conway <mail@joeconway.com> writes:
> On 09/15/2015 10:58 AM, Robert Haas wrote:
>> I can't argue with that, I suppose, but I think row_security=force is
>> a pretty useful convenience. If we must remove it, so be it, but I'd
>> be a little sad.
> There are use cases where row_security=force will be set in production
> environments, not only in testing.
What cases, exactly, and is there another way to solve those problems?
I concur with Noah's feeling that allowing security behavior to depend on
a GUC is risky.
regards, tom lane
On 09/15/2015 12:53 PM, Tom Lane wrote: > Joe Conway <mail@joeconway.com> writes: >> There are use cases where row_security=force will be set in production >> environments, not only in testing. > > What cases, exactly, and is there another way to solve those problems? > I concur with Noah's feeling that allowing security behavior to depend on > a GUC is risky. There are environments where there is a strong desire to use RLS to control access in production, even for table owners and superusers. Obviously there are more steps needed to completely achieve this level of control, but removing the ability to force row security is going in the wrong direction. Noah's suggestion of using a per table attribute would work -- in fact I like the idea of that better than using the current GUC. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On Tue, Sep 15, 2015 at 2:26 PM, Joe Conway <mail@joeconway.com> wrote: > On 09/15/2015 12:53 PM, Tom Lane wrote: >> Joe Conway <mail@joeconway.com> writes: >>> There are use cases where row_security=force will be set in production >>> environments, not only in testing. >> >> What cases, exactly, and is there another way to solve those problems? >> I concur with Noah's feeling that allowing security behavior to depend on >> a GUC is risky. > > There are environments where there is a strong desire to use RLS to > control access in production, even for table owners and superusers. > Obviously there are more steps needed to completely achieve this level > of control, but removing the ability to force row security is going in > the wrong direction. Noah's suggestion of using a per table attribute > would work -- in fact I like the idea of that better than using the > current GUC. FWIW, I also concur with a per table attribute for this purpose. In fact, I think I really like the per-table flexibility over an 'all-or-nothing' approach better too. I do, however, think that it makes it a bit more difficult for testing, specifically, I'm not sure how much I like the idea of 'altering' a table so that it can be tested, but that might a price worth paying for security sake. I could also see a potential gap in such approach. Specifically, I could see a case were there are two separate roles, one that is entrusted with defining the policies but not able to create/modify tables, and one with the opposite capability (I understand this to be a fairly common use-case, at least at a system level). Since you can't GRANT 'alter' rights to the table, then obviously the policy definer would have to either be the owner of the table or a member of the role that owns it, right? Given that, if by definition the policy definer is not allowed to do anything other than define policies, then obviously putting such a role in the table owners group would allow it to do much more, correct? -Adam -- Adam Brightwell - adam.brightwell@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
> I could also see a potential gap in such approach. Specifically, I > could see a case were there are two separate roles, one that is > entrusted with defining the policies but not able to create/modify > tables, and one with the opposite capability (I understand this to be > a fairly common use-case, at least at a system level). Since you > can't GRANT 'alter' rights to the table, then obviously the policy > definer would have to either be the owner of the table or a member of > the role that owns it, right? Given that, if by definition the policy > definer is not allowed to do anything other than define policies, then > obviously putting such a role in the table owners group would allow it > to do much more, correct? Actually, disregard, I forgot about "You must be the owner of a table to create or change policies for it." So that would obviously negate my concern. -Adam -- Adam Brightwell - adam.brightwell@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
On Tue, Sep 15, 2015 at 03:18:21PM -0400, Adam Brightwell wrote: > On Tue, Sep 15, 2015 at 2:26 PM, Joe Conway <mail@joeconway.com> wrote: > >> Joe Conway <mail@joeconway.com> writes: > >>> There are use cases where row_security=force will be set in production > >>> environments, not only in testing. > > Noah's suggestion of using a per table attribute > > would work -- in fact I like the idea of that better than using the > > current GUC. > > FWIW, I also concur with a per table attribute for this purpose. In > fact, I think I really like the per-table flexibility over an > 'all-or-nothing' approach better too. Great. Robert, does that work for you, too? If so, this sub-thread is looking at three patches: 1. remove row_security=force 2. remove SECURITY_ROW_LEVEL_DISABLED; make ri_triggers.c subject to policies 3. add DDL-controlled, per-table policy forcing They ought to land in that order. PostgreSQL 9.5 would need at least (1) and (2); would RLS experts find it beneficial for me to take care of those? Thanks, nm
> 2. remove SECURITY_ROW_LEVEL_DISABLED; make ri_triggers.c subject to policies I believe this one has already been addressed by Stephen (20150910192313.GT3685@tamriel.snowman.net)? Are there further considerations for his proposed changes? -Adam -- Adam Brightwell - adam.brightwell@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
On Fri, Sep 18, 2015 at 2:07 AM, Noah Misch <noah@leadboat.com> wrote: > On Tue, Sep 15, 2015 at 03:18:21PM -0400, Adam Brightwell wrote: >> On Tue, Sep 15, 2015 at 2:26 PM, Joe Conway <mail@joeconway.com> wrote: >> >> Joe Conway <mail@joeconway.com> writes: >> >>> There are use cases where row_security=force will be set in production >> >>> environments, not only in testing. > >> > Noah's suggestion of using a per table attribute >> > would work -- in fact I like the idea of that better than using the >> > current GUC. >> >> FWIW, I also concur with a per table attribute for this purpose. In >> fact, I think I really like the per-table flexibility over an >> 'all-or-nothing' approach better too. > > Great. Robert, does that work for you, too? Yes, that seems like a fine design from my point of view. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 09/18/2015 01:07 AM, Noah Misch wrote: > Great. Robert, does that work for you, too? If so, this sub-thread is > looking at three patches: > > 1. remove row_security=force > 2. remove SECURITY_ROW_LEVEL_DISABLED; make ri_triggers.c subject to policies > 3. add DDL-controlled, per-table policy forcing > > They ought to land in that order. PostgreSQL 9.5 would need at least (1) and > (2); would RLS experts find it beneficial for me to take care of those? That would be awesome, but I would say that if we do #1 & 2 for 9.5, we also need #3. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
>> 1. remove row_security=force >> 2. remove SECURITY_ROW_LEVEL_DISABLED; make ri_triggers.c subject to policies >> 3. add DDL-controlled, per-table policy forcing >> >> They ought to land in that order. PostgreSQL 9.5 would need at least (1) and >> (2); would RLS experts find it beneficial for me to take care of those? > > That would be awesome, but I would say that if we do #1 & 2 for 9.5, we > also need #3. Agreed. Please let me know if there is anything I can do to help. -Adam -- Adam Brightwell - adam.brightwell@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
On 09/18/2015 09:25 AM, Adam Brightwell wrote: >>> 1. remove row_security=force >>> 2. remove SECURITY_ROW_LEVEL_DISABLED; make ri_triggers.c subject to policies >>> 3. add DDL-controlled, per-table policy forcing >>> >>> They ought to land in that order. PostgreSQL 9.5 would need at least (1) and >>> (2); would RLS experts find it beneficial for me to take care of those? >> >> That would be awesome, but I would say that if we do #1 & 2 for 9.5, we >> also need #3. > > Agreed. Please let me know if there is anything I can do to help. Yes, same here. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On Fri, Sep 18, 2015 at 09:01:15AM -0400, Adam Brightwell wrote: > > 2. remove SECURITY_ROW_LEVEL_DISABLED; make ri_triggers.c subject to policies > > I believe this one has already been addressed by Stephen > (20150910192313.GT3685@tamriel.snowman.net)? Are there further > considerations for his proposed changes? Right. I'll use that patch, minus the bits examining InLocalUserIdChange().
* Joe Conway (mail@joeconway.com) wrote: > On 09/18/2015 01:07 AM, Noah Misch wrote: > > Great. Robert, does that work for you, too? If so, this sub-thread is > > looking at three patches: > > > > 1. remove row_security=force > > 2. remove SECURITY_ROW_LEVEL_DISABLED; make ri_triggers.c subject to policies > > 3. add DDL-controlled, per-table policy forcing > > > > They ought to land in that order. PostgreSQL 9.5 would need at least (1) and > > (2); would RLS experts find it beneficial for me to take care of those? > > That would be awesome, but I would say that if we do #1 & 2 for 9.5, we > also need #3. Agreed on all of the above. Thanks! Stephen
On Sun, Sep 20, 2015 at 5:35 PM, Stephen Frost <sfrost@snowman.net> wrote: > * Joe Conway (mail@joeconway.com) wrote: >> On 09/18/2015 01:07 AM, Noah Misch wrote: >> > Great. Robert, does that work for you, too? If so, this sub-thread is >> > looking at three patches: >> > >> > 1. remove row_security=force >> > 2. remove SECURITY_ROW_LEVEL_DISABLED; make ri_triggers.c subject to policies >> > 3. add DDL-controlled, per-table policy forcing >> > >> > They ought to land in that order. PostgreSQL 9.5 would need at least (1) and >> > (2); would RLS experts find it beneficial for me to take care of those? >> >> That would be awesome, but I would say that if we do #1 & 2 for 9.5, we >> also need #3. > > Agreed on all of the above. Well, then, we should get cracking. beta1 is coming soon, and it would be best if this were done before then. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Sep 20, 2015 at 5:35 PM, Stephen Frost <sfrost@snowman.net> wrote:
>> * Joe Conway (mail@joeconway.com) wrote:
>>> That would be awesome, but I would say that if we do #1 & 2 for 9.5, we
>>> also need #3.
>> Agreed on all of the above.
> Well, then, we should get cracking. beta1 is coming soon, and it
> would be best if this were done before then.
I'd say it's *necessary*. We're not adding new features after beta1.
regards, tom lane
On Fri, Sep 18, 2015 at 09:27:13AM -0500, Joe Conway wrote: > On 09/18/2015 09:25 AM, Adam Brightwell wrote: > >>> 1. remove row_security=force > >>> 2. remove SECURITY_ROW_LEVEL_DISABLED; make ri_triggers.c subject to policies > >>> 3. add DDL-controlled, per-table policy forcing > >>> > >>> They ought to land in that order. PostgreSQL 9.5 would need at least (1) and > >>> (2); would RLS experts find it beneficial for me to take care of those? Done. > >> That would be awesome, but I would say that if we do #1 & 2 for 9.5, we > >> also need #3. Understood. > > Agreed. Please let me know if there is anything I can do to help. > > Yes, same here. Thanks. https://wiki.postgresql.org/wiki/PostgreSQL_9.5_Open_Items is the big board of things needing work. These items are long-idle: - 84 days: UPSERT on partition - 81 days: Refactoring speculative insertion with unique indexes a little - 49 days: Arguable RLS security bug, EvalPlanQual() paranoia If you grab one or more of those and figure out what it/they need to get moving again, that would help move us toward 9.5 final. (Don't feel the need to limit yourself to those three, but they are the low-hanging fruit.)
On Mon, Sep 14, 2015 at 03:29:16AM -0400, Noah Misch wrote: > On a related topic, check_enable_rls() has two kinds of RLS bypass semantics. > Under row_security=off|on, superusers bypass every policy, and table owners > bypass policies of their own tables. Under row_security=off only, roles with > BYPASSRLS privilege additionally bypass every policy; BYPASSRLS doesn't affect > semantics under row_security=on. Is that distinction a good thing? Making > BYPASSRLS GUC-independent, like superuser bypass, would simplify things for > the user and would make row_security no longer a behavior-changing GUC. > row_security would come to mean simply "if a policy would otherwise attach to > one of my queries, raise an error instead." Again, if you have cause to > toggle BYPASSRLS, use multiple roles. Implementing that with GUCs creates > harder problems. Having pondered this further in the course of finalizing commit 537bd17, arming BYPASSRLS independent of the row_security GUC is indeed a good thing. Right now, if a BYPASSRLS user creates a SECURITY DEFINER function, any caller can change that function's behavior by toggling the GUC. Users won't test accordingly; better to have just one success-case behavior. On Wed, Jul 29, 2015 at 09:09:27AM -0400, Stephen Frost wrote: > For superuser (the only similar precedent that we have, I believe), we > go based on the view owner, but that isn't quite the same as BYPASSRLS. > > The reason this doesn't hold is that you have to use a combination of > BYPASSRLS and row_security=off to actually bypass RLS, unlike the > superuser role attribute which is just always "on" if you've got it. If > having BYPASSRLS simply always meant "don't do any RLS" then we could > use the superuser precedent to use what the view owner has, but at least > for my part, I'm a lot happier with BYPASSRLS and row_security than with > superuser and would rather we continue in that direction, where the user > has the choice of if they want their role attribute to be in effect or > not. If I make BYPASSRLS GUC-independent, I should then also make it take effect when the BYPASSRLS role owns a view. Barring objections, I will change both. I do share your wish for an ability to suppress privileges temporarily. I have no specific design in mind, but privilege activation and suppression should be subject to the approval of roles affected. GUCs probably can't serve here; apart from the grandfathered search_path, functions can ignore them. GUCs are mostly a property of the whole session. By the way, is there a reason for RI_Initial_Check() to hard-code the rules for RLS enablement instead of calling check_enable_rls(..., InvalidOid, true) twice? I refer to this code: > /* > * Also punt if RLS is enabled on either table unless this role has the > * bypassrls right or is the table owner of the table(s) involved which > * have RLS enabled. > */ > if (!has_bypassrls_privilege(GetUserId()) && > ((pk_rel->rd_rel->relrowsecurity && > !pg_class_ownercheck(pkrte->relid, GetUserId())) || > (fk_rel->rd_rel->relrowsecurity && > !pg_class_ownercheck(fkrte->relid, GetUserId())))) > return false;
* Noah Misch (noah@leadboat.com) wrote: > Right now, if a BYPASSRLS user creates a SECURITY DEFINER function, any caller > can change that function's behavior by toggling the GUC. Users won't test > accordingly; better to have just one success-case behavior. I agree that's not good, though the function definer could set the row_security GUC on the function, no? Similar to how we encourage setting of search_path, we should be encouraging a similar approach to anything which might be security relevant. > On Wed, Jul 29, 2015 at 09:09:27AM -0400, Stephen Frost wrote: > > For superuser (the only similar precedent that we have, I believe), we > > go based on the view owner, but that isn't quite the same as BYPASSRLS. > > > > The reason this doesn't hold is that you have to use a combination of > > BYPASSRLS and row_security=off to actually bypass RLS, unlike the > > superuser role attribute which is just always "on" if you've got it. If > > having BYPASSRLS simply always meant "don't do any RLS" then we could > > use the superuser precedent to use what the view owner has, but at least > > for my part, I'm a lot happier with BYPASSRLS and row_security than with > > superuser and would rather we continue in that direction, where the user > > has the choice of if they want their role attribute to be in effect or > > not. > > If I make BYPASSRLS GUC-independent, I should then also make it take effect > when the BYPASSRLS role owns a view. Barring objections, I will change both. I agree that if it's GUC-independent then it should operate the same as superuser does for views and security definer functions. On the one hand, I don't like that BYPASSRLS roles will now behave differently from non-BYPASSRLS roles, but on the other hand, the above isn't good and having BYPASSRLS always enabled may make individuals shy away from giving it out except when strictly necessary and treat it more similar to superuser, which would be a good thing. > I do share your wish for an ability to suppress privileges temporarily. I > have no specific design in mind, but privilege activation and suppression > should be subject to the approval of roles affected. GUCs probably can't > serve here; apart from the grandfathered search_path, functions can ignore > them. GUCs are mostly a property of the whole session. Perhaps GUCs won't work, but they own a pretty handy namespace (SET X = Y) and we are able to attach specific GUC settings to functions already. I don't like the idea that we'd invent a whole new syntax or bits of grammar to do the same for whatever approach we come up to for suppressing privileges temporarily (such as in SECURITY DEFINER functions). The odd case here is really views, since they operate somewhere inbetween regular queries and security definer functions, regarding permissions. > By the way, is there a reason for RI_Initial_Check() to hard-code the rules > for RLS enablement instead of calling check_enable_rls(..., InvalidOid, true) > twice? I refer to this code: I don't see a reason for it now, though I recall one existing when the code was originally written. That might have simply been a bit of extra (though unnecessary) paranoia though, as returning 'false' is a safe route. Are you planning to handle the ALTER TABLE .. FORCE ROW SECURITY (#3) as well? I have no complaints if so; just want to make sure we aren't doing double-work during this crunch time and didn't see your name listed next to it, but the nearby thread seemed to imply you were looking at it. One item which wasn't discussed, that I recall, is just how it will work without SECURITY_ROW_LEVEL_DISABLED, or something similar, to differentiate when internal referencial integrity queries are being run, which should still bypass RLS (even in the FORCE ROW SECURITY case), and when regular or SECURITY DEFINER originated queries are being run. The concensus, as I understood it, was that removing the ability to do SET ROW_SECURITY = force is good, but if done, we need to support ALTER TABLE .. FORCE ROW SECURITY. I'm trying to figure out if that means we end up not actually addressing the original concern you raised regarding SECURITY_ROW_LEVEL_DISABLED. Thanks! Stephen
On Mon, Sep 21, 2015 at 09:30:15AM -0400, Stephen Frost wrote: > * Noah Misch (noah@leadboat.com) wrote: > > Right now, if a BYPASSRLS user creates a SECURITY DEFINER function, any caller > > can change that function's behavior by toggling the GUC. Users won't test > > accordingly; better to have just one success-case behavior. > > I agree that's not good, though the function definer could set the > row_security GUC on the function, no? Similar to how we encourage > setting of search_path, we should be encouraging a similar approach to > anything which might be security relevant. Functions can do that. New features should not mimic search_path in their demands on SECURITY DEFINER authors. > Are you planning to handle the ALTER TABLE .. FORCE ROW SECURITY (#3) as > well? I have no complaints if so; just want to make sure we aren't > doing double-work during this crunch time and didn't see your name > listed next to it, but the nearby thread seemed to imply you were > looking at it. I'm not. > One item which wasn't discussed, that I recall, is just how it will work > without SECURITY_ROW_LEVEL_DISABLED, or something similar, to > differentiate when internal referencial integrity queries are being run, > which should still bypass RLS (even in the FORCE ROW SECURITY case), and > when regular or SECURITY DEFINER originated queries are being run. If the table owner enables FORCE ROW SECURITY, policies will affect referential integrity queries. Choose policies accordingly. For example, given only ON UPDATE NO ACTION constraints, it would be no problem to set owner-affecting policies for INSERT, UPDATE and/or DELETE.
* Noah Misch (noah@leadboat.com) wrote: > On Mon, Sep 21, 2015 at 09:30:15AM -0400, Stephen Frost wrote: > > * Noah Misch (noah@leadboat.com) wrote: > > > Right now, if a BYPASSRLS user creates a SECURITY DEFINER function, any caller > > > can change that function's behavior by toggling the GUC. Users won't test > > > accordingly; better to have just one success-case behavior. > > > > I agree that's not good, though the function definer could set the > > row_security GUC on the function, no? Similar to how we encourage > > setting of search_path, we should be encouraging a similar approach to > > anything which might be security relevant. > > Functions can do that. New features should not mimic search_path in their > demands on SECURITY DEFINER authors. I'm not convinced that having such a limitation on new features would be a good thing. What was missing here was a safe default. > > Are you planning to handle the ALTER TABLE .. FORCE ROW SECURITY (#3) as > > well? I have no complaints if so; just want to make sure we aren't > > doing double-work during this crunch time and didn't see your name > > listed next to it, but the nearby thread seemed to imply you were > > looking at it. > > I'm not. I'll work on it then, if we can get agreement as to how it will work. > > One item which wasn't discussed, that I recall, is just how it will work > > without SECURITY_ROW_LEVEL_DISABLED, or something similar, to > > differentiate when internal referencial integrity queries are being run, > > which should still bypass RLS (even in the FORCE ROW SECURITY case), and > > when regular or SECURITY DEFINER originated queries are being run. > > If the table owner enables FORCE ROW SECURITY, policies will affect > referential integrity queries. Choose policies accordingly. For example, > given only ON UPDATE NO ACTION constraints, it would be no problem to set > owner-affecting policies for INSERT, UPDATE and/or DELETE. Perhaps I'm not following correctly, but the above doesn't look correct to me. An ON UPDATE NO ACTION constraint would run a query against the referring table (which has FORCE ROW SECURITY set, perhaps by mistake after a debugging session of the owner, with a policy that does not allow any records to be seen by the owner), fail to find any rows, and conclude that no error needs to be thrown, resulting in the referring table having records which refer to keys in the referred-to table that no longer exist (the UPDATE having changed them). As a test, I hacked the pg_class_ownercheck() case in check_enable_rls() to return RLS_ENABLED always. Then did this: CREATE ROLE r1; CREATE ROLE r2; CREATE TABLE t1 (c1 INT PRIMARY KEY); CREATE TABLE t2 (c1 INT REFERENCES t1 ON UPDATE NO ACTION); ALTER TABLE t1 OWNER TO r1; ALTER TABLE t2 OWNER TO r2; GRANT SELECT ON t1 TO r2; INSERT INTO t1 VALUES (1); INSERT INTO t2 VALUES (1); ALTER TABLE t2 ENABLE ROW LEVEL SECURITY; UPDATE t1 SET c1 = 2; ALTER TABLE t2 DISABLE ROW LEVEL SECURITY; TABLE t1; TABLE t2; With results: =*# TABLE t1;c1 ---- 2 (1 row) =*# TABLE t2; c1 ---- 1 (1 row) This would lead to trivial to cause (and likely hard to diagnose) referential integrity data corruption issues. I find that a very hard pill to swallow in favor of a theoretical concern that new code may open avenues of exploit for a new security context mode to bypass RLS when doing internal referential integrity checks. Further, it changes this additional capability, which was agreed would be added to offset removal of the 'row_security = force' option, from useful to downright dangerous. Hopefully, I'm simply misunderstanding your proposal for the FORCE ROW LEVEL SECURITY option. Thanks! Stephen
On Tue, Sep 22, 2015 at 10:38:53AM -0400, Stephen Frost wrote: > * Noah Misch (noah@leadboat.com) wrote: > > On Mon, Sep 21, 2015 at 09:30:15AM -0400, Stephen Frost wrote: > > > One item which wasn't discussed, that I recall, is just how it will work > > > without SECURITY_ROW_LEVEL_DISABLED, or something similar, to > > > differentiate when internal referencial integrity queries are being run, > > > which should still bypass RLS (even in the FORCE ROW SECURITY case), and > > > when regular or SECURITY DEFINER originated queries are being run. > > > > If the table owner enables FORCE ROW SECURITY, policies will affect > > referential integrity queries. Choose policies accordingly. For example, > > given only ON UPDATE NO ACTION constraints, it would be no problem to set > > owner-affecting policies for INSERT, UPDATE and/or DELETE. > > Perhaps I'm not following correctly, but the above doesn't look correct > to me. An ON UPDATE NO ACTION constraint would run a query against the > referring table (which has FORCE ROW SECURITY set, perhaps by mistake > after a debugging session of the owner, with a policy that does not > allow any records to be seen by the owner), fail to find any rows, and > conclude that no error needs to be thrown, resulting in the referring > table having records which refer to keys in the referred-to table that > no longer exist (the UPDATE having changed them). Yes, the table owner could define policies that thwart referential integrity. > This would lead to trivial to cause (and likely hard to diagnose) > referential integrity data corruption issues. I find that a very hard > pill to swallow in favor of a theoretical concern that new code may open > avenues of exploit for a new security context mode to bypass RLS when > doing internal referential integrity checks. Further, it changes this > additional capability, which was agreed would be added to offset removal > of the 'row_security = force' option, from useful to downright > dangerous. In schema reviews, I will raise a red flag for use of this feature; the best designs will instead use additional roles. I forecast that PostgreSQL would fare better with no owner-constrained-by-RLS capability. Even so, others want it, and FORCE ROW SECURITY would deliver it with an acceptable risk profile. "SET row_security=force" was too risky, and not in a way particular to foreign key constraints, because the session user chose row_security=force independent of object owners. With FORCE ROW SECURITY, each table owner would make both decisions. A foreign key constraint, plus a SELECT policy hiding rows from the table owner, plus FORCE ROW SECURITY is one example of self-contradictory policy design. That example is unexceptional amidst the countless ways a table owner can get security policy wrong. SECURITY_ROW_LEVEL_DISABLED could have been okay. I removed an incomplete implementation (e.g. didn't affect CASCADE constraints). Writing a full one would be a mammoth job, and for what? Setting the wrong SELECT policies can disrupt _any_ application logic; no foreign keys or FORCE ROW SECURITY need be involved. Protecting just foreign keys brings some value, but it will not materially reduce the vigilance demanded of RLS policy authors and reviewers. nm
* Noah Misch (noah@leadboat.com) wrote: > On Thu, Sep 10, 2015 at 03:23:13PM -0400, Stephen Frost wrote: > > First off, thanks again for your review and comments on RLS. Hopefully > > this addresses the last remaining open item from that review. > > Item (3a), too, remains open. Provided I didn't break the buildfarm (keeping an eye on it), this has been done. Will update the open items wiki once it looks like the buildfarm is happy. Thanks! Stephen
* Noah Misch (noah@leadboat.com) wrote: > On Tue, Sep 22, 2015 at 10:38:53AM -0400, Stephen Frost wrote: > > This would lead to trivial to cause (and likely hard to diagnose) > > referential integrity data corruption issues. I find that a very hard > > pill to swallow in favor of a theoretical concern that new code may open > > avenues of exploit for a new security context mode to bypass RLS when > > doing internal referential integrity checks. Further, it changes this > > additional capability, which was agreed would be added to offset removal > > of the 'row_security = force' option, from useful to downright > > dangerous. > > In schema reviews, I will raise a red flag for use of this feature; the best > designs will instead use additional roles. I forecast that PostgreSQL would > fare better with no owner-constrained-by-RLS capability. Even so, others want > it, and FORCE ROW SECURITY would deliver it with an acceptable risk profile. I've attached a patch to implement it. It's not fully polished but it's sufficient for comment, I believe. Additional comments, documentation and regression tests are to be added, if we have agreement on the grammer and implementation approach. > "SET row_security=force" was too risky, and not in a way particular to foreign > key constraints, because the session user chose row_security=force independent > of object owners. With FORCE ROW SECURITY, each table owner would make both > decisions. A foreign key constraint, plus a SELECT policy hiding rows from > the table owner, plus FORCE ROW SECURITY is one example of self-contradictory > policy design. That example is unexceptional amidst the countless ways a > table owner can get security policy wrong. I agree that a table owner can get security policy wrong in any number of ways and that having a FORCE RLS option at the table level is better than the GUC. > SECURITY_ROW_LEVEL_DISABLED could have been okay. I removed an incomplete > implementation (e.g. didn't affect CASCADE constraints). Writing a full one > would be a mammoth job, and for what? Setting the wrong SELECT policies can > disrupt _any_ application logic; no foreign keys or FORCE ROW SECURITY need be > involved. Protecting just foreign keys brings some value, but it will not > materially reduce the vigilance demanded of RLS policy authors and reviewers. I have a hard time with this. We're not talking about the application logic in this case, we're talking about the guarantees which the database is making to the user, be it an application or an individual. I've included a patch (the second in the set attached) which adds a SECURITY_NOFORCE_RLS bit which has a much tighter scope- it only applies after the regular owner check is done. This reduces the risk of it being set mistakenly dramatically, I believe. Further, the cascaded case is correctly handled. This also needs more polish and regression tests, but I wanted to solicit for comment before moving forward with that since we don't have a consensus about if this should be done. Thanks! Stephen
Attachment
> I've attached a patch to implement it. It's not fully polished but it's
> sufficient for comment, I believe. Additional comments, documentation
> and regression tests are to be added, if we have agreement on the
> grammer and implementation approach.
I have given the first patch a quick review. I think I agree with the
grammar, it makes sense to me. At first I didn't really like NO
FORCE, but I couldn't think of anything better.
One comment:
if (pg_class_ownercheck(relid, user_id))
- return RLS_NONE_ENV;
+ {
+ if (relforcerowsecurity)
+ return RLS_ENABLED;
+ else
+ return RLS_NONE_ENV;
+ }
Is the 'else' even necessary in this block?
Other than that, the approach looks good to me.
The patch applied cleanly against master (758fcfd). As well
'check-world' was successful (obviously understanding that more
related tests are necessary).
> I have a hard time with this. We're not talking about the application
> logic in this case, we're talking about the guarantees which the
> database is making to the user, be it an application or an individual.
>
> I've included a patch (the second in the set attached) which adds a
> SECURITY_NOFORCE_RLS bit which has a much tighter scope- it only applies
> after the regular owner check is done. This reduces the risk of it
> being set mistakenly dramatically, I believe. Further, the cascaded
> case is correctly handled. This also needs more polish and regression
> tests, but I wanted to solicit for comment before moving forward with
> that since we don't have a consensus about if this should be done.
I'm not sure that I understand the previous concerns around this bit
well enough to speak intelligently on this point. However, with that
said, I believe the changes made by this patch make sense.
-Adam
--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
On Mon, Sep 28, 2015 at 05:13:56PM -0400, Stephen Frost wrote: > * Noah Misch (noah@leadboat.com) wrote: > > In schema reviews, I will raise a red flag for use of this feature; the best > > designs will instead use additional roles. I forecast that PostgreSQL would > > fare better with no owner-constrained-by-RLS capability. Even so, others want > > it, and FORCE ROW SECURITY would deliver it with an acceptable risk profile. > > I've attached a patch to implement it. It's not fully polished but it's > sufficient for comment, I believe. Additional comments, documentation > and regression tests are to be added, if we have agreement on the > grammer and implementation approach. This patch has FORCE ROW LEVEL SECURITY take precedence over row_security=off, which thwarts pg_dump use of row_security=off to ensure dump completeness. Should this be a table-level flag, or should it be a policy-level flag? A policy-level flag is more powerful. If nobody really anticipates using that power, this table-level flag works for me. > > SECURITY_ROW_LEVEL_DISABLED could have been okay. I removed an incomplete > > implementation (e.g. didn't affect CASCADE constraints). Writing a full one > > would be a mammoth job, and for what? Setting the wrong SELECT policies can > > disrupt _any_ application logic; no foreign keys or FORCE ROW SECURITY need be > > involved. Protecting just foreign keys brings some value, but it will not > > materially reduce the vigilance demanded of RLS policy authors and reviewers. > > I have a hard time with this. We're not talking about the application > logic in this case, we're talking about the guarantees which the > database is making to the user, be it an application or an individual. If disabling policies has an effect, table owners must be feeding conflicting requirements into the system. Violating policies during referential integrity queries is not, in general, less serious than violating referential integrity itself. Rules and triggers pose the same threat, and we let those break referential integrity. I think the ideal in all of these cases is rather to detect the conflict and raise an error. SECURITY_ROW_LEVEL_DISABLED and SECURITY_NOFORCE_RLS send policies in a third direction, neither the beaten path of rules/triggers nor the ideal. > I've included a patch (the second in the set attached) which adds a > SECURITY_NOFORCE_RLS bit which has a much tighter scope- it only applies > after the regular owner check is done. This reduces the risk of it > being set mistakenly dramatically, I believe. Yes, that's far safer than SECURITY_ROW_LEVEL_DISABLED. I assume the final design will let table owners completely bypass FORCE ROW LEVEL SECURITY under "SET row_security = off". If so, SECURITY_NOFORCE_RLS poses negligible risk. Even if not, SECURITY_NOFORCE_RLS poses low risk. > @@ -3667,6 +3673,12 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel, > case AT_DisableRowSecurity: > ATExecDisableRowSecurity(rel); > break; > + case AT_ForceRowSecurity: > + ATExecForceRowSecurity(rel); > + break; > + case AT_NoForceRowSecurity: > + ATExecNoForceRowSecurity(rel); > + break; Functions differing only in s/ = true/ = false/? ATExecEnableDisableTrigger() is a better model for this. nm
On Thu, Oct 1, 2015 at 11:10 PM, Noah Misch <noah@leadboat.com> wrote: > On Mon, Sep 28, 2015 at 05:13:56PM -0400, Stephen Frost wrote: >> * Noah Misch (noah@leadboat.com) wrote: >> > In schema reviews, I will raise a red flag for use of this feature; the best >> > designs will instead use additional roles. I forecast that PostgreSQL would >> > fare better with no owner-constrained-by-RLS capability. Even so, others want >> > it, and FORCE ROW SECURITY would deliver it with an acceptable risk profile. >> >> I've attached a patch to implement it. It's not fully polished but it's >> sufficient for comment, I believe. Additional comments, documentation >> and regression tests are to be added, if we have agreement on the >> grammer and implementation approach. > > This patch has FORCE ROW LEVEL SECURITY take precedence over row_security=off, > which thwarts pg_dump use of row_security=off to ensure dump completeness. Yeah, I think that's NOT ok. > Should this be a table-level flag, or should it be a policy-level flag? A > policy-level flag is more powerful. If nobody really anticipates using that > power, this table-level flag works for me. Either works for me. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
* Noah Misch (noah@leadboat.com) wrote: > On Mon, Sep 28, 2015 at 05:13:56PM -0400, Stephen Frost wrote: > > * Noah Misch (noah@leadboat.com) wrote: > > > In schema reviews, I will raise a red flag for use of this feature; the best > > > designs will instead use additional roles. I forecast that PostgreSQL would > > > fare better with no owner-constrained-by-RLS capability. Even so, others want > > > it, and FORCE ROW SECURITY would deliver it with an acceptable risk profile. > > > > I've attached a patch to implement it. It's not fully polished but it's > > sufficient for comment, I believe. Additional comments, documentation > > and regression tests are to be added, if we have agreement on the > > grammer and implementation approach. > > This patch has FORCE ROW LEVEL SECURITY take precedence over row_security=off, > which thwarts pg_dump use of row_security=off to ensure dump completeness. Fixed. > Should this be a table-level flag, or should it be a policy-level flag? A > policy-level flag is more powerful. If nobody really anticipates using that > power, this table-level flag works for me. table-level seems the right level to me and no one is calling for policy-level. Further, policy-level could be added later if there ends up being significant interest later. > > > SECURITY_ROW_LEVEL_DISABLED could have been okay. I removed an incomplete > > > implementation (e.g. didn't affect CASCADE constraints). Writing a full one > > > would be a mammoth job, and for what? Setting the wrong SELECT policies can > > > disrupt _any_ application logic; no foreign keys or FORCE ROW SECURITY need be > > > involved. Protecting just foreign keys brings some value, but it will not > > > materially reduce the vigilance demanded of RLS policy authors and reviewers. > > > > I have a hard time with this. We're not talking about the application > > logic in this case, we're talking about the guarantees which the > > database is making to the user, be it an application or an individual. > > If disabling policies has an effect, table owners must be feeding conflicting > requirements into the system. Violating policies during referential integrity > queries is not, in general, less serious than violating referential integrity > itself. Rules and triggers pose the same threat, and we let those break > referential integrity. I think the ideal in all of these cases is rather to > detect the conflict and raise an error. SECURITY_ROW_LEVEL_DISABLED and > SECURITY_NOFORCE_RLS send policies in a third direction, neither the beaten > path of rules/triggers nor the ideal. The agreement between the user and the system with regard to permissions and referential integrity is that referential integrity takes priority over permissions. Prior to FORCE and SECURITY_NOFORCE_RLS that is true for RLS. I don't believe it makes sense that adding FORCE would change that agreement, nor do I agree that the ideal would be for the system to throw errors when the permissions system would deny access during RI checks. While I appreciate that rules and triggers can break RI, the way RLS works is consistent with the ACL system and FORCE should be consistent with the ACL system and normal/non-FORCE RLS with regard to referential integrity. > > I've included a patch (the second in the set attached) which adds a > > SECURITY_NOFORCE_RLS bit which has a much tighter scope- it only applies > > after the regular owner check is done. This reduces the risk of it > > being set mistakenly dramatically, I believe. > > Yes, that's far safer than SECURITY_ROW_LEVEL_DISABLED. I assume the final > design will let table owners completely bypass FORCE ROW LEVEL SECURITY under > "SET row_security = off". If so, SECURITY_NOFORCE_RLS poses negligible risk. I've made that change. > Functions differing only in s/ = true/ = false/? ATExecEnableDisableTrigger() > is a better model for this. I've changed that to be one function. As an independent patch, I'll do the same for ATExecEnable/DisableRowSecurity. Apologies about the timing, I had intended to get this done yesterday. Barring further concerns, I'll push the attached later today with the necessary catversion bump. Thanks! Stephen
Attachment
On Tue, Jul 28, 2015 at 04:04:29PM -0700, Joe Conway wrote: > On 07/27/2015 05:34 PM, Joe Conway wrote: > > On 07/27/2015 01:13 PM, Alvaro Herrera wrote: > >> Hmm, these are not ACL objects, so conceptually it seems cleaner > >> to use a different symbol for this. I think the catalog state > >> and the error messages would be a bit confusing otherwise. > > > > Ok -- done > Pushed to HEAD and 9.5 I reviewed this commit, f781a0f "Create a pg_shdepend entry for each role in TO clause of policies." This commit rendered the http://www.postgresql.org/docs/devel/static/role-removal.html procedure[1] incomplete. Before dropping a role, one must additionally drop each policy mentioning the role in pg_policy.polroles: begin; create role alice; create table t (c int); grant select on table t to alice; create policy p0 on t to alice using (true); reassign owned by alice to current_user; drop owned by alice; drop role alice; rollback; shdepDropOwned() ignores SHARED_DEPENDENCY_POLICY entries. Should it instead remove the role from polroles, dropping the policy if that would empty polroles? (Which should change, the documented role-removal procedure or the DROP OWNED treatment of policies?) Independently, http://www.postgresql.org/docs/devel/static/sql-drop-owned.html deserves an update since it discusses every other object type having role dependencies. Thanks, nm [1] That page did not exist until 2015-10-07 (commit 1ea0c73), after the commit I'm reviewing here.
Noah, * Noah Misch (noah@leadboat.com) wrote: > On Tue, Jul 28, 2015 at 04:04:29PM -0700, Joe Conway wrote: > > Pushed to HEAD and 9.5 > > I reviewed this commit, f781a0f "Create a pg_shdepend entry for each role in > TO clause of policies." Thanks for the review! > This commit rendered the > http://www.postgresql.org/docs/devel/static/role-removal.html procedure[1] > incomplete. Before dropping a role, one must additionally drop each policy > mentioning the role in pg_policy.polroles: > > begin; > create role alice; > create table t (c int); > grant select on table t to alice; > create policy p0 on t to alice using (true); > reassign owned by alice to current_user; > drop owned by alice; > drop role alice; > rollback; > > shdepDropOwned() ignores SHARED_DEPENDENCY_POLICY entries. Should it instead > remove the role from polroles, dropping the policy if that would empty > polroles? (Which should change, the documented role-removal procedure or the > DROP OWNED treatment of policies?) I would expect the DROP OWNED treatment of policies to be similar to the DROP OWNED treatment of GRANTs. I'm certainly of the opinion that this is a bug which should be addressed. As an FYI, Joe's laptop recently got stolen and he's working to get back up to speed as quickly as he can. I've just put his new key into place on gitmaster (along with a few other pginfra-related bits), but there's obviously a lot more for him to be completely up and working again. > Independently, > http://www.postgresql.org/docs/devel/static/sql-drop-owned.html deserves an > update since it discusses every other object type having role dependencies. Agreed. Thanks! Stephen