Re: copy.c handling for RLS is insecure

Noah,

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