Re: copy.c handling for RLS is insecure - Mailing list pgsql-hackers

On Mon, Oct 6, 2014 at 2:49 PM, Stephen Frost <sfrost@snowman.net> wrote:
> * Robert Haas (robertmhaas@gmail.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