Noah, * Noah Misch ([email protected]) wrote: > 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.
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
