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 (n...@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-as5bjal0om7yrppvwn...@mail.gmail.com http://www.postgresql.org/message-id/cakrt6cqnghzwugwb5pkwg5gfxwd+-joy8mmmenqh+o6vply...@mail.gmail.com http://www.postgresql.org/message-id/20150729130927.gs3...@tamriel.snowman.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
