Security vulnerability regarding SET ROLE and REINDEX - Mailing list pgsql-hackers
From | Gurjeet Singh |
---|---|
Subject | Security vulnerability regarding SET ROLE and REINDEX |
Date | |
Msg-id | dcc0ebd90912310252i1987cc3bgf1e757eb537f3bcb@mail.gmail.com Whole thread Raw |
List | pgsql-hackers |
Hi All,
We are seeking to enable SET ROLE in security-definer functions, since D.E Shaw has a usage for this, 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: 31d0bddf77b9e2b5581816aa96d3a392ab7d8543.
See also: http://gurjeet-tech.blogspot.com/2009/12/conversation-on-fixing-security-issue.html
We are seeking to enable SET ROLE in security-definer functions, since D.E Shaw has a usage for this, 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: 31d0bddf77b9e2b5581816aa96d3a392ab7d8543.
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:Maybe you had better read that statement again, and remember that the
> 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.
session user is typically a superuser in exactly the cases we are
concerned about.
regards, tom lane
--
gurjeet[.singh]@EnterpriseDB.com
EnterpriseDB http://www.enterprisedb.com
singh.gurjeet@{ gmail | hotmail | indiatimes | yahoo }.com
gurjeet[.singh]@EnterpriseDB.com
EnterpriseDB http://www.enterprisedb.com
singh.gurjeet@{ gmail | hotmail | indiatimes | yahoo }.com
--
gurjeet[.singh]@EnterpriseDB.com
EnterpriseDB http://www.enterprisedb.com
singh.gurjeet@{ gmail | hotmail | indiatimes | yahoo }.com
Attachment
pgsql-hackers by date: