Обсуждение: Re-enabling SET ROLE in security definer functions

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

> -----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
privilegesfor a particular operation. Our particular use case is that we want to evaluate an expression provided by the
callerbut 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
privilegesfor a particular operation. Our particular use case is that we want to evaluate an expression provided by the
callerbut 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
privilegesfor a particular operation. Our particular use case is that we want to evaluate an expression provided by the
callerbut 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:
Wecould create an in-transaction SECURITY DEFINER procedure which executes the expression, then drop the procedure
beforecommitting. 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
excessivelyrestrictive. I'd rather have something less kludgey, of course. This will also become a real nightmare if
newidentifier 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