Some notes about the index-functions security vulnerability - Mailing list pgsql-hackers

Now that the dust has settled, I want to post some notes about CVE-2007-6600,
which is to my mind the most important of the five security problems fixed
in our recent security updates.  There are some unfinished issues here.

Itagaki Takahiro originally identified the issue.  The crux of it is that
VACUUM FULL and ANALYZE need to execute functions in index definitions
(both expression index columns and partial index predicates).  Up to now
this has just happened without any special steps being taken, which means
that such functions were executed with the privileges of whoever is doing
VACUUM/ANALYZE, who is very likely to be a superuser.  Now CREATE INDEX
requires such functions to be marked IMMUTABLE, which makes them unable to
write anything, so the damage is seemingly limited; but it's easy to get
around that.  Hence, a nefarious user need only put some trojan-horse code
into a PL-language function, use the function in an index on one of his
tables, and wait for the next routine vacuuming in order to get his code
executed as superuser.

There are a whole bunch of related scenarios involving trojan-horse code
in triggers, view definitions, etc.  pgsql-core wasted quite a lot of time
(months, actually :-() trying to devise an all-encompassing solution for
all of them.  However, those other scenarios have been publicly known for
years, and haven't seemed to cause a lot of problems in practice, in part
because it requires intentional use of a table or view in order to expose
yourself to subversion.  The index function attack is more nasty than
these because it can subvert a superuser during required routine
maintenance (including autovacuum).  Moreover we couldn't find any way to
deal with these other issues that doesn't involve nontrivial semantic
incompatibilities, which wouldn't be suitable for back-patching.  So the
decision was to deal with only the index function problem as a security
exercise, and after that try to get people to think some more about
plugging those other holes in a future release.

Takahiro-san's initial suggestion for fixing this was to try to make
the marking of a function as IMMUTABLE into an air-tight guarantee
that it couldn't modify the database.  Right now it is not air-tight
for a number of reasons: you can alter the volatility marking of a
function after-the-fact, you can call a volatile function from an
immutable one, etc.  I originally argued against this fix on the grounds
that making a planner hint into a security classification was a bad idea,
since people routinely want to lie to the planner, and often have good
reasons for it.  But there is a better argument: even if you guarantee
that a function can't write the database, it'll still be able to read
the database and thereby read data the user shouldn't be able to get at.
At that point you are reduced to hoping that the user cannot think of any
covert channel by which to transmit the interesting info; and there are
*always* covert channels, eg timing or CPU usage.  We'd have to try to
restrict IMMUTABLE functions so that they could not read the DB either,
which seems impractical, as well as likely to break a lot of existing
applications.

So the direction we've pursued instead is to arrange for index expressions
to be evaluated as if they were being executed by the table owner,
that is, there's an implicit SECURITY DEFINER property attached to them.

Up to now I think we've always thought of SECURITY DEFINER functions as
being a mechanism for increasing one's privilege level.  However, in this
context we want to use them as a mechanism for *decreasing* privilege
level, and if we want to use them that way then the privilege loss has to
be air-tight.  The problem there is that so far it's been possible for a
SECURITY DEFINER function to execute SET SESSION AUTHORIZATION or SET ROLE
and thereby regain whatever privileges are held at the outermost level.
The patch as applied disallows both these operations inside a
security-definer context.

One reason for doing this restrictive fix is that GUC currently isn't
being told about fmgr_security_definer's manipulations of CurrentUserId.
There was actually a separate bug here: if you did SET ROLE inside a
sec-def function and then exited without any error, SHOW ROLE continued to
report the SET value as the current role, even though in reality the
session had reverted to the previous CurrentUserId.  Worse yet, a
subsequent ABORT could cause GUC's idea of the setting to become the
reality.

The thinking among core was that we'd be happy with leaving SET SESSION
AUTHORIZATION disabled forever, but it would be nice to allow SET ROLE,
with the modified semantics that the set of accessible roles would be
determined by the innermost security-definer function's owner, rather than
the session authorization; and that the effects of SET ROLE would roll
back at function exit.

To implement that we'd need to redo the interface between GUC and
miscinit.c's tracking of privilege state, but I'm not clear on how.
I thought about changing fmgr_security_definer to call GUC each time,
but that has truly unpleasant performance implications --- notably,
updating is_superuser would cause sending a ParameterStatus message to
the client for each sec-def function call or return.  We could probably
optimize that, but it would take work that didn't seem appropriate for a
security patch.  So if anyone cares about SET ROLE inside a SECURITY
DEFINER function, this is a TODO item.

The other issue that ought to be on the TODO radar is that we've only
plugged the hole for the very limited case of maintenance operations that
are likely to be executed by superusers.  If user A modifies user B's
table (via INSERT/UPDATE/DELETE), there are a lot of bits of code that are
controlled by B but will be executed with A's permissions; so A must trust
B a whole lot.  This general issue has been understood for quite some
time, I think, but maybe it's time to make a serious push to solve it.
Offhand I can cite the following ways in which B could exploit A's
privileges:* triggers* functions in indexes* functions in CHECK constraints* functions in DEFAULT expressions*
functionsin rules (including VIEW definitions)
 
The first three of these are probably not too difficult to solve: we could
switch privilege state to the table owner before executing such functions,
because the backend knows perfectly well when it's doing each of those
things.  But default expressions and rules get intertwined freely with
query fragments supplied by the calling user, and it's not so easy to see
how to know what to execute as which user.

Another point is that triggers very often use CURRENT_USER and/or
SESSION_USER to record who made a particular table update, so we'd have
to be very careful about making changes that affect the reported value
of either.  (This thought was part of the reason for thinking that SET
SESSION AUTHORIZATION, which determines SESSION_USER, should just stay
disabled forever in these contexts.)  We were willing to break any index
functions that call those things, because an allegedly immutable
function should surely never do so; but that argument won't wash for
triggers.

Lastly, there's a performance issue if we try to attack both of these
areas.  Right now, permissions switching (Get/SetUserIdAndContext) is
cheap enough that I don't think anyone would carp at putting it into
performance-critical places like ExecInsertIndexTuples.  But if we try to
involve GUC in permissions-switching it's unlikely that'd still be true.

So there's still work to do here, both in terms of defining how it should
behave and in terms of making it do that (with acceptable performance).

</brain dump>
        regards, tom lane