Hi,
There is at present a comment at the top of transformStatsStmt which
says "To avoid race conditions, it's important that this function
relies only on the passed-in relid (and not on stmt->relation) to
determine the target relation." However, doing what the comment says
we need to do doesn't actually avoid the problem we need to avoid. The
issue here, as I understand it, is that if we look up the same
less-than-fully-qualified name multiple times, we might get different
answers due to concurrent activity, and that might create a security
vulnerability, along the lines of CVE-2014-0062. So what the code
should be doing is looking up the user-provided name just once and
then using that value throughout all subsequent processing stages, but
it doesn't actually. The caller of transformStatsStmt() looks up the
RangeVar and gets an OID, but that value is then discarded and
CreateStatistics does another lookup on the same name, which means
that we're not really avoiding the hazard about which the comment
seems to be concerned.
So that leads to the question of whether there's a security
vulnerability here. I and a few other members of the pgsql-security
haven't been able to find one in brief review, but we may have missed
something. Fortunately, the permissions checks happen after the second
name lookup inside CreateStatistics(), so it doesn't seem that, for
example, you can leverage this to create extended statistics on a
table that you don't own. You can possibly get the first part of the
operation, where we transform the CREATE STATISTICS command's WHERE
clause, to operate on one table that you do own and then the second
part on another table that you don't own, but even if that's so, the
second part is just going to fail before doing anything interesting,
so it doesn't seem like there's a problem. If anyone reading this can
spot an exploit, please speak up!
So the attached patch is proposed as code cleanup, rather than
security patches. It changes the code to avoid the duplicate name
lookup altogether. There is no reason that I know of why this needs to
be back-patched for correctness, but I think it's worth putting into
master to make the code nicer and avoid doing things that in some
circumstances can be risky.
Thanks,
--
Robert Haas
EDB: http://www.enterprisedb.com