Обсуждение: Some notes about the index-functions security vulnerability
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
On Wed, 2008-01-09 at 00:22 -0500, Tom Lane wrote: > pgsql-core wasted quite a lot of time Core's efforts are appreciated by all, so not time wasted. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com
On 1/8/08, Tom Lane <tgl@sss.pgh.pa.us> wrote: > 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. High-level brain dump, does not cover all the use cases I'm sure... Given Invoker executing code created by Definer, there are basically 3 situations: 1) Definer does not trust Invoker, but Invoker trusts Definer. -> use Invoker's permission set This is probably the case for most system/library generic functions, such as the various trigger templates They are typically owned by a superuser 2) Invoker does not trust Definer, but Definer trusts Invoker. -> use Definer's permission set This case covers most triggers, since they are there to maintain Definer's data, and Invoker's input is inherently controlled. 3) Neither trusts the other. -> use the intersection of Invoker's and Definer's permission sets This is essentially the case for any arbitrary functions floating around, where Invoker's input is not inherently controlled, and Definer is an unknown entity. Situation 1 is covered by SECURITY INVOKER, and 2 is covered by SECURITY DEFINER. Suppose another function option is added for situation 3, "SECURITY INTERSECTION". Also suppose there is a new role option, "TRUSTED" (needs a better name). * A function is created with SECURITY INTERSECTION by default. * A function's owner can choose SECURITY DEFINER. * Only a role with TRUSTED can choose SECURITY INVOKER. * Only the superuser has TRUSTED by default. The idea here is that by default, neither Invoker nor Definer need to be terribly concerned. If Definer is creating the function specifically to operate on its own data, and is checking input appropriately, SECURITY DEFINER will allow it to work. If Definer is creating the function for generic use purposes, Invoker will want to apply it to its own data, and SECURITY INVOKER is appropriate for that. A Definer's trustworthiness for all Invokers is determined by the superuser via the TRUSTED role option. > Offhand I can cite the following ways in which B could exploit A's > privileges: > * triggers Ideally Invoker's permission set would be replaced by the trigger owner's for the duration of the call. However it doesn't look like there actually is an owner concept for triggers, despite there being a TRIGGER permission for the associated table. The next appropriate option is to assign the table owner's permission set to Invoker. In the case of functions marked SECURITY INVOKER, this leaves a hole: a role that has TRIGGER permission on the table can elevate its permissions to that of the table owner's when calling that function. If the role with TRIGGER permission is not TRUSTED, it can only create new functions with SECURITY INTERSECTION, which will result in executing with its own permissions at best. This seems reasonable. > * functions in indexes > * functions in CHECK constraints > * functions in DEFAULT expressions > * functions in rules (including VIEW definitions) Replace the Invoker's permission set with the table owner's for the duration of the call. These all require you to be the owner of the associated object, so there is no potential hole as with triggers. > 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. I'll just wave my hands wildly here and say functions in expressions supplied by the Invoker magically avoid being called with the object owner's permission set instead. I don't know how, they just do :) What this doesn't allow is actually executing things like VIEW expressions using the calling user's permission set. I don't have an actual use case for that, but I feel it's a problem somehow. I've also completely avoided things like CURRENT_USER by talking about permission sets only.
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@momjian.us> http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +