< Extracted with GMail's Forward All feature >
Forwarded conversation
Subject: Security vulnerability regarding SET ROLE and REINDEX
------------------------
From: Turner, Ian <Ian.Turner@deshaw.com>
Date: Wed, Oct 7, 2009 at 8:35 PM
To: security@postgresql.org
Cc: Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
Hello PostgreSQL team,
The following security issue was discovered by Gurjeet Singh at EnterpriseDB while performing consulting work for D. E. Shaw & Co. Below is the problem description and a patch. Credit for this patch should go to Gurjeet Singh.
As background, recall that when a REINDEX operation is performed on a functional index, the database creates a new security context using the index owner's role. This is to prevent the index function from executing with elevated privileges. Unfortunately, it is still possible for the index function to affect the session configuration of the user running REINDEX. Consider the following example, where a malicious user creates a functional index and then modifies the underlying function to change a GUC parameter.
mydb=> create table t( a int );
CREATE TABLE
mydb=> create function f() returns int as $$ begin return 0; end; $$ language plpgsql immutable;
CREATE FUNCTION
mydb=> create index i_t on t( f() );
CREATE INDEX
mydb=> create or replace function f() returns int as $$ begin set enable_seqscan = false; return 0; end; $$ language plpgsql;
CREATE FUNCTION
mydb=> insert into t values( 1 );
INSERT 0 1
Now, when a DBA does a REINDEX on this table, as part of a regular maintenance operation or for any other reason, the GUC session setting parameter enable_seqscan will be changed without her knowledge. This effect is demonstrated in the following session:
postgres=# show enable_seqscan ;
enable_seqscan
----------------
on
(1 row)
mydb=# reindex table t;
REINDEX
mydb=# show enable_seqscan ;
enable_seqscan
----------------
off
(1 row)
Using this mechanism quite a lot of parameters can be changed; the complete list can be found by running the query $$SELECT name FROM pg_settings WHERE context = 'user'. Some of these parameter changes, such as search_path, can prove to be devastating from a security standpoint. Other examples include setting vacuum_cost_delay to 0, so that any VACUUMs done in that session will consume all resources; and setting work_mem or maintenance_work_mem to low/high values so as to cause performance issues affecting the whole DB.
The patch, attached, addresses this issue by wrapping all security definer security contexts in a new GUC transaction, so that GUC changes will be reverted when the context ends. The one place where this is not done is when security definer functions are directly executed by expression, on the theory that this is more consistent with the way that functions are directly executed today.
This patch also re-enables the use of SET ROLE in security definer contexts, as any session user changes will be reverted when the GUC transaction completes.
Looking forward to your thoughtful response,
--Ian Turner
D. E. Shaw & Co.
----------
From: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
Date: Tue, Oct 13, 2009 at 5:29 PM
To: "Turner, Ian" <Ian.Turner@deshaw.com>
Cc: security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
Seems fine to me.
One way to look at this is that a function that changes GUCs and doesn't
reset them before exiting is not IMMUTABLE, even though it was marked as
such. (it must be marked as IMMUTABLE in order to be used as an index
function). Rolling back the GUC changes seems reasonable, although you
could argue for throwing an error too.
Does anyone else see a problem with the approach used in the patch?
We won't re-enable SET ROLE in back-branches, that's 8.5 material.
--
Heikki Linnakangas
----------
From: Turner, Ian <Ian.Turner@deshaw.com>
Date: Tue, Oct 13, 2009 at 9:20 PM
To: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
Cc: security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
Banning SET in IMMUTABLE functions is certainly adequate from a security perspective, though it will break some existing functions with no workaround.
The most complete, flexible, and complex solution to this problem is probably a combination of both approaches: Allow a function to specify whether or not GUC changes should be rolled back on exit via a new keyword, and ban the use of SET if the function (a) is not VOLATILE; and (b) does not roll back GUC changes.
In analogy with SECURITY [ DEFINER | INVOKER ] and with SET [ SESSION | LOCAL ], a function could be defined as CONFIGURATION [ SESSION | LOCAL], where the default would be CONFIGURATION SESSION and match the current behavior. Then a function would only be able to use SET if it were defined as VOLATILE or as CONFIGURATION LOCAL.
As already noted, such new syntax is probably not suitable for 8.4.2.
--Ian
----------
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Tue, Oct 13, 2009 at 11:46 PM
To: "Turner, Ian" <Ian.Turner@deshaw.com>
Cc: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
We already have the ability to set GUCs locally to a function via
function SET options. I think it would probably be feasible to
ban explicit SET within immutable functions, at least going forward.
But there's a bigger problem: we have never enforced that immutable
functions can't call non-immutable ones. Unless you want to start
making that an absolute prohibition --- which would undoubtedly break a
lot of things and is right out as a back-patched change --- any security
fence based on volatility is meaningless.
regards, tom lane
----------
From: Josh Berkus <josh@postgresql.org>
Date: Wed, Oct 14, 2009 at 12:06 AM
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: "Turner, Ian" <Ian.Turner@deshaw.com>, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
Tom,
That's not really feasible: example: SET timezone and date_trunc.
If I want to make an immutable version of date_trunc so that I can use
it for indexing (something I often do) then I'll want to set the time
zone either in the function or the function definition. I do this
specifically so that the function *can be* immutable.
While that particular use of SET is work-aroundable, it would involve
re-writing the functions ... and means that I'm not convinced that there
aren't uses of SET in immutable functions which *aren't* work-aroundable.
Which would *also* break the date_trunc example.
Agreed.
--Josh Berkus
----------
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed, Oct 14, 2009 at 1:06 AM
To: Josh Berkus <josh@postgresql.org>
Cc: "Turner, Ian" <Ian.Turner@deshaw.com>, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
Uh, your example seems to me to be solved perfectly well by adding SET
as a function option. However, it's sort of moot as I don't think we
can rely on volatility for this anyway. (Depending on volatility to
be completely bulletproof has been proposed and rejected before.)
regards, tom lane
----------
From: Greg Stark <gsstark@mit.edu>
Date: Thu, Oct 15, 2009 at 12:29 AM
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Josh Berkus <josh@postgresql.org>, "Turner, Ian" <Ian.Turner@deshaw.com>, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
If I understand his point correctly I think he's saying he would make
a function like
format_time_at_timezone(abstime,text) returns text immutable
Whereas if he did something like
format_time(abstime) returns text
Even if this worked with SET parameters on the function (I'm not sure
if changing them breaks the dependency the index has on it?) it sure
isn't as flexible as the first case which can handle a mixture of
timezones being used to format data in the same table.
--
greg
----------
From: Josh Berkus <josh@postgresql.org>
Date: Thu, Oct 15, 2009 at 12:43 AM
To: Greg Stark <gsstark@mit.edu>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, "Turner, Ian" <Ian.Turner@deshaw.com>, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
Correct.
--Josh Berkus
Subject: Security vulnerability regarding SET ROLE and REINDEX
------------------------
From: Turner, Ian <Ian.Turner@deshaw.com>
Date: Wed, Oct 7, 2009 at 8:35 PM
To: security@postgresql.org
Cc: Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
Hello PostgreSQL team,
The following security issue was discovered by Gurjeet Singh at EnterpriseDB while performing consulting work for D. E. Shaw & Co. Below is the problem description and a patch. Credit for this patch should go to Gurjeet Singh.
As background, recall that when a REINDEX operation is performed on a functional index, the database creates a new security context using the index owner's role. This is to prevent the index function from executing with elevated privileges. Unfortunately, it is still possible for the index function to affect the session configuration of the user running REINDEX. Consider the following example, where a malicious user creates a functional index and then modifies the underlying function to change a GUC parameter.
mydb=> create table t( a int );
CREATE TABLE
mydb=> create function f() returns int as $$ begin return 0; end; $$ language plpgsql immutable;
CREATE FUNCTION
mydb=> create index i_t on t( f() );
CREATE INDEX
mydb=> create or replace function f() returns int as $$ begin set enable_seqscan = false; return 0; end; $$ language plpgsql;
CREATE FUNCTION
mydb=> insert into t values( 1 );
INSERT 0 1
Now, when a DBA does a REINDEX on this table, as part of a regular maintenance operation or for any other reason, the GUC session setting parameter enable_seqscan will be changed without her knowledge. This effect is demonstrated in the following session:
postgres=# show enable_seqscan ;
enable_seqscan
----------------
on
(1 row)
mydb=# reindex table t;
REINDEX
mydb=# show enable_seqscan ;
enable_seqscan
----------------
off
(1 row)
Using this mechanism quite a lot of parameters can be changed; the complete list can be found by running the query $$SELECT name FROM pg_settings WHERE context = 'user'. Some of these parameter changes, such as search_path, can prove to be devastating from a security standpoint. Other examples include setting vacuum_cost_delay to 0, so that any VACUUMs done in that session will consume all resources; and setting work_mem or maintenance_work_mem to low/high values so as to cause performance issues affecting the whole DB.
The patch, attached, addresses this issue by wrapping all security definer security contexts in a new GUC transaction, so that GUC changes will be reverted when the context ends. The one place where this is not done is when security definer functions are directly executed by expression, on the theory that this is more consistent with the way that functions are directly executed today.
This patch also re-enables the use of SET ROLE in security definer contexts, as any session user changes will be reverted when the GUC transaction completes.
Looking forward to your thoughtful response,
--Ian Turner
D. E. Shaw & Co.
----------
From: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
Date: Tue, Oct 13, 2009 at 5:29 PM
To: "Turner, Ian" <Ian.Turner@deshaw.com>
Cc: security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
Seems fine to me.
One way to look at this is that a function that changes GUCs and doesn't
reset them before exiting is not IMMUTABLE, even though it was marked as
such. (it must be marked as IMMUTABLE in order to be used as an index
function). Rolling back the GUC changes seems reasonable, although you
could argue for throwing an error too.
Does anyone else see a problem with the approach used in the patch?
We won't re-enable SET ROLE in back-branches, that's 8.5 material.
--
Heikki Linnakangas
----------
From: Turner, Ian <Ian.Turner@deshaw.com>
Date: Tue, Oct 13, 2009 at 9:20 PM
To: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
Cc: security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
Banning SET in IMMUTABLE functions is certainly adequate from a security perspective, though it will break some existing functions with no workaround.
The most complete, flexible, and complex solution to this problem is probably a combination of both approaches: Allow a function to specify whether or not GUC changes should be rolled back on exit via a new keyword, and ban the use of SET if the function (a) is not VOLATILE; and (b) does not roll back GUC changes.
In analogy with SECURITY [ DEFINER | INVOKER ] and with SET [ SESSION | LOCAL ], a function could be defined as CONFIGURATION [ SESSION | LOCAL], where the default would be CONFIGURATION SESSION and match the current behavior. Then a function would only be able to use SET if it were defined as VOLATILE or as CONFIGURATION LOCAL.
As already noted, such new syntax is probably not suitable for 8.4.2.
--Ian
----------
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Tue, Oct 13, 2009 at 11:46 PM
To: "Turner, Ian" <Ian.Turner@deshaw.com>
Cc: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
We already have the ability to set GUCs locally to a function via
function SET options. I think it would probably be feasible to
ban explicit SET within immutable functions, at least going forward.
But there's a bigger problem: we have never enforced that immutable
functions can't call non-immutable ones. Unless you want to start
making that an absolute prohibition --- which would undoubtedly break a
lot of things and is right out as a back-patched change --- any security
fence based on volatility is meaningless.
regards, tom lane
----------
From: Josh Berkus <josh@postgresql.org>
Date: Wed, Oct 14, 2009 at 12:06 AM
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: "Turner, Ian" <Ian.Turner@deshaw.com>, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
Tom,
That's not really feasible: example: SET timezone and date_trunc.
If I want to make an immutable version of date_trunc so that I can use
it for indexing (something I often do) then I'll want to set the time
zone either in the function or the function definition. I do this
specifically so that the function *can be* immutable.
While that particular use of SET is work-aroundable, it would involve
re-writing the functions ... and means that I'm not convinced that there
aren't uses of SET in immutable functions which *aren't* work-aroundable.
Which would *also* break the date_trunc example.
Agreed.
--Josh Berkus
----------
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed, Oct 14, 2009 at 1:06 AM
To: Josh Berkus <josh@postgresql.org>
Cc: "Turner, Ian" <Ian.Turner@deshaw.com>, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
Uh, your example seems to me to be solved perfectly well by adding SET
as a function option. However, it's sort of moot as I don't think we
can rely on volatility for this anyway. (Depending on volatility to
be completely bulletproof has been proposed and rejected before.)
regards, tom lane
----------
From: Greg Stark <gsstark@mit.edu>
Date: Thu, Oct 15, 2009 at 12:29 AM
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Josh Berkus <josh@postgresql.org>, "Turner, Ian" <Ian.Turner@deshaw.com>, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
If I understand his point correctly I think he's saying he would make
a function like
format_time_at_timezone(
Whereas if he did something like
format_time(abstime) returns text
Even if this worked with SET parameters on the function (I'm not sure
if changing them breaks the dependency the index has on it?) it sure
isn't as flexible as the first case which can handle a mixture of
timezones being used to format data in the same table.
--
greg
----------
From: Josh Berkus <josh@postgresql.org>
Date: Thu, Oct 15, 2009 at 12:43 AM
To: Greg Stark <gsstark@mit.edu>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, "Turner, Ian" <Ian.Turner@deshaw.com>, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
Correct.
--Josh Berkus
========== New Thread starts here ==========
Forwarded conversation
Subject: RE: Security vulnerability regarding SET ROLE and REINDEX
------------------------
From: Turner, Ian <Ian.Turner@deshaw.com>
Date: Tue, Nov 3, 2009 at 12:33 AM
To: "Turner, Ian" <Ian.Turner@deshaw.com>, security@postgresql.org
Cc: Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
Hi guys,
Just checking in --- what's the latest status on this patch?
Cheers,
--Ian
-----Original Message-----
From: Turner, Ian
Sent: Wednesday, October 07, 2009 11:05 AM
To: security@postgresql.org
Cc: Gurjeet Singh; SysDB Developers (World)
Subject: Security vulnerability regarding SET ROLE and REINDEX
Hello PostgreSQL team,
The following security issue was discovered by Gurjeet Singh at EnterpriseDB while performing consulting work for D. E. Shaw & Co. Below is the problem description and a patch. Credit for this patch should go to Gurjeet Singh.
As background, recall that when a REINDEX operation is performed on a functional index, the database creates a new security context using the index owner's role. This is to prevent the index function from executing with elevated privileges. Unfortunately, it is still possible for the index function to affect the session configuration of the user running REINDEX. Consider the following example, where a malicious user creates a functional index and then modifies the underlying function to change a GUC parameter.
mydb=> create table t( a int );
CREATE TABLE
mydb=> create function f() returns int as $$ begin return 0; end; $$ language plpgsql immutable;
CREATE FUNCTION
mydb=> create index i_t on t( f() );
CREATE INDEX
mydb=> create or replace function f() returns int as $$ begin set enable_seqscan = false; return 0; end; $$ language plpgsql;
CREATE FUNCTION
mydb=> insert into t values( 1 );
INSERT 0 1
Now, when a DBA does a REINDEX on this table, as part of a regular maintenance operation or for any other reason, the GUC session setting parameter enable_seqscan will be changed without her knowledge. This effect is demonstrated in the following session:
postgres=# show enable_seqscan ;
enable_seqscan
----------------
on
(1 row)
mydb=# reindex table t;
REINDEX
mydb=# show enable_seqscan ;
enable_seqscan
----------------
off
(1 row)
Using this mechanism quite a lot of parameters can be changed; the complete list can be found by running the query $$SELECT name FROM pg_settings WHERE context = 'user'. Some of these parameter changes, such as search_path, can prove to be devastating from a security standpoint. Other examples include setting vacuum_cost_delay to 0, so that any VACUUMs done in that session will consume all resources; and setting work_mem or maintenance_work_mem to low/high values so as to cause performance issues affecting the whole DB.
The patch, attached, addresses this issue by wrapping all security definer security contexts in a new GUC transaction, so that GUC changes will be reverted when the context ends. The one place where this is not done is when security definer functions are directly executed by expression, on the theory that this is more consistent with the way that functions are directly executed today.
This patch also re-enables the use of SET ROLE in security definer contexts, as any session user changes will be reverted when the GUC transaction completes.
Looking forward to your thoughtful response,
--Ian Turner
D. E. Shaw & Co.
----------
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Tue, Nov 3, 2009 at 1:19 AM
To: "Turner, Ian" <Ian.Turner@deshaw.com>
Cc: security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
We're still thinking about it. So far none of the proposals look
good to me, fwiw.
regards, tom lane
----------
From: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
Date: Tue, Dec 1, 2009 at 7:41 PM
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: "Turner, Ian" <Ian.Turner@deshaw.com>, security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
We've been procrastinating on this for too long already...
I took another look at the patch Ian submitted, and it looks OK to me,
except that we still shouldn't enable SET ROLE in security definer
functions. I'm not sure if it would indeed be safe with the patch, but
it nevertheless seems like something we shouldn't do in back-branches.
(I suspect it wouldn't be safe to enable SET ROLE, if we don't start a
new GUC nesting level when a security definer function is called directly.)
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
----------
From: Turner, Ian <Ian.Turner@deshaw.com>
Date: Wed, Dec 2, 2009 at 2:14 AM
To: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: "security@postgresql.org" <security@postgresql.org>, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
FWIW, SET ROLE was enabled in 8.4.0 and we had no problem removing it in 8.4.1. But I don't have a strong opinion on this; it probably makes sense to commit the basic fix in 8.3 and 8.4 and then take care of SECURITY DEFINER properly in 8.5.
This has been discussed to death. The bottom line is that lots of important GUC parameters can be changed by functions, and it's not clear why current_user should be treated specially in this way. What we need to do is to actually pick one of the many approaches which have been proposed thus far:
1. Do SECURITY DEFINER without a new GUC context, new role may leak out. (Ian/Gurjeet)
2. Do SECURITY DEFINER with a new GUC context, persistent config changes not possible. (Heikki)
3. Do SECURITY DEFINER without a new GUC context, require functions that change GUC be VOLATILE. (Heikki)
4. Do SECURITY DEFINER and allow user to specify if they want a new context or not (Ian)
5. Do something else, to be proposed (Tom)
Cheers,
--Ian
----------
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sat, Dec 12, 2009 at 6:18 AM
To: "Turner, Ian" <Ian.Turner@deshaw.com>
Cc: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, "security@postgresql.org" <security@postgresql.org>, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
BTW, we should close the loop on this issue, since the update is about
to be released. The conclusion after further discussion among
pgsql-security was that the possibility of subverting the caller's
session by changing GUC values is just one instance of a more general
problem: almost none of a Postgres backend's session-local state has any
security mechanism attached to it, and much of that state could be
subverted given guesses about what the caller will do later in the
session. Two examples are replacing a prepared statement with
trojan-horse code, and closing a cursor and re-opening it with a new
definition that includes trojan-horse code.
We stuck with Gurjeet's idea of handling GUC values by introducing a
local rollback context, at least for the branches where that could be
made to work. It was right out for 7.4 :-(, so the eventual solution
for that branch was to mark search_path as not changeable within these
contexts. There's some chance that 7.4 could still be subverted by
changing other settings, but it would certainly be a lot harder.
It didn't seem practical to do anything of the kind for prepared
statements or other session context, so we just made those commands
throw errors when invoked inside a restricted operation.
The proposal to remove the restriction on SET ROLE was rejected,
as that is closing more holes than you apparently realize. In
particular, it doesn't help to roll back a bad guy's change of
"role" after he's already had superuser privileges for long enough
to do plenty of damage.
This issue is assigned CVE-2009-4136. Update releases will be
announced on Monday.
regards, tom lane
----------
From: Gurjeet Singh <gurjeet.singh@enterprisedb.com>
Date: Sat, Dec 12, 2009 at 2:25 PM
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: "Turner, Ian" <Ian.Turner@deshaw.com>, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, "security@postgresql.org" <security@postgresql.org>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>, George Williams <george.williams@enterprisedb.com>, Faiz Husain <faiz.husain@enterprisedb.com>
Please find attached the text I wrote as the analysis of the whole problem.
Per the "Background" and the "Analysis" sections in the attached file, SET ROLE is a safe command and SET SESSION AUTHORIZATION is not, that's why it was understood by D.E Shaw that asking for removing restriction on SET SESSION AUTHORIZATION wouldn't be acceptable to the community, and hence removing restriction just on SET ROLE is being pursued.
SET ROLE is safe in any context since it can be used to switch to only to those roles that the Session User is a member of, whereas SET SESSION AUTHORIZATION is unsafe since it can be used to switch to any role in the cluster iff the Authenticated User is a superuser.
Specifically, allowing SET ROLE does not imply that the perpetrator can gain superuser privileges, even if the Authenticated User was a superuser. SET ROLE works with Session User, and SET SESSION AUTHORIZATION works with Authenticated User.
With the current GUC-Rollback+other-safeguards patch in place, the perpetrator cannot affect any GUC of the Authenticated User, and even with SET ROLE allowed in Sec-Def context, the perpetrator can't switch to a role which is otherwise not allowed.
Best regards,
--
gurjeet[.singh]@EnterpriseDB.comsingh.gurjeet@{ gmail | hotmail | indiatimes | yahoo }.com
----------
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sat, Dec 12, 2009 at 9:00 PM
To: Gurjeet Singh <gurjeet.singh@enterprisedb.com>
Cc: "Turner, Ian" <Ian.Turner@deshaw.com>, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, "security@postgresql.org" <security@postgresql.org>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>, George Williams <george.williams@enterprisedb.com>, Faiz Husain <faiz.husain@enterprisedb.com>
Maybe you had better read that statement again, and remember that the
session user is typically a superuser in exactly the cases we are
concerned about.
regards, tom lane
Subject: RE: Security vulnerability regarding SET ROLE and REINDEX
------------------------
From: Turner, Ian <Ian.Turner@deshaw.com>
Date: Tue, Nov 3, 2009 at 12:33 AM
To: "Turner, Ian" <Ian.Turner@deshaw.com>, security@postgresql.org
Cc: Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
Hi guys,
Just checking in --- what's the latest status on this patch?
Cheers,
--Ian
-----Original Message-----
From: Turner, Ian
Sent: Wednesday, October 07, 2009 11:05 AM
To: security@postgresql.org
Cc: Gurjeet Singh; SysDB Developers (World)
Subject: Security vulnerability regarding SET ROLE and REINDEX
Hello PostgreSQL team,
The following security issue was discovered by Gurjeet Singh at EnterpriseDB while performing consulting work for D. E. Shaw & Co. Below is the problem description and a patch. Credit for this patch should go to Gurjeet Singh.
As background, recall that when a REINDEX operation is performed on a functional index, the database creates a new security context using the index owner's role. This is to prevent the index function from executing with elevated privileges. Unfortunately, it is still possible for the index function to affect the session configuration of the user running REINDEX. Consider the following example, where a malicious user creates a functional index and then modifies the underlying function to change a GUC parameter.
mydb=> create table t( a int );
CREATE TABLE
mydb=> create function f() returns int as $$ begin return 0; end; $$ language plpgsql immutable;
CREATE FUNCTION
mydb=> create index i_t on t( f() );
CREATE INDEX
mydb=> create or replace function f() returns int as $$ begin set enable_seqscan = false; return 0; end; $$ language plpgsql;
CREATE FUNCTION
mydb=> insert into t values( 1 );
INSERT 0 1
Now, when a DBA does a REINDEX on this table, as part of a regular maintenance operation or for any other reason, the GUC session setting parameter enable_seqscan will be changed without her knowledge. This effect is demonstrated in the following session:
postgres=# show enable_seqscan ;
enable_seqscan
----------------
on
(1 row)
mydb=# reindex table t;
REINDEX
mydb=# show enable_seqscan ;
enable_seqscan
----------------
off
(1 row)
Using this mechanism quite a lot of parameters can be changed; the complete list can be found by running the query $$SELECT name FROM pg_settings WHERE context = 'user'. Some of these parameter changes, such as search_path, can prove to be devastating from a security standpoint. Other examples include setting vacuum_cost_delay to 0, so that any VACUUMs done in that session will consume all resources; and setting work_mem or maintenance_work_mem to low/high values so as to cause performance issues affecting the whole DB.
The patch, attached, addresses this issue by wrapping all security definer security contexts in a new GUC transaction, so that GUC changes will be reverted when the context ends. The one place where this is not done is when security definer functions are directly executed by expression, on the theory that this is more consistent with the way that functions are directly executed today.
This patch also re-enables the use of SET ROLE in security definer contexts, as any session user changes will be reverted when the GUC transaction completes.
Looking forward to your thoughtful response,
--Ian Turner
D. E. Shaw & Co.
----------
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Tue, Nov 3, 2009 at 1:19 AM
To: "Turner, Ian" <Ian.Turner@deshaw.com>
Cc: security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
We're still thinking about it. So far none of the proposals look
good to me, fwiw.
regards, tom lane
----------
From: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
Date: Tue, Dec 1, 2009 at 7:41 PM
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: "Turner, Ian" <Ian.Turner@deshaw.com>, security@postgresql.org, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
We've been procrastinating on this for too long already...
I took another look at the patch Ian submitted, and it looks OK to me,
except that we still shouldn't enable SET ROLE in security definer
functions. I'm not sure if it would indeed be safe with the patch, but
it nevertheless seems like something we shouldn't do in back-branches.
(I suspect it wouldn't be safe to enable SET ROLE, if we don't start a
new GUC nesting level when a security definer function is called directly.)
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
----------
From: Turner, Ian <Ian.Turner@deshaw.com>
Date: Wed, Dec 2, 2009 at 2:14 AM
To: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: "security@postgresql.org" <security@postgresql.org>, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
FWIW, SET ROLE was enabled in 8.4.0 and we had no problem removing it in 8.4.1. But I don't have a strong opinion on this; it probably makes sense to commit the basic fix in 8.3 and 8.4 and then take care of SECURITY DEFINER properly in 8.5.
This has been discussed to death. The bottom line is that lots of important GUC parameters can be changed by functions, and it's not clear why current_user should be treated specially in this way. What we need to do is to actually pick one of the many approaches which have been proposed thus far:
1. Do SECURITY DEFINER without a new GUC context, new role may leak out. (Ian/Gurjeet)
2. Do SECURITY DEFINER with a new GUC context, persistent config changes not possible. (Heikki)
3. Do SECURITY DEFINER without a new GUC context, require functions that change GUC be VOLATILE. (Heikki)
4. Do SECURITY DEFINER and allow user to specify if they want a new context or not (Ian)
5. Do something else, to be proposed (Tom)
Cheers,
--Ian
----------
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sat, Dec 12, 2009 at 6:18 AM
To: "Turner, Ian" <Ian.Turner@deshaw.com>
Cc: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, "security@postgresql.org" <security@postgresql.org>, Gurjeet Singh <gurjeet.singh@enterprisedb.com>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>
BTW, we should close the loop on this issue, since the update is about
to be released. The conclusion after further discussion among
pgsql-security was that the possibility of subverting the caller's
session by changing GUC values is just one instance of a more general
problem: almost none of a Postgres backend's session-local state has any
security mechanism attached to it, and much of that state could be
subverted given guesses about what the caller will do later in the
session. Two examples are replacing a prepared statement with
trojan-horse code, and closing a cursor and re-opening it with a new
definition that includes trojan-horse code.
We stuck with Gurjeet's idea of handling GUC values by introducing a
local rollback context, at least for the branches where that could be
made to work. It was right out for 7.4 :-(, so the eventual solution
for that branch was to mark search_path as not changeable within these
contexts. There's some chance that 7.4 could still be subverted by
changing other settings, but it would certainly be a lot harder.
It didn't seem practical to do anything of the kind for prepared
statements or other session context, so we just made those commands
throw errors when invoked inside a restricted operation.
The proposal to remove the restriction on SET ROLE was rejected,
as that is closing more holes than you apparently realize. In
particular, it doesn't help to roll back a bad guy's change of
"role" after he's already had superuser privileges for long enough
to do plenty of damage.
This issue is assigned CVE-2009-4136. Update releases will be
announced on Monday.
regards, tom lane
----------
From: Gurjeet Singh <gurjeet.singh@enterprisedb.com>
Date: Sat, Dec 12, 2009 at 2:25 PM
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: "Turner, Ian" <Ian.Turner@deshaw.com>, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, "security@postgresql.org" <security@postgresql.org>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>, George Williams <george.williams@enterprisedb.com>, Faiz Husain <faiz.husain@enterprisedb.com>
Please find attached the text I wrote as the analysis of the whole problem.
Per the "Background" and the "Analysis" sections in the attached file, SET ROLE is a safe command and SET SESSION AUTHORIZATION is not, that's why it was understood by D.E Shaw that asking for removing restriction on SET SESSION AUTHORIZATION wouldn't be acceptable to the community, and hence removing restriction just on SET ROLE is being pursued.
SET ROLE is safe in any context since it can be used to switch to only to those roles that the Session User is a member of, whereas SET SESSION AUTHORIZATION is unsafe since it can be used to switch to any role in the cluster iff the Authenticated User is a superuser.
Specifically, allowing SET ROLE does not imply that the perpetrator can gain superuser privileges, even if the Authenticated User was a superuser. SET ROLE works with Session User, and SET SESSION AUTHORIZATION works with Authenticated User.
With the current GUC-Rollback+other-safeguards patch in place, the perpetrator cannot affect any GUC of the Authenticated User, and even with SET ROLE allowed in Sec-Def context, the perpetrator can't switch to a role which is otherwise not allowed.
Best regards,
gurjeet[.singh]@EnterpriseDB.
----------
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sat, Dec 12, 2009 at 9:00 PM
To: Gurjeet Singh <gurjeet.singh@enterprisedb.com>
Cc: "Turner, Ian" <Ian.Turner@deshaw.com>, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>, "security@postgresql.org" <security@postgresql.org>, "SysDB Developers (World)" <sysdb-dev@world.deshaw.com>, George Williams <george.williams@enterprisedb.com>, Faiz Husain <faiz.husain@enterprisedb.com>
Maybe you had better read that statement again, and remember that the
session user is typically a superuser in exactly the cases we are
concerned about.
regards, tom lane
========== The Patch ==========
doc/src/sgml/ref/set_role.sgml | 5 -----
src/backend/catalog/index.c | 11 +++++++++++
src/backend/commands/analyze.c | 6 ++++++
src/backend/commands/vacuum.c | 6 ++++++
src/backend/utils/adt/ri_triggers.c | 5 +++++
src/backend/utils/init/miscinit.c | 1 -
src/backend/utils/misc/guc.c | 2 +-
7 files changed, 29 insertions(+), 7 deletions(-)
diff --git a/doc/src/sgml/ref/set_role.sgml b/doc/src/sgml/ref/set_role.sgml
index a55c343..76587a1 100644
--- a/doc/src/sgml/ref/set_role.sgml
+++ b/doc/src/sgml/ref/set_role.sgml
@@ -97,11 +97,6 @@ RESET ROLE
endterm="sql-alterrole-title"> settings; this only happens during
login.
</para>
-
- <para>
- <command>SET ROLE</> cannot be used within a
- <literal>SECURITY DEFINER</> function.
- </para>
</refsect1>
<refsect1>
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 824d4ef..9b6b018 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -58,6 +58,7 @@
#include "storage/smgr.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
+#include "utils/guc.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -1431,6 +1432,7 @@ index_build(Relation heapRelation,
IndexBuildResult *stats;
Oid save_userid;
bool save_secdefcxt;
+ int save_guc_level;
/*
* sanity checks
@@ -1447,6 +1449,7 @@ index_build(Relation heapRelation,
*/
GetUserIdAndContext(&save_userid, &save_secdefcxt);
SetUserIdAndContext(heapRelation->rd_rel->relowner, true);
+ save_guc_level = NewGUCNestLevel();
/*
* Call the access method's build procedure
@@ -1458,6 +1461,9 @@ index_build(Relation heapRelation,
PointerGetDatum(indexInfo)));
Assert(PointerIsValid(stats));
+ /* Undo GUC changes */
+ AtEOXact_GUC(false, save_guc_level);
+
/* Restore userid */
SetUserIdAndContext(save_userid, save_secdefcxt);
@@ -1968,6 +1974,7 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
v_i_state state;
Oid save_userid;
bool save_secdefcxt;
+ int save_guc_level;
/* Open and lock the parent heap relation */
heapRelation = heap_open(heapId, ShareUpdateExclusiveLock);
@@ -1990,6 +1997,7 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
*/
GetUserIdAndContext(&save_userid, &save_secdefcxt);
SetUserIdAndContext(heapRelation->rd_rel->relowner, true);
+ save_guc_level = NewGUCNestLevel();
/*
* Scan the index and gather up all the TIDs into a tuplesort object.
@@ -2030,6 +2038,9 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot)
"validate_index found %.0f heap tuples, %.0f index tuples; inserted %.0f missing tuples",
state.htups, state.itups, state.tups_inserted);
+ /* Undo GUC changes */
+ AtEOXact_GUC(false, save_guc_level);
+
/* Restore userid */
SetUserIdAndContext(save_userid, save_secdefcxt);
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 71db483..59265be 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -38,6 +38,7 @@
#include "storage/procarray.h"
#include "utils/acl.h"
#include "utils/datum.h"
+#include "utils/guc.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/pg_rusage.h"
@@ -134,6 +135,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
TimestampTz starttime = 0;
Oid save_userid;
bool save_secdefcxt;
+ int save_guc_level;
if (vacstmt->verbose)
elevel = INFO;
@@ -239,6 +241,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
*/
GetUserIdAndContext(&save_userid, &save_secdefcxt);
SetUserIdAndContext(onerel->rd_rel->relowner, true);
+ save_guc_level = NewGUCNestLevel();
/* let others know what I'm doing */
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
@@ -548,6 +551,9 @@ cleanup:
MyProc->vacuumFlags &= ~PROC_IN_ANALYZE;
LWLockRelease(ProcArrayLock);
+ /* Undo GUC changes */
+ AtEOXact_GUC(false, save_guc_level);
+
/* Restore userid */
SetUserIdAndContext(save_userid, save_secdefcxt);
}
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index c9c9baa..e708c19 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -47,6 +47,7 @@
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
+#include "utils/guc.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -1020,6 +1021,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound,
Oid toast_relid;
Oid save_userid;
bool save_secdefcxt;
+ int save_guc_level;
if (scanned_all)
*scanned_all = false;
@@ -1181,6 +1183,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound,
*/
GetUserIdAndContext(&save_userid, &save_secdefcxt);
SetUserIdAndContext(onerel->rd_rel->relowner, true);
+ save_guc_level = NewGUCNestLevel();
/*
* Do the actual work --- either FULL or "lazy" vacuum
@@ -1190,6 +1193,9 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound,
else
lazy_vacuum_rel(onerel, vacstmt, vac_strategy, scanned_all);
+ /* Undo GUC changes */
+ AtEOXact_GUC(false, save_guc_level);
+
/* Restore userid */
SetUserIdAndContext(save_userid, save_secdefcxt);
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 8a9d7d2..8b0bf62 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -3264,6 +3264,7 @@ ri_PerformCheck(RI_QueryKey *qkey, SPIPlanPtr qplan,
int spi_result;
Oid save_userid;
bool save_secdefcxt;
+ int save_guc_level;
Datum vals[RI_MAX_NUMKEYS * 2];
char nulls[RI_MAX_NUMKEYS * 2];
@@ -3345,6 +3346,7 @@ ri_PerformCheck(RI_QueryKey *qkey, SPIPlanPtr qplan,
/* Switch to proper UID to perform check as */
GetUserIdAndContext(&save_userid, &save_secdefcxt);
SetUserIdAndContext(RelationGetForm(query_rel)->relowner, true);
+ save_guc_level = NewGUCNestLevel();
/* Finally we can run the query. */
spi_result = SPI_execute_snapshot(qplan,
@@ -3352,6 +3354,9 @@ ri_PerformCheck(RI_QueryKey *qkey, SPIPlanPtr qplan,
test_snapshot, crosscheck_snapshot,
false, false, limit);
+ /* Undo GUC changes */
+ AtEOXact_GUC(false, save_guc_level);
+
/* Restore UID */
SetUserIdAndContext(save_userid, save_secdefcxt);
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index b76be53..fda4766 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -311,7 +311,6 @@ GetOuterUserId(void)
static void
SetOuterUserId(Oid userid)
{
- AssertState(!SecurityDefinerContext);
AssertArg(OidIsValid(userid));
OuterUserId = userid;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index fc55e90..bd69ec5 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2332,7 +2332,7 @@ static struct config_string ConfigureNamesString[] =
{"role", PGC_USERSET, UNGROUPED,
gettext_noop("Sets the current role."),
NULL,
- GUC_IS_NAME | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_NOT_WHILE_SEC_DEF
+ GUC_IS_NAME | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
},
&role_string,
"none", assign_role, show_role
--
1.6.0.6
========== The analysis text mentioned above ==========
The Problem:
============
Postgres has clamped down SET SESSION AUTHORISATION (SSA) and SET ROLE (SRL) from executing in security definer (Sec-Def) context functions. We wish to re-enable these commands in Sec-Def contexts.
Background:
===========
At any instant, Postgres maintains three notions of connected users:
1) Authenticated user.
2) Session User.
3) Current user.
Authenticated User is the one which was used to connect initially to perform the connection. This is never changed after the initial assignment.
Session User denotes the currently connected user, and is usually the same as Authenticated user. This can be changed by SSA command.
Current user denotes the user whose privileges are checked for any operation to be performed. This is usually the same as Session User, but SSA and SRL commands can be used to change this temporarily.
SRL command changes the Current User. It can be used to switch to any of the roles that the Session User is a member of. If the Session User is a superuser, then any role can be chosen as Current User using this command.
SSA command changes the Session User as well as Current User. When the Authenticated User is a normal user (non-superuser) the role name to switch to should be the same as the Authenticated User, else an ERROR is thrown. If the Authenticated User is a superuser, then any role can be specified to switch to.
When performing any maintenance operation on a table (or any other object), the code (internally and automatically) switches the identity to be that of the object owner, and sets the Sec-Def context. This means that, that maintenance operation is done with the privileges of the object owner, and not as the superuser even if the Authenticated User is a superuser (which is the case when regular maintenance tasks like VACUUM, REINDEX, etc. are done).
This clampdown avoids the situation where a superuser (Authenticated User) is performing some maintenance operation (possibly, database wide), and in the course of that operation some user-defined function has to be executed. If this clampdown was not implemented, then the user-defined function would be able to become any DB user using SSA since the Authenticated User is a superuser.
Analysis:
=========
As is evident from above, SSA is a powerful command for (only) superusers to switch to any database user. Using this mechanism, a superuser can switch to being a normal user (Session as well as Current User) to perform a maintenance operation, and later switch back to being a superuser conveniently.
On the other hand, SRL command looks at the Session User and allows to switch Current User to perform operations as that user. This command, when enabled in Sec-Def context, does not actually open any security hole, since this command cannot be used to switch to being a user which the Session User does not have access to. My analysis shows that this command (SRL) was still clamped down just because the GUC (Grand Unified Configuration, or simply, runtime parameters) infrastructure was not capable of resetting the ROLE setting upon exiting the Sec-Def context.
So, if a Sec-Def function used 'SET ROLE' to switch to a different user, upon exiting that function, although the original Current User setting is restored, the 'SHOW ROLE' command would still be showing the changed ROLE. The clampdown on SRL command was implemented to avoid this inconsistency; and IMHO, this is what that XXX comment in the code referred to.
Conclusion:
===========
SSA command should never be allowed in Sec-Def context. Even if we did allow that (which we can), the Postgres community will not accept it in the mainline code.
The SRL command can be allowed in Sec-Def context though. I have a patch that enables the GUC to reset the ROLE setting after exiting a Sec-Def context.
In fact, I have discovered another security flaw that this patch will resolve.
The above mentioned newfound security flaw will be discussed separately.
Implementation:
===============
I have tracked down all those places which perform some code execution after switching to a security definer context, and have plugged in code to setup a GUC transaction and then roll this transaction back when the operation is complete. This rolls back any SET ROLE effect that the user-code might have done within that operation; in fact, this rolls back all GUC changes done by the user code.
Exceptions:
===========
One such place left out where a Sec-Def context is not wrapped in a GUC transaction is when Referential Integrity code prepares a plan of a dynamically built internal query. I did not feel the need to setup and rollback a GUC transaction since no user code is actually executed while preparing a query.
Another place where it felt counter intuitive to setup and rollback a GUC transaction is when the FMGR code invokes a Sec-Def function. Reason being that, that any user executing someone else's Sec-Def functions should be well aware of the side effects of the function. So, if that Sec-Def function does a SET ROLE, then either that should be RESET by that same function, or the caller must be aware of this change since the ROLE setting will persist even after the Sec-Def function has returned.
One gotcha with a user defined Sec-Def function could be where the function owner did SET ROLE to perform an operation, but then accidentally forgot to RESET it before returning. But to rollback the SET ROLE effect, FMGR will have to rollback every GUC change, which might not be desirable.
