Re: copy.c handling for RLS is insecure

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

Ok, makes sense, I see now.

> > 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.

Sorry, wasn't clear- was referring to when 'relation' in CopyStmt is
NULL (which happens when the production with the sub-select is used).

> > 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.

Yeah, that's correct.  I suppose there's some possible risk of things
changing between when you parse the query and when it actually gets
analyzed and rewritten, but that's not a security risk per-se..

> 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.

Right, ok.  Will need to think a bit more about how to address this such
that we aren't doing another lookup...
Thanks!
    Stephen