Re: Some notes about the index-functions security vulnerability

Added to TODO:

* Prevent malicious functions from being executed with the permissions
of unsuspecting users

Index functions are safe, so VACUUM and ANALYZE are safe too.
Triggers, CHECK and DEFAULT expressions, and rules are still vulnerable.
http://archives.postgresql.org/pgsql-hackers/2008-01/msg00268.php

---------------------------------------------------------------------------

Tom Lane wrote:
> 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
> * functions in 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
>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: In versions below 8.0, the planner will ignore your desire to
> choose an index scan if your joining column's datatypes do not
> match

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +