Skip to content

Commit cfa928f

Browse files
jconwayjconway
committed
Plug RLS related information leak in pg_stats view.
The pg_stats view is supposed to be restricted to only show rows about tables the user can read. However, it sometimes can leak information which could not otherwise be seen when row level security is enabled. Fix that by not showing pg_stats rows to users that would be subject to RLS on the table the row is related to. This is done by creating/using the newly introduced SQL visible function, row_security_active(). Along the way, clean up three call sites of check_enable_rls(). The second argument of that function should only be specified as other than InvalidOid when we are checking as a different user than the current one, as in when querying through a view. These sites were passing GetUserId() instead of InvalidOid, which can cause the function to return incorrect results if the current user has the BYPASSRLS privilege and row_security has been set to OFF. Additionally fix a bug causing RI Trigger error messages to unintentionally leak information when RLS is enabled, and other minor cleanup and improvements. Also add WITH (security_barrier) to the definition of pg_stats. Bumped CATVERSION due to new SQL functions and pg_stats view definition. Back-patch to 9.5 where RLS was introduced. Reported by Yaroslav. Patch by Joe Conway and Dean Rasheed with review and input by Michael Paquier and Stephen Frost.
1 parent 6087d95 commit cfa928f

File tree

16 files changed

+159
-31
lines changed

16 files changed

+159
-31
lines changed

‎doc/src/sgml/func.sgml

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15244,6 +15244,12 @@ SET search_path TO <replaceable>schema</> <optional>, <replaceable>schema</>, ..
1524415244
<entry><type>boolean</type></entry>
1524515245
<entry>does current user have privilege for role</entry>
1524615246
</row>
15247+
<row>
15248+
<entry><literal><function>row_security_active</function>(<parameter>table</parameter>)</literal>
15249+
</entry>
15250+
<entry><type>boolean</type></entry>
15251+
<entry>does current user have row level security active for table</entry>
15252+
</row>
1524715253
</tbody>
1524815254
</tgroup>
1524915255
</table>
@@ -15284,6 +15290,9 @@ SET search_path TO <replaceable>schema</> <optional>, <replaceable>schema</>, ..
1528415290
<indexterm>
1528515291
<primary>pg_has_role</primary>
1528615292
</indexterm>
15293+
<indexterm>
15294+
<primary>row_security_active</primary>
15295+
</indexterm>
1528715296

1528815297
<para>
1528915298
<function>has_table_privilege</function> checks whether a user
@@ -15447,6 +15456,13 @@ SELECT has_function_privilege('joeuser', 'myfunc(int, text)', 'execute');
1544715456
are immediately available without doing <command>SET ROLE</>.
1544815457
</para>
1544915458

15459+
<para>
15460+
<function>row_security_active</function> checks whether row level
15461+
security is active for the specified table in the context of the
15462+
<function>current_user</function> and environment. The table can
15463+
be specified by name or by OID.
15464+
</para>
15465+
1545015466
<para>
1545115467
<xref linkend="functions-info-schema-table"> shows functions that
1545215468
determine whether a certain object is <firstterm>visible</> in the

‎src/backend/access/index/genam.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ BuildIndexValueDescription(Relation indexRelation,
204204
Assert(indexrelid == idxrec->indexrelid);
205205

206206
/* RLS check- if RLS is enabled then we don't return anything. */
207-
if (check_enable_rls(indrelid, GetUserId(), true) == RLS_ENABLED)
207+
if (check_enable_rls(indrelid, InvalidOid, true) == RLS_ENABLED)
208208
{
209209
ReleaseSysCache(ht_idx);
210210
return NULL;

‎src/backend/catalog/system_views.sql

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ CREATE VIEW pg_indexes AS
150150
LEFT JOIN pg_tablespace T ON (T.oid = I.reltablespace)
151151
WHERE C.relkind IN ('r', 'm') AND I.relkind = 'i';
152152

153-
CREATE VIEW pg_stats AS
153+
CREATE VIEW pg_stats WITH (security_barrier) AS
154154
SELECT
155155
nspname AS schemaname,
156156
relname AS tablename,
@@ -211,7 +211,9 @@ CREATE VIEW pg_stats AS
211211
FROM pg_statistic s JOIN pg_class c ON (c.oid = s.starelid)
212212
JOIN pg_attribute a ON (c.oid = attrelid AND attnum = s.staattnum)
213213
LEFT JOIN pg_namespace n ON (n.oid = c.relnamespace)
214-
WHERE NOT attisdropped AND has_column_privilege(c.oid, a.attnum, 'select');
214+
WHERE NOT attisdropped
215+
AND has_column_privilege(c.oid, a.attnum, 'select')
216+
AND (c.relrowsecurity = false OR NOT row_security_active(c.oid));
215217

216218
REVOKE ALL on pg_statistic FROM public;
217219

‎src/backend/executor/execMain.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1874,7 +1874,7 @@ ExecBuildSlotValueDescription(Oid reloid,
18741874
* then don't return anything. Otherwise, go through normal permission
18751875
* checks.
18761876
*/
1877-
if (check_enable_rls(reloid, GetUserId(), true) == RLS_ENABLED)
1877+
if (check_enable_rls(reloid, InvalidOid, true) == RLS_ENABLED)
18781878
return NULL;
18791879

18801880
initStringInfo(&buf);

‎src/backend/rewrite/rowsecurity.c

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,6 @@ get_row_security_policies(Query *root, CmdType commandType, RangeTblEntry *rte,
107107

108108
Relation rel;
109109
Oid user_id;
110-
int sec_context;
111110
int rls_status;
112111
bool defaultDeny = false;
113112

@@ -117,22 +116,13 @@ get_row_security_policies(Query *root, CmdType commandType, RangeTblEntry *rte,
117116
*hasRowSecurity = false;
118117
*hasSubLinks = false;
119118

120-
/* This is just to get the security context */
121-
GetUserIdAndSecContext(&user_id, &sec_context);
119+
/* If this is not a normal relation, just return immediately */
120+
if (rte->relkind != RELKIND_RELATION)
121+
return;
122122

123123
/* Switch to checkAsUser if it's set */
124124
user_id = rte->checkAsUser ? rte->checkAsUser : GetUserId();
125125

126-
/*
127-
* If this is not a normal relation, or we have been told to explicitly
128-
* skip RLS (perhaps because this is an FK check) then just return
129-
* immediately.
130-
*/
131-
if (rte->relid < FirstNormalObjectId
132-
|| rte->relkind != RELKIND_RELATION
133-
|| (sec_context & SECURITY_ROW_LEVEL_DISABLED))
134-
return;
135-
136126
/* Determine the state of RLS for this, pass checkAsUser explicitly */
137127
rls_status = check_enable_rls(rte->relid, rte->checkAsUser, false);
138128

‎src/backend/utils/adt/ri_triggers.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3243,7 +3243,7 @@ ri_ReportViolation(const RI_ConstraintInfo *riinfo,
32433243
* privileges.
32443244
*/
32453245

3246-
if (check_enable_rls(rel_oid, GetUserId(), true) != RLS_ENABLED)
3246+
if (check_enable_rls(rel_oid, InvalidOid, true) != RLS_ENABLED)
32473247
{
32483248
aclresult = pg_class_aclcheck(rel_oid, GetUserId(), ACL_SELECT);
32493249
if (aclresult != ACLCHECK_OK)
@@ -3264,6 +3264,8 @@ ri_ReportViolation(const RI_ConstraintInfo *riinfo,
32643264
}
32653265
}
32663266
}
3267+
else
3268+
has_perm = false;
32673269

32683270
if (has_perm)
32693271
{

‎src/backend/utils/cache/plancache.c

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -153,8 +153,6 @@ CreateCachedPlan(Node *raw_parse_tree,
153153
CachedPlanSource *plansource;
154154
MemoryContext source_context;
155155
MemoryContext oldcxt;
156-
Oid user_id;
157-
int security_context;
158156

159157
Assert(query_string != NULL); /* required as of 8.4 */
160158

@@ -177,8 +175,6 @@ CreateCachedPlan(Node *raw_parse_tree,
177175
*/
178176
oldcxt = MemoryContextSwitchTo(source_context);
179177

180-
GetUserIdAndSecContext(&user_id, &security_context);
181-
182178
plansource = (CachedPlanSource *) palloc0(sizeof(CachedPlanSource));
183179
plansource->magic = CACHEDPLANSOURCE_MAGIC;
184180
plansource->raw_parse_tree = copyObject(raw_parse_tree);
@@ -208,8 +204,7 @@ CreateCachedPlan(Node *raw_parse_tree,
208204
plansource->total_custom_cost = 0;
209205
plansource->num_custom_plans = 0;
210206
plansource->hasRowSecurity = false;
211-
plansource->rowSecurityDisabled
212-
= (security_context & SECURITY_ROW_LEVEL_DISABLED) != 0;
207+
plansource->rowSecurityDisabled = InRowLevelSecurityDisabled();
213208
plansource->row_security_env = row_security;
214209
plansource->planUserId = InvalidOid;
215210

‎src/backend/utils/init/miscinit.c

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,7 @@ GetAuthenticatedUserId(void)
341341
* GetUserIdAndSecContext/SetUserIdAndSecContext - get/set the current user ID
342342
* and the SecurityRestrictionContext flags.
343343
*
344-
* Currently there are two valid bits in SecurityRestrictionContext:
344+
* Currently there are three valid bits in SecurityRestrictionContext:
345345
*
346346
* SECURITY_LOCAL_USERID_CHANGE indicates that we are inside an operation
347347
* that is temporarily changing CurrentUserId via these functions. This is
@@ -359,6 +359,9 @@ GetAuthenticatedUserId(void)
359359
* where the called functions are really supposed to be side-effect-free
360360
* anyway, such as VACUUM/ANALYZE/REINDEX.
361361
*
362+
* SECURITY_ROW_LEVEL_DISABLED indicates that we are inside an operation that
363+
* needs to bypass row level security checks, for example FK checks.
364+
*
362365
* Unlike GetUserId, GetUserIdAndSecContext does *not* Assert that the current
363366
* value of CurrentUserId is valid; nor does SetUserIdAndSecContext require
364367
* the new value to be valid. In fact, these routines had better not
@@ -401,6 +404,15 @@ InSecurityRestrictedOperation(void)
401404
return (SecurityRestrictionContext & SECURITY_RESTRICTED_OPERATION) != 0;
402405
}
403406

407+
/*
408+
* InRowLevelSecurityDisabled - are we inside a RLS-disabled operation?
409+
*/
410+
bool
411+
InRowLevelSecurityDisabled(void)
412+
{
413+
return (SecurityRestrictionContext & SECURITY_ROW_LEVEL_DISABLED) != 0;
414+
}
415+
404416

405417
/*
406418
* These are obsolete versions of Get/SetUserIdAndSecContext that are

‎src/backend/utils/misc/rls.c

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,12 @@
1616

1717
#include "access/htup.h"
1818
#include "access/htup_details.h"
19+
#include "access/transam.h"
1920
#include "catalog/pg_class.h"
21+
#include "catalog/namespace.h"
2022
#include "miscadmin.h"
2123
#include "utils/acl.h"
24+
#include "utils/builtins.h"
2225
#include "utils/elog.h"
2326
#include "utils/rls.h"
2427
#include "utils/syscache.h"
@@ -37,7 +40,10 @@ extern int check_enable_rls(Oid relid, Oid checkAsUser, bool noError);
3740
* for the table and the plan cache needs to be invalidated if the environment
3841
* changes.
3942
*
40-
* Handle checking as another role via checkAsUser (for views, etc).
43+
* Handle checking as another role via checkAsUser (for views, etc). Note that
44+
* if *not* checking as another role, the caller should pass InvalidOid rather
45+
* than GetUserId(). Otherwise the check for row_security = OFF is skipped, and
46+
* so we may falsely report that RLS is active when the user has bypassed it.
4147
*
4248
* If noError is set to 'true' then we just return RLS_ENABLED instead of doing
4349
* an ereport() if the user has attempted to bypass RLS and they are not
@@ -53,6 +59,17 @@ check_enable_rls(Oid relid, Oid checkAsUser, bool noError)
5359
bool relrowsecurity;
5460
Oid user_id = checkAsUser ? checkAsUser : GetUserId();
5561

62+
/* Nothing to do for built-in relations */
63+
if (relid < FirstNormalObjectId)
64+
return RLS_NONE;
65+
66+
/*
67+
* Check if we have been told to explicitly skip RLS (perhaps because this
68+
* is a foreign key check)
69+
*/
70+
if (InRowLevelSecurityDisabled())
71+
return RLS_NONE;
72+
5673
tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
5774
if (!HeapTupleIsValid(tuple))
5875
return RLS_NONE;
@@ -111,3 +128,37 @@ check_enable_rls(Oid relid, Oid checkAsUser, bool noError)
111128
/* RLS should be fully enabled for this relation. */
112129
return RLS_ENABLED;
113130
}
131+
132+
/*
133+
* row_security_active
134+
*
135+
* check_enable_rls wrapped as a SQL callable function except
136+
* RLS_NONE_ENV and RLS_NONE are the same for this purpose.
137+
*/
138+
Datum
139+
row_security_active(PG_FUNCTION_ARGS)
140+
{
141+
/* By OID */
142+
Oid tableoid = PG_GETARG_OID(0);
143+
int rls_status;
144+
145+
rls_status = check_enable_rls(tableoid, InvalidOid, true);
146+
PG_RETURN_BOOL(rls_status == RLS_ENABLED);
147+
}
148+
149+
Datum
150+
row_security_active_name(PG_FUNCTION_ARGS)
151+
{
152+
/* By qualified name */
153+
text *tablename = PG_GETARG_TEXT_P(0);
154+
RangeVar *tablerel;
155+
Oid tableoid;
156+
int rls_status;
157+
158+
/* Look up table name. Can't lock it - we might not have privileges. */
159+
tablerel = makeRangeVarFromNameList(textToQualifiedNameList(tablename));
160+
tableoid = RangeVarGetRelid(tablerel, NoLock, false);
161+
162+
rls_status = check_enable_rls(tableoid, InvalidOid, true);
163+
PG_RETURN_BOOL(rls_status == RLS_ENABLED);
164+
}

‎src/include/catalog/catversion.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,6 @@
5353
*/
5454

5555
/* yyyymmddN */
56-
#define CATALOG_VERSION_NO 201507251
56+
#define CATALOG_VERSION_NO 201507281
5757

5858
#endif

‎src/include/catalog/pg_proc.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5337,6 +5337,12 @@ DESCR("get progress for all replication origins");
53375337
#define PROVOLATILE_STABLE 's' /* does not change within a scan */
53385338
#define PROVOLATILE_VOLATILE 'v' /* can change even within a scan */
53395339

5340+
/* rls */
5341+
DATA(insert OID = 3298 ( row_security_active PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 16 "26" _null_ _null_ _null_ _null_ _null_ row_security_active _null_ _null_ _null_ ));
5342+
DESCR("row security for current context active on table by table oid");
5343+
DATA(insert OID = 3299 ( row_security_active PGNSP PGUID 12 1 0 0 0 f f f f t f s 1 0 16 "25" _null_ _null_ _null_ _null_ _null_ row_security_active_name _null_ _null_ _null_ ));
5344+
DESCR("row security for current context active on table by table name");
5345+
53405346
/*
53415347
* Symbolic values for proargmodes column. Note that these must agree with
53425348
* the FunctionParameterMode enum in parsenodes.h; we declare them here to

‎src/include/miscadmin.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -305,6 +305,7 @@ extern void GetUserIdAndSecContext(Oid *userid, int *sec_context);
305305
extern void SetUserIdAndSecContext(Oid userid, int sec_context);
306306
extern bool InLocalUserIdChange(void);
307307
extern bool InSecurityRestrictedOperation(void);
308+
extern bool InRowLevelSecurityDisabled(void);
308309
extern void GetUserIdAndContext(Oid *userid, bool *sec_def_context);
309310
extern void SetUserIdAndContext(Oid userid, bool sec_def_context);
310311
extern void InitializeSessionUserId(const char *rolename, Oid useroid);

‎src/include/utils/builtins.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1120,6 +1120,10 @@ extern Datum set_config_by_name(PG_FUNCTION_ARGS);
11201120
extern Datum show_all_settings(PG_FUNCTION_ARGS);
11211121
extern Datum show_all_file_settings(PG_FUNCTION_ARGS);
11221122

1123+
/* rls.c */
1124+
extern Datum row_security_active(PG_FUNCTION_ARGS);
1125+
extern Datum row_security_active_name(PG_FUNCTION_ARGS);
1126+
11231127
/* lockfuncs.c */
11241128
extern Datum pg_lock_status(PG_FUNCTION_ARGS);
11251129
extern Datum pg_advisory_lock_int8(PG_FUNCTION_ARGS);

‎src/test/regress/expected/rowsecurity.out

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,7 @@ SELECT * FROM document d FULL OUTER JOIN category c on d.cid = c.cid;
307307

308308
DELETE FROM category WHERE cid = 33; -- fails with FK violation
309309
ERROR: update or delete on table "category" violates foreign key constraint "document_cid_fkey" on table "document"
310-
DETAIL: Key (cid)=(33) is still referenced from table "document".
310+
DETAIL: Key is still referenced from table "document".
311311
-- can insert FK referencing invisible PK
312312
SET SESSION AUTHORIZATION rls_regress_user2;
313313
SELECT * FROM document d FULL OUTER JOIN category c on d.cid = c.cid;
@@ -2886,11 +2886,45 @@ SELECT * FROM current_check;
28862886
(1 row)
28872887

28882888
COMMIT;
2889+
--
2890+
-- check pg_stats view filtering
2891+
--
2892+
SET row_security TO ON;
2893+
SET SESSION AUTHORIZATION rls_regress_user0;
2894+
ANALYZE current_check;
2895+
-- Stats visible
2896+
SELECT row_security_active('current_check');
2897+
row_security_active
2898+
---------------------
2899+
f
2900+
(1 row)
2901+
2902+
SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
2903+
most_common_vals
2904+
---------------------
2905+
2906+
2907+
{rls_regress_user1}
2908+
(3 rows)
2909+
2910+
SET SESSION AUTHORIZATION rls_regress_user1;
2911+
-- Stats not visible
2912+
SELECT row_security_active('current_check');
2913+
row_security_active
2914+
---------------------
2915+
t
2916+
(1 row)
2917+
2918+
SELECT most_common_vals FROM pg_stats where tablename = 'current_check';
2919+
most_common_vals
2920+
------------------
2921+
(0 rows)
2922+
28892923
--
28902924
-- Collation support
28912925
--
28922926
BEGIN;
2893-
SET row_security = force;
2927+
SET row_security TO FORCE;
28942928
CREATE TABLE coll_t (c) AS VALUES ('bar'::text);
28952929
CREATE POLICY coll_p ON coll_t USING (c < ('foo'::text COLLATE "C"));
28962930
ALTER TABLE coll_t ENABLE ROW LEVEL SECURITY;

‎src/test/regress/expected/rules.out

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2061,7 +2061,7 @@ pg_stats| SELECT n.nspname AS schemaname,
20612061
JOIN pg_class c ON ((c.oid = s.starelid)))
20622062
JOIN pg_attribute a ON (((c.oid = a.attrelid) AND (a.attnum = s.staattnum))))
20632063
LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
2064-
WHERE ((NOT a.attisdropped) AND has_column_privilege(c.oid, a.attnum, 'select'::text));
2064+
WHERE ((NOT a.attisdropped) AND has_column_privilege(c.oid, a.attnum, 'select'::text) AND ((c.relrowsecurity = false) OR (NOT row_security_active(c.oid))));
20652065
pg_tables| SELECT n.nspname AS schemaname,
20662066
c.relname AS tablename,
20672067
pg_get_userbyid(c.relowner) AS tableowner,

0 commit comments

Comments
 (0)