Re: copy.c handling for RLS is insecure

* 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