Re: [HACKERS] copy.c handling for RLS is insecure

[Stephen Frost]

All,

* Stephen Frost (sfr...@snowman.net) wrote:
 * Andres Freund (and...@anarazel.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


signature.asc
Description: Digital signature

Re: [HACKERS] copy.c handling for RLS is insecure

[Andres Freund]

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.


-- 
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.c handling for RLS is insecure

[Stephen Frost]

Noah,

* Noah Misch (n...@leadboat.com) wrote:
 On Tue, Dec 02, 2014 at 11:32:27AM -0500, Stephen Frost wrote:
  * Robert Haas (robertmh...@gmail.com) wrote:
   On Thu, Nov 27, 2014 at 2:03 AM, Stephen Frost sfr...@snowman.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


signature.asc
Description: Digital signature

Re: [HACKERS] copy.c handling for RLS is insecure

[Noah Misch]

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.


-- 
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.c handling for RLS is insecure

[Stephen Frost]

Noah,

* Noah Misch (n...@leadboat.com) wrote:
 On Tue, Dec 02, 2014 at 11:32:27AM -0500, Stephen Frost wrote:
  * Robert Haas (robertmh...@gmail.com) wrote:
   On Thu, Nov 27, 2014 at 2:03 AM, Stephen Frost sfr...@snowman.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


signature.asc
Description: Digital signature

Re: [HACKERS] copy.c handling for RLS is insecure

[Noah Misch]

On Tue, Dec 02, 2014 at 11:32:27AM -0500, Stephen Frost wrote:
 * Robert Haas (robertmh...@gmail.com) wrote:
  On Thu, Nov 27, 2014 at 2:03 AM, Stephen Frost sfr...@snowman.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.


-- 
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.c handling for RLS is insecure

[Robert Haas]

On Thu, Nov 27, 2014 at 2:03 AM, Stephen Frost sfr...@snowman.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


-- 
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.c handling for RLS is insecure

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
 On Thu, Nov 27, 2014 at 2:03 AM, Stephen Frost sfr...@snowman.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


signature.asc
Description: Digital signature

Re: [HACKERS] copy.c handling for RLS is insecure

[Stephen Frost]

Robert,

* Stephen Frost (sfr...@snowman.net) wrote:
 * Robert Haas (robertmh...@gmail.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


signature.asc
Description: Digital signature

[HACKERS] copy.c handling for RLS is insecure

[Robert Haas]

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


-- 
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.c handling for RLS is insecure

[Stephen Frost]

* Robert Haas (robertmh...@gmail.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


signature.asc
Description: Digital signature

Re: [HACKERS] copy.c handling for RLS is insecure

[Tom Lane]

Stephen Frost sfr...@snowman.net writes:
 * Robert Haas (robertmh...@gmail.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


-- 
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.c handling for RLS is insecure

[Robert Haas]

On Mon, Oct 6, 2014 at 2:49 PM, Stephen Frost sfr...@snowman.net wrote:
 * Robert Haas (robertmh...@gmail.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


-- 
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.c handling for RLS is insecure

[Robert Haas]

I left out a few words there.

On Mon, Oct 6, 2014 at 3:07 PM, Robert Haas robertmh...@gmail.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


-- 
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.c handling for RLS is insecure

[David Fetter]

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 da...@fetter.org http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.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


-- 
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.c handling for RLS is insecure

[Stephen Frost]

David,

On Monday, October 6, 2014, David Fetter da...@fetter.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