Re-enabling SET ROLE in security definer functions
Hi All,
We are seeking to enable SET ROLE in security-definer functions, since @
D.E Shaw there are scripts from the past that used this feature, and I think
you'd also agree that SET ROLE is security definer functions has it's uses.
As the code stands right now, I see that the only concern against
enabling SET ROLE in SecDef functions is that, that when the local-user-id
change context is exited, the GUC might be left out of sync.
We currently have two bits indicating separately whether we are in
context where (i) the CurrentUserId has changed, or (ii) Security concerns
do not allow certain operation. But we have only one flag for GUC that stops
us from performing any of SET ROLE or SET SESSION AUTHORIZATION while any of
the above two flags are set.
I propose that we have a separate GUC flag to indicate whether we are in
UserId-changed context. So, we disallow SET ROLE only when we are in
Security-Restricted context, and disallow SET SESSION AUTHORIZATION when we
are either in Security-Restricted context or in UserId-changed context.
So SET ROLE would be prohibited in maintenance operations, but allowed
in SecDef functions (only if they are not invoked on a stack where
maintenance operation was initiated earlier). And SET SESSION AUTHORIZATION
will be disallowed when we are in either of the UserId-changed or
Security-Restricted contexts.
To address the problem of GUC getting out of sync when a SecDef function
is exited, we can perform a check at the end, just before reverting to the
calling userid, that if the called function stack has used SET ROLE to
change the CurrentUserId, then we keep that user id to be in sync with GUC,
rather than sync the GUC with current settings. This keeps the current
semantics of GUC where if the called function (whether SecDef or not) used
SET to change a GUC parameter, then that setting prevails even after the
function has exited successfully.
Attaching patch to implement the above proposals.
I have given some thought to nesting of such call scenarios, and haven't
found one which could cause an issue with this approach. Hope I haven't
overlooked something.
NB: In the patch, the block surrounded with
"if(InSecurityRestrictedOperation())" in guc.c will never be called, since
the GUC parameter that it applies to (session_auth) is also marked as not
allowed while in UserId-changed context, and that condition is cecked in
previous block of code. This can be remedied by swapping the two relevant
"if" blocks. I did not do it to keep the patch simple and small.
Best regards,
PS: For some context, this started with an aim to enable SET ROLE command in
security definer functions, which D.E Shaw needed. This command is still not
enabled in SecDef functions, but it led to a security exposure followed by
the security fix; commit id: 31d0bddf77b9e2b5581816aa96d3a3
92ab7d8543.
See also:
http://gurjeet-tech.blogspot.com/2009/12/conversation-on-fixing-security-issue.html
On Sat, Dec 12, 2009 at 9:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Gurjeet Singh <gurjeet.singh@enterprisedb.com> writes:
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.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
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.enterprisedb.com
singh.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
Attachments:
allow_set_role.2.patchapplication/octet-stream; name=allow_set_role.2.patchDownload
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index 9a1da59..28ab39f 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -981,8 +981,22 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
if (fcache->proconfig)
AtEOXact_GUC(true, save_nestlevel);
+
if (OidIsValid(fcache->userid))
- SetUserIdAndSecContext(save_userid, save_sec_context);
+ {
+ Oid current_userid;
+ int current_sec_context;
+ /*
+ * If the called function stack used SET ROLE to change CurrentUserId
+ * then do not revert the setting back to the saved user.
+ */
+ GetUserIdAndSecContext(¤t_userid, ¤t_sec_context);
+
+ if(fcache->userid == current_userid)
+ SetUserIdAndSecContext(save_userid, save_sec_context);
+ else
+ SetUserIdAndSecContext(current_userid, save_sec_context);
+ }
return result;
}
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 359edee..a565836 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -313,7 +313,7 @@ GetOuterUserId(void)
static void
SetOuterUserId(Oid userid)
{
- AssertState(SecurityRestrictionContext == 0);
+ AssertState(!InSecurityRestrictedOperation());
AssertArg(OidIsValid(userid));
OuterUserId = userid;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index b530720..4a54df3 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2381,7 +2381,8 @@ static struct config_string ConfigureNamesString[] =
{"session_authorization", PGC_USERSET, UNGROUPED,
gettext_noop("Sets the session user name."),
NULL,
- GUC_IS_NAME | GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_NOT_WHILE_SEC_REST
+ GUC_IS_NAME | GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+ | GUC_NOT_WHILE_SEC_REST | GUC_NOT_WHILE_USER_CHANGE
},
&session_authorization_string,
NULL, assign_session_authorization, show_session_authorization
@@ -4747,20 +4748,20 @@ set_config_option(const char *name, const char *value,
* we prohibit changing these in a security-restricted operation because
* otherwise RESET could be used to regain the session user's privileges.
*/
- if (record->flags & GUC_NOT_WHILE_SEC_REST)
+ if (record->flags & GUC_NOT_WHILE_USER_CHANGE)
{
if (InLocalUserIdChange())
{
- /*
- * Phrasing of this error message is historical, but it's the
- * most common case.
- */
ereport(elevel,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("cannot set parameter \"%s\" within security-definer function",
+ errmsg("cannot set parameter \"%s\" while user-id changed temporarily",
name)));
return false;
}
+ }
+
+ if (record->flags & GUC_NOT_WHILE_SEC_REST)
+ {
if (InSecurityRestrictedOperation())
{
ereport(elevel,
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 9c95b60..469c787 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -149,7 +149,9 @@ typedef enum
#define GUC_UNIT_MIN 0x4000 /* value is in minutes */
#define GUC_UNIT_TIME 0x7000 /* mask for MS, S, MIN */
-#define GUC_NOT_WHILE_SEC_REST 0x8000 /* can't set if security restricted */
+#define GUC_NOT_WHILE_SEC_REST 0x08000 /* can't set if security restricted */
+#define GUC_NOT_WHILE_USER_CHANGE 0x10000 /* can't set if current-user-id temporarily changed */
+
/* GUC vars that are actually declared in guc.c, rather than elsewhere */
extern bool log_duration;
Gurjeet Singh <singh.gurjeet@gmail.com> writes:
We are seeking to enable SET ROLE in security-definer functions, since @
D.E Shaw there are scripts from the past that used this feature, and I think
you'd also agree that SET ROLE is security definer functions has it's uses.
Actually, I don't find that to be a given. Exactly what use-cases have
you got that aren't solved as well or better by calling a SECURITY DEFINER
function owned by the target role?
As the code stands right now, I see that the only concern against
enabling SET ROLE in SecDef functions is that, that when the local-user-id
change context is exited, the GUC might be left out of sync.
This statement represents a complete lack of understanding of the actual
security problem. The actual security problem is that SET ROLE allows
you to become any role that the *session* user is allowed to become.
The reason for locking it down in security-restricted contexts is that
we don't want that to happen: we need to confine the available
privileges to only those that, say, the owner of the table being
vacuumed would have.
While it's possible that we could design some mechanism that would
enforce this properly, I fear that it would be tricky and a likely
source of future new security problems. In any case the net result
would be that SET ROLE would behave differently from spec, so it would
still be non-standard-compliant, just differently from before. So IMHO
you really need to offer a convincing reason why we should even try to
solve this, as opposed to telling people to use security definer
functions.
regards, tom lane
On Thu, Dec 31, 2009 at 10:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Gurjeet Singh <singh.gurjeet@gmail.com> writes:
We are seeking to enable SET ROLE in security-definer functions,
since @
D.E Shaw there are scripts from the past that used this feature, and I
think
you'd also agree that SET ROLE is security definer functions has it's
uses.
Actually, I don't find that to be a given. Exactly what use-cases have
you got that aren't solved as well or better by calling a SECURITY DEFINER
function owned by the target role?As the code stands right now, I see that the only concern against
enabling SET ROLE in SecDef functions is that, that when thelocal-user-id
change context is exited, the GUC might be left out of sync.
This statement represents a complete lack of understanding of the actual
security problem. The actual security problem is that SET ROLE allows
you to become any role that the *session* user is allowed to become.
I understand that reasoning very well, its just that I forgot to cover that
in the statement above.
The reason for locking it down in security-restricted contexts is that
we don't want that to happen: we need to confine the available
privileges to only those that, say, the owner of the table being
vacuumed would have.
The patch submitted still prohibits SET ROLE in security restricted
contexts, and yet allows it in security definer functions iff the function
is not executed while security restrictions are enabled. I think I covered
that here:
<quote>
So SET ROLE would be prohibited in maintenance operations, but allowed in
SecDef functions (only if they are not invoked on a stack where maintenance
operation was initiated earlier).
</quote>
While it's possible that we could design some mechanism that would
enforce this properly, I fear that it would be tricky and a likely
source of future new security problems. In any case the net result
would be that SET ROLE would behave differently from spec, so it would
still be non-standard-compliant, just differently from before. So IMHO
you really need to offer a convincing reason why we should even try to
solve this, as opposed to telling people to use security definer
functions.
Ian would be in a better position to provide a use-case.
Best regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.enterprisedb.com
singh.gurjeet@{ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet
Mail sent from my BlackLaptop device
-----Original Message-----
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
Actually, I don't find that to be a given. Exactly what use-cases have
you got that aren't solved as well or better by calling a SECURITY DEFINER
function owned by the target role?
Oh, that's easy: If you want to do the equivalent of setreuid(geteuid(), getuid()); that is, if you want to drop privileges for a particular operation. Our particular use case is that we want to evaluate an expression provided by the caller but with the caller's privileges.
Cheers,
--Ian
Turner, Ian wrote:
-----Original Message-----
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
Actually, I don't find that to be a given. Exactly what use-cases have
you got that aren't solved as well or better by calling a SECURITY DEFINER
function owned by the target role?Oh, that's easy: If you want to do the equivalent of setreuid(geteuid(), getuid()); that is, if you want to drop privileges for a particular operation. Our particular use case is that we want to evaluate an expression provided by the caller but with the caller's privileges.
Now *that's* what we should focus on. That's a reasonable use case, but
it doesn't seem like SET ROLE quite cuts it. For starters, wouldn't it
be possible for the caller's expression to call SET ROLE or RESET ROLE
to regain the privileges?
You could write a user-defined C function that does the same that
VACUUM/ANALYZE etc. do (now that we've fixed the vulnerabilities). Ie.
something like:
GetUserIdAndSecContext(&save_userid, &save_sec_context);
SetUserIdAndSecContext(<userid with less privileges>, save_sec_context |
SECURITY_RESTRICTED_OPERATION);
<call function>
/* Restore userid and security context */
SetUserIdAndSecContext(save_userid, save_sec_context);
No modifications to the server code required. Another question is, could
we provide some built-in support for dropping privileges like this?
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
Turner, Ian wrote:
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]Actually, I don't find that to be a given. Exactly what use-cases have
you got that aren't solved as well or better by calling a SECURITY DEFINER
function owned by the target role?Oh, that's easy: If you want to do the equivalent of setreuid(geteuid(), getuid()); that is, if you want to drop privileges for a particular operation. Our particular use case is that we want to evaluate an expression provided by the caller but with the caller's privileges.
Now *that's* what we should focus on. That's a reasonable use case, but
it doesn't seem like SET ROLE quite cuts it.
Exactly. If that's what you want, we can talk about it, but *SET ROLE
doesn't solve that problem*. In fact, a security definer function is a
lot closer to solving that problem than SET ROLE is. The premise of SET
ROLE is that you can always get to any role that the session user could
get to, so it doesn't "give up permissions" in any non-subvertible
fashion.
regards, tom lane
-----Original Message-----
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
Exactly. If that's what you want, we can talk about it, but *SET ROLE
doesn't solve that problem*. In fact, a security definer function is a
lot closer to solving that problem than SET ROLE is. The premise of SET
ROLE is that you can always get to any role that the session user could
get to, so it doesn't "give up permissions" in any non-subvertible
fashion.
For our purposes, SET ROLE is adequate, because the expression can't contain function calls. But there are alternative: We could create an in-transaction SECURITY DEFINER procedure which executes the expression, then drop the procedure before committing. A built-in feature for doing something like what Heikki suggests could be even more useful.
Cheers,
--Ian
"Turner, Ian" <Ian.Turner@deshaw.com> writes:
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
Exactly. If that's what you want, we can talk about it, but *SET ROLE
doesn't solve that problem*. In fact, a security definer function is a
lot closer to solving that problem than SET ROLE is. The premise of SET
ROLE is that you can always get to any role that the session user could
get to, so it doesn't "give up permissions" in any non-subvertible
fashion.
For our purposes, SET ROLE is adequate, because the expression can't
contain function calls.
Really? What can it contain, and how are you enforcing that? Even more
to the point, if you have managed to restrict it to the point where
there's no possibility of someone executing a SET ROLE, why do you need
any permissions switch at all? That's isomorphic to claiming that it
won't execute any SQL command at all, in which case you needn't worry about
changing permissions.
regards, tom lane
-----Original Message-----
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
Really? What can it contain, and how are you enforcing that?
Anything except a function call. We look for non-keyword identifier followed by open parenthesis, which is probably excessively restrictive. I'd rather have something less kludgey, of course. This will also become a real nightmare if new identifier quoting approaches are introduced.
Even more
to the point, if you have managed to restrict it to the point where
there's no possibility of someone executing a SET ROLE, why do you need
any permissions switch at all?
We don't want to have to check access privileges on every object referenced by the statement, which (as I'm sure you're aware) can get real nasty real quick.
That's isomorphic to claiming that it
won't execute any SQL command at all, in which case you needn't worry about
changing permissions.
Not sure what you mean here, can you elaborate?
--Ian
"Turner, Ian" <Ian.Turner@deshaw.com> writes:
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
Really? What can it contain, and how are you enforcing that?
Anything except a function call. We look for non-keyword identifier
followed by open parenthesis, which is probably excessively
restrictive.
I'm afraid this is security by wishful thinking. Operators and casts,
to name two obvious examples, can invoke user-defined code. Postgres
is sufficiently extensible that preventing any execution of non-core
code is nearly impossible, at least not without limiting things to
the point of uselessness.
Even more
to the point, if you have managed to restrict it to the point where
there's no possibility of someone executing a SET ROLE, why do you need
any permissions switch at all?
That's isomorphic to claiming that it
won't execute any SQL command at all, in which case you needn't worry about
changing permissions.
Not sure what you mean here, can you elaborate?
If you had a mechanism that ensured that the untrusted user couldn't
execute SET ROLE (which you don't), they couldn't execute anything else
either, and therefore the question of what permissions they're running
with isn't really important.
I agree that you have a problem to solve, but defining the problem as
"please can we have SET ROLE back" is not going to lead you to a secure
solution.
regards, tom lane
-----Original Message-----
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
I agree that you have a problem to solve, but defining the problem as
"please can we have SET ROLE back" is not going to lead you to a secure
solution.
Fair enough. Thanks for the analysis.
--Ian