copy.c handling for RLS is insecure

In DoCopy, some RLS-specific code constructs a SelectStmt to handle
the case where COPY TO is invoked on an RLS-protected relation. But I
think this step is bogus in two ways:

/* Build FROM clause */
from = makeRangeVar(NULL, RelationGetRelationName(rel), 1);

First, because relations are schema objects, there could be multiple
relations with the same name. The RangeVar might end up referring to
a different one of those objects than the user originally specified.
That seems like it could be surprising at best and a security
vulnerability at worst. So at the very list I think this needs to
pass the schema name as well as the relation name. I'm not 100% sure
it's OK even then, because what about a concurrent rename of the
schema?

Second, the third argument to makeRangeVar is a parse location, and I
believe it it is conventional to use -1, rather than 1, when no
location can be identified.

Thanks,

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> In DoCopy, some RLS-specific code constructs a SelectStmt to handle
> the case where COPY TO is invoked on an RLS-protected relation. But I
> think this step is bogus in two ways:
>
> /* Build FROM clause */
> from = makeRangeVar(NULL, RelationGetRelationName(rel), 1);
>
> First, because relations are schema objects, there could be multiple
> relations with the same name. The RangeVar might end up referring to
> a different one of those objects than the user originally specified.

Argh. That's certainly no good. It should just be using the RangeVar
relation passed in from CopyStmt, no? We don't have to address the case
where it's NULL (tho we should perhaps Assert(), just to be sure), as
that would only happen in the COPY select_with_parens ... production and
this is only for the normal 'COPY relname' case.

> That seems like it could be surprising at best and a security
> vulnerability at worst. So at the very list I think this needs to
> pass the schema name as well as the relation name. I'm not 100% sure
> it's OK even then, because what about a concurrent rename of the
> schema?

Hmm, that's certainly an interesting point, but I'm trying to work out
how this is different from normal COPY..? pg_analyze_and_rewrite()
happens for both cases down in BeginCopy().

> Second, the third argument to makeRangeVar is a parse location, and I
> believe it it is conventional to use -1, rather than 1, when no
> location can be identified.

Err, you're right, but I think we should just eliminate the entire
makeRangeVar() call, and then the location would be correctly set too.

Thanks!

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> First, because relations are schema objects, there could be multiple
>> relations with the same name. The RangeVar might end up referring to
>> a different one of those objects than the user originally specified.

> Argh. That's certainly no good. It should just be using the RangeVar
> relation passed in from CopyStmt, no?

No, it shouldn't be doing that either. That would imply looking up the
relation a second time, and then you have a race condition against
concurrent renames (the same type of security bug Robert spent a great
deal of time on, not so long ago).

Once you've identified the target relation by OID, nothing else later in
the command should be doing a fresh lookup by name. Period. If you've
got APIs in here that depend on passing RangeVars to identify relations,
those APIs are broken and need to be changed.

regards, tom lane

On Mon, Oct 6, 2014 at 2:49 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> In DoCopy, some RLS-specific code constructs a SelectStmt to handle
>> the case where COPY TO is invoked on an RLS-protected relation. But I
>> think this step is bogus in two ways:
>>
>> /* Build FROM clause */
>> from = makeRangeVar(NULL, RelationGetRelationName(rel), 1);
>>
>> First, because relations are schema objects, there could be multiple
>> relations with the same name. The RangeVar might end up referring to
>> a different one of those objects than the user originally specified.
>
> Argh. That's certainly no good. It should just be using the RangeVar
> relation passed in from CopyStmt, no?

I don't think that's adequate. You can't do a RangeVar-to-OID
translation, use the resulting OID for some security-relevant
decision, and then repeat the RangeVar-to-OID translation and hope you
get the same answer. That's what led to CVE-2014-0062, fixed by
commit 5f173040e324f6c2eebb90d86cf1b0cdb5890f0a. I can't work out
off-hand whether the issue is exploitable in this instance, but it
sure seems like a bad idea to rely on it not being so.

> We don't have to address the case
> where it's NULL (tho we should perhaps Assert(), just to be sure), as
> that would only happen in the COPY select_with_parens ... production and
> this is only for the normal 'COPY relname' case.

The antecedent of "it" in "the case where it's NULL" is unclear to me.

> Hmm, that's certainly an interesting point, but I'm trying to work out
> how this is different from normal COPY..? pg_analyze_and_rewrite()
> happens for both cases down in BeginCopy().

As far as I can see, the previous code only looked up any given name
once. If you got a relation name, DoCopy() looked it up, and then
BeginCopy() references it only by the passed-down Relation descriptor;
if you got a query, DoCopy() ignores it, and then BeginCopy. All of
which is fine, at least AFAICS; if you think otherwise, that should be
reported to pgsql-security. The problem with your code is that you
start with a relation name (and thus look it up in DoCopy()) and then
construct a query (which causes the name to be looked up again when
the query is passed to pg_analyze_and_rewrite() from BeginCopy()) --
and the lookup might not get the same answer both times. That is, not
to put to fine a point on it, bad news.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

I left out a few words there.

On Mon, Oct 6, 2014 at 3:07 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Hmm, that's certainly an interesting point, but I'm trying to work out
>> how this is different from normal COPY..? pg_analyze_and_rewrite()
>> happens for both cases down in BeginCopy().
>
> As far as I can see, the previous code only looked up any given name
> once. If you got a relation name, DoCopy() looked it up, and then
> BeginCopy() references it only by the passed-down Relation descriptor;
> if you got a query, DoCopy() ignores it, and then BeginCopy.

...passes it to pg_analyze_and_rewrite(), which looks up any names it contains.

> All of
> which is fine, at least AFAICS; if you think otherwise, that should be
> reported to pgsql-security. The problem with your code is that you
> start with a relation name (and thus look it up in DoCopy()) and then
> construct a query (which causes the name to be looked up again when
> the query is passed to pg_analyze_and_rewrite() from BeginCopy()) --
> and the lookup might not get the same answer both times. That is, not
> to put to fine a point on it, bad news.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Mon, Oct 6, 2014 at 2:49 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > Argh. That's certainly no good. It should just be using the RangeVar
> > relation passed in from CopyStmt, no?
>
> I don't think that's adequate. You can't do a RangeVar-to-OID
> translation, use the resulting OID for some security-relevant
> decision, and then repeat the RangeVar-to-OID translation and hope you
> get the same answer. That's what led to CVE-2014-0062, fixed by
> commit 5f173040e324f6c2eebb90d86cf1b0cdb5890f0a. I can't work out
> off-hand whether the issue is exploitable in this instance, but it
> sure seems like a bad idea to rely on it not being so.

Ok, makes sense, I see now.

> > We don't have to address the case
> > where it's NULL (tho we should perhaps Assert(), just to be sure), as
> > that would only happen in the COPY select_with_parens ... production and
> > this is only for the normal 'COPY relname' case.
>
> The antecedent of "it" in "the case where it's NULL" is unclear to me.

Sorry, wasn't clear- was referring to when 'relation' in CopyStmt is
NULL (which happens when the production with the sub-select is used).

> > Hmm, that's certainly an interesting point, but I'm trying to work out
> > how this is different from normal COPY..? pg_analyze_and_rewrite()
> > happens for both cases down in BeginCopy().
>
> As far as I can see, the previous code only looked up any given name
> once. If you got a relation name, DoCopy() looked it up, and then
> BeginCopy() references it only by the passed-down Relation descriptor;
> if you got a query, DoCopy() ignores it, and then BeginCopy. All of
> which is fine, at least AFAICS; if you think otherwise, that should be
> reported to pgsql-security.

Yeah, that's correct. I suppose there's some possible risk of things
changing between when you parse the query and when it actually gets
analyzed and rewritten, but that's not a security risk per-se..

> The problem with your code is that you
> start with a relation name (and thus look it up in DoCopy()) and then
> construct a query (which causes the name to be looked up again when
> the query is passed to pg_analyze_and_rewrite() from BeginCopy()) --
> and the lookup might not get the same answer both times. That is, not
> to put to fine a point on it, bad news.

Right, ok. Will need to think a bit more about how to address this such
that we aren't doing another lookup...

Thanks!

Stephen

On Mon, Oct 06, 2014 at 03:15:25PM -0400, Stephen Frost wrote:

> > As far as I can see, the previous code only looked up any given name
> > once. If you got a relation name, DoCopy() looked it up, and then
> > BeginCopy() references it only by the passed-down Relation descriptor;
> > if you got a query, DoCopy() ignores it, and then BeginCopy. All of
> > which is fine, at least AFAICS; if you think otherwise, that should be
> > reported to pgsql-security.
>
> Yeah, that's correct. I suppose there's some possible risk of things
> changing between when you parse the query and when it actually gets
> analyzed and rewritten, but that's not a security risk per-se..

I'm not sure I understand. If that change violates an access control,
it's a security risk /per se/, as you put it.

Are you saying that such changes, even though they might be bugs,
categorically couldn't violate an access control?

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David,

On Monday, October 6, 2014, David Fetter <david(at)fetter(dot)org> wrote:

> On Mon, Oct 06, 2014 at 03:15:25PM -0400, Stephen Frost wrote:
>
> > > As far as I can see, the previous code only looked up any given name
> > > once. If you got a relation name, DoCopy() looked it up, and then
> > > BeginCopy() references it only by the passed-down Relation descriptor;
> > > if you got a query, DoCopy() ignores it, and then BeginCopy. All of
> > > which is fine, at least AFAICS; if you think otherwise, that should be
> > > reported to pgsql-security.
> >
> > Yeah, that's correct. I suppose there's some possible risk of things
> > changing between when you parse the query and when it actually gets
> > analyzed and rewritten, but that's not a security risk per-se..
>
> I'm not sure I understand. If that change violates an access control,
> it's a security risk /per se/, as you put it.

The case I was referring to doesn't violate an access control. I was merely
pointing out that things can change between when the query is submitted by
the user (or even later, during parse analysis) and when we
actually resolve names to OIDs.

Thanks,

Stephen

Robert,

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> > In DoCopy, some RLS-specific code constructs a SelectStmt to handle
> > the case where COPY TO is invoked on an RLS-protected relation. But I
> > think this step is bogus in two ways:
> >
> > /* Build FROM clause */
> > from = makeRangeVar(NULL, RelationGetRelationName(rel), 1);
> >
> > First, because relations are schema objects, there could be multiple
> > relations with the same name. The RangeVar might end up referring to
> > a different one of those objects than the user originally specified.
>
> Argh. That's certainly no good. It should just be using the RangeVar
> relation passed in from CopyStmt, no? We don't have to address the case
> where it's NULL (tho we should perhaps Assert(), just to be sure), as
> that would only happen in the COPY select_with_parens ... production and
> this is only for the normal 'COPY relname' case.

Alright, I've done the change to use the RangeVar from CopyStmt, but
also added a check wherein we verify that the relation's OID returned
from the planned query is the same as the relation's OID that we did the
RLS check on- if they're different, we throw an error. Please let me
know if there are any remaining concerns.

Thanks!

Stephen

On Thu, Nov 27, 2014 at 2:03 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Alright, I've done the change to use the RangeVar from CopyStmt, but
> also added a check wherein we verify that the relation's OID returned
> from the planned query is the same as the relation's OID that we did the
> RLS check on- if they're different, we throw an error. Please let me
> know if there are any remaining concerns.

That's clearly an improvement, but I'm not sure it's water-tight.
What if the name that originally referenced a table ended up
referencing a view? Then you could get
list_length(plan->relationOids) != 1.

(And, in that case, I also wonder if you could get
eval_const_expressions() to do evil things on your behalf while
planning.)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Thu, Nov 27, 2014 at 2:03 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > Alright, I've done the change to use the RangeVar from CopyStmt, but
> > also added a check wherein we verify that the relation's OID returned
> > from the planned query is the same as the relation's OID that we did the
> > RLS check on- if they're different, we throw an error. Please let me
> > know if there are any remaining concerns.
>
> That's clearly an improvement, but I'm not sure it's water-tight.
> What if the name that originally referenced a table ended up
> referencing a view? Then you could get
> list_length(plan->relationOids) != 1.

I'll test it out and see what happens. Certainly a good question and
if there's an issue there then I'll get it addressed.

> (And, in that case, I also wonder if you could get
> eval_const_expressions() to do evil things on your behalf while
> planning.)

If it can be made to reference a view then there's an issue as the view
might include a function call itself which is provided by the attacker..
I'm not sure that we have to really worry about anything more
complicated than that.

Clearly, if we found a relation originally then we need that same
relation with the same OID after the conversion to a query.

Thanks,

Stephen

On Tue, Dec 02, 2014 at 11:32:27AM -0500, Stephen Frost wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> > On Thu, Nov 27, 2014 at 2:03 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > > Alright, I've done the change to use the RangeVar from CopyStmt, but
> > > also added a check wherein we verify that the relation's OID returned
> > > from the planned query is the same as the relation's OID that we did the
> > > RLS check on- if they're different, we throw an error. Please let me
> > > know if there are any remaining concerns.

Here is the check in question (added in commit 143b39c):

plan = planner(query, 0, NULL);

/*
* If we were passed in a relid, make sure we got the same one back
* after planning out the query. It's possible that it changed
* between when we checked the policies on the table and decided to
* use a query and now.
*/
if (queryRelId != InvalidOid)
{
Oid relid = linitial_oid(plan->relationOids);

/*
* There should only be one relationOid in this case, since we
* will only get here when we have changed the command for the
* user from a "COPY relation TO" to "COPY (SELECT * FROM
* relation) TO", to allow row level security policies to be
* applied.
*/
Assert(list_length(plan->relationOids) == 1);

if (relid != queryRelId)
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("relation referenced by COPY statement has changed")));
}

> > That's clearly an improvement, but I'm not sure it's water-tight.
> > What if the name that originally referenced a table ended up
> > referencing a view? Then you could get
> > list_length(plan->relationOids) != 1.
>
> I'll test it out and see what happens. Certainly a good question and
> if there's an issue there then I'll get it addressed.

Yes, it can be made to reference a view and trip the assertion.

> > (And, in that case, I also wonder if you could get
> > eval_const_expressions() to do evil things on your behalf while
> > planning.)
>
> If it can be made to reference a view then there's an issue as the view
> might include a function call itself which is provided by the attacker..

Indeed. As the parenthetical remark supposed, the check happens too late to
prevent a security breach. planner() has run eval_const_expressions(),
executing code of the view owner's choosing.

> Clearly, if we found a relation originally then we need that same
> relation with the same OID after the conversion to a query.

That is necessary but not sufficient. CREATE RULE can convert a table to a
view without changing the OID, thereby fooling the check. Test procedure:

-- as superuser (or createrole)
create user blackhat;
create user alice;

-- as blackhat
begin;
create table exploit_rls_copy (c int);
alter table exploit_rls_copy enable row level security;
grant select on exploit_rls_copy to public;
commit;

-- as alice
-- first, set breakpoint on BeginCopy
copy exploit_rls_copy to stdout;

-- as blackhat
begin;
create or replace function leak() returns int immutable as $$begin
raise notice 'in leak()'; return 7; end$$ language plpgsql;
create rule "_RETURN" as on select to exploit_rls_copy do instead
select leak() as c from (values (0)) dummy;
commit;

-- Release breakpoint. leak() function call happens. After that, assertion
-- fires if enabled. ERROR does not fire in any case.

Noah,

* Noah Misch (noah(at)leadboat(dot)com) wrote:
> On Tue, Dec 02, 2014 at 11:32:27AM -0500, Stephen Frost wrote:
> > * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> > > On Thu, Nov 27, 2014 at 2:03 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > > > Alright, I've done the change to use the RangeVar from CopyStmt, but
> > > > also added a check wherein we verify that the relation's OID returned
> > > > from the planned query is the same as the relation's OID that we did the
> > > > RLS check on- if they're different, we throw an error. Please let me
> > > > know if there are any remaining concerns.
>
> Here is the check in question (added in commit 143b39c):
>
> plan = planner(query, 0, NULL);
>
> /*
> * If we were passed in a relid, make sure we got the same one back
> * after planning out the query. It's possible that it changed
> * between when we checked the policies on the table and decided to
> * use a query and now.
> */
> if (queryRelId != InvalidOid)
> {
> Oid relid = linitial_oid(plan->relationOids);
>
> /*
> * There should only be one relationOid in this case, since we
> * will only get here when we have changed the command for the
> * user from a "COPY relation TO" to "COPY (SELECT * FROM
> * relation) TO", to allow row level security policies to be
> * applied.
> */
> Assert(list_length(plan->relationOids) == 1);
>
> if (relid != queryRelId)
> ereport(ERROR,
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> errmsg("relation referenced by COPY statement has changed")));
> }
>
> > > That's clearly an improvement, but I'm not sure it's water-tight.
> > > What if the name that originally referenced a table ended up
> > > referencing a view? Then you could get
> > > list_length(plan->relationOids) != 1.
> >
> > I'll test it out and see what happens. Certainly a good question and
> > if there's an issue there then I'll get it addressed.
>
> Yes, it can be made to reference a view and trip the assertion.

Ok. We can certainly make that assertion be a run-time consideration
instead, though I'm not thrilled with that being the only safe-guard.

> > > (And, in that case, I also wonder if you could get
> > > eval_const_expressions() to do evil things on your behalf while
> > > planning.)
> >
> > If it can be made to reference a view then there's an issue as the view
> > might include a function call itself which is provided by the attacker..
>
> Indeed. As the parenthetical remark supposed, the check happens too late to
> prevent a security breach. planner() has run eval_const_expressions(),
> executing code of the view owner's choosing.

Right.

> > Clearly, if we found a relation originally then we need that same
> > relation with the same OID after the conversion to a query.
>
> That is necessary but not sufficient. CREATE RULE can convert a table to a
> view without changing the OID, thereby fooling the check. Test procedure:

Ugh, yes, rules would cause a problem for this..

> -- as superuser (or createrole)
> create user blackhat;
> create user alice;
>
> -- as blackhat
> begin;
> create table exploit_rls_copy (c int);
> alter table exploit_rls_copy enable row level security;
> grant select on exploit_rls_copy to public;
> commit;
>
> -- as alice
> -- first, set breakpoint on BeginCopy
> copy exploit_rls_copy to stdout;
>
> -- as blackhat
> begin;
> create or replace function leak() returns int immutable as $$begin
> raise notice 'in leak()'; return 7; end$$ language plpgsql;
> create rule "_RETURN" as on select to exploit_rls_copy do instead
> select leak() as c from (values (0)) dummy;
> commit;
>
> -- Release breakpoint. leak() function call happens. After that, assertion
> -- fires if enabled. ERROR does not fire in any case.

Thanks for pointing this out. I'll look into it.

Stephen

Noah,

* Noah Misch (noah(at)leadboat(dot)com) wrote:
> On Tue, Dec 02, 2014 at 11:32:27AM -0500, Stephen Frost wrote:
> > * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> > > On Thu, Nov 27, 2014 at 2:03 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > > > Alright, I've done the change to use the RangeVar from CopyStmt, but
> > > > also added a check wherein we verify that the relation's OID returned
> > > > from the planned query is the same as the relation's OID that we did the
> > > > RLS check on- if they're different, we throw an error. Please let me
> > > > know if there are any remaining concerns.
>
> Here is the check in question (added in commit 143b39c):
>
> plan = planner(query, 0, NULL);
>
> /*
> * If we were passed in a relid, make sure we got the same one back
> * after planning out the query. It's possible that it changed
> * between when we checked the policies on the table and decided to
> * use a query and now.
> */
> if (queryRelId != InvalidOid)
> {
> Oid relid = linitial_oid(plan->relationOids);
>
> /*
> * There should only be one relationOid in this case, since we
> * will only get here when we have changed the command for the
> * user from a "COPY relation TO" to "COPY (SELECT * FROM
> * relation) TO", to allow row level security policies to be
> * applied.
> */
> Assert(list_length(plan->relationOids) == 1);
>
> if (relid != queryRelId)
> ereport(ERROR,
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> errmsg("relation referenced by COPY statement has changed")));
> }
>
> > > That's clearly an improvement, but I'm not sure it's water-tight.
> > > What if the name that originally referenced a table ended up
> > > referencing a view? Then you could get
> > > list_length(plan->relationOids) != 1.
> >
> > I'll test it out and see what happens. Certainly a good question and
> > if there's an issue there then I'll get it addressed.
>
> Yes, it can be made to reference a view and trip the assertion.

There's a different issue with that Assertion, actually- if you've got
an RLS policy which references another table directly in a subselect
then you'll also trip it up, but more below..

> > > (And, in that case, I also wonder if you could get
> > > eval_const_expressions() to do evil things on your behalf while
> > > planning.)
> >
> > If it can be made to reference a view then there's an issue as the view
> > might include a function call itself which is provided by the attacker..
>
> Indeed. As the parenthetical remark supposed, the check happens too late to
> prevent a security breach. planner() has run eval_const_expressions(),
> executing code of the view owner's choosing.

It happens too late to prevent the user from running code specified by
the table owner- but there's not a solution to that risk except to set
'row_security = off' and use a mechanism which doesn't respect on-select
rules, which is only COPY, right? A normal SELECT will certainly fire
the on-select rule.

> > Clearly, if we found a relation originally then we need that same
> > relation with the same OID after the conversion to a query.
>
> That is necessary but not sufficient. CREATE RULE can convert a table to a
> view without changing the OID, thereby fooling the check. Test procedure:

It's interesting to consider that COPY purportedly operates under the
SELECT privilege, yet fails to respect on-select rules.

Having spent a bit of time thinking about this now, it occurs to me that
we've drifted from the original concern and are now trying to solve an
insolvable issue here. We're not trying to prevent against an attacker
who owns the table we're going to COPY and wants to get us to run code
they've written- that can happen by simply having RLS and that's why
it's not enabled by default for superusers and why we have
'row_security = off', which pg_dump sets by default.

The original issue that Robert brought up was the concern about multiple
lookups of RangeVar->Oid. That was a problem in the CVE highlighted and
the original/current coding because we weren't doing fully qualified
lookups based on the originally found and locked Oid. I'm trying to
figure out why weren't not simply doing that here.

After a bit of discussion with Andres, my thinking on this is to do the
following:

- Fully qualify the name based on the opened relation
- Keep the initial lock on the relation throughout
- Remove the Assert() (other relations can be pulled in by RLS)
- Keep the OID check, shouldn't hurt to have it

Thoughts?

Thanks!

Stephen

On Wed, Jul 08, 2015 at 10:55:47AM -0400, Stephen Frost wrote:
> It's interesting to consider that COPY purportedly operates under the
> SELECT privilege, yet fails to respect on-select rules.

In released branches, COPY consistently refuses to operate directly on a view.
(There's no (longer?) such thing as a table with an ON SELECT rule. CREATE
RULE "_RETURN" AS ON SELECT ... converts a table to a view.)

> Having spent a bit of time thinking about this now, it occurs to me that
> we've drifted from the original concern and are now trying to solve an
> insolvable issue here. We're not trying to prevent against an attacker
> who owns the table we're going to COPY and wants to get us to run code
> they've written- that can happen by simply having RLS and that's why
> it's not enabled by default for superusers and why we have
> 'row_security = off', which pg_dump sets by default.

The problem I wanted to see solved was the fact that, by running a DDL command
concurrent with a "COPY rel TO" command, you can make the COPY read from a
view. That is not possible in any serial execution of COPY with DDL. Now,
you make a good point that before this undesirable outcome can happen, the
user issuing the COPY command will have already trusted the roles that can
pass owner checks for "rel". That probably makes this useless as an attack
tool. Nonetheless, I don't want "COPY rejects views" to soften into "COPY
rejects views, except in XYZ race condition."

> After a bit of discussion with Andres, my thinking on this is to do the
> following:
>
> - Fully qualify the name based on the opened relation
> - Keep the initial lock on the relation throughout
> - Remove the Assert() (other relations can be pulled in by RLS)

That's much better. We have considerable experience with designs like that.

> - Keep the OID check, shouldn't hurt to have it

What benefit is left? The planner does not promise any particular order
within relationOids, and an order change could make false alarms here.

On 2015-07-09 01:28:28 -0400, Noah Misch wrote:
> > - Keep the OID check, shouldn't hurt to have it
>
> What benefit is left?

A bit of defense in depth. We execute user defined code in COPY
(e.g. BEFORE triggers). That user defined code could very well replace
the relation. Now I think right now that'd happen late enough, so the
second lookup already happened. But a bit more robust defense against
that sounds good to me.

Noah, Andres,

* Andres Freund (andres(at)anarazel(dot)de) wrote:
> On 2015-07-09 01:28:28 -0400, Noah Misch wrote:
> > > - Keep the OID check, shouldn't hurt to have it
> >
> > What benefit is left?
>
> A bit of defense in depth. We execute user defined code in COPY
> (e.g. BEFORE triggers). That user defined code could very well replace
> the relation. Now I think right now that'd happen late enough, so the
> second lookup already happened. But a bit more robust defense against
> that sounds good to me.

Attached patch keeps the relation locked, fully qualifies it when
building up the query, and uses list_member_oid() to check that the
relation's OID ends up in the resulting relationOids list (to address
Noah's point that the planner doesn't guarantee the ordering; I doubt
that list will ever be more than a few entries long).

Also removes the misguided Assert().

Barring objections, I'll commit this (and backpatch to 9.5) tomorrow.

Thanks!

Stephen

All,

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> * Andres Freund (andres(at)anarazel(dot)de) wrote:
> > On 2015-07-09 01:28:28 -0400, Noah Misch wrote:
> > > > - Keep the OID check, shouldn't hurt to have it
> > >
> > > What benefit is left?
> >
> > A bit of defense in depth. We execute user defined code in COPY
> > (e.g. BEFORE triggers). That user defined code could very well replace
> > the relation. Now I think right now that'd happen late enough, so the
> > second lookup already happened. But a bit more robust defense against
> > that sounds good to me.
>
> Attached patch keeps the relation locked, fully qualifies it when
> building up the query, and uses list_member_oid() to check that the
> relation's OID ends up in the resulting relationOids list (to address
> Noah's point that the planner doesn't guarantee the ordering; I doubt
> that list will ever be more than a few entries long).
>
> Also removes the misguided Assert().
>
> Barring objections, I'll commit this (and backpatch to 9.5) tomorrow.

Apologies for not pushing this before I left on vacation. I've done so
now.

Thanks!

Stephen