Re: copy.c handling for RLS is insecure

On Mon, Oct 6, 2014 at 2:49 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * 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?

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