Re: [HACKERS] COPY command with RLS bug
* Michael Paquier (michael.paqu...@gmail.com) wrote: > On Sat, Oct 1, 2016 at 3:11 AM, Stephen Frost wrote: > > Comments and testing welcome, of course, though it's looking pretty good > > to me at this point and I'll likely commit it in another day or two > > unless issues are found. > > +* nodes/value.h) that correspond to the column name > "correspondS"? > > Except that, it looks good to me. Ah, good point, fixed. Just working through back-patching it and doing final checks, will be pushing it shortly. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] COPY command with RLS bug
On Sat, Oct 1, 2016 at 3:11 AM, Stephen Frost wrote: > Comments and testing welcome, of course, though it's looking pretty good > to me at this point and I'll likely commit it in another day or two > unless issues are found. +* nodes/value.h) that correspond to the column name "correspondS"? Except that, it looks good to me. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] COPY command with RLS bug
Adam, Michael,
* Adam Brightwell (adam.brightw...@crunchydata.com) wrote:
> > Looking for and improving test coverage for RLS is a good suggestion,
> > but let's not link the fate of the issue reported here with this
> > requirement. I have spent some time looking at this patch and this
> > looks in rather good shape to me (you even remembered to use the
> > prefix regress_* for the role name that you are adding!). So I have
> > marked this bug fix as ready for committer.
>
> Excellent, thanks for the review and feedback. I appreciate it.
Attached is an updated patch which fixes a few things and adds a few
regression tests. In particular, 'location' should be set to -1 as this
is an internally-generated query and there's no location inside the
query string passed in by the user that would make sense (and we
shouldn't ever end up in a situation where this query is failing in a
way that it would make sense to report a location to the user, either).
Also fixed a comment or two.
Comments and testing welcome, of course, though it's looking pretty good
to me at this point and I'll likely commit it in another day or two
unless issues are found.
Thanks!
Stephen
From ddf1247c550e7b30c9b8dc51cda438b7425bc7fa Mon Sep 17 00:00:00 2001
From: Stephen Frost
Date: Fri, 30 Sep 2016 13:31:22 -0400
Subject: [PATCH] Fix RLS with COPY (col1, col2) FROM tab
Attempting to COPY a subset of columns from a table with RLS enabled
would fail due to an invalid query being constructed (using a single
ColumnRef with the list of fields to exact in 'fields', but that's for
the different levels of an indirection for a single column, not for
specifying multiple columns).
Correct by building a ColumnRef and then RestTarget for each column
being requested and then adding those to the targetList for the select
query. Include regression tests to hopefully catch if this is broken
again in the future.
Patch-By: Adam Brightwell
Reviewed-By: Michael Paquier
---
src/backend/commands/copy.c | 63 --
src/test/regress/expected/copy2.out | 78 +
src/test/regress/sql/copy2.sql | 63 ++
3 files changed, 192 insertions(+), 12 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 432b0ca..e665f06 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -871,6 +871,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, uint64 *processed)
ColumnRef *cr;
ResTarget *target;
RangeVar *from;
+ List *targetList = NIL;
if (is_from)
ereport(ERROR,
@@ -878,21 +879,59 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, uint64 *processed)
errmsg("COPY FROM not supported with row-level security"),
errhint("Use INSERT statements instead.")));
- /* Build target list */
- cr = makeNode(ColumnRef);
-
+ /*
+ * Build target list
+ *
+ * If no columns are specified in the attribute list of the COPY
+ * command, then the target list is 'all' columns. Therefore, '*'
+ * should be used as the target list for the resulting SELECT
+ * statement.
+ *
+ * In the case that columns are specified in the attribute list,
+ * create a ColumnRef and ResTarget for each column and add them to
+ * the target list for the resulting SELECT statement.
+ */
if (!stmt->attlist)
+ {
+cr = makeNode(ColumnRef);
cr->fields = list_make1(makeNode(A_Star));
- else
-cr->fields = stmt->attlist;
+cr->location = -1;
+
+target = makeNode(ResTarget);
+target->name = NULL;
+target->indirection = NIL;
+target->val = (Node *) cr;
+target->location = -1;
- cr->location = 1;
+targetList = list_make1(target);
+ }
+ else
+ {
+ListCell *lc;
- target = makeNode(ResTarget);
- target->name = NULL;
- target->indirection = NIL;
- target->val = (Node *) cr;
- target->location = 1;
+foreach(lc, stmt->attlist)
+{
+ /*
+ * Build the ColumnRef for each column. The ColumnRef
+ * 'fields' property is a String 'Value' node (see
+ * nodes/value.h) that correspond to the column name
+ * respectively.
+ */
+ cr = makeNode(ColumnRef);
+ cr->fields = list_make1(lfirst(lc));
+ cr->location = -1;
+
+ /* Build the ResTarget and add the ColumnRef to it. */
+ target = makeNode(ResTarget);
+ target->name = NULL;
+ target->indirection = NIL;
+ target->val = (Node *) cr;
+ target->location = -1;
+
+ /* Add each column to the SELECT statement's target list */
+ targetList = lappend(targetList, target);
+}
+ }
/*
* Build RangeVar for from clause, fully qualified based on the
@@ -903,7 +942,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, uint64 *processed)
/* Build query */
select = makeNode(SelectStmt);
- select->targetList = list_make1(target);
+ select->targetList = targetList;
select->fro
Re: [HACKERS] COPY command with RLS bug
> Looking for and improving test coverage for RLS is a good suggestion, > but let's not link the fate of the issue reported here with this > requirement. I have spent some time looking at this patch and this > looks in rather good shape to me (you even remembered to use the > prefix regress_* for the role name that you are adding!). So I have > marked this bug fix as ready for committer. Excellent, thanks for the review and feedback. I appreciate it. -Adam -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] COPY command with RLS bug
On Sat, Sep 10, 2016 at 3:55 AM, Adam Brightwell wrote: >> Perhaps we should extend rowsecurity test with a more comprehensive >> set of tests rather than just fix the COPY one? > > I think more tests that provide value are always a *good* thing, > however, would argue that other tests 'unrelated' to this fix are more > of a TODO item than something to include with this fix. Though, I am > certainly willing to attempt to find/add more test cases around this > specific functionality if that is desired. Looking for and improving test coverage for RLS is a good suggestion, but let's not link the fate of the issue reported here with this requirement. I have spent some time looking at this patch and this looks in rather good shape to me (you even remembered to use the prefix regress_* for the role name that you are adding!). So I have marked this bug fix as ready for committer. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] COPY command with RLS bug
> Thanks for the report and the fix. Yup. I have added it to the 2016-11 commitfest: https://commitfest.postgresql.org/11/794/ > This seems a rather basic error to occur a year after release. > > Is this a problem with the testing of RLS? What other RLS related > failures exist in other commands? I think was simply due to a small gap in the test suite. > Perhaps we should extend rowsecurity test with a more comprehensive > set of tests rather than just fix the COPY one? I think more tests that provide value are always a *good* thing, however, would argue that other tests 'unrelated' to this fix are more of a TODO item than something to include with this fix. Though, I am certainly willing to attempt to find/add more test cases around this specific functionality if that is desired. -Adam -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] COPY command with RLS bug
On 8 September 2016 at 20:13, Adam Brightwell wrote: > I have discovered a bug with the COPY command, specifically related to RLS. ... > Connecting as a non-privileged user provides the following results: ... > COPY foo (a, b, c) TO stdout; -- fail > ERROR: missing FROM-clause entry for table "b" > LINE 1: copy foo (a, b, c) to stdout; ... > The issue seems to be that the target list for the resulting SELECT > statement is not being built correctly. I have attached a proposed > patch to fix this issue. As well, I have added a few regression tests > for this case. Thanks for the report and the fix. This seems a rather basic error to occur a year after release. Is this a problem with the testing of RLS? What other RLS related failures exist in other commands? Perhaps we should extend rowsecurity test with a more comprehensive set of tests rather than just fix the COPY one? -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] COPY command with RLS bug
All,
I have discovered a bug with the COPY command, specifically related to RLS.
The issue:
When running COPY as superuser on a table that has RLS enabled, RLS is
bypassed and therefore no issue exists. However, when performing a
COPY as a non-privileged user on the same table causes issues when
more than one column is specified as part of the command.
Assuming:
CREATE TABLE foo (a int, b int, c int);
... set up RLS
Connecting as a non-privileged user provides the following results:
COPY foo TO stdout; -- pass
COPY foo (a) TO stdout; -- pass
COPY foo (a, b, c) TO stdout; -- fail
The error related to the failure is the following:
ERROR: missing FROM-clause entry for table "b"
LINE 1: copy foo (a, b, c) to stdout;
I don't believe this to be a vulnerability simply because it doesn't
'leak' any data to a non-privileged user, it will just throw an error.
As well, this is only an issue when more than one column is specified
and '*' isn't implied. I have attached a 'test' file that can be used
to observe this behavior.
I have verified that this is an issue on REL9_5_STABLE, REL9_6_STABLE
and master.
Solution:
The issue seems to be that the target list for the resulting SELECT
statement is not being built correctly. I have attached a proposed
patch to fix this issue. As well, I have added a few regression tests
for this case.
If deemed appropriate, then I'll add this to the currently open Commitfest.
Please let me know if there are any questions.
Thanks,
Adam
test-copy-rls.sql
Description: application/sql
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 432b0ca..a3777d9 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -871,6 +871,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, uint64 *processed)
ColumnRef *cr;
ResTarget *target;
RangeVar *from;
+ List *targetList = NIL;
if (is_from)
ereport(ERROR,
@@ -878,21 +879,62 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, uint64 *processed)
errmsg("COPY FROM not supported with row-level security"),
errhint("Use INSERT statements instead.")));
- /* Build target list */
- cr = makeNode(ColumnRef);
-
+ /*
+ * Build target list
+ *
+ * If no columns are specified in the attribute list of the COPY
+ * command, then the target list is 'all' columns. Therefore, '*'
+ * should be used as the target list for the resulting SELECT
+ * statement.
+ *
+ * In the case that columns are specified in the attribute list,
+ * create a ColumnRef and ResTarget for each column and add them to
+ * the target list for the resulting SELECT statement.
+ */
if (!stmt->attlist)
+ {
+cr = makeNode(ColumnRef);
cr->fields = list_make1(makeNode(A_Star));
+cr->location = 1;
+
+target = makeNode(ResTarget);
+target->name = NULL;
+target->indirection = NIL;
+target->val = (Node *) cr;
+target->location = 1;
+
+targetList = list_make1(target);
+ }
else
-cr->fields = stmt->attlist;
+ {
+ListCell *lc;
+int location = 1;
+
+foreach(lc, stmt->attlist)
+{
+ /*
+ * Build the ColumnRef for each column. The ColumnRef
+ * 'fields' property is a String 'Value' node (see
+ * nodes/value.h) that correspond to the column name
+ * respectively.
+ */
+ cr = makeNode(ColumnRef);
+ cr->fields = list_make1(lfirst(lc));
+ cr->location = location;
+
+ /* Build the ResTarget and add the ColumnRef to it. */
+ target = makeNode(ResTarget);
+ target->name = NULL;
+ target->indirection = NIL;
+ target->val = (Node *) cr;
+ target->location = location;
- cr->location = 1;
+ /* Add each column to the SELECT statements target list */
+ targetList = lappend(targetList, target);
- target = makeNode(ResTarget);
- target->name = NULL;
- target->indirection = NIL;
- target->val = (Node *) cr;
- target->location = 1;
+ location += 1;
+}
+ }
/*
* Build RangeVar for from clause, fully qualified based on the
@@ -903,7 +945,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, uint64 *processed)
/* Build query */
select = makeNode(SelectStmt);
- select->targetList = list_make1(target);
+ select->targetList = targetList;
select->fromClause = list_make1(from);
query = (Node *) select;
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index 5f6260a..13251c6 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -460,9 +460,57 @@ select * from check_con_tbl;
(2 rows)
+-- test with RLS enabled.
+CREATE USER regress_rls_copy_user;
+CREATE TABLE rls_t1 (a int, b int, c int);
+COPY rls_t1 (a, b, c) from stdin;
+CREATE POLICY p1 ON rls_t1 FOR SELECT USING (a % 2 = 0);
+ALTER TABLE rls_t1 ENABLE ROW LEVEL SECURITY;
+ALTER TABLE rls_t1 FORCE ROW LEVEL SECURITY;
+GRANT SELECT ON TABLE rls_t1 TO
