* Noah Misch (n...@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
From 32f0298403dcf3ea72c104fa9bddadfa75f3114e Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfr...@snowman.net>
Date: Mon, 28 Sep 2015 11:28:15 -0400
Subject: [PATCH 1/2] ALTER TABLE .. FORCE ROW LEVEL SECURITY
To allow users to force RLS to always be applied, even for table owners,
add ALTER TABLE .. FORCE ROW LEVEL SECURITY.
Note that this ends up applying RLS even to referential integrity checks
and can therefore result in corruption.
---
doc/src/sgml/catalogs.sgml | 10 +++
doc/src/sgml/ref/alter_table.sgml | 17 +++++
src/backend/catalog/heap.c | 1 +
src/backend/commands/tablecmds.c | 68 ++++++++++++++++++++
src/backend/parser/gram.y | 14 +++++
src/backend/utils/misc/rls.c | 16 +++--
src/bin/pg_dump/pg_dump.c | 20 +++++-
src/bin/pg_dump/pg_dump.h | 1 +
src/bin/psql/describe.c | 44 +++++++------
src/include/catalog/pg_class.h | 72 +++++++++++-----------
src/include/nodes/parsenodes.h | 2 +
.../modules/test_ddl_deparse/test_ddl_deparse.c | 6 ++
12 files changed, 213 insertions(+), 58 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index e8bd3a1..5c6a480 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1972,6 +1972,16 @@
</row>
<row>
+ <entry><structfield>relforcerowsecurity</structfield></entry>
+ <entry><type>bool</type></entry>
+ <entry></entry>
+ <entry>
+ True if row level security (when enabled) will also apply to table owner; see
+ <link linkend="catalog-pg-policy"><structname>pg_policy</structname></link> catalog
+ </entry>
+ </row>
+
+ <row>
<entry><structfield>relispopulated</structfield></entry>
<entry><type>bool</type></entry>
<entry></entry>
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 8e35cd9..aca40f5 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -61,6 +61,8 @@ ALTER TABLE ALL IN TABLESPACE <replaceable class="PARAMETER">name</replaceable>
ENABLE ALWAYS RULE <replaceable class="PARAMETER">rewrite_rule_name</replaceable>
DISABLE ROW LEVEL SECURITY
ENABLE ROW LEVEL SECURITY
+ FORCE ROW LEVEL SECURITY
+ NO FORCE ROW LEVEL SECURITY
CLUSTER ON <replaceable class="PARAMETER">index_name</replaceable>
SET WITHOUT CLUSTER
SET WITH OIDS
@@ -434,6 +436,21 @@ ALTER TABLE ALL IN TABLESPACE <replaceable class="PARAMETER">name</replaceable>
</varlistentry>
<varlistentry>
+ <term><literal>NO FORCE</literal>/<literal>FORCE ROW LEVEL SECURITY</literal></term>
+ <listitem>
+ <para>
+ These forms control the application of row security policies belonging
+ to the table when the user is the table owner. If enabled, row level
+ security policies will be applied when the user is the table owner. If
+ disabled (the default) then row level security will not be applied when
+ the user is the table owner.
+ See also
+ <xref linkend="SQL-CREATEPOLICY">.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><literal>CLUSTER ON</literal></term>
<listitem>
<para>
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index d04e94d..7d7d062 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -802,6 +802,7 @@ InsertPgClassTuple(Relation pg_class_desc,
values[Anum_pg_class_relhasrules - 1] = BoolGetDatum(rd_rel->relhasrules);
values[Anum_pg_class_relhastriggers - 1] = BoolGetDatum(rd_rel->relhastriggers);
values[Anum_pg_class_relrowsecurity - 1] = BoolGetDatum(rd_rel->relrowsecurity);
+ values[Anum_pg_class_relforcerowsecurity - 1] = BoolGetDatum(rd_rel->relforcerowsecurity);
values[Anum_pg_class_relhassubclass - 1] = BoolGetDatum(rd_rel->relhassubclass);
values[Anum_pg_class_relispopulated - 1] = BoolGetDatum(rd_rel->relispopulated);
values[Anum_pg_class_relreplident - 1] = CharGetDatum(rd_rel->relreplident);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 126b119..b494dbf 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -419,6 +419,8 @@ static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKM
static void ATExecGenericOptions(Relation rel, List *options);
static void ATExecEnableRowSecurity(Relation rel);
static void ATExecDisableRowSecurity(Relation rel);
+static void ATExecForceRowSecurity(Relation rel);
+static void ATExecNoForceRowSecurity(Relation rel);
static void copy_relation_data(SMgrRelation rel, SMgrRelation dst,
ForkNumber forkNum, char relpersistence);
@@ -2930,6 +2932,8 @@ AlterTableGetLockLevel(List *cmds)
case AT_SetNotNull:
case AT_EnableRowSecurity:
case AT_DisableRowSecurity:
+ case AT_ForceRowSecurity:
+ case AT_NoForceRowSecurity:
cmd_lockmode = AccessExclusiveLock;
break;
@@ -3351,6 +3355,8 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
case AT_DropOf: /* NOT OF */
case AT_EnableRowSecurity:
case AT_DisableRowSecurity:
+ case AT_ForceRowSecurity:
+ case AT_NoForceRowSecurity:
ATSimplePermissions(rel, ATT_TABLE);
/* These commands never recurse */
/* No command-specific prep needed */
@@ -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;
case AT_GenericOptions:
ATExecGenericOptions(rel, (List *) cmd->def);
break;
@@ -11067,6 +11079,62 @@ ATExecDisableRowSecurity(Relation rel)
}
/*
+ * ALTER TABLE FORCE/NO FORCE ROW LEVEL SECURITY
+ */
+static void
+ATExecForceRowSecurity(Relation rel)
+{
+ Relation pg_class;
+ Oid relid;
+ HeapTuple tuple;
+
+ relid = RelationGetRelid(rel);
+
+ pg_class = heap_open(RelationRelationId, RowExclusiveLock);
+
+ tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relid));
+
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", relid);
+
+ ((Form_pg_class) GETSTRUCT(tuple))->relforcerowsecurity = true;
+ simple_heap_update(pg_class, &tuple->t_self, tuple);
+
+ /* keep catalog indexes current */
+ CatalogUpdateIndexes(pg_class, tuple);
+
+ heap_close(pg_class, RowExclusiveLock);
+ heap_freetuple(tuple);
+}
+
+static void
+ATExecNoForceRowSecurity(Relation rel)
+{
+ Relation pg_class;
+ Oid relid;
+ HeapTuple tuple;
+
+ relid = RelationGetRelid(rel);
+
+ /* Pull the record for this relation and update it */
+ pg_class = heap_open(RelationRelationId, RowExclusiveLock);
+
+ tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relid));
+
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", relid);
+
+ ((Form_pg_class) GETSTRUCT(tuple))->relforcerowsecurity = false;
+ simple_heap_update(pg_class, &tuple->t_self, tuple);
+
+ /* keep catalog indexes current */
+ CatalogUpdateIndexes(pg_class, tuple);
+
+ heap_close(pg_class, RowExclusiveLock);
+ heap_freetuple(tuple);
+}
+
+/*
* ALTER FOREIGN TABLE <name> OPTIONS (...)
*/
static void
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index fb84937..2168a08 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -2353,6 +2353,20 @@ alter_table_cmd:
n->subtype = AT_DisableRowSecurity;
$$ = (Node *)n;
}
+ /* ALTER TABLE <name> FORCE ROW LEVEL SECURITY */
+ | FORCE ROW LEVEL SECURITY
+ {
+ AlterTableCmd *n = makeNode(AlterTableCmd);
+ n->subtype = AT_ForceRowSecurity;
+ $$ = (Node *)n;
+ }
+ /* ALTER TABLE <name> NO FORCE ROW LEVEL SECURITY */
+ | NO FORCE ROW LEVEL SECURITY
+ {
+ AlterTableCmd *n = makeNode(AlterTableCmd);
+ n->subtype = AT_NoForceRowSecurity;
+ $$ = (Node *)n;
+ }
| alter_generic_options
{
AlterTableCmd *n = makeNode(AlterTableCmd);
diff --git a/src/backend/utils/misc/rls.c b/src/backend/utils/misc/rls.c
index c900c98..b4540ff 100644
--- a/src/backend/utils/misc/rls.c
+++ b/src/backend/utils/misc/rls.c
@@ -57,6 +57,7 @@ check_enable_rls(Oid relid, Oid checkAsUser, bool noError)
HeapTuple tuple;
Form_pg_class classform;
bool relrowsecurity;
+ bool relforcerowsecurity;
Oid user_id = checkAsUser ? checkAsUser : GetUserId();
/* Nothing to do for built-in relations */
@@ -70,6 +71,7 @@ check_enable_rls(Oid relid, Oid checkAsUser, bool noError)
classform = (Form_pg_class) GETSTRUCT(tuple);
relrowsecurity = classform->relrowsecurity;
+ relforcerowsecurity = classform->relforcerowsecurity;
ReleaseSysCache(tuple);
@@ -80,12 +82,18 @@ check_enable_rls(Oid relid, Oid checkAsUser, bool noError)
/*
* Check permissions
*
- * Table owners always bypass RLS. Note that superuser is always
- * considered an owner. Return RLS_NONE_ENV to indicate that this
- * decision depends on the environment (in this case, the user_id).
+ * Table owners always bypass RLS, unless the table has been set to
+ * FORCE ROW SECURITY. Note that superuser is always considered an
+ * owner. Return RLS_NONE_ENV to indicate that this decision depends
+ * on the environment (in this case, the user_id).
*/
if (pg_class_ownercheck(relid, user_id))
- return RLS_NONE_ENV;
+ {
+ if (relforcerowsecurity)
+ return RLS_ENABLED;
+ else
+ return RLS_NONE_ENV;
+ }
/*
* If the row_security GUC is 'off', check if the user has permission to
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 56c256d..e5be640 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4540,6 +4540,7 @@ getTables(Archive *fout, DumpOptions *dopt, int *numTables)
int i_relhasindex;
int i_relhasrules;
int i_relrowsec;
+ int i_relforcerowsec;
int i_relhasoids;
int i_relfrozenxid;
int i_relminmxid;
@@ -4593,7 +4594,7 @@ getTables(Archive *fout, DumpOptions *dopt, int *numTables)
"(%s c.relowner) AS rolname, "
"c.relchecks, c.relhastriggers, "
"c.relhasindex, c.relhasrules, c.relhasoids, "
- "c.relrowsecurity, "
+ "c.relrowsecurity, c.relforcerowsecurity, "
"c.relfrozenxid, c.relminmxid, tc.oid AS toid, "
"tc.relfrozenxid AS tfrozenxid, "
"tc.relminmxid AS tminmxid, "
@@ -4635,6 +4636,7 @@ getTables(Archive *fout, DumpOptions *dopt, int *numTables)
"c.relchecks, c.relhastriggers, "
"c.relhasindex, c.relhasrules, c.relhasoids, "
"'f'::bool AS relrowsecurity, "
+ "'f'::bool AS relforcerowsecurity, "
"c.relfrozenxid, c.relminmxid, tc.oid AS toid, "
"tc.relfrozenxid AS tfrozenxid, "
"tc.relminmxid AS tminmxid, "
@@ -4676,6 +4678,7 @@ getTables(Archive *fout, DumpOptions *dopt, int *numTables)
"c.relchecks, c.relhastriggers, "
"c.relhasindex, c.relhasrules, c.relhasoids, "
"'f'::bool AS relrowsecurity, "
+ "'f'::bool AS relforcerowsecurity, "
"c.relfrozenxid, c.relminmxid, tc.oid AS toid, "
"tc.relfrozenxid AS tfrozenxid, "
"tc.relminmxid AS tminmxid, "
@@ -4717,6 +4720,7 @@ getTables(Archive *fout, DumpOptions *dopt, int *numTables)
"c.relchecks, c.relhastriggers, "
"c.relhasindex, c.relhasrules, c.relhasoids, "
"'f'::bool AS relrowsecurity, "
+ "'f'::bool AS relforcerowsecurity, "
"c.relfrozenxid, 0 AS relminmxid, tc.oid AS toid, "
"tc.relfrozenxid AS tfrozenxid, "
"0 AS tminmxid, "
@@ -4756,6 +4760,7 @@ getTables(Archive *fout, DumpOptions *dopt, int *numTables)
"c.relchecks, c.relhastriggers, "
"c.relhasindex, c.relhasrules, c.relhasoids, "
"'f'::bool AS relrowsecurity, "
+ "'f'::bool AS relforcerowsecurity, "
"c.relfrozenxid, 0 AS relminmxid, tc.oid AS toid, "
"tc.relfrozenxid AS tfrozenxid, "
"0 AS tminmxid, "
@@ -4794,6 +4799,7 @@ getTables(Archive *fout, DumpOptions *dopt, int *numTables)
"c.relchecks, c.relhastriggers, "
"c.relhasindex, c.relhasrules, c.relhasoids, "
"'f'::bool AS relrowsecurity, "
+ "'f'::bool AS relforcerowsecurity, "
"c.relfrozenxid, 0 AS relminmxid, tc.oid AS toid, "
"tc.relfrozenxid AS tfrozenxid, "
"0 AS tminmxid, "
@@ -4832,6 +4838,7 @@ getTables(Archive *fout, DumpOptions *dopt, int *numTables)
"c.relchecks, (c.reltriggers <> 0) AS relhastriggers, "
"c.relhasindex, c.relhasrules, c.relhasoids, "
"'f'::bool AS relrowsecurity, "
+ "'f'::bool AS relforcerowsecurity, "
"c.relfrozenxid, 0 AS relminmxid, tc.oid AS toid, "
"tc.relfrozenxid AS tfrozenxid, "
"0 AS tminmxid, "
@@ -4870,6 +4877,7 @@ getTables(Archive *fout, DumpOptions *dopt, int *numTables)
"relchecks, (reltriggers <> 0) AS relhastriggers, "
"relhasindex, relhasrules, relhasoids, "
"'f'::bool AS relrowsecurity, "
+ "'f'::bool AS relforcerowsecurity, "
"0 AS relfrozenxid, 0 AS relminmxid,"
"0 AS toid, "
"0 AS tfrozenxid, 0 AS tminmxid,"
@@ -4907,6 +4915,7 @@ getTables(Archive *fout, DumpOptions *dopt, int *numTables)
"relchecks, (reltriggers <> 0) AS relhastriggers, "
"relhasindex, relhasrules, relhasoids, "
"'f'::bool AS relrowsecurity, "
+ "'f'::bool AS relforcerowsecurity, "
"0 AS relfrozenxid, 0 AS relminmxid,"
"0 AS toid, "
"0 AS tfrozenxid, 0 AS tminmxid,"
@@ -4940,6 +4949,7 @@ getTables(Archive *fout, DumpOptions *dopt, int *numTables)
"relchecks, (reltriggers <> 0) AS relhastriggers, "
"relhasindex, relhasrules, relhasoids, "
"'f'::bool AS relrowsecurity, "
+ "'f'::bool AS relforcerowsecurity, "
"0 AS relfrozenxid, 0 AS relminmxid,"
"0 AS toid, "
"0 AS tfrozenxid, 0 AS tminmxid,"
@@ -4968,6 +4978,7 @@ getTables(Archive *fout, DumpOptions *dopt, int *numTables)
"relhasindex, relhasrules, "
"'t'::bool AS relhasoids, "
"'f'::bool AS relrowsecurity, "
+ "'f'::bool AS relforcerowsecurity, "
"0 AS relfrozenxid, 0 AS relminmxid,"
"0 AS toid, "
"0 AS tfrozenxid, 0 AS tminmxid,"
@@ -5006,6 +5017,7 @@ getTables(Archive *fout, DumpOptions *dopt, int *numTables)
"relhasindex, relhasrules, "
"'t'::bool AS relhasoids, "
"'f'::bool AS relrowsecurity, "
+ "'f'::bool AS relforcerowsecurity, "
"0 AS relfrozenxid, 0 AS relminmxid,"
"0 AS toid, "
"0 AS tfrozenxid, 0 AS tminmxid,"
@@ -5054,6 +5066,7 @@ getTables(Archive *fout, DumpOptions *dopt, int *numTables)
i_relhasindex = PQfnumber(res, "relhasindex");
i_relhasrules = PQfnumber(res, "relhasrules");
i_relrowsec = PQfnumber(res, "relrowsecurity");
+ i_relforcerowsec = PQfnumber(res, "relforcerowsecurity");
i_relhasoids = PQfnumber(res, "relhasoids");
i_relfrozenxid = PQfnumber(res, "relfrozenxid");
i_relminmxid = PQfnumber(res, "relminmxid");
@@ -5106,6 +5119,7 @@ getTables(Archive *fout, DumpOptions *dopt, int *numTables)
tblinfo[i].hasrules = (strcmp(PQgetvalue(res, i, i_relhasrules), "t") == 0);
tblinfo[i].hastriggers = (strcmp(PQgetvalue(res, i, i_relhastriggers), "t") == 0);
tblinfo[i].rowsec = (strcmp(PQgetvalue(res, i, i_relrowsec), "t") == 0);
+ tblinfo[i].forcerowsec = (strcmp(PQgetvalue(res, i, i_relforcerowsec), "t") == 0);
tblinfo[i].hasoids = (strcmp(PQgetvalue(res, i, i_relhasoids), "t") == 0);
tblinfo[i].relispopulated = (strcmp(PQgetvalue(res, i, i_relispopulated), "t") == 0);
tblinfo[i].relreplident = *(PQgetvalue(res, i, i_relreplident));
@@ -14412,6 +14426,10 @@ dumpTableSchema(Archive *fout, DumpOptions *dopt, TableInfo *tbinfo)
appendPQExpBuffer(q, "\nALTER TABLE ONLY %s SET WITH OIDS;\n",
fmtId(tbinfo->dobj.name));
+ if (tbinfo->forcerowsec)
+ appendPQExpBuffer(q, "\nALTER TABLE ONLY %s FORCE ROW LEVEL SECURITY;\n",
+ fmtId(tbinfo->dobj.name));
+
if (dopt->binary_upgrade)
binary_upgrade_extension_member(q, &tbinfo->dobj, labelq->data);
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index b40b816..3c64a82 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -211,6 +211,7 @@ typedef struct _tableInfo
bool hasrules; /* does it have any rules? */
bool hastriggers; /* does it have any triggers? */
bool rowsec; /* is row security enabled? */
+ bool forcerowsec; /* is row security forced? */
bool hasoids; /* does it have OIDs? */
uint32 frozenxid; /* for restore frozen xid */
uint32 minmxid; /* for restore min multi xid */
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 898f8b3..92ed6e2 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1234,6 +1234,7 @@ describeOneTableDetails(const char *schemaname,
bool hasrules;
bool hastriggers;
bool rowsecurity;
+ bool forcerowsecurity;
bool hasoids;
Oid tablespace;
char *reloptions;
@@ -1259,8 +1260,8 @@ describeOneTableDetails(const char *schemaname,
{
printfPQExpBuffer(&buf,
"SELECT c.relchecks, c.relkind, c.relhasindex, c.relhasrules, "
- "c.relhastriggers, c.relrowsecurity, c.relhasoids, "
- "%s, c.reltablespace, "
+ "c.relhastriggers, c.relrowsecurity, c.relforcerowsecurity, "
+ "c.relhasoids, %s, c.reltablespace, "
"CASE WHEN c.reloftype = 0 THEN '' ELSE c.reloftype::pg_catalog.regtype::pg_catalog.text END, "
"c.relpersistence, c.relreplident\n"
"FROM pg_catalog.pg_class c\n "
@@ -1276,7 +1277,7 @@ describeOneTableDetails(const char *schemaname,
{
printfPQExpBuffer(&buf,
"SELECT c.relchecks, c.relkind, c.relhasindex, c.relhasrules, "
- "c.relhastriggers, false, c.relhasoids, "
+ "c.relhastriggers, false, false, c.relhasoids, "
"%s, c.reltablespace, "
"CASE WHEN c.reloftype = 0 THEN '' ELSE c.reloftype::pg_catalog.regtype::pg_catalog.text END, "
"c.relpersistence, c.relreplident\n"
@@ -1293,7 +1294,7 @@ describeOneTableDetails(const char *schemaname,
{
printfPQExpBuffer(&buf,
"SELECT c.relchecks, c.relkind, c.relhasindex, c.relhasrules, "
- "c.relhastriggers, false, c.relhasoids, "
+ "c.relhastriggers, false, false, c.relhasoids, "
"%s, c.reltablespace, "
"CASE WHEN c.reloftype = 0 THEN '' ELSE c.reloftype::pg_catalog.regtype::pg_catalog.text END, "
"c.relpersistence\n"
@@ -1310,7 +1311,7 @@ describeOneTableDetails(const char *schemaname,
{
printfPQExpBuffer(&buf,
"SELECT c.relchecks, c.relkind, c.relhasindex, c.relhasrules, "
- "c.relhastriggers, false, c.relhasoids, "
+ "c.relhastriggers, false, false, c.relhasoids, "
"%s, c.reltablespace, "
"CASE WHEN c.reloftype = 0 THEN '' ELSE c.reloftype::pg_catalog.regtype::pg_catalog.text END\n"
"FROM pg_catalog.pg_class c\n "
@@ -1326,7 +1327,7 @@ describeOneTableDetails(const char *schemaname,
{
printfPQExpBuffer(&buf,
"SELECT c.relchecks, c.relkind, c.relhasindex, c.relhasrules, "
- "c.relhastriggers, false, c.relhasoids, "
+ "c.relhastriggers, false, false, c.relhasoids, "
"%s, c.reltablespace\n"
"FROM pg_catalog.pg_class c\n "
"LEFT JOIN pg_catalog.pg_class tc ON (c.reltoastrelid = tc.oid)\n"
@@ -1341,7 +1342,7 @@ describeOneTableDetails(const char *schemaname,
{
printfPQExpBuffer(&buf,
"SELECT relchecks, relkind, relhasindex, relhasrules, "
- "reltriggers <> 0, false, relhasoids, "
+ "reltriggers <> 0, false, false, relhasoids, "
"%s, reltablespace\n"
"FROM pg_catalog.pg_class WHERE oid = '%s';",
(verbose ?
@@ -1352,7 +1353,7 @@ describeOneTableDetails(const char *schemaname,
{
printfPQExpBuffer(&buf,
"SELECT relchecks, relkind, relhasindex, relhasrules, "
- "reltriggers <> 0, false, relhasoids, "
+ "reltriggers <> 0, false, false, relhasoids, "
"'', reltablespace\n"
"FROM pg_catalog.pg_class WHERE oid = '%s';",
oid);
@@ -1361,7 +1362,7 @@ describeOneTableDetails(const char *schemaname,
{
printfPQExpBuffer(&buf,
"SELECT relchecks, relkind, relhasindex, relhasrules, "
- "reltriggers <> 0, false, relhasoids, "
+ "reltriggers <> 0, false, false, relhasoids, "
"'', ''\n"
"FROM pg_catalog.pg_class WHERE oid = '%s';",
oid);
@@ -1385,18 +1386,19 @@ describeOneTableDetails(const char *schemaname,
tableinfo.hasrules = strcmp(PQgetvalue(res, 0, 3), "t") == 0;
tableinfo.hastriggers = strcmp(PQgetvalue(res, 0, 4), "t") == 0;
tableinfo.rowsecurity = strcmp(PQgetvalue(res, 0, 5), "t") == 0;
- tableinfo.hasoids = strcmp(PQgetvalue(res, 0, 6), "t") == 0;
+ tableinfo.forcerowsecurity = strcmp(PQgetvalue(res, 0, 6), "t") == 0;
+ tableinfo.hasoids = strcmp(PQgetvalue(res, 0, 7), "t") == 0;
tableinfo.reloptions = (pset.sversion >= 80200) ?
- pg_strdup(PQgetvalue(res, 0, 7)) : NULL;
+ pg_strdup(PQgetvalue(res, 0, 8)) : NULL;
tableinfo.tablespace = (pset.sversion >= 80000) ?
- atooid(PQgetvalue(res, 0, 8)) : 0;
+ atooid(PQgetvalue(res, 0, 9)) : 0;
tableinfo.reloftype = (pset.sversion >= 90000 &&
- strcmp(PQgetvalue(res, 0, 9), "") != 0) ?
- pg_strdup(PQgetvalue(res, 0, 9)) : NULL;
+ strcmp(PQgetvalue(res, 0, 10), "") != 0) ?
+ pg_strdup(PQgetvalue(res, 0, 10)) : NULL;
tableinfo.relpersistence = (pset.sversion >= 90100) ?
- *(PQgetvalue(res, 0, 10)) : 0;
+ *(PQgetvalue(res, 0, 11)) : 0;
tableinfo.relreplident = (pset.sversion >= 90400) ?
- *(PQgetvalue(res, 0, 11)) : 'd';
+ *(PQgetvalue(res, 0, 12)) : 'd';
PQclear(res);
res = NULL;
@@ -2057,12 +2059,18 @@ describeOneTableDetails(const char *schemaname,
* there aren't policies, or RLS isn't enabled but there are
* policies
*/
- if (tableinfo.rowsecurity && tuples > 0)
+ if (tableinfo.rowsecurity && !tableinfo.forcerowsecurity && tuples > 0)
printTableAddFooter(&cont, _("Policies:"));
- if (tableinfo.rowsecurity && tuples == 0)
+ if (tableinfo.rowsecurity && tableinfo.forcerowsecurity && tuples > 0)
+ printTableAddFooter(&cont, _("Policies (Forced Row Security Enabled):"));
+
+ if (tableinfo.rowsecurity && !tableinfo.forcerowsecurity && tuples == 0)
printTableAddFooter(&cont, _("Policies (Row Security Enabled): (None)"));
+ if (tableinfo.rowsecurity && tableinfo.forcerowsecurity && tuples == 0)
+ printTableAddFooter(&cont, _("Policies (Forced Row Security Enabled): (None)"));
+
if (!tableinfo.rowsecurity && tuples > 0)
printTableAddFooter(&cont, _("Policies (Row Security Disabled):"));
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index 25247b5..06d287e 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -66,6 +66,7 @@ CATALOG(pg_class,1259) BKI_BOOTSTRAP BKI_ROWTYPE_OID(83) BKI_SCHEMA_MACRO
bool relhastriggers; /* has (or has had) any TRIGGERs */
bool relhassubclass; /* has (or has had) derived classes */
bool relrowsecurity; /* row security is enabled or not */
+ bool relforcerowsecurity; /* row security forced for owners or not */
bool relispopulated; /* matview currently holds query results */
char relreplident; /* see REPLICA_IDENTITY_xxx constants */
TransactionId relfrozenxid; /* all Xids < this are frozen in this rel */
@@ -95,37 +96,38 @@ typedef FormData_pg_class *Form_pg_class;
* ----------------
*/
-#define Natts_pg_class 30
-#define Anum_pg_class_relname 1
-#define Anum_pg_class_relnamespace 2
-#define Anum_pg_class_reltype 3
-#define Anum_pg_class_reloftype 4
-#define Anum_pg_class_relowner 5
-#define Anum_pg_class_relam 6
-#define Anum_pg_class_relfilenode 7
-#define Anum_pg_class_reltablespace 8
-#define Anum_pg_class_relpages 9
-#define Anum_pg_class_reltuples 10
-#define Anum_pg_class_relallvisible 11
-#define Anum_pg_class_reltoastrelid 12
-#define Anum_pg_class_relhasindex 13
-#define Anum_pg_class_relisshared 14
-#define Anum_pg_class_relpersistence 15
-#define Anum_pg_class_relkind 16
-#define Anum_pg_class_relnatts 17
-#define Anum_pg_class_relchecks 18
-#define Anum_pg_class_relhasoids 19
-#define Anum_pg_class_relhaspkey 20
-#define Anum_pg_class_relhasrules 21
-#define Anum_pg_class_relhastriggers 22
-#define Anum_pg_class_relhassubclass 23
-#define Anum_pg_class_relrowsecurity 24
-#define Anum_pg_class_relispopulated 25
-#define Anum_pg_class_relreplident 26
-#define Anum_pg_class_relfrozenxid 27
-#define Anum_pg_class_relminmxid 28
-#define Anum_pg_class_relacl 29
-#define Anum_pg_class_reloptions 30
+#define Natts_pg_class 31
+#define Anum_pg_class_relname 1
+#define Anum_pg_class_relnamespace 2
+#define Anum_pg_class_reltype 3
+#define Anum_pg_class_reloftype 4
+#define Anum_pg_class_relowner 5
+#define Anum_pg_class_relam 6
+#define Anum_pg_class_relfilenode 7
+#define Anum_pg_class_reltablespace 8
+#define Anum_pg_class_relpages 9
+#define Anum_pg_class_reltuples 10
+#define Anum_pg_class_relallvisible 11
+#define Anum_pg_class_reltoastrelid 12
+#define Anum_pg_class_relhasindex 13
+#define Anum_pg_class_relisshared 14
+#define Anum_pg_class_relpersistence 15
+#define Anum_pg_class_relkind 16
+#define Anum_pg_class_relnatts 17
+#define Anum_pg_class_relchecks 18
+#define Anum_pg_class_relhasoids 19
+#define Anum_pg_class_relhaspkey 20
+#define Anum_pg_class_relhasrules 21
+#define Anum_pg_class_relhastriggers 22
+#define Anum_pg_class_relhassubclass 23
+#define Anum_pg_class_relrowsecurity 24
+#define Anum_pg_class_relforcerowsecurity 25
+#define Anum_pg_class_relispopulated 26
+#define Anum_pg_class_relreplident 27
+#define Anum_pg_class_relfrozenxid 28
+#define Anum_pg_class_relminmxid 29
+#define Anum_pg_class_relacl 30
+#define Anum_pg_class_reloptions 31
/* ----------------
* initial contents of pg_class
@@ -140,13 +142,13 @@ typedef FormData_pg_class *Form_pg_class;
* Note: "3" in the relfrozenxid column stands for FirstNormalTransactionId;
* similarly, "1" in relminmxid stands for FirstMultiXactId
*/
-DATA(insert OID = 1247 ( pg_type PGNSP 71 0 PGUID 0 0 0 0 0 0 0 f f p r 30 0 t f f f f f t n 3 1 _null_ _null_ ));
+DATA(insert OID = 1247 ( pg_type PGNSP 71 0 PGUID 0 0 0 0 0 0 0 f f p r 30 0 t f f f f f f t n 3 1 _null_ _null_ ));
DESCR("");
-DATA(insert OID = 1249 ( pg_attribute PGNSP 75 0 PGUID 0 0 0 0 0 0 0 f f p r 21 0 f f f f f f t n 3 1 _null_ _null_ ));
+DATA(insert OID = 1249 ( pg_attribute PGNSP 75 0 PGUID 0 0 0 0 0 0 0 f f p r 21 0 f f f f f f f t n 3 1 _null_ _null_ ));
DESCR("");
-DATA(insert OID = 1255 ( pg_proc PGNSP 81 0 PGUID 0 0 0 0 0 0 0 f f p r 29 0 t f f f f f t n 3 1 _null_ _null_ ));
+DATA(insert OID = 1255 ( pg_proc PGNSP 81 0 PGUID 0 0 0 0 0 0 0 f f p r 29 0 t f f f f f f t n 3 1 _null_ _null_ ));
DESCR("");
-DATA(insert OID = 1259 ( pg_class PGNSP 83 0 PGUID 0 0 0 0 0 0 0 f f p r 30 0 t f f f f f t n 3 1 _null_ _null_ ));
+DATA(insert OID = 1259 ( pg_class PGNSP 83 0 PGUID 0 0 0 0 0 0 0 f f p r 31 0 t f f f f f f t n 3 1 _null_ _null_ ));
DESCR("");
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 770866a..fdf19fa 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1514,6 +1514,8 @@ typedef enum AlterTableType
AT_ReplicaIdentity, /* REPLICA IDENTITY */
AT_EnableRowSecurity, /* ENABLE ROW SECURITY */
AT_DisableRowSecurity, /* DISABLE ROW SECURITY */
+ AT_ForceRowSecurity, /* FORCE ROW SECURITY */
+ AT_NoForceRowSecurity, /* NO FORCE ROW SECURITY */
AT_GenericOptions /* OPTIONS (...) */
} AlterTableType;
diff --git a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
index e2dc4b5..4fca737 100644
--- a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
+++ b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
@@ -275,6 +275,12 @@ get_altertable_subcmdtypes(PG_FUNCTION_ARGS)
case AT_DisableRowSecurity:
strtype = "DISABLE ROW SECURITY";
break;
+ case AT_ForceRowSecurity:
+ strtype = "FORCE ROW SECURITY";
+ break;
+ case AT_NoForceRowSecurity:
+ strtype = "NO FORCE ROW SECURITY";
+ break;
case AT_GenericOptions:
strtype = "SET OPTIONS";
break;
--
1.9.1
From 2ffb7729679faf5def2d39f224947fa2ca3db364 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfr...@snowman.net>
Date: Mon, 28 Sep 2015 16:47:42 -0400
Subject: [PATCH 2/2] Add SECURITY_NOFORCE_RLS context
To avoid data corruption when ALTER TABLE .. FORCE ROW SECURITY is
being used, add a SECURITY_NOFORCE_RLS security context and use it
during referential integrity checks. Note that this is only set during
referential integrity checks and is only checked after we have already
checked that the current user is the owner of the relation (which should
always be the case during referential integrity checks).
---
src/backend/utils/adt/ri_triggers.c | 6 ++++--
src/backend/utils/init/miscinit.c | 18 +++++++++++++++++-
src/backend/utils/misc/rls.c | 17 ++++++++++++++++-
src/include/miscadmin.h | 2 ++
4 files changed, 39 insertions(+), 4 deletions(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 018cb99..6569fdb 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -3014,7 +3014,8 @@ ri_PlanCheck(const char *querystr, int nargs, Oid *argtypes,
/* Switch to proper UID to perform check as */
GetUserIdAndSecContext(&save_userid, &save_sec_context);
SetUserIdAndSecContext(RelationGetForm(query_rel)->relowner,
- save_sec_context | SECURITY_LOCAL_USERID_CHANGE);
+ save_sec_context | SECURITY_LOCAL_USERID_CHANGE |
+ SECURITY_NOFORCE_RLS);
/* Create the plan */
qplan = SPI_prepare(querystr, nargs, argtypes);
@@ -3134,7 +3135,8 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
/* Switch to proper UID to perform check as */
GetUserIdAndSecContext(&save_userid, &save_sec_context);
SetUserIdAndSecContext(RelationGetForm(query_rel)->relowner,
- save_sec_context | SECURITY_LOCAL_USERID_CHANGE);
+ save_sec_context | SECURITY_LOCAL_USERID_CHANGE |
+ SECURITY_NOFORCE_RLS);
/* Finally we can run the query. */
spi_result = SPI_execute_snapshot(qplan,
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index f0099d3..e871fef 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -341,7 +341,7 @@ GetAuthenticatedUserId(void)
* GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID
* and the SecurityRestrictionContext flags.
*
- * Currently there are two valid bits in SecurityRestrictionContext:
+ * Currently there are three valid bits in SecurityRestrictionContext:
*
* SECURITY_LOCAL_USERID_CHANGE indicates that we are inside an operation
* that is temporarily changing CurrentUserId via these functions. This is
@@ -359,6 +359,13 @@ GetAuthenticatedUserId(void)
* where the called functions are really supposed to be side-effect-free
* anyway, such as VACUUM/ANALYZE/REINDEX.
*
+ * SECURITY_NOFORCE_RLS indicates that we are inside an operation which should
+ * ignore the FORCE ROW LEVEL SECURITY per-table indication. This is used to
+ * ensure that FORCE RLS does not mistakenly break referential integrity
+ * checks. Note that this is intentionally only checked when running as the
+ * owner of the table (which should always be the case for referential
+ * integrity 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
@@ -401,6 +408,15 @@ InSecurityRestrictedOperation(void)
return (SecurityRestrictionContext & SECURITY_RESTRICTED_OPERATION) != 0;
}
+/*
+ * InNoForceRLSOperation - are we ignoring FORCE ROW LEVEL SECURITY ?
+ */
+bool
+InNoForceRLSOperation(void)
+{
+ return (SecurityRestrictionContext & SECURITY_NOFORCE_RLS) != 0;
+}
+
/*
* These are obsolete versions of Get/SetUserIdAndSecContext that are
diff --git a/src/backend/utils/misc/rls.c b/src/backend/utils/misc/rls.c
index b4540ff..e68de47 100644
--- a/src/backend/utils/misc/rls.c
+++ b/src/backend/utils/misc/rls.c
@@ -89,7 +89,22 @@ check_enable_rls(Oid relid, Oid checkAsUser, bool noError)
*/
if (pg_class_ownercheck(relid, user_id))
{
- if (relforcerowsecurity)
+ /*
+ * If FORCE ROW LEVEL SECURITY has been set on the relation then we
+ * return RLS_ENABLED to indicate that RLS should still be applied,
+ * except if we are in a SECURITY_NOFORCE_RLS context, in which case
+ * we return RLS_NONE_ENV.
+ *
+ * The SECURITY_NOFORCE_RLS indicates that we should not apply RLS even
+ * if the table FORCE RLS set- IF the current user is the owner. This
+ * is specifically to ensure that referential integrity checks are
+ * able to still run correctly.
+ *
+ * This is intentionally only done after we have checked that the user
+ * is the table owner, which should always be the case for referential
+ * integrity checks.
+ */
+ if (relforcerowsecurity && !InNoForceRLSOperation())
return RLS_ENABLED;
else
return RLS_NONE_ENV;
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 80ac732..a737dd2 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -286,6 +286,7 @@ extern int trace_recovery(int trace_level);
/* flags to be OR'd to form sec_context */
#define SECURITY_LOCAL_USERID_CHANGE 0x0001
#define SECURITY_RESTRICTED_OPERATION 0x0002
+#define SECURITY_NOFORCE_RLS 0x0004
extern char *DatabasePath;
@@ -304,6 +305,7 @@ extern void GetUserIdAndSecContext(Oid *userid, int *sec_context);
extern void SetUserIdAndSecContext(Oid userid, int sec_context);
extern bool InLocalUserIdChange(void);
extern bool InSecurityRestrictedOperation(void);
+extern bool InNoForceRLSOperation(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);
--
1.9.1
signature.asc
Description: Digital signature
