Thread: 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
Attachment
On 8 September 2016 at 20:13, Adam Brightwell <adam.brightwell@crunchydata.com> 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 Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> 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
On Sat, Sep 10, 2016 at 3:55 AM, Adam Brightwell <adam.brightwell@crunchydata.com> 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
> 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
Adam, Michael, * Adam Brightwell (adam.brightwell@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
Attachment
On Sat, Oct 1, 2016 at 3:11 AM, Stephen Frost <sfrost@snowman.net> 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
* Michael Paquier (michael.paquier@gmail.com) wrote: > On Sat, Oct 1, 2016 at 3:11 AM, Stephen Frost <sfrost@snowman.net> 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