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
v2-0001-Refactor-transformStatsStmt-to-avoid-repeated-nam.patch
Description: Binary data
