pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedRe: Infinite recursion in row-security based on updatable s.b. views
6+ messages / 2 participants
[nested] [flat]
* Re: Infinite recursion in row-security based on updatable s.b. views
@ 2014-01-24 09:04 Craig Ringer <craig@2ndquadrant.com>
0 siblings, 1 reply; 6+ messages in thread
From: Craig Ringer @ 2014-01-24 09:04 UTC (permalink / raw)
To: Gregory Smith <gregsmithpgsql@gmail.com>; Greg Stark <stark@mit.edu>; Kohei KaiGai <kaigai@kaigai.gr.jp>; pgsql-hackers
Hi all
I've hit an interesting wrinkle and am interested in opinions. By
integrating updatable security barrier view support with row-security,
it has become a lot harder to detect and prevent infinite recursion
(including mutual recursion) in row-security policy expansion.
The code is attached, but it's not an independent patch so it's way
easier to grab it from git:
git@github.com:ringerc/postgres.git
branch rls-9.4-upd-sb-views (subject to rebase); or
tag rls-9.4-upd-sb-views-v1
(Only contains half the old row-security patch; I'll rebase the docs,
tests, etc on top of this once it's working properly).
I've integrated the updatable security barrier view support into RLS by
injecting securityQuals in subquery_planner() just before
preprocess_rowmarks . (I'm still thinking about some inheritance related
aspects to that, but that's for later).
The problem is that this causes infinite recursion - the securityQuals
get expanded into a subquery over the original RTE that had the
row-security policy on it. Then subquery_planner is re-entered when
processing the subquery, a row-security qual is found on the target RTE,
and ... *boom*.
Fixing this is not as simple as suppressing expansion of row-security
policy when processing a security barrier subquery created by a
row-security policy, as it is desirable to respect the row-security
policy of *other* tables that get referenced in the expanded
row-security qual.
If we just record the relid of the relation a subquery was expanded from
and avoid expanding that inside the generated subquery we prevent simple
linear recursion, but not mutual recursion between two or more rels with
row-security policy.
KaiGai's row-security patch handles this because it does its own
recursive expansion, so (like the rewriter) it can keep a breadcrumb
trail and detect when it is repeating a path. That's not so practical
when row-security code tags RTEs with policy, then updatable s.b. views
goes and expands them.
So. Options.
1.Come up with a reasonable way to track recursive row-security
expansion, detect infinite recursion, and bail out. Likely to involve
a new global structure with planner/optimizer lifetime that gets
checked and maintained by apply_row_security_rangetbl.
2.Solve the linear recursion case by storing a relid that should not get
expanded for security quals when processing a subquery. Say "don't do
that, expect stack exceeded crashes if you do" for the mutual
recursion case. Seems user hostile, incomplete, and likely to break
people's systems when they really don't expect it.
3.Disregard row-security policy on referenced tables when expanding
row-security qualifiers. There's precendent here in foreign keys,
which ignore row-security policy, but I don't think this is at all
appealing.
4.Magic?
Unless someone has a suggestion for #4, I'll be going with #1. I'd
appreciate tips on doing this in a sane and efficient manner if anyone
has them. I'll be reading over the rewriter's infinite loop protection
and that of KaiGai's RLS patch for ideas in the mean time, and will give
it a good go.
BTW, I feel like we should be letting the rewriter do this job; it's
good at dealing with recursion problems already. That won't work so long
as security barrier qual expansion happens during planning, not rewrite,
though - and we've already explored the fun problems with doing upd.
s.b. qual expansion in rewrite.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
Attachments:
[text/x-patch] 0001-RLS-First-attempt-to-integrate-with-updatable-s.b.-v.patch (9.4K, ../../52E22C8E.2020702@2ndquadrant.com/2-0001-RLS-First-attempt-to-integrate-with-updatable-s.b.-v.patch)
download | inline diff:
From 4f8ed71e4cf18db7a4285da9b9c9f8f7f1030e32 Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Fri, 24 Jan 2014 16:18:40 +0800
Subject: [PATCH] RLS: First attempt to integrate with updatable s.b. views
Enters infinite recursion because the rls-protected table gets expanded
and the new RTE gets checked for row-security quals, gets securityQuals added,
and ... infinite recursion!
---
src/backend/optimizer/plan/planner.c | 7 ++
src/backend/optimizer/prep/prepunion.c | 6 +
src/backend/optimizer/util/Makefile | 3 +-
src/backend/optimizer/util/rowsecurity.c | 185 +++++++++++++++++++++++++++++++
src/include/optimizer/rowsecurity.h | 27 +++++
5 files changed, 227 insertions(+), 1 deletion(-)
create mode 100644 src/backend/optimizer/util/rowsecurity.c
create mode 100644 src/include/optimizer/rowsecurity.h
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 80940ea..8cdd580 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -388,6 +388,13 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
}
/*
+ * Check RTEs for row-security policies and set securityQuals on the
+ * RTE if a policy is found. This must happen before inherited table
+ * expansion so that the quals get copied to the child rels.
+ */
+ apply_row_security_policy(root);
+
+ /*
* Preprocess RowMark information. We need to do this after subquery
* pullup (so that all non-inherited RTEs are present) and before
* inheritance expansion (so that the info is available for
diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c
index c7e0199..20a634d 100644
--- a/src/backend/optimizer/prep/prepunion.c
+++ b/src/backend/optimizer/prep/prepunion.c
@@ -1349,6 +1349,12 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
childRTindex = list_length(parse->rtable);
/*
+ * Check for row-security quals on the newly added child
+ * relation and update securityQuals as appropriate.
+ */
+ apply_row_security_rte(root, childRTindex);
+
+ /*
* Build an AppendRelInfo for this parent and child.
*/
appinfo = makeNode(AppendRelInfo);
diff --git a/src/backend/optimizer/util/Makefile b/src/backend/optimizer/util/Makefile
index c54d0a6..d42a285 100644
--- a/src/backend/optimizer/util/Makefile
+++ b/src/backend/optimizer/util/Makefile
@@ -13,6 +13,7 @@ top_builddir = ../../../..
include $(top_builddir)/src/Makefile.global
OBJS = clauses.o joininfo.o orclauses.o pathnode.o placeholder.o \
- plancat.o predtest.o relnode.o restrictinfo.o tlist.o var.o
+ plancat.o predtest.o relnode.o restrictinfo.o tlist.o var.o \
+ rowsecurity.o
include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/optimizer/util/rowsecurity.c b/src/backend/optimizer/util/rowsecurity.c
new file mode 100644
index 0000000..f002891
--- /dev/null
+++ b/src/backend/optimizer/util/rowsecurity.c
@@ -0,0 +1,185 @@
+/*
+ * rewrite/rowsecurity.c
+ * Routines to support row-security feature
+ *
+ * Portions Copyright (c) 1996-2012, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ */
+#include "postgres.h"
+
+#include "access/heapam.h"
+#include "access/htup_details.h"
+#include "access/sysattr.h"
+#include "catalog/pg_class.h"
+#include "catalog/pg_inherits_fn.h"
+#include "catalog/pg_rowsecurity.h"
+#include "catalog/pg_type.h"
+#include "miscadmin.h"
+#include "nodes/makefuncs.h"
+#include "nodes/nodeFuncs.h"
+#include "nodes/plannodes.h"
+#include "optimizer/clauses.h"
+#include "optimizer/prep.h"
+#include "optimizer/rowsecurity.h"
+#include "parser/parsetree.h"
+#include "rewrite/rewriteHandler.h"
+#include "utils/lsyscache.h"
+#include "utils/rel.h"
+#include "utils/syscache.h"
+#include "tcop/utility.h"
+
+/* hook to allow extensions to apply their own security policy */
+row_security_policy_hook_type row_security_policy_hook = NULL;
+
+/*
+ * pull_row_security_policy
+ *
+ * Fetches the configured row-security policy of both built-in catalogs and any
+ * extensions. If any policy is found a list of qualifier expressions is
+ * returned, where each is treated as a securityQual.
+ */
+static List *
+pull_row_security_policy(CmdType cmd, Relation relation)
+{
+ List *quals = NIL;
+ Expr *qual = NULL;
+
+ /*
+ * Pull the row-security policy configured with built-in features,
+ * if unprivileged users. Please note that superuser can bypass it.
+ */
+ if (relation->rsdesc && !superuser())
+ {
+ RowSecurityDesc *rsdesc = relation->rsdesc;
+ qual = copyObject(rsdesc->rsall.qual);
+ quals = lcons(qual, quals);
+ }
+
+ /*
+ * Also, ask extensions whether they want to apply their own
+ * row-security policy. If both built-in and extension has
+ * their own policy they're applied as nested qualifiers.
+ */
+ if (row_security_policy_hook)
+ {
+ List *temp;
+
+ temp = (*row_security_policy_hook)(cmd, relation);
+ if (temp != NIL)
+ lcons(temp, quals);
+ }
+ return quals;
+}
+
+/*
+ * apply_row_security_rte
+ *
+ * Apply row-security policy to a RangeTblEntry if policy for the
+ * RTE's Relation exists.
+ *
+ * In addition to the initial RTE scan in subquery planner, this also
+ * gets called directly from expand_inherited_rtentry to ensure
+ * that row-security quals applied to children get added.
+ */
+bool
+apply_row_security_rte(PlannerInfo *root, RangeTblEntry* rte)
+{
+ Relation rel;
+ List *rowsecquals;
+ bool result = false;
+
+ Assert(rte->rtekind == RTE_RELATION);
+ rel = heap_open(rte->relid, NoLock);
+ rowsecquals = pull_row_security_policy(root->parse->commandType, rel);
+ if (rowsecquals)
+ {
+ rte->securityQuals = lcons(rowsecquals, rte->securityQuals);
+ result = true;
+ }
+ heap_close(rel, NoLock);
+ return result;
+}
+
+/*
+ * Scan the rangetable of the Query, looking for relations with
+ * row-security policies. Where a policy is found, add its securityQuals
+ * to the RTE then recurse into the quals
+ */
+static bool
+apply_row_security_rangetbl(PlannerInfo *root)
+{
+ ListCell *lc;
+ RangeTblEntry * rte;
+ bool policy_found = false;
+
+ foreach (lc, root->parse->rtable)
+ {
+ rte = (RangeTblEntry*) lfirst(lc);
+ /* Only act on normal tables that are not builtin system catalogs */
+ if (rte->rtekind == RTE_RELATION && rte->relid >= FirstNormalObjectId)
+ policy_found &= apply_row_security_rte(root, rte);
+ }
+ return policy_found;
+}
+
+/*
+ * apply_row_security_policy
+ *
+ * Entrypoint to apply configured row-security policy of the relation.
+ *
+ * In the case when the supplied query references relations with row-security
+ * policy, the relation's RangeTblEntry has the row-security qualifiers
+ * appended to the RTE's securityQual list. This will cause the optimizer to
+ * replace the relation with a security_barrier subquery over the original
+ * relation
+ *
+ * That happens after inheritance expansion has occurred, so any qual on a
+ * parent relation automatically applies to the child as the RTE is copied.
+ * RLS quals are checked for during inheritance expansion and any child
+ * table-specific quals are applied then, producing a second level of
+ * security_barrier subquery.
+ */
+void
+apply_row_security_policy(PlannerInfo *root)
+{
+ Query *parse = root->parse;
+ Oid curr_userid;
+ int curr_seccxt;
+
+ /*
+ * Mode checks. In case when SECURITY_ROW_LEVEL_DISABLED is set,
+ * no row-level security policy should be applied regardless
+ * whether it is built-in or extension.
+ */
+ GetUserIdAndSecContext(&curr_userid, &curr_seccxt);
+ if (curr_seccxt & SECURITY_ROW_LEVEL_DISABLED)
+ return;
+
+ if (apply_row_security_rangetbl(root))
+ {
+ /*
+ * We added at least one securityQual, making the plan
+ * potentially dependent on whether the current user
+ * is row-security exempt.
+ */
+
+ PlannerGlobal *glob = root->glob;
+ PlanInvalItem *pi_item;
+
+ if (!OidIsValid(glob->planUserId))
+ {
+ /* Plan invalidation on session user-id */
+ glob->planUserId = GetUserId();
+
+ /* Plan invalidation on catalog updates of pg_authid */
+ pi_item = makeNode(PlanInvalItem);
+ pi_item->cacheId = AUTHOID;
+ pi_item->hashValue =
+ GetSysCacheHashValue1(AUTHOID,
+ ObjectIdGetDatum(glob->planUserId));
+ glob->invalItems = lappend(glob->invalItems, pi_item);
+ }
+ else
+ Assert(glob->planUserId == GetUserId());
+ }
+}
diff --git a/src/include/optimizer/rowsecurity.h b/src/include/optimizer/rowsecurity.h
new file mode 100644
index 0000000..a996479
--- /dev/null
+++ b/src/include/optimizer/rowsecurity.h
@@ -0,0 +1,27 @@
+/* -------------------------------------------------------------------------
+ *
+ * rowsecurity.h
+ * prototypes for optimizer/rowsecurity.c
+ *
+ * Portions Copyright (c) 1996-2012, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * -------------------------------------------------------------------------
+ */
+#ifndef ROWSECURITY_H
+#define ROWSECURITY_H
+
+#include "nodes/execnodes.h"
+#include "nodes/parsenodes.h"
+#include "nodes/relation.h"
+#include "utils/rel.h"
+
+typedef List *(*row_security_policy_hook_type)(CmdType cmdtype,
+ Relation relation);
+extern PGDLLIMPORT row_security_policy_hook_type row_security_policy_hook;
+
+extern bool apply_row_security_rte(PlannerInfo *root, RangeTblEntry* rte);
+
+extern void apply_row_security_policy(PlannerInfo *root);
+
+#endif /* ROWSECURITY_H */
--
1.8.3.1
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Infinite recursion in row-security based on updatable s.b. views
@ 2014-01-24 11:16 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Craig Ringer <craig@2ndquadrant.com>
0 siblings, 4 replies; 6+ messages in thread
From: Dean Rasheed @ 2014-01-24 11:16 UTC (permalink / raw)
To: Craig Ringer <craig@2ndquadrant.com>; +Cc: Gregory Smith <gregsmithpgsql@gmail.com>; Greg Stark <stark@mit.edu>; Kohei KaiGai <kaigai@kaigai.gr.jp>; pgsql-hackers
On 24 January 2014 09:04, Craig Ringer <craig@2ndquadrant.com> wrote:
> Hi all
>
> I've hit an interesting wrinkle and am interested in opinions. By
> integrating updatable security barrier view support with row-security,
> it has become a lot harder to detect and prevent infinite recursion
> (including mutual recursion) in row-security policy expansion.
>
> The code is attached, but it's not an independent patch so it's way
> easier to grab it from git:
>
> git@github.com:ringerc/postgres.git
> branch rls-9.4-upd-sb-views (subject to rebase); or
> tag rls-9.4-upd-sb-views-v1
>
> (Only contains half the old row-security patch; I'll rebase the docs,
> tests, etc on top of this once it's working properly).
>
>
> I've integrated the updatable security barrier view support into RLS by
> injecting securityQuals in subquery_planner() just before
> preprocess_rowmarks . (I'm still thinking about some inheritance related
> aspects to that, but that's for later).
>
> The problem is that this causes infinite recursion - the securityQuals
> get expanded into a subquery over the original RTE that had the
> row-security policy on it. Then subquery_planner is re-entered when
> processing the subquery, a row-security qual is found on the target RTE,
> and ... *boom*.
>
> Fixing this is not as simple as suppressing expansion of row-security
> policy when processing a security barrier subquery created by a
> row-security policy, as it is desirable to respect the row-security
> policy of *other* tables that get referenced in the expanded
> row-security qual.
>
> If we just record the relid of the relation a subquery was expanded from
> and avoid expanding that inside the generated subquery we prevent simple
> linear recursion, but not mutual recursion between two or more rels with
> row-security policy.
>
> KaiGai's row-security patch handles this because it does its own
> recursive expansion, so (like the rewriter) it can keep a breadcrumb
> trail and detect when it is repeating a path. That's not so practical
> when row-security code tags RTEs with policy, then updatable s.b. views
> goes and expands them.
>
> So. Options.
>
> 1.Come up with a reasonable way to track recursive row-security
> expansion, detect infinite recursion, and bail out. Likely to involve
> a new global structure with planner/optimizer lifetime that gets
> checked and maintained by apply_row_security_rangetbl.
>
> 2.Solve the linear recursion case by storing a relid that should not get
> expanded for security quals when processing a subquery. Say "don't do
> that, expect stack exceeded crashes if you do" for the mutual
> recursion case. Seems user hostile, incomplete, and likely to break
> people's systems when they really don't expect it.
>
> 3.Disregard row-security policy on referenced tables when expanding
> row-security qualifiers. There's precendent here in foreign keys,
> which ignore row-security policy, but I don't think this is at all
> appealing.
>
> 4.Magic?
>
My first thought is to add a boolean flag to RangeTblEntry (similar to
the "inh" flag) to say whether RLS expansion is requested for that
RTE. Then set it to false on each RTE as you add new RLS quals to it's
securityQuals.
In addition, when adding RLS quals to an RTE, I think they should be
fully and recursively expanded immediately, before setting the new
flag to false and moving on --- think recursively calling the rewriter
to expand view references in the new RLS qual, and
expand_security_qual() to expand any additional RLS quals in the
securityQuals list --- with loop detection there. I'm not pretending
that's going to be easy, but there are a couple of existing pieces of
code to borrow ideas from. Doing it this way should make it possible
to do the loop detection without any global structures.
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Infinite recursion in row-security based on updatable s.b. views
@ 2014-01-28 06:11 Craig Ringer <craig@2ndquadrant.com>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
3 siblings, 0 replies; 6+ messages in thread
From: Craig Ringer @ 2014-01-28 06:11 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: Gregory Smith <gregsmithpgsql@gmail.com>; Greg Stark <stark@mit.edu>; Kohei KaiGai <kaigai@kaigai.gr.jp>; pgsql-hackers
On 01/24/2014 07:16 PM, Dean Rasheed wrote:
> My first thought is to add a boolean flag to RangeTblEntry (similar to
> the "inh" flag) to say whether RLS expansion is requested for that
> RTE. Then set it to false on each RTE as you add new RLS quals to it's
> securityQuals.
That's what I was getting at with adding flags to RangeTblEntry, yes.
Given the number of flags we're growing I wonder if they should be
consolodated into a bitmask, but I'll leave that problem for later.
I'll do this part first, since it seems you agree that a RangeTblEntry
flag is the appropriate path. That'll make row-security checking work
and make the patch testable.
It won't deal with recursive rules, they'll still crash the backend.
I'll deal with that as a further step.
> In addition, when adding RLS quals to an RTE, I think they should be
> fully and recursively expanded immediately, before setting the new
> flag to false and moving on --- think recursively calling the rewriter
> to expand view references in the new RLS qual, and
> expand_security_qual() to expand any additional RLS quals in the
> securityQuals list --- with loop detection there. I'm not pretending
> that's going to be easy, but there are a couple of existing pieces of
> code to borrow ideas from. Doing it this way should make it possible
> to do the loop detection without any global structures.
Ugh. I was really hoping to avoid *another* place where we do recursive
expansion and infinite recursion checking, especially when the rewriter
already does this job.
Unfortunately, so long as the rewriter doesn't know about inheritance,
it cannot fully solve this problem. A mutually recursive rule involving
inheritance wouldn't get detected by rewriter based checking.
The original RLS patch only detects direct recursion, btw, it looks like
it won't catch mutual recursion.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Infinite recursion in row-security based on updatable s.b. views
@ 2014-01-28 08:39 Craig Ringer <craig@2ndquadrant.com>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
3 siblings, 0 replies; 6+ messages in thread
From: Craig Ringer @ 2014-01-28 08:39 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: Gregory Smith <gregsmithpgsql@gmail.com>; Greg Stark <stark@mit.edu>; Kohei KaiGai <kaigai@kaigai.gr.jp>; pgsql-hackers
On 01/24/2014 07:16 PM, Dean Rasheed wrote:
> think recursively calling the rewriter
> to expand view references in the new RLS qual, and
> expand_security_qual() to expand any additional RLS quals in the
> securityQuals list
With this, it'd be helpful if expand_security_qual(...) took a
RangeTblEntry instead of an rt_index.
That'd also be much more efficient with large rtables if we can arrange
a scan through the rtable when looking for security quals.
Like other places that operate on the rangetable while it's being
modified, we can walk the rangetable list up until the final entry that
existed when we started walking. This approach saves the series of
rt_fetch calls, which are something like O(n log n) for n relations.
It's safe because the operation will only append rangetable entries.
(I can't help wonder how much we'd gain by making the rtable an array
that gets doubled in size and copied whenever it overflows, rather than
a linked list, given all the walking of it that gets done, and how dead
entries to get flagged as dead rather than actually removed.)
I'm looking for where I found the code that already does this so I can
point and say "I'm not crazy, we already do it here". Will follow up
with a patch.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Infinite recursion in row-security based on updatable s.b. views
@ 2014-01-28 10:27 Craig Ringer <craig@2ndquadrant.com>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
3 siblings, 0 replies; 6+ messages in thread
From: Craig Ringer @ 2014-01-28 10:27 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: Gregory Smith <gregsmithpgsql@gmail.com>; Greg Stark <stark@mit.edu>; Kohei KaiGai <kaigai@kaigai.gr.jp>; pgsql-hackers
On 01/28/2014 04:39 PM, Craig Ringer wrote:
> I'm looking for where I found the code that already does this so I can
> point and say "I'm not crazy, we already do it here". Will follow up
> with a patch.
I spoke to soon. The code I'm talking about is
expand_inherited_tables(...) and it still uses rt_fetch, it just avoids
foreach(...) in favour of stopping scanning at the end of the original
rtable.
So I get to be crazy after all.
I really don't like how many places we're rt_fetch'ing the same RTE from
in updatable s.b. views and its interaction with row-security, but that
can be a "later" problem. For now I'll stick with RTIs.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Infinite recursion in row-security based on updatable s.b. views
@ 2014-01-29 13:47 Craig Ringer <craig@2ndquadrant.com>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
3 siblings, 0 replies; 6+ messages in thread
From: Craig Ringer @ 2014-01-29 13:47 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: Gregory Smith <gregsmithpgsql@gmail.com>; Greg Stark <stark@mit.edu>; Kohei KaiGai <kaigai@kaigai.gr.jp>; pgsql-hackers
On 01/28/2014 02:11 PM, Craig Ringer wrote:
>> > My first thought is to add a boolean flag to RangeTblEntry (similar to
>> > the "inh" flag) to say whether RLS expansion is requested for that
>> > RTE. Then set it to false on each RTE as you add new RLS quals to it's
>> > securityQuals.
> That's what I was getting at with adding flags to RangeTblEntry, yes.
>
> Given the number of flags we're growing I wonder if they should be
> consolodated into a bitmask, but I'll leave that problem for later.
>
> I'll do this part first, since it seems you agree that a RangeTblEntry
> flag is the appropriate path. That'll make row-security checking work
> and make the patch testable.
>
> It won't deal with recursive rules, they'll still crash the backend.
> I'll deal with that as a further step.
>
I've put together a working RLS patch on top of updatable security
barrier views.
It has some known issues remaining; it doesn't do recursion checking
yet, and it fails some of the regression tests in exciting ways. I'm
looking into them step by step.
Some differences in the tests behaviours that have changed due to the
inheritance rules changing; many appear to be oversights or bugs yet to
be chased down.
You can find it here;
https://github.com/ringerc/postgres/compare/rls-9.4-upd-sb-views
i.e. https://github.com/ringerc/postgres.git ,
branch rls-9.4-upd-sb-views
(subject to rebasing) or the non-rebased tag rls-9.4-upd-sb-views-v2
The guts of the patch appear as a diff, attached, but it's not
standalone so I suggest using git.
I'll be looking into recursion issues and the test failures tomorrow.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
Attachments:
[text/x-patch] 0001-RLS-Add-rowsec_done-flag-to-RangeTblEntry.patch (2.3K, ../../52E90667.2040103@2ndquadrant.com/2-0001-RLS-Add-rowsec_done-flag-to-RangeTblEntry.patch)
download | inline diff:
From 03c02389c7b475d06864f9a7f38fd583c6e891e9 Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Tue, 28 Jan 2014 14:23:23 +0800
Subject: [PATCH 1/4] RLS: Add rowsec_done flag to RangeTblEntry
To be used to track completion status of row security expansion on a
range table entry. Rels with this flag set must not be row-security
expanded.
---
src/backend/nodes/copyfuncs.c | 1 +
src/backend/nodes/outfuncs.c | 1 +
src/backend/nodes/readfuncs.c | 1 +
src/include/nodes/parsenodes.h | 1 +
4 files changed, 4 insertions(+)
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 3e2acf0..9607911 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -1975,6 +1975,7 @@ _copyRangeTblEntry(const RangeTblEntry *from)
COPY_SCALAR_FIELD(rtekind);
COPY_SCALAR_FIELD(relid);
COPY_SCALAR_FIELD(relkind);
+ COPY_SCALAR_FIELD(rowsec_done);
COPY_NODE_FIELD(subquery);
COPY_SCALAR_FIELD(security_barrier);
COPY_SCALAR_FIELD(rowsec_relid);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index dd447ed..974d169 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2372,6 +2372,7 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
case RTE_RELATION:
WRITE_OID_FIELD(relid);
WRITE_CHAR_FIELD(relkind);
+ WRITE_BOOL_FIELD(rowsec_done);
break;
case RTE_SUBQUERY:
WRITE_NODE_FIELD(subquery);
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index a0cb369..7a43b59 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1214,6 +1214,7 @@ _readRangeTblEntry(void)
case RTE_RELATION:
READ_OID_FIELD(relid);
READ_CHAR_FIELD(relkind);
+ READ_BOOL_FIELD(rowsec_done);
break;
case RTE_SUBQUERY:
READ_NODE_FIELD(subquery);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index b3e718a..b3ae722 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -736,6 +736,7 @@ typedef struct RangeTblEntry
*/
Oid relid; /* OID of the relation */
char relkind; /* relation kind (see pg_class.relkind) */
+ bool rowsec_done; /* True if row-security quals checked for and applied already */
/*
* Fields valid for a subquery RTE (else NULL):
--
1.8.3.1
[text/x-patch] 0002-RLS-Enforce-row-security-constraints.patch (8.6K, ../../52E90667.2040103@2ndquadrant.com/3-0002-RLS-Enforce-row-security-constraints.patch)
download | inline diff:
From b0f5797d81a4b856f26818e14c93a0ec453a35de Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Fri, 24 Jan 2014 16:18:40 +0800
Subject: [PATCH 2/4] RLS: Enforce row-security constraints
Row-security constraints are enforced here by having the securityQual expansion
code check for a row-security constraint before expanding quals.
---
src/backend/optimizer/prep/prepsecurity.c | 17 +++-
src/backend/optimizer/prep/prepunion.c | 5 ++
src/backend/optimizer/util/Makefile | 3 +-
src/backend/optimizer/util/rowsecurity.c | 144 ++++++++++++++++++++++++++++++
src/include/optimizer/rowsecurity.h | 25 ++++++
5 files changed, 192 insertions(+), 2 deletions(-)
create mode 100644 src/backend/optimizer/util/rowsecurity.c
create mode 100644 src/include/optimizer/rowsecurity.h
diff --git a/src/backend/optimizer/prep/prepsecurity.c b/src/backend/optimizer/prep/prepsecurity.c
index e7a3f56..ad682bb 100644
--- a/src/backend/optimizer/prep/prepsecurity.c
+++ b/src/backend/optimizer/prep/prepsecurity.c
@@ -1,7 +1,8 @@
/*-------------------------------------------------------------------------
*
* prepsecurity.c
- * Routines for preprocessing security barrier quals.
+ * Routines for preprocessing security barrier quals and applying row-security
+ * policies.
*
* Portions Copyright (c) 1996-2013, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
@@ -15,6 +16,7 @@
#include "postgres.h"
#include "access/heapam.h"
+#include "access/transam.h"
#include "access/sysattr.h"
#include "catalog/heap.h"
#include "nodes/makefuncs.h"
@@ -23,6 +25,7 @@
#include "parser/parsetree.h"
#include "rewrite/rewriteManip.h"
#include "utils/rel.h"
+#include "optimizer/rowsecurity.h"
typedef struct
@@ -75,6 +78,18 @@ expand_security_quals(PlannerInfo *root, List *tlist)
rt_index++;
rte = rt_fetch(rt_index, parse->rtable);
+ /*
+ * Check for row-security quals on the relation and, if found, prepend them
+ * as new inner-most security quals. Ignore the return, we don't care at the
+ * moment.
+ *
+ * This will set rowsec_done on the RTE, which we'll copy if we expand
+ * it, ensuring that no infinitely recursive expansion of row security quals
+ * is done.
+ */
+ (void) prepend_row_security_quals(root, rte);
+
+
if (rte->securityQuals == NIL)
continue;
diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c
index c7e0199..0ae60c1 100644
--- a/src/backend/optimizer/prep/prepunion.c
+++ b/src/backend/optimizer/prep/prepunion.c
@@ -1349,6 +1349,11 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
childRTindex = list_length(parse->rtable);
/*
+ * XXX TODO We need to apply row-security policy after inheritance
+ * expansion.
+ */
+
+ /*
* Build an AppendRelInfo for this parent and child.
*/
appinfo = makeNode(AppendRelInfo);
diff --git a/src/backend/optimizer/util/Makefile b/src/backend/optimizer/util/Makefile
index c54d0a6..d42a285 100644
--- a/src/backend/optimizer/util/Makefile
+++ b/src/backend/optimizer/util/Makefile
@@ -13,6 +13,7 @@ top_builddir = ../../../..
include $(top_builddir)/src/Makefile.global
OBJS = clauses.o joininfo.o orclauses.o pathnode.o placeholder.o \
- plancat.o predtest.o relnode.o restrictinfo.o tlist.o var.o
+ plancat.o predtest.o relnode.o restrictinfo.o tlist.o var.o \
+ rowsecurity.o
include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/optimizer/util/rowsecurity.c b/src/backend/optimizer/util/rowsecurity.c
new file mode 100644
index 0000000..3f26d2f
--- /dev/null
+++ b/src/backend/optimizer/util/rowsecurity.c
@@ -0,0 +1,144 @@
+/*
+ * rewrite/rowsecurity.c
+ * Routines to support row-security feature
+ *
+ * Portions Copyright (c) 1996-2012, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ */
+#include "postgres.h"
+
+#include "access/heapam.h"
+#include "access/htup_details.h"
+#include "access/sysattr.h"
+#include "catalog/pg_class.h"
+#include "catalog/pg_inherits_fn.h"
+#include "catalog/pg_rowsecurity.h"
+#include "catalog/pg_type.h"
+#include "miscadmin.h"
+#include "nodes/makefuncs.h"
+#include "nodes/nodeFuncs.h"
+#include "nodes/plannodes.h"
+#include "optimizer/clauses.h"
+#include "optimizer/prep.h"
+#include "optimizer/rowsecurity.h"
+#include "parser/parsetree.h"
+#include "rewrite/rewriteHandler.h"
+#include "utils/lsyscache.h"
+#include "utils/rel.h"
+#include "utils/syscache.h"
+#include "tcop/utility.h"
+
+/* hook to allow extensions to apply their own security policy */
+row_security_policy_hook_type row_security_policy_hook = NULL;
+
+static void
+add_uid_plan_inval(PlannerGlobal* glob);
+
+static List *
+pull_row_security_policy(CmdType cmd, Relation relation);
+
+/*
+ * Check the given RTE to see whether it's already had row-security
+ * quals expanded and, if not, prepend any row-security rules
+ * from built-in or plug-in sources to the securityQuals.
+ *
+ * Returns true if any quals were added.
+ */
+bool
+prepend_row_security_quals(PlannerInfo* root, RangeTblEntry* rte)
+{
+ List *rowsecquals;
+ Relation rel;
+ Oid userid;
+ int sec_context;
+
+ GetUserIdAndSecContext(&userid, &sec_context);
+
+ bool qualsAdded = false;
+ if (rte->relid >= FirstNormalObjectId
+ && rte->relkind == 'r'
+ && !rte->rowsec_done
+ && !(sec_context & SECURITY_ROW_LEVEL_DISABLED))
+ {
+ rel = heap_open(rte->relid, NoLock);
+ rowsecquals = pull_row_security_policy(root->parse->commandType, rel);
+ if (rowsecquals)
+ {
+ rte->securityQuals = list_concat(rowsecquals, rte->securityQuals);
+ qualsAdded = true;
+ }
+ heap_close(rel, NoLock);
+ rte->rowsec_done = true;
+ }
+ if (qualsAdded)
+ add_uid_plan_inval(root->glob);
+ return qualsAdded;
+}
+
+/*
+ * pull_row_security_policy
+ *
+ * Fetches the configured row-security policy of both built-in catalogs and any
+ * extensions. If any policy is found a list of qualifier expressions is
+ * returned, where each is treated as a securityQual.
+ */
+static List *
+pull_row_security_policy(CmdType cmd, Relation relation)
+{
+ List *quals = NIL;
+ Expr *qual = NULL;
+
+ /*
+ * Pull the row-security policy configured with built-in features,
+ * if unprivileged users. Please note that superuser can bypass it.
+ */
+ if (relation->rsdesc && !superuser())
+ {
+ RowSecurityDesc *rsdesc = relation->rsdesc;
+ qual = copyObject(rsdesc->rsall.qual);
+ quals = lcons(qual, quals);
+ }
+
+ /*
+ * Also, ask extensions whether they want to apply their own
+ * row-security policy. If both built-in and extension has
+ * their own policy they're applied as nested qualifiers.
+ */
+ if (row_security_policy_hook)
+ {
+ List *temp;
+
+ temp = (*row_security_policy_hook)(cmd, relation);
+ if (temp != NIL)
+ lcons(temp, quals);
+ }
+ return quals;
+}
+
+/*
+ * Row-security plans are dependent on the current user id because of the if
+ * (superuser) test. So row-security plans must be invalidated if the user id
+ * changes.
+ */
+static void
+add_uid_plan_inval(PlannerGlobal* glob)
+{
+ PlanInvalItem *pi_item;
+
+ if (!OidIsValid(glob->planUserId))
+ {
+ /* Plan invalidation on session user-id */
+ glob->planUserId = GetUserId();
+
+ /* Plan invalidation on catalog updates of pg_authid */
+ pi_item = makeNode(PlanInvalItem);
+ pi_item->cacheId = AUTHOID;
+ pi_item->hashValue =
+ GetSysCacheHashValue1(AUTHOID,
+ ObjectIdGetDatum(glob->planUserId));
+ glob->invalItems = lappend(glob->invalItems, pi_item);
+ }
+ else
+ Assert(glob->planUserId == GetUserId());
+
+}
diff --git a/src/include/optimizer/rowsecurity.h b/src/include/optimizer/rowsecurity.h
new file mode 100644
index 0000000..453d79a
--- /dev/null
+++ b/src/include/optimizer/rowsecurity.h
@@ -0,0 +1,25 @@
+/* -------------------------------------------------------------------------
+ *
+ * rowsecurity.h
+ * prototypes for optimizer/rowsecurity.c
+ *
+ * Portions Copyright (c) 1996-2012, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * -------------------------------------------------------------------------
+ */
+#ifndef ROWSECURITY_H
+#define ROWSECURITY_H
+
+#include "nodes/execnodes.h"
+#include "nodes/parsenodes.h"
+#include "nodes/relation.h"
+#include "utils/rel.h"
+
+typedef List *(*row_security_policy_hook_type)(CmdType cmdtype,
+ Relation relation);
+extern PGDLLIMPORT row_security_policy_hook_type row_security_policy_hook;
+
+extern bool prepend_row_security_quals(PlannerInfo* root, RangeTblEntry* rte);
+
+#endif /* ROWSECURITY_H */
--
1.8.3.1
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2014-01-29 13:47 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2014-01-24 09:04 Re: Infinite recursion in row-security based on updatable s.b. views Craig Ringer <craig@2ndquadrant.com>
2014-01-24 11:16 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2014-01-28 06:11 ` Craig Ringer <craig@2ndquadrant.com>
2014-01-28 08:39 ` Craig Ringer <craig@2ndquadrant.com>
2014-01-28 10:27 ` Craig Ringer <craig@2ndquadrant.com>
2014-01-29 13:47 ` Craig Ringer <craig@2ndquadrant.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox