Re: copy.c handling for RLS is insecure

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