Re: [HACKERS] more RLS oversights
Noah, * Noah Misch (n...@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 signature.asc Description: Digital signature
Re: [HACKERS] more RLS oversights
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
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 (n...@leadboat.com) wrote: > On Wed, Feb 25, 2015 at 11:37:24PM -0500, Stephen Frost wrote: > > + > > + 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 diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c new file mode 100644 index 61edde9..647ea77 *** a/src/backend/utils/adt/ri_triggers.c --- b/src/backend/utils/adt/ri_triggers.c *** ri_PlanCheck(const char *querystr, int n *** 2984,3001 /* Switch to proper UID to perform check as */ GetUserIdAndSecContext(&save_userid, &save_sec_context); - /* - * 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; - SetUserIdAndSecContext(RelationGetForm(query_rel)->relowner, t
Re: [HACKERS] more RLS oversights
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
Robert Haas wrote: > On Wed, Jul 29, 2015 at 4:57 PM, Joe Conway > 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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
On Wed, Jul 29, 2015 at 4:57 PM, Joe Conway wrote: > On 07/29/2015 01:26 PM, Robert Haas wrote: >> On Wed, Jul 29, 2015 at 3:58 PM, Alvaro Herrera >> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
On 07/29/2015 01:26 PM, Robert Haas wrote: > On Wed, Jul 29, 2015 at 3:58 PM, Alvaro Herrera > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
On Wed, Jul 29, 2015 at 3:58 PM, Alvaro Herrera 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
On 29 July 2015 at 20:36, 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. > > >> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
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
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index d8b4390..bcf4a8f 100644
*** a/src/backend/commands/policy.c
--- b/src/backend/commands/policy.c
*** CreatePolicy(CreatePolicyStmt *stmt)
*** 534,545
qual = transformWhereClause(qual_pstate,
copyObject(stmt->qual),
! EXPR_KIND_WHERE,
"POLICY");
with_check_qual = transformWhereClause(with_check_pstate,
copyObject(stmt->with_check),
! EXPR_KIND_WHERE,
"POLICY");
/* Fix up collation information */
--- 534,545
qual = transformWhereClause(qual_pstate,
copyObject(stmt->qual),
! EXPR_KIND_POLICY,
"POLICY");
with_check_qual = transformWhereClause(with_check_pstate,
copyObject(stmt->with_check),
! EXPR_KIND_POLICY,
"POLICY");
/* Fix up collation information */
*** AlterPolicy(AlterPolicyStmt *stmt)
*** 707,713
addRTEtoQuery(qual_pstate, rte, false, true, true);
qual = transformWhereClause(qual_pstate, copyObject(stmt->qual),
! EXPR_KIND_WHERE,
"POLICY");
/* Fix up collation information */
--- 707,713
addRTEtoQuery(qual_pstate, rte, false, true, true);
qual = transformWhereClause(qual_pstate, copyObject(stmt->qual),
! EXPR_KIND_POLICY,
"POLICY");
/* Fix up collation information */
*** AlterPolicy(AlterPolicyStmt *stmt)
*** 730,736
with_check_qual = transformWhereClause(with_check_pstate,
copyObject(stmt->with_check),
! EXPR_KIND_WHERE,
"POLICY");
/* Fix up collation information */
--- 730,736
with_check_qual = transformWhereClause(with_check_pstate,
copyObject(stmt->with_check),
! EXPR_KIND_POLICY,
"POLICY");
/* Fix up collation information */
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 478d8ca..e33adf5 100644
*** a/src/backend/parser/parse_agg.c
--- b/src/backend/parser/parse_agg.c
*** check_agglevels_and_constraints(ParseSta
*** 373,378
--- 373,385
case EXPR_KIND_WHERE:
errkind = true;
break;
+ 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");
+
+ break;
case EXPR_KIND_HAVING:
/* okay */
break;
*** transformWindowFuncCall(ParseState *psta
*** 770,775
--- 777,785
case EXPR_KIND_WHERE:
errkind = true;
break;
+ case EXPR_KIND_POLICY:
+ err = _("aggregate functions are not allowed in POLICY USING and WITH CHECK expressions");
+ break;
case EXPR_KIND_HAVING:
errkind = true;
break;
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 0ff46dd..fa77ef1 100644
*** a/src/backend/parser/parse_expr.c
--- b/src/backend/parser/parse_expr.c
*** transformSubLink(ParseState *pstate, Sub
*** 1672,1677
--- 1672,1678
case EXPR_KIND_FROM_SUBSELECT:
case EXPR_KIND_FROM_FUNCTION:
case EXPR_KIND_WHERE:
+ case EXPR_KIND_POLICY:
case EXPR_KIND_HAVING:
case EXPR_KIND_FILTER:
case EXPR_KIND_WINDOW_PARTITION:
*** ParseExprKindName(ParseExprKind exprKind
*** 3173,3178
--- 3174,3181
return "function in FROM";
case EXPR_KIND_WHERE:
return "WHERE";
+ case EXPR_KIND_POLICY:
+ return "POLICY";
case EXPR_KIND_HAVING:
return "HAVING";
case EXPR_KIND_FILTER:
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index 7ecaffc..5249945 100644
*** a/src/include/parser/parse_node.h
--- b/src/include/parser/parse_node.h
*** typedef enum ParseExprKind
*** 63,69
EXPR_KIND_INDEX_PREDICATE, /* index predicate */
EXPR_
Re: [HACKERS] more RLS oversights
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
On 29 July 2015 at 16:52, 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. > > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
On 29 July 2015 at 05:02, Joe Conway 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
On 29 July 2015 at 02:36, Joe Conway 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
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
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 4642d7c..d8ef496 100644
*** a/src/backend/commands/policy.c
--- b/src/backend/commands/policy.c
*** CreatePolicy(CreatePolicyStmt *stmt)
*** 532,545
NULL, false, false);
addRTEtoQuery(with_check_pstate, rte, false, true, true);
! qual = transformWhereClause(qual_pstate,
copyObject(stmt->qual),
! EXPR_KIND_WHERE,
"POLICY");
! with_check_qual = transformWhereClause(with_check_pstate,
copyObject(stmt->with_check),
! EXPR_KIND_WHERE,
"POLICY");
/* Fix up collation information */
--- 532,545
NULL, false, false);
addRTEtoQuery(with_check_pstate, rte, false, true, true);
! qual = transformPolicyClause(qual_pstate,
copyObject(stmt->qual),
! EXPR_KIND_POLICY,
"POLICY");
! with_check_qual = transformPolicyClause(with_check_pstate,
copyObject(stmt->with_check),
! EXPR_KIND_POLICY,
"POLICY");
/* Fix up collation information */
*** AlterPolicy(AlterPolicyStmt *stmt)
*** 704,711
addRTEtoQuery(qual_pstate, rte, false, true, true);
! qual = transformWhereClause(qual_pstate, copyObject(stmt->qual),
! EXPR_KIND_WHERE,
"POLICY");
/* Fix up collation information */
--- 704,711
addRTEtoQuery(qual_pstate, rte, false, true, true);
! qual = transformPolicyClause(qual_pstate, copyObject(stmt->qual),
! EXPR_KIND_POLICY,
"POLICY");
/* Fix up collation information */
*** AlterPolicy(AlterPolicyStmt *stmt)
*** 726,734
addRTEtoQuery(with_check_pstate, rte, false, true, true);
! with_check_qual = transformWhereClause(with_check_pstate,
copyObject(stmt->with_check),
! EXPR_KIND_WHERE,
"POLICY");
/* Fix up collation information */
--- 726,734
addRTEtoQuery(with_check_pstate, rte, false, true, true);
! with_check_qual = transformPolicyClause(with_check_pstate,
copyObject(stmt->with_check),
! EXPR_KIND_POLICY,
"POLICY");
/* Fix up collation information */
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 478d8ca..b79e73d 100644
*** a/src/backend/parser/parse_agg.c
--- b/src/backend/parser/parse_agg.c
*** check_agglevels_and_constraints(ParseSta
*** 373,378
--- 373,381
case EXPR_KIND_WHERE:
errkind = true;
break;
+ case EXPR_KIND_POLICY:
+ errkind = true;
+ break;
case EXPR_KIND_HAVING:
/* okay */
break;
*** transformWindowFuncCall(ParseState *psta
*** 770,775
--- 773,781
case EXPR_KIND_WHERE:
errkind = true;
break;
+ case EXPR_KIND_POLICY:
+ errkind = true;
+ break;
case EXPR_KIND_HAVING:
errkind = true;
break;
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 5980856..7063e5f 100644
*** a/src/backend/parser/parse_clause.c
--- b/src/backend/parser/parse_clause.c
*** transformWhereClause(ParseState *pstate,
*** 1465,1470
--- 1465,1491
return qual;
}
+ /*
+ * transformPolicyClause -
+ * Transform USING and WITH CHECK expr to make sure it is of type boolean.
+ *
+ * constructName does not affect the semantics, but is used in error messages
+ */
+ Node *
+ transformPolicyClause(ParseState *pstate, Node *clause,
+ ParseExprKind exprKind, const char *constructName)
+ {
+ Node *qual;
+
+ if (clause == NULL)
+ return NULL;
+
+ qual = transformExpr(pstate, clause, exprKind);
+
+ qual = coerce_to_boolean(pstate, qual, constructName);
+
+ return qual;
+ }
/*
* transformLimitClause -
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 0ff46dd..fa77ef1 100644
*** a/src/backend/parser/parse_expr.c
--- b/src/backend/parser/parse_expr.c
*** transformSubLink(ParseState *pstate, Sub
*** 1672,1677
--- 1672,1678
case EXPR_KIND_FROM_SUBSELECT:
case EXPR_KIND_FROM_FUNCTION:
case EXPR_KIND_WHERE:
+ case EXPR_KIND_POLICY:
case EXPR_KIND_HAVING:
case EXPR_KIND_FILTER:
case EXPR_KIND_WINDOW_PARTITION:
*** ParseExprKindName(ParseExprKind exprKind
*** 3173,3178
--- 3174,3181
return "function in FROM";
case EXPR_KIND_WHERE:
return "WHERE";
+ case EXPR_KIND_POLICY:
+ return "POLICY";
case EXPR_KIND_HAVING:
return "HAVING";
case EXPR_KIND_FILTER:
diff --git a/src/include/parser/parse_claus
Re: [HACKERS] more RLS oversights
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 diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c index 4642d7c..77aaaca 100644 *** a/src/backend/commands/policy.c --- b/src/backend/commands/policy.c *** RemovePolicyById(Oid policy_id) *** 410,415 --- 410,417 if (!HeapTupleIsValid(tuple)) elog(ERROR, "could not find tuple for policy %u", policy_id); + InvokeObjectDropHook(PolicyRelationId, policy_id, 0); + /* * Open and exclusive-lock the relation the policy belong to. */ *** CreatePolicy(CreatePolicyStmt *stmt) *** 629,634 --- 631,638 SHARED_DEPENDENCY_POLICY); } + InvokeObjectPostCreateHook(PolicyRelationId, policy_id, 0); + /* Invalidate Relation Cache */ CacheInvalidateRelcache(target_table); *** AlterPolicy(AlterPolicyStmt *stmt) *** 860,865 --- 864,871 SHARED_DEPENDENCY_POLICY); } + InvokeObjectPostAlterHook(PolicyRelationId, policy_id, 0); + heap_freetuple(new_tuple); /* Invalidate Relation Cache */ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 07/28/2015 11:50 AM, Stephen Frost wrote: > * Joe Conway (joe.con...@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- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
-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- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
* Joe Conway (joe.con...@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 signature.asc Description: Digital signature
Re: [HACKERS] more RLS oversights
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
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 17b48d4..aa858fa 100644
*** a/src/backend/commands/policy.c
--- b/src/backend/commands/policy.c
*** get_relation_policy_oid(Oid relid, const
*** 1002,1004
--- 1002,1033
return policy_oid;
}
+
+ /*
+ * relation_has_policies - Determine if relation has any policies
+ */
+ bool
+ relation_has_policies(Relation rel)
+ {
+ Relation catalog;
+ ScanKeyData skey;
+ SysScanDesc sscan;
+ HeapTuple policy_tuple;
+ bool ret = false;
+
+ catalog = heap_open(PolicyRelationId, AccessShareLock);
+ ScanKeyInit(&skey,
+ Anum_pg_policy_polrelid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(RelationGetRelid(rel)));
+ sscan = systable_beginscan(catalog, PolicyPolrelidPolnameIndexId, true,
+ NULL, 1, &skey);
+ policy_tuple = systable_getnext(sscan);
+ if (HeapTupleIsValid(policy_tuple))
+ ret = true;
+
+ systable_endscan(sscan);
+ heap_close(catalog, AccessShareLock);
+
+ return ret;
+ }
diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index a88d73e..39c83a6 100644
*** a/src/backend/rewrite/rewriteDefine.c
--- b/src/backend/rewrite/rewriteDefine.c
***
*** 27,32
--- 27,33
#include "catalog/objectaccess.h"
#include "catalog/pg_rewrite.h"
#include "catalog/storage.h"
+ #include "commands/policy.h"
#include "miscadmin.h"
#include "nodes/nodeFuncs.h"
#include "parser/parse_utilcmd.h"
*** DefineQueryRewrite(char *rulename,
*** 410,420
*
* If so, check that the relation is empty because the storage for the
* relation is going to be deleted. Also insist that the rel not have
! * any triggers, indexes, or child tables. (Note: these tests are too
! * strict, because they will reject relations that once had such but
! * don't anymore. But we don't really care, because this whole
! * business of converting relations to views is just a kluge to allow
! * dump/reload of views that participate in circular dependencies.)
*/
if (event_relation->rd_rel->relkind != RELKIND_VIEW &&
event_relation->rd_rel->relkind != RELKIND_MATVIEW)
--- 411,422
*
* If so, check that the relation is empty because the storage for the
* relation is going to be deleted. Also insist that the rel not have
! * any triggers, indexes, child tables, policies, or RLS enabled.
! * (Note: these tests are too strict, because they will reject
! * relations that once had such but don't anymore. But we don't
! * really care, because this whole business of converting relations
! * to views is just a kluge to allow dump/reload of views that
! * participate in circular dependencies.)
*/
if (event_relation->rd_rel->relkind != RELKIND_VIEW &&
event_relation->rd_rel->relkind != RELKIND_MATVIEW)
*** DefineQueryRewrite(char *rulename,
*** 451,456
--- 453,470
errmsg("could not convert table \"%s\" to a view because it has child tables",
RelationGetRelationName(event_relation;
+ if (event_relation->rd_rel->relrowsecurity)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("could not convert table \"%s\" to a view because it has row security enabled",
+ RelationGetRelationName(event_relation;
+
+ if (relation_has_policies(event_relation))
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("could not convert table \"%s\" to a view because it has row security policies",
+ RelationGetRelationName(event_relation;
+
RelisBecomingView = true;
}
}
diff --git a/src/include/commands/policy.h b/src/include/commands/policy.h
index ac322e0..be00043 100644
*** a/src/include/commands/policy.h
--- b/src/include/commands/policy.h
*** extern Oid get_relation_policy_oid(Oid r
*** 31,35
--- 31,36
extern ObjectAddress rename_policy(RenameStmt *stmt);
+ extern bool relation_has_policies(Relation rel);
#endif /* POLICY_H */
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 72361e8..2a8db6d 100644
*** a/src/test/regress/expected/rowsecurity.out
--- b/src/test/regress/expected/rowsecurity.out
*** SELECT * FROM coll_t;
*** 2908,2913
--- 2908,2936
ROLLBACK;
--
+ -- Converting table to view
+ --
+ BEGIN;
+ SET ROW_SECURITY = FORCE;
+ CREATE TABLE t (c int);
+ CREATE POLICY p ON t USING (c % 2 = 1);
+
Re: [HACKERS] more RLS oversights
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
-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-
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 9096ee5..7781c56 100644
*** a/doc/src/sgml/catalogs.sgml
--- b/doc/src/sgml/catalogs.sgml
***
*** 5793,5798
--- 5793,5808
+ SHARED_DEPENDENCY_POLICY (r)
+
+
+The referenced object (which must be a role) is mentioned as the
+target of a dependent policy object.
+
+
+
+
+
SHARED_DEPENDENCY_PIN (p)
diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index 34fe4e2..43076c9 100644
*** a/src/backend/catalog/pg_shdepend.c
--- b/src/backend/catalog/pg_shdepend.c
*** storeObjectDescription(StringInfo descs,
*** 1083,1088
--- 1083,1090
appendStringInfo(descs, _("owner of %s"), objdesc);
else if (deptype == SHARED_DEPENDENCY_ACL)
appendStringInfo(descs, _("privileges for %s"), objdesc);
+ else if (deptype == SHARED_DEPENDENCY_POLICY)
+ appendStringInfo(descs, _("target of %s"), objdesc);
else
elog(ERROR, "unrecognized dependency type: %d",
(int) deptype);
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 17b48d4..9544f75 100644
*** a/src/backend/commands/policy.c
--- b/src/backend/commands/policy.c
***
*** 22,27
--- 22,28
#include "catalog/indexing.h"
#include "catalog/namespace.h"
#include "catalog/objectaccess.h"
+ #include "catalog/pg_authid.h"
#include "catalog/pg_policy.h"
#include "catalog/pg_type.h"
#include "commands/policy.h"
***
*** 48,54
static void RangeVarCallbackForPolicy(const RangeVar *rv,
Oid relid, Oid oldrelid, void *arg);
static char parse_policy_command(const char *cmd_name);
! static ArrayType *policy_role_list_to_array(List *roles);
/*
* Callback to RangeVarGetRelidExtended().
--- 49,55
static void RangeVarCallbackForPolicy(const RangeVar *rv,
Oid relid, Oid oldrelid, void *arg);
static char parse_policy_command(const char *cmd_name);
! static Datum *policy_role_list_to_array(List *roles, int *num_roles);
/*
* Callback to RangeVarGetRelidExtended().
*** parse_policy_command(const char *cmd_nam
*** 130,159
/*
* policy_role_list_to_array
! * helper function to convert a list of RoleSpecs to an array of role ids.
*/
! static ArrayType *
! policy_role_list_to_array(List *roles)
{
! ArrayType *role_ids;
! Datum *temp_array;
ListCell *cell;
- int num_roles;
int i = 0;
/* Handle no roles being passed in as being for public */
if (roles == NIL)
{
! temp_array = (Datum *) palloc(sizeof(Datum));
! temp_array[0] = ObjectIdGetDatum(ACL_ID_PUBLIC);
! role_ids = construct_array(temp_array, 1, OIDOID, sizeof(Oid), true,
! 'i');
! return role_ids;
}
! num_roles = list_length(roles);
! temp_array = (Datum *) palloc(num_roles * sizeof(Datum));
foreach(cell, roles)
{
--- 131,158
/*
* policy_role_list_to_array
! * helper function to convert a list of RoleSpecs to an array of
! * role id Datums.
*/
! static Datum *
! policy_role_list_to_array(List *roles, int *num_roles)
{
! Datum *role_oids;
ListCell *cell;
int i = 0;
/* Handle no roles being passed in as being for public */
if (roles == NIL)
{
! *num_roles = 1;
! role_oids = (Datum *) palloc(*num_roles * sizeof(Datum));
! role_oids[0] = ObjectIdGetDatum(ACL_ID_PUBLIC);
! return role_oids;
}
!*num_roles = li
Re: [HACKERS] more RLS oversights
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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
-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-
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 17b48d4..20ac54e 100644
*** a/src/backend/commands/policy.c
--- b/src/backend/commands/policy.c
***
*** 22,27
--- 22,28
#include "catalog/indexing.h"
#include "catalog/namespace.h"
#include "catalog/objectaccess.h"
+ #include "catalog/pg_authid.h"
#include "catalog/pg_policy.h"
#include "catalog/pg_type.h"
#include "commands/policy.h"
***
*** 48,54
static void RangeVarCallbackForPolicy(const RangeVar *rv,
Oid relid, Oid oldrelid, void *arg);
static char parse_policy_command(const char *cmd_name);
! static ArrayType *policy_role_list_to_array(List *roles);
/*
* Callback to RangeVarGetRelidExtended().
--- 49,55
static void RangeVarCallbackForPolicy(const RangeVar *rv,
Oid relid, Oid oldrelid, void *arg);
static char parse_policy_command(const char *cmd_name);
! static Datum *policy_role_list_to_array(List *roles, int *num_roles);
/*
* Callback to RangeVarGetRelidExtended().
*** parse_policy_command(const char *cmd_nam
*** 130,159
/*
* policy_role_list_to_array
! * helper function to convert a list of RoleSpecs to an array of role ids.
*/
! static ArrayType *
! policy_role_list_to_array(List *roles)
{
! ArrayType *role_ids;
! Datum *temp_array;
ListCell *cell;
- int num_roles;
int i = 0;
/* Handle no roles being passed in as being for public */
if (roles == NIL)
{
! temp_array = (Datum *) palloc(sizeof(Datum));
! temp_array[0] = ObjectIdGetDatum(ACL_ID_PUBLIC);
! role_ids = construct_array(temp_array, 1, OIDOID, sizeof(Oid), true,
! 'i');
! return role_ids;
}
! num_roles = list_length(roles);
! temp_array = (Datum *) palloc(num_roles * sizeof(Datum));
foreach(cell, roles)
{
--- 131,158
/*
* policy_role_list_to_array
! * helper function to convert a list of RoleSpecs to an array of
! * role id Datums.
*/
! static Datum *
! policy_role_list_to_array(List *roles, int *num_roles)
{
! Datum *role_oids;
ListCell *cell;
int i = 0;
/* Handle no roles being passed in as being for public */
if (roles == NIL)
{
! *num_roles = 1;
! role_oids = (Datum *) palloc(*num_roles * sizeof(Datum));
! role_oids[0] = ObjectIdGetDatum(ACL_ID_PUBLIC);
! return role_oids;
}
!*num_roles = list_length(roles);
! role_oids = (Datum *) palloc(*num_roles * sizeof(Datum));
foreach(cell, roles)
{
*** policy_role_list_to_array(List *roles)
*** 164,187
*/
if (spec->roletype == ROLESPEC_PUBLIC)
{
! 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.")));
! temp_array[0] = ObjectIdGetDatum(ACL_ID_PUBLIC);
! num_roles = 1;
! break;
}
else
! temp_array[i++] =
ObjectIdGetDatum(get_rolespec_oid((Node *) spec, false));
}
! role_ids = construct_array(temp_array, num_roles, OIDOID, sizeof(Oid), true,
! 'i');
!
! return role_ids;
}
/*
--- 163,187
*/
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);
!
! return tmp_r
Re: [HACKERS] more RLS oversights
-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- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
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
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 11efc9f..7232983 100644
*** a/src/backend/commands/policy.c
--- b/src/backend/commands/policy.c
*** CreatePolicy(CreatePolicyStmt *stmt)
*** 538,543
--- 538,547
EXPR_KIND_WHERE,
"POLICY");
+ /* Fix up collation information */
+ assign_expr_collations(qual_pstate, qual);
+ assign_expr_collations(with_check_pstate, with_check_qual);
+
/* Open pg_policy catalog */
pg_policy_rel = heap_open(PolicyRelationId, RowExclusiveLock);
*** AlterPolicy(AlterPolicyStmt *stmt)
*** 681,686
--- 685,693
EXPR_KIND_WHERE,
"POLICY");
+ /* Fix up collation information */
+ assign_expr_collations(qual_pstate, qual);
+
qual_parse_rtable = qual_pstate->p_rtable;
free_parsestate(qual_pstate);
}
*** AlterPolicy(AlterPolicyStmt *stmt)
*** 701,706
--- 708,716
EXPR_KIND_WHERE,
"POLICY");
+ /* Fix up collation information */
+ assign_expr_collations(with_check_pstate, with_check_qual);
+
with_check_parse_rtable = with_check_pstate->p_rtable;
free_parsestate(with_check_pstate);
}
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 4073c1b..eabfd93 100644
*** a/src/test/regress/expected/rowsecurity.out
--- b/src/test/regress/expected/rowsecurity.out
*** ERROR: permission denied for relation c
*** 2730,2735
--- 2730,2756
RESET SESSION AUTHORIZATION;
DROP TABLE copy_t;
--
+ -- Collation support
+ --
+ BEGIN;
+ SET row_security = force;
+ CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
+ CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
+ ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
+ SELECT (string_to_array(polqual, ':'))[7] AS inputcollid FROM pg_policy WHERE polrelid = 'coll_t'::regclass;
+inputcollid
+ --
+ inputcollid 950
+ (1 row)
+
+ SELECT * FROM coll_t;
+ c
+ -
+ bar
+ (1 row)
+
+ ROLLBACK;
+ --
-- Clean up objects
--
RESET SESSION AUTHORIZATION;
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
index fdd9b89..782824a 100644
*** a/src/test/regress/sql/rowsecurity.sql
--- b/src/test/regress/sql/rowsecurity.sql
*** RESET SESSION AUTHORIZATION;
*** 1088,1093
--- 1088,1105
DROP TABLE copy_t;
--
+ -- Collation support
+ --
+ BEGIN;
+ SET row_security = force;
+ CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
+ CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
+ ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;
+ SELECT (string_to_array(polqual, ':'))[7] AS inputcollid FROM pg_policy WHERE polrelid = 'coll_t'::regclass;
+ SELECT * FROM coll_t;
+ ROLLBACK;
+
+ --
-- Clean up objects
--
RESET SESSION AUTHORIZATION;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] more RLS oversights
Noah, * Noah Misch (n...@leadboat.com) wrote: > On Wed, Feb 25, 2015 at 11:37:24PM -0500, Stephen Frost wrote: > > * Stephen Frost (sfr...@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. > > + > > + 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 signature.asc Description: Digital signature
Re: [HACKERS] more RLS oversights
On Wed, Feb 25, 2015 at 11:37:24PM -0500, Stephen Frost wrote:
> * Stephen Frost (sfr...@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;
> +
> + 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
Re: [HACKERS] more RLS oversights
Robert, all, * Stephen Frost (sfr...@snowman.net) wrote: > * Robert Haas (robertmh...@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 signature.asc Description: Digital signature
Re: [HACKERS] more RLS oversights
Robert, * Robert Haas (robertmh...@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 signature.asc Description: Digital signature
[HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
