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: > > + <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
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,
temp_sec_context);
--- 2984,2990 ----
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
new file mode 100644
index 525794f..d0bcff9
*** a/src/backend/utils/cache/plancache.c
--- b/src/backend/utils/cache/plancache.c
*************** CreateCachedPlan(Node *raw_parse_tree,
*** 204,211 ****
plansource->total_custom_cost = 0;
plansource->num_custom_plans = 0;
plansource->hasRowSecurity = false;
! plansource->rowSecurityDisabled = InRowLevelSecurityDisabled();
! plansource->row_security_env = row_security;
plansource->planUserId = InvalidOid;
MemoryContextSwitchTo(oldcxt);
--- 204,212 ----
plansource->total_custom_cost = 0;
plansource->num_custom_plans = 0;
plansource->hasRowSecurity = false;
! plansource->row_security_env = InLocalUserIdChange() &&
! row_security == ROW_SECURITY_FORCE ?
! ROW_SECURITY_ON : row_security;
plansource->planUserId = InvalidOid;
MemoryContextSwitchTo(oldcxt);
*************** RevalidateCachedQuery(CachedPlanSource *
*** 580,586 ****
if (!OidIsValid(plansource->planUserId))
{
plansource->planUserId = GetUserId();
! plansource->row_security_env = row_security;
}
/*
--- 581,588 ----
if (!OidIsValid(plansource->planUserId))
{
plansource->planUserId = GetUserId();
! plansource->row_security_env = InLocalUserIdChange() ? ROW_SECURITY_ON :
! row_security;
}
/*
*************** RevalidateCachedQuery(CachedPlanSource *
*** 611,617 ****
* setting has been changed.
*/
if (plansource->is_valid
- && !plansource->rowSecurityDisabled
&& plansource->hasRowSecurity
&& (plansource->planUserId != GetUserId()
|| plansource->row_security_env != row_security))
--- 613,618 ----
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
new file mode 100644
index 5bf595c..6de266e
*** a/src/backend/utils/init/miscinit.c
--- b/src/backend/utils/init/miscinit.c
*************** GetAuthenticatedUserId(void)
*** 359,367 ****
* where the called functions are really supposed to be side-effect-free
* anyway, such as VACUUM/ANALYZE/REINDEX.
*
- * SECURITY_ROW_LEVEL_DISABLED indicates that we are inside an operation that
- * needs to bypass row level security checks, for example FK checks.
- *
* Unlike GetUserId, GetUserIdAndSecContext does *not* Assert that the current
* value of CurrentUserId is valid; nor does SetUserIdAndSecContext require
* the new value to be valid. In fact, these routines had better not
--- 359,364 ----
*************** InSecurityRestrictedOperation(void)
*** 404,418 ****
return (SecurityRestrictionContext & SECURITY_RESTRICTED_OPERATION) != 0;
}
- /*
- * InRowLevelSecurityDisabled - are we inside a RLS-disabled operation?
- */
- bool
- InRowLevelSecurityDisabled(void)
- {
- return (SecurityRestrictionContext & SECURITY_ROW_LEVEL_DISABLED) != 0;
- }
-
/*
* These are obsolete versions of Get/SetUserIdAndSecContext that are
--- 401,406 ----
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
new file mode 100644
index 8ebf424..c941b42
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** static struct config_enum ConfigureNames
*** 3633,3639 ****
{
{"row_security", PGC_USERSET, CONN_AUTH_SECURITY,
gettext_noop("Enable row security."),
! gettext_noop("When enabled, row security will be applied to all users.")
},
&row_security,
ROW_SECURITY_ON, row_security_options,
--- 3633,3640 ----
{
{"row_security", PGC_USERSET, CONN_AUTH_SECURITY,
gettext_noop("Enable row security."),
! gettext_noop("When enabled, row security will be applied to all users."),
! GUC_NOT_WHILE_SEC_REST
},
&row_security,
ROW_SECURITY_ON, row_security_options,
diff --git a/src/backend/utils/misc/rls.c b/src/backend/utils/misc/rls.c
new file mode 100644
index 7b8d51d..a94b9a7
*** a/src/backend/utils/misc/rls.c
--- b/src/backend/utils/misc/rls.c
*************** check_enable_rls(Oid relid, Oid checkAsU
*** 63,75 ****
if (relid < FirstNormalObjectId)
return RLS_NONE;
- /*
- * Check if we have been told to explicitly skip RLS (perhaps because this
- * is a foreign key check)
- */
- if (InRowLevelSecurityDisabled())
- return RLS_NONE;
-
tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
if (!HeapTupleIsValid(tuple))
return RLS_NONE;
--- 63,68 ----
*************** check_enable_rls(Oid relid, Oid checkAsU
*** 99,105 ****
* environment (in this case, what the current values of user_id and
* row_security are).
*/
! if (row_security != ROW_SECURITY_FORCE
&& (pg_class_ownercheck(relid, user_id)))
return RLS_NONE_ENV;
--- 92,98 ----
* environment (in this case, what the current values of user_id and
* row_security are).
*/
! if ((InLocalUserIdChange() || row_security != ROW_SECURITY_FORCE)
&& (pg_class_ownercheck(relid, user_id)))
return RLS_NONE_ENV;
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
new file mode 100644
index e0cc69f..80ac732
*** a/src/include/miscadmin.h
--- b/src/include/miscadmin.h
*************** extern int trace_recovery(int trace_leve
*** 286,292 ****
/* flags to be OR'd to form sec_context */
#define SECURITY_LOCAL_USERID_CHANGE 0x0001
#define SECURITY_RESTRICTED_OPERATION 0x0002
- #define SECURITY_ROW_LEVEL_DISABLED 0x0004
extern char *DatabasePath;
--- 286,291 ----
*************** extern void GetUserIdAndSecContext(Oid *
*** 305,311 ****
extern void SetUserIdAndSecContext(Oid userid, int sec_context);
extern bool InLocalUserIdChange(void);
extern bool InSecurityRestrictedOperation(void);
- extern bool InRowLevelSecurityDisabled(void);
extern void GetUserIdAndContext(Oid *userid, bool *sec_def_context);
extern void SetUserIdAndContext(Oid userid, bool sec_def_context);
extern void InitializeSessionUserId(const char *rolename, Oid useroid);
--- 304,309 ----
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
new file mode 100644
index 90a0180..1c50f8d
*** a/src/include/utils/plancache.h
--- b/src/include/utils/plancache.h
*************** typedef struct CachedPlanSource
*** 111,117 ****
int num_custom_plans; /* number of plans included in total */
bool hasRowSecurity; /* planned with row security? */
int row_security_env; /* row security setting when planned */
- bool rowSecurityDisabled; /* is row security disabled? */
} CachedPlanSource;
/*
--- 111,116 ----
signature.asc
Description: Digital signature
