Thread

[v9.2] Add GUC sepgsql.client_label

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-01-10T11:28:14Z

This patch adds a new GUC "sepgsql.client_label" that allows client
process to switch its privileges into another one, as long as the
system security policy admits this transition.
Because of this feature, I ported two permissions from "process" class
of SELinux; "setcurrent" and "dyntransition". The first one checks
whether the client has a right to switch its privilege. And the other
one checks a particular transition path from X to Y.

This feature might seem to break assumption of the sepgsql's security
model. However, single-directed domain transition from
bigger-privileges to smaller-privileged domain by users' operation is
also supported on operating system, and useful feature to restrict
applications capability at beginning of the session.

A few weeks ago, I got a requirement from Joshua Brindle. He is
working for Web-application that uses CAC (Common Access Card) for its
authentication, and wanted to integrate its security credential and
security label of selinux/sepgsql.
One problem was the system environment unavailable to use
labeled-networking (IPsec), thus, it was not an option to switch the
security label of processes on web-server side. An other solution is
to port dynamic-transition feature into sepgsql, as an analogy of
operating system.

An expected scenario is below:
The web-server is running with WEBSERV domain. It is allowed to
connect to PostgreSQL, and also allowed to invoke an trusted-procedure
that takes an argument of security-credential within CAC, but, nothing
else are allowed.
The trusted-procedure is allowed to reference a table between
security-credential and security-label to be assigned on, then it
switches the security label of client into CLIENT_n.
The CLIENT_n shall be allowed to access tables, functions and others
according to the security policy, and also allowed to reset
"sepgsql.security_label" to revert WEBSERV. However, he is not
available to switch other domain without security-credential stored
within CAC card.

I and Joshua agreed this scenario is reasonable and secure.
So, we'd like to suggest this new feature towards v9.2 timeline.

Thanks,

[*1] CAC - Common Access Card
http://en.wikipedia.org/wiki/Common_Access_Card
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Re: [v9.2] Add GUC sepgsql.client_label

Robert Haas <robertmhaas@gmail.com> — 2012-01-26T17:24:55Z

On Tue, Jan 10, 2012 at 6:28 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> This patch adds a new GUC "sepgsql.client_label" that allows client
> process to switch its privileges into another one, as long as the
> system security policy admits this transition.
> Because of this feature, I ported two permissions from "process" class
> of SELinux; "setcurrent" and "dyntransition". The first one checks
> whether the client has a right to switch its privilege. And the other
> one checks a particular transition path from X to Y.
>
> This feature might seem to break assumption of the sepgsql's security
> model. However, single-directed domain transition from
> bigger-privileges to smaller-privileged domain by users' operation is
> also supported on operating system, and useful feature to restrict
> applications capability at beginning of the session.
>
> A few weeks ago, I got a requirement from Joshua Brindle. He is
> working for Web-application that uses CAC (Common Access Card) for its
> authentication, and wanted to integrate its security credential and
> security label of selinux/sepgsql.
> One problem was the system environment unavailable to use
> labeled-networking (IPsec), thus, it was not an option to switch the
> security label of processes on web-server side. An other solution is
> to port dynamic-transition feature into sepgsql, as an analogy of
> operating system.
>
> An expected scenario is below:
> The web-server is running with WEBSERV domain. It is allowed to
> connect to PostgreSQL, and also allowed to invoke an trusted-procedure
> that takes an argument of security-credential within CAC, but, nothing
> else are allowed.
> The trusted-procedure is allowed to reference a table between
> security-credential and security-label to be assigned on, then it
> switches the security label of client into CLIENT_n.
> The CLIENT_n shall be allowed to access tables, functions and others
> according to the security policy, and also allowed to reset
> "sepgsql.security_label" to revert WEBSERV. However, he is not
> available to switch other domain without security-credential stored
> within CAC card.
>
> I and Joshua agreed this scenario is reasonable and secure.
> So, we'd like to suggest this new feature towards v9.2 timeline.

I'm wondering if a function would be a better fit than a GUC.  I don't
think you can really restrict the ability to revert a GUC change -
i.e. if someone does a SET and then a RESET, you pretty much have to
allow that.  I think.  But if you expose a function then it can work
however you like.

On another note, this is an awfully large patch.  Is there a separate
patch here that just does code rearrangement that should be separated
out?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [v9.2] Add GUC sepgsql.client_label

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-01-26T19:07:41Z

2012/1/26 Robert Haas <robertmhaas@gmail.com>:
> I'm wondering if a function would be a better fit than a GUC.  I don't
> think you can really restrict the ability to revert a GUC change -
> i.e. if someone does a SET and then a RESET, you pretty much have to
> allow that.  I think.  But if you expose a function then it can work
> however you like.
>
One benefit to use GUC is that we can utilize existing mechanism to
revert a value set within a transaction block on error.
If we implement same functionality with functions, XactCallback allows
sepgsql to get control on appropriate timing?

> On another note, this is an awfully large patch.  Is there a separate
> patch here that just does code rearrangement that should be separated
> out?
>
OK. I moved some routines related to client_label into label.c.
I'll separate this part from the new functionality part.

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Re: [v9.2] Add GUC sepgsql.client_label

Robert Haas <robertmhaas@gmail.com> — 2012-01-26T19:15:00Z

On Thu, Jan 26, 2012 at 2:07 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> 2012/1/26 Robert Haas <robertmhaas@gmail.com>:
>> I'm wondering if a function would be a better fit than a GUC.  I don't
>> think you can really restrict the ability to revert a GUC change -
>> i.e. if someone does a SET and then a RESET, you pretty much have to
>> allow that.  I think.  But if you expose a function then it can work
>> however you like.
>>
> One benefit to use GUC is that we can utilize existing mechanism to
> revert a value set within a transaction block on error.
> If we implement same functionality with functions, XactCallback allows
> sepgsql to get control on appropriate timing?

Not sure, but I thought the use case was to set this at connection
startup time and then hand the connection off to a client.  What keeps
the client from just issuing RESET?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [v9.2] Add GUC sepgsql.client_label

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-01-28T19:20:58Z

2012/1/26 Robert Haas <robertmhaas@gmail.com>:
> On Thu, Jan 26, 2012 at 2:07 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> 2012/1/26 Robert Haas <robertmhaas@gmail.com>:
>>> I'm wondering if a function would be a better fit than a GUC.  I don't
>>> think you can really restrict the ability to revert a GUC change -
>>> i.e. if someone does a SET and then a RESET, you pretty much have to
>>> allow that.  I think.  But if you expose a function then it can work
>>> however you like.
>>>
>> One benefit to use GUC is that we can utilize existing mechanism to
>> revert a value set within a transaction block on error.
>> If we implement same functionality with functions, XactCallback allows
>> sepgsql to get control on appropriate timing?
>
> Not sure, but I thought the use case was to set this at connection
> startup time and then hand the connection off to a client.  What keeps
> the client from just issuing RESET?
>
In the use case of Joshua, the original security label does not privileges
to reference any tables, and it cannot translate any domains without
credential within IC-cards. Thus, RESET is harmless.

However, I also assume one other possible use-case; the originator
has privileges to translate 10 of different domains, but disallows to
revert it. In this case, RESET without permission checks are harmful.
(This scenario will be suitable with multi-category-model.)

Apart from this issue, I found a problem on implementation using GUC
options. It makes backend crash, if it tries to reset sepgsql.client_label
without permissions within error handler because of double-errors.

So, I'll try to modify the patch with an idea to use a function.

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Re: [v9.2] Add GUC sepgsql.client_label

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-01-29T12:27:32Z

2012/1/28 Kohei KaiGai <kaigai@kaigai.gr.jp>:
> 2012/1/26 Robert Haas <robertmhaas@gmail.com>:
>> On Thu, Jan 26, 2012 at 2:07 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>> 2012/1/26 Robert Haas <robertmhaas@gmail.com>:
>>>> I'm wondering if a function would be a better fit than a GUC.  I don't
>>>> think you can really restrict the ability to revert a GUC change -
>>>> i.e. if someone does a SET and then a RESET, you pretty much have to
>>>> allow that.  I think.  But if you expose a function then it can work
>>>> however you like.
>>>>
>>> One benefit to use GUC is that we can utilize existing mechanism to
>>> revert a value set within a transaction block on error.
>>> If we implement same functionality with functions, XactCallback allows
>>> sepgsql to get control on appropriate timing?
>>
>> Not sure, but I thought the use case was to set this at connection
>> startup time and then hand the connection off to a client.  What keeps
>> the client from just issuing RESET?
>>
> In the use case of Joshua, the original security label does not privileges
> to reference any tables, and it cannot translate any domains without
> credential within IC-cards. Thus, RESET is harmless.
>
> However, I also assume one other possible use-case; the originator
> has privileges to translate 10 of different domains, but disallows to
> revert it. In this case, RESET without permission checks are harmful.
> (This scenario will be suitable with multi-category-model.)
>
> Apart from this issue, I found a problem on implementation using GUC
> options. It makes backend crash, if it tries to reset sepgsql.client_label
> without permissions within error handler because of double-errors.
>
I found an idea to avoid this scenario.

The problematic case is reset GUC variable because of transaction
rollback and permission violation, however, we don't intend to apply
permission checks, since it is not committed yet.
Thus, I considered to check state of the transaction using
IsTransactionState() on the assign_hook of GUC variable.

Unlike function based implementation, it is available to utilize existing
infrastructure to set the client_label to be transaction-aware.

It shall be implemented as:

void
sepgsql_assign_client_label(const char *newval, void *extra)
{
    if (!IsTransactionState)
        return;

    /* check whether valid security context */

    /* check permission check to switch current context */
}

How about such an implementation?

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Re: [v9.2] Add GUC sepgsql.client_label

Robert Haas <robertmhaas@gmail.com> — 2012-01-30T18:19:40Z

On Sun, Jan 29, 2012 at 7:27 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> 2012/1/28 Kohei KaiGai <kaigai@kaigai.gr.jp>:
>> 2012/1/26 Robert Haas <robertmhaas@gmail.com>:
>>> On Thu, Jan 26, 2012 at 2:07 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>>> 2012/1/26 Robert Haas <robertmhaas@gmail.com>:
>>>>> I'm wondering if a function would be a better fit than a GUC.  I don't
>>>>> think you can really restrict the ability to revert a GUC change -
>>>>> i.e. if someone does a SET and then a RESET, you pretty much have to
>>>>> allow that.  I think.  But if you expose a function then it can work
>>>>> however you like.
>>>>>
>>>> One benefit to use GUC is that we can utilize existing mechanism to
>>>> revert a value set within a transaction block on error.
>>>> If we implement same functionality with functions, XactCallback allows
>>>> sepgsql to get control on appropriate timing?
>>>
>>> Not sure, but I thought the use case was to set this at connection
>>> startup time and then hand the connection off to a client.  What keeps
>>> the client from just issuing RESET?
>>>
>> In the use case of Joshua, the original security label does not privileges
>> to reference any tables, and it cannot translate any domains without
>> credential within IC-cards. Thus, RESET is harmless.
>>
>> However, I also assume one other possible use-case; the originator
>> has privileges to translate 10 of different domains, but disallows to
>> revert it. In this case, RESET without permission checks are harmful.
>> (This scenario will be suitable with multi-category-model.)
>>
>> Apart from this issue, I found a problem on implementation using GUC
>> options. It makes backend crash, if it tries to reset sepgsql.client_label
>> without permissions within error handler because of double-errors.
>>
> I found an idea to avoid this scenario.
>
> The problematic case is reset GUC variable because of transaction
> rollback and permission violation, however, we don't intend to apply
> permission checks, since it is not committed yet.
> Thus, I considered to check state of the transaction using
> IsTransactionState() on the assign_hook of GUC variable.
>
> Unlike function based implementation, it is available to utilize existing
> infrastructure to set the client_label to be transaction-aware.
>
> It shall be implemented as:
>
> void
> sepgsql_assign_client_label(const char *newval, void *extra)
> {
>    if (!IsTransactionState)
>        return;
>
>    /* check whether valid security context */
>
>    /* check permission check to switch current context */
> }
>
> How about such an implementation?

Well, I think the idea that GUC changes can be reverted at any time is
fairly deeply ingrained in the GUC machinery.  So I think that's a
hack: it might happen to work today (or at least on the cases we can
think of to test), but I wouldn't be a bit surprised if there are
other cases where it fails, either now or in the future.  I don't see
any good reason to assume that unrevertable changes are OK even inside
a transaction context.  For example, if someone applies a context to a
function with ALTER FUNCTION, they're going to expect that the altered
context remains in effect while the function is running and gets
reverted on exit.  If you throw an error when the system tries to
revert the change at function-exit time, it might not crash the
server, but it's certainly going to violate the principle of least
astonishment.

Of course, we already have a trusted procedure mechanism which is
intended to support temporary changes to the effective security label,
so you might say, hey, people shouldn't do that.  But they might.  And
I wouldn't like to bet that's the only case that could be problematic
either.  What about a setting in postgresql.conf?  You could end up
being asked to set the security label to some other value before
you've initialized it.  What about SET LOCAL?  It's not OK to fail to
revert that at transaction exit.

Hence my suggestion of a function: if you use functions, you can
implement whatever semantics you want.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [v9.2] Add GUC sepgsql.client_label

Yeb Havinga <yebhavinga@gmail.com> — 2012-01-31T09:36:05Z

On 2012-01-30 19:19, Robert Haas wrote:
> On Sun, Jan 29, 2012 at 7:27 AM, Kohei KaiGai<kaigai@kaigai.gr.jp>  wrote:
>> However, I also assume one other possible use-case; the originator
>> has privileges to translate 10 of different domains, but disallows to
>> revert it. In this case, RESET without permission checks are harmful.
>> (This scenario will be suitable with multi-category-model.)
> Of course, we already have a trusted procedure mechanism which is 
> intended to support temporary changes to the effective security label, 
> so you might say, hey, people shouldn't do that. But they might. And I 
> wouldn't like to bet that's the only case that could be problematic 
> either. What about a setting in postgresql.conf? You could end up 
> being asked to set the security label to some other value before 
> you've initialized it. What about SET LOCAL? It's not OK to fail to 
> revert that at transaction exit. Hence my suggestion of a function: if 
> you use functions, you can implement whatever semantics you want. 

What about always allowing a transition to the default / postgresql.conf 
configured client label, so in the case of errors, or RESET, the 
transition to this domain is hardcoded to succeed. This would also be 
useful in conjunction with a connection pooler. This would still allow 
the option to prevent a back transition to non-default client labels.

-- 
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

Re: [v9.2] Add GUC sepgsql.client_label

Robert Haas <robertmhaas@gmail.com> — 2012-01-31T13:06:51Z

On Tue, Jan 31, 2012 at 4:36 AM, Yeb Havinga <yebhavinga@gmail.com> wrote:
> What about always allowing a transition to the default / postgresql.conf
> configured client label, so in the case of errors, or RESET, the transition
> to this domain is hardcoded to succeed. This would also be useful in
> conjunction with a connection pooler. This would still allow the option to
> prevent a back transition to non-default client labels.

I don't think you can make that work, because someone can still
attempt to RESET to a different value, and it's still not safe to make
that fail.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [v9.2] Add GUC sepgsql.client_label

Yeb Havinga <yebhavinga@gmail.com> — 2012-01-31T14:10:35Z

On 2012-01-31 14:06, Robert Haas wrote:
> On Tue, Jan 31, 2012 at 4:36 AM, Yeb Havinga<yebhavinga@gmail.com>  wrote:
>> What about always allowing a transition to the default / postgresql.conf
>> configured client label, so in the case of errors, or RESET, the transition
>> to this domain is hardcoded to succeed. This would also be useful in
>> conjunction with a connection pooler. This would still allow the option to
>> prevent a back transition to non-default client labels.
> I don't think you can make that work, because someone can still
> attempt to RESET to a different value, and it's still not safe to make
> that fail.

But the idea is that if that different value is a (possibly changed into 
a new) allowed default value, a RESET to that new default value will be 
allowed, by definition, because it is a default value.


-- 
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

Re: [v9.2] Add GUC sepgsql.client_label

Robert Haas <robertmhaas@gmail.com> — 2012-01-31T14:28:38Z

On Tue, Jan 31, 2012 at 9:10 AM, Yeb Havinga <yebhavinga@gmail.com> wrote:
> On 2012-01-31 14:06, Robert Haas wrote:
>> On Tue, Jan 31, 2012 at 4:36 AM, Yeb Havinga<yebhavinga@gmail.com>  wrote:
>>>
>>> What about always allowing a transition to the default / postgresql.conf
>>> configured client label, so in the case of errors, or RESET, the
>>> transition
>>> to this domain is hardcoded to succeed. This would also be useful in
>>> conjunction with a connection pooler. This would still allow the option
>>> to
>>> prevent a back transition to non-default client labels.
>>
>> I don't think you can make that work, because someone can still
>> attempt to RESET to a different value, and it's still not safe to make
>> that fail.
>
> But the idea is that if that different value is a (possibly changed into a
> new) allowed default value, a RESET to that new default value will be
> allowed, by definition, because it is a default value.

*scratches head*

I'm not sure I follow you.  If you're saying that we can make this
work by always allowing the value to be reset, then I agree with you,
but I'm not sure those are the semantics KaiGai wants.  For instance,
if a connection pooler does:

SET sepgsql.client_label = 'bob_t';

...and then hands off to the client, the client can then do:

RESET sepgsql.client_label;
SET sepgsql.client_label = 'alice_t';

....and that's bad.  More generally, the system security policy is
designed to answer questions about whether it's OK to transition from
A->B, and the fact that A->B is OK does not mean that B->A is OK, but
our GUC mechanism pretty much forces you to allow both of those
things, or neither.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [v9.2] Add GUC sepgsql.client_label

Yeb Havinga <yebhavinga@gmail.com> — 2012-01-31T15:27:08Z

On 2012-01-31 15:28, Robert Haas wrote:
>
> *scratches head*
>
> I'm not sure I follow you.  If you're saying that we can make this
> work by always allowing the value to be reset, then I agree with you,
> but I'm not sure those are the semantics KaiGai wants.  For instance,
> if a connection pooler does:
>
> SET sepgsql.client_label = 'bob_t';
>
> ...and then hands off to the client, the client can then do:
>
> RESET sepgsql.client_label;
> SET sepgsql.client_label = 'alice_t';
>
> ....and that's bad.

Hmm yes this is a problem. Reading the original post better, it is also 
not the intended behaviour to support repeatable client_label switches.

"However, single-directed domain transition from bigger-privileges to 
smaller-privileged domain by users' operation is also supported on 
operating system, and useful feature to restrict applications capability 
at beginning of the session."

-- 
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

Re: [v9.2] Add GUC sepgsql.client_label

Tom Lane <tgl@sss.pgh.pa.us> — 2012-01-31T20:55:16Z

Robert Haas <robertmhaas@gmail.com> writes:
> ....and that's bad.  More generally, the system security policy is
> designed to answer questions about whether it's OK to transition from
> A->B, and the fact that A->B is OK does not mean that B->A is OK, but
> our GUC mechanism pretty much forces you to allow both of those
> things, or neither.

More to the point, a GUC rollback transition *has to always succeed*.
Period.  Now, the value that it's trying to roll back to was presumably
considered legitimate at some previous time, but if you're designing a
system that is based purely on state transitions it could very well see
the rollback transition as invalid.  That is just going to be too
fragile to be acceptable.

I think that this will have to be set up so that it understands the
difference between a forward transition and a rollback and only checks
the former.  If that's not possible, this is not going to get in.

			regards, tom lane

Re: [v9.2] Add GUC sepgsql.client_label

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-02-05T09:09:18Z

2012/1/30 Robert Haas <robertmhaas@gmail.com>:
> On Sun, Jan 29, 2012 at 7:27 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> 2012/1/28 Kohei KaiGai <kaigai@kaigai.gr.jp>:
>>> 2012/1/26 Robert Haas <robertmhaas@gmail.com>:
>>>> On Thu, Jan 26, 2012 at 2:07 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>>>> 2012/1/26 Robert Haas <robertmhaas@gmail.com>:
>>>>>> I'm wondering if a function would be a better fit than a GUC.  I don't
>>>>>> think you can really restrict the ability to revert a GUC change -
>>>>>> i.e. if someone does a SET and then a RESET, you pretty much have to
>>>>>> allow that.  I think.  But if you expose a function then it can work
>>>>>> however you like.
>>>>>>
>>>>> One benefit to use GUC is that we can utilize existing mechanism to
>>>>> revert a value set within a transaction block on error.
>>>>> If we implement same functionality with functions, XactCallback allows
>>>>> sepgsql to get control on appropriate timing?
>>>>
>>>> Not sure, but I thought the use case was to set this at connection
>>>> startup time and then hand the connection off to a client.  What keeps
>>>> the client from just issuing RESET?
>>>>
>>> In the use case of Joshua, the original security label does not privileges
>>> to reference any tables, and it cannot translate any domains without
>>> credential within IC-cards. Thus, RESET is harmless.
>>>
>>> However, I also assume one other possible use-case; the originator
>>> has privileges to translate 10 of different domains, but disallows to
>>> revert it. In this case, RESET without permission checks are harmful.
>>> (This scenario will be suitable with multi-category-model.)
>>>
>>> Apart from this issue, I found a problem on implementation using GUC
>>> options. It makes backend crash, if it tries to reset sepgsql.client_label
>>> without permissions within error handler because of double-errors.
>>>
>> I found an idea to avoid this scenario.
>>
>> The problematic case is reset GUC variable because of transaction
>> rollback and permission violation, however, we don't intend to apply
>> permission checks, since it is not committed yet.
>> Thus, I considered to check state of the transaction using
>> IsTransactionState() on the assign_hook of GUC variable.
>>
>> Unlike function based implementation, it is available to utilize existing
>> infrastructure to set the client_label to be transaction-aware.
>>
>> It shall be implemented as:
>>
>> void
>> sepgsql_assign_client_label(const char *newval, void *extra)
>> {
>>    if (!IsTransactionState)
>>        return;
>>
>>    /* check whether valid security context */
>>
>>    /* check permission check to switch current context */
>> }
>>
>> How about such an implementation?
>
> Well, I think the idea that GUC changes can be reverted at any time is
> fairly deeply ingrained in the GUC machinery.  So I think that's a
> hack: it might happen to work today (or at least on the cases we can
> think of to test), but I wouldn't be a bit surprised if there are
> other cases where it fails, either now or in the future.  I don't see
> any good reason to assume that unrevertable changes are OK even inside
> a transaction context.  For example, if someone applies a context to a
> function with ALTER FUNCTION, they're going to expect that the altered
> context remains in effect while the function is running and gets
> reverted on exit.  If you throw an error when the system tries to
> revert the change at function-exit time, it might not crash the
> server, but it's certainly going to violate the principle of least
> astonishment.
>
> Of course, we already have a trusted procedure mechanism which is
> intended to support temporary changes to the effective security label,
> so you might say, hey, people shouldn't do that.  But they might.  And
> I wouldn't like to bet that's the only case that could be problematic
> either.  What about a setting in postgresql.conf?  You could end up
> being asked to set the security label to some other value before
> you've initialized it.  What about SET LOCAL?  It's not OK to fail to
> revert that at transaction exit.
>
> Hence my suggestion of a function: if you use functions, you can
> implement whatever semantics you want.
>
I tried to implement it based on the function approach.

The new sepgsql_setcon(TEXT) enables to set and reset alternative
security label of the database client, unless it violates permission
checks.

The supplied alternative security label is once appended on the
client_label_pending list, then an active item shall be saved on
the transaction commit time. If sub-transaction is rollbacked to
a particular savepoint, items on the pending-list shall be removed
according to its sub-transaction-id.

It extends idea of client_label into three types; peer-label, setcon-label
and func-label. The first one is obtained via getpeercon(3) on
database logon time, and always not null. The second one is set
using the new sepgsql_setcon(), and the other one is set during
execution of trusted procedure.

Thus, the logic of sepgsql_get_client_label() were modified to handle
these three kind of client labels correctly.

The attached part-1 patch moves related routines from hooks.c to
label.c because of references to static variables.
The part-2 patch implements above mechanism.

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Re: [v9.2] Add GUC sepgsql.client_label

Robert Haas <robertmhaas@gmail.com> — 2012-02-15T19:05:32Z

On Sun, Feb 5, 2012 at 4:09 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> The attached part-1 patch moves related routines from hooks.c to
> label.c because of references to static variables.

I have committed this part.  Seems like a better location for that code, anyhow.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [v9.2] Add GUC sepgsql.client_label

Yeb Havinga <yebhavinga@gmail.com> — 2012-02-20T14:35:13Z

On 2012-02-05 10:09, Kohei KaiGai wrote:
> The attached part-1 patch moves related routines from hooks.c to 
> label.c because of references to static variables. The part-2 patch 
> implements above mechanism. 

I took a short look at this patch but am stuck getting the regression 
test to run properly.

First, patch 2 misses the file sepgsql.sql.in and therefore the creation 
function command for sepgsql_setcon is missing.

When that was solved, ./test_psql failed on the message that the psql 
file may not be of object type unconfined_t. Once it was changed to 
bin_t, the result output for the domain transition gives differences on 
this output (the other parts of label.sql were ok) :

--
-- Test for Dynamic Domain Transition
--
-- validation of transaction aware dynamic-transition
/usr/bin/runcon: /usr/local/pgsql/bin/psql: Permission denied
/usr/bin/runcon: /usr/local/pgsql/bin/psql: Permission denied
/usr/bin/runcon: /usr/local/pgsql/bin/psql: Permission denied

However when I connect to the regression database directly, I can 
execute the first setcon command but get
regression=# SELECT 
sepgsql_setcon('unconfined_u:unconfined_r:unconfined_t:s0:c0.c12');
ERROR:  SELinux: security policy violation

logfile shows
LOG:  SELinux: denied { dyntransition } 
scontext=unconfined_u:unconfined_r:unconfined_t:s0 
tcontext=unconfined_u:unconfined_r:unconfined_t:s0:c0.c12 tclass=process

So maybe this is because my start domain is not s0-s0:c0.c1023

However, when trying to run bash or psql in domain 
unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 I get permission 
denied.

Distribution is FC15, sestatus
SELinux status:                 enabled
SELinuxfs mount:                /selinux
Current mode:                   enforcing
Mode from config file:          enforcing
Policy version:                 24
Policy from config file:        targeted

-- 
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

Re: [v9.2] Add GUC sepgsql.client_label

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-02-23T11:17:14Z

2012/2/20 Yeb Havinga <yebhavinga@gmail.com>:
> On 2012-02-05 10:09, Kohei KaiGai wrote:
>>
>> The attached part-1 patch moves related routines from hooks.c to label.c
>> because of references to static variables. The part-2 patch implements above
>> mechanism.
>
>
> I took a short look at this patch but am stuck getting the regression test
> to run properly.
>
> First, patch 2 misses the file sepgsql.sql.in and therefore the creation
> function command for sepgsql_setcon is missing.
>
Thanks for your comments.

I added the definition of sepgsql_setcon function to sepgsql.sql.in file,
in addition to patch rebasing.

> So maybe this is because my start domain is not s0-s0:c0.c1023
>
> However, when trying to run bash or psql in domain
> unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 I get permission
> denied.
>
> Distribution is FC15, sestatus
> SELinux status:                 enabled
> SELinuxfs mount:                /selinux
> Current mode:                   enforcing
> Mode from config file:          enforcing
> Policy version:                 24
> Policy from config file:        targeted
>
The "default" security policy does not permit dynamic domain transition
even if unconfined domain, in contradiction to its name.
(IMO, it is fair enough design to avoid single point of failure like root user.)

The security policy of regression test contains a set of rules to reduce
categories assigned to unconfined domain.
So, could you try the following steps.
1. Build the latest policy
    % make -f /usr/share/selinux/devel/Makefile -C contrib/sepgsql
2. Install the policy module
    % sudo semodule -i contrib/sepgsql/sepgsql-regtest.pp
3. Turn on the sepgsql_regression_test_mode
    % sudo setsebool -P sepgsql_regression_test_mode=1

I believe it allows to switch security label of the client, as long as we try to
reduce categories.

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Re: [v9.2] Add GUC sepgsql.client_label

Yeb Havinga <yebhavinga@gmail.com> — 2012-02-24T12:50:01Z

On 2012-02-23 12:17, Kohei KaiGai wrote:
> 2012/2/20 Yeb Havinga<yebhavinga@gmail.com>:
>> So maybe this is because my start domain is not s0-s0:c0.c1023
>>
>> However, when trying to run bash or psql in domain
>> unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 I get permission
>> denied.
>>
>> Distribution is FC15, sestatus
>> SELinux status:                 enabled
>> SELinuxfs mount:                /selinux
>> Current mode:                   enforcing
>> Mode from config file:          enforcing
>> Policy version:                 24
>> Policy from config file:        targeted
>>
> The "default" security policy does not permit dynamic domain transition
> even if unconfined domain, in contradiction to its name.
> (IMO, it is fair enough design to avoid single point of failure like root user.)
>
> The security policy of regression test contains a set of rules to reduce
> categories assigned to unconfined domain.
> So, could you try the following steps.
> 1. Build the latest policy
>      % make -f /usr/share/selinux/devel/Makefile -C contrib/sepgsql
> 2. Install the policy module
>      % sudo semodule -i contrib/sepgsql/sepgsql-regtest.pp
> 3. Turn on the sepgsql_regression_test_mode
>      % sudo setsebool -P sepgsql_regression_test_mode=1
>
> I believe it allows to switch security label of the client, as long as we try to
> reduce categories.

I remember these commands from the sepgsql contrib module documentation 
(though the semodule invocation in the documentation is with -u and the 
setsebool does not have the -P flag). semodule -l showed I had already 
installed version 1.04.

I just repeated all steps with the new patch, and get the same result:

LOG:  SELinux: denied { dyntransition } 
scontext=unconfined_u:unconfined_r:unconfined_t:s0 
tcontext=unconfined_u:unconfined_r:unconfined_t:s0:c0.c15 tclass=process
STATEMENT:  SELECT 
sepgsql_setcon('unconfined_u:unconfined_r:unconfined_t:s0:c0.c15');

[mgrid@mgfedora sepgsql]$ getsebool sepgsql_regression_test_mode
sepgsql_regression_test_mode --> on
[root@mgfedora sepgsql]# semodule -l | egrep 'pgsql|postgres'
postgresql      1.12.1
sepgsql-regtest 1.04

Do I need Fedora 16 to run it?


-- 
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

Re: [v9.2] Add GUC sepgsql.client_label

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-02-24T13:20:25Z

2012/2/24 Yeb Havinga <yebhavinga@gmail.com>:
> On 2012-02-23 12:17, Kohei KaiGai wrote:
>>
>> 2012/2/20 Yeb Havinga<yebhavinga@gmail.com>:
>>
>>> So maybe this is because my start domain is not s0-s0:c0.c1023
>>>
>>> However, when trying to run bash or psql in domain
>>> unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 I get permission
>>> denied.
>>>
>>> Distribution is FC15, sestatus
>>> SELinux status:                 enabled
>>> SELinuxfs mount:                /selinux
>>> Current mode:                   enforcing
>>> Mode from config file:          enforcing
>>> Policy version:                 24
>>> Policy from config file:        targeted
>>>
>> The "default" security policy does not permit dynamic domain transition
>> even if unconfined domain, in contradiction to its name.
>> (IMO, it is fair enough design to avoid single point of failure like root
>> user.)
>>
>> The security policy of regression test contains a set of rules to reduce
>> categories assigned to unconfined domain.
>> So, could you try the following steps.
>> 1. Build the latest policy
>>     % make -f /usr/share/selinux/devel/Makefile -C contrib/sepgsql
>> 2. Install the policy module
>>     % sudo semodule -i contrib/sepgsql/sepgsql-regtest.pp
>> 3. Turn on the sepgsql_regression_test_mode
>>     % sudo setsebool -P sepgsql_regression_test_mode=1
>>
>> I believe it allows to switch security label of the client, as long as we
>> try to
>> reduce categories.
>
>
> I remember these commands from the sepgsql contrib module documentation
> (though the semodule invocation in the documentation is with -u and the
> setsebool does not have the -P flag). semodule -l showed I had already
> installed version 1.04.
>
> I just repeated all steps with the new patch, and get the same result:
>
> LOG:  SELinux: denied { dyntransition }
> scontext=unconfined_u:unconfined_r:unconfined_t:s0
> tcontext=unconfined_u:unconfined_r:unconfined_t:s0:c0.c15 tclass=process
> STATEMENT:  SELECT
> sepgsql_setcon('unconfined_u:unconfined_r:unconfined_t:s0:c0.c15');
>
> [mgrid@mgfedora sepgsql]$ getsebool sepgsql_regression_test_mode
> sepgsql_regression_test_mode --> on
> [root@mgfedora sepgsql]# semodule -l | egrep 'pgsql|postgres'
> postgresql      1.12.1
> sepgsql-regtest 1.04
>
> Do I need Fedora 16 to run it?
>
Thanks for your continuous testing.

It seems to me you try to expand categories of the client.
The log saids sepgsql_setcon() tries to switch to "...:s0:c0.c15" from "...:s0".
It is not an admitted operations because of increasion of categories.

> LOG:  SELinux: denied { dyntransition }
> scontext=unconfined_u:unconfined_r:unconfined_t:s0
> tcontext=unconfined_u:unconfined_r:unconfined_t:s0:c0.c15 tclass=process

May I see your /etc/selinux/targeted/seusers ?

I think "__default__" entry is configured to "unconfined_u:s0", instead of
"unconfined_u:s0:c0.c1023" as default.

In my environment, it is configured as follows:

  [root@iwashi ~]# cat /etc/selinux/targeted/seusers
  # This file is auto-generated by libsemanage
  # Do not edit directly.

  system_u:system_u:s0-s0:c0.c1023
  root:unconfined_u:s0-s0:c0.c1023
  __default__:unconfined_u:s0-s0:c0.c1023   <=== (*)

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Re: [v9.2] Add GUC sepgsql.client_label

Yeb Havinga <yebhavinga@gmail.com> — 2012-02-24T14:17:12Z

On 2012-02-24 14:20, Kohei KaiGai wrote:
>
> It seems to me you try to expand categories of the client.
> The log saids sepgsql_setcon() tries to switch to "...:s0:c0.c15" from "...:s0".
> It is not an admitted operations because of increasion of categories.

Yes I had my eye on the missing c0.c1023 before but couldn't remember 
changing it, so wrongfully assumed that it would be semantically 
equivalent to c0.c1023.
>> LOG:  SELinux: denied { dyntransition }
>> scontext=unconfined_u:unconfined_r:unconfined_t:s0
>> tcontext=unconfined_u:unconfined_r:unconfined_t:s0:c0.c15 tclass=process
> May I see your /etc/selinux/targeted/seusers ?
>
> I think "__default__" entry is configured to "unconfined_u:s0", instead of
> "unconfined_u:s0:c0.c1023" as default.
>
> In my environment, it is configured as follows:
>
>    [root@iwashi ~]# cat /etc/selinux/targeted/seusers
>    # This file is auto-generated by libsemanage
>    # Do not edit directly.
>
>    system_u:system_u:s0-s0:c0.c1023
>    root:unconfined_u:s0-s0:c0.c1023
>    __default__:unconfined_u:s0-s0:c0.c1023<=== (*)
>

[mgrid@mgfedora ~]$ cat /etc/selinux/targeted/seusers
# This file is auto-generated by libsemanage
# Do not edit directly.

system_u:system_u:s0-s0:c0.c1023
root:unconfined_u:s0-s0:c0.c1023
__default__:unconfined_u:s0-s0:c0.c1023

but still

[mgrid@mgfedora ~]$ id -Z
system_u:unconfined_r:unconfined_t:s0
(I changed bash to run in the unconfined_u context before starting the 
regression test)

and

[root@mgfedora targeted]# id -Z
system_u:unconfined_r:unconfined_t:s0

When I created a new test user, it's selinux context showed the c0.c1023 
- I don't know what's fishy about the mgrid user and root that causes 
c0.c1023 to be absent. Maybe I should reinstall this virtual machine. 
After setting the user "mgrid" on s0-s0:c0.c1023 with

semanage login -a -S targeted -s "unconfined_u" -r s0-s0:c0.c1023 mgrid

the regression tests pass :-)

test label                    ... ok
test dml                      ... ok
test create                   ... ok
test misc                     ... ok

I'll continue reviewing the patch.

-- 
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

Re: [v9.2] Add GUC sepgsql.client_label

Yeb Havinga <yebhavinga@gmail.com> — 2012-02-24T14:40:24Z

On 2012-02-24 15:17, Yeb Havinga wrote:
> I don't know what's fishy about the mgrid user and root that causes 
> c0.c1023 to be absent. 

more info:

In shells started in a x environment under Xvnc, id -Z shows the 
system_u and c0.c1023 absent.

In shells started from the ssh daemon, the id -Z matches what it should 
be according to the seusers file: unconfined_u and c0.c1023 present.

-- Yeb

Re: [v9.2] Add GUC sepgsql.client_label

Yeb Havinga <yebhavinga@gmail.com> — 2012-02-24T16:25:45Z

On 2012-02-23 12:17, Kohei KaiGai wrote:
> 2012/2/20 Yeb Havinga<yebhavinga@gmail.com>:
>> On 2012-02-05 10:09, Kohei KaiGai wrote:
>>> The attached part-1 patch moves related routines from hooks.c to label.c
>>> because of references to static variables. The part-2 patch implements above
>>> mechanism.
>>
>> I took a short look at this patch but am stuck getting the regression test
>> to run properly.
>>
>> First, patch 2 misses the file sepgsql.sql.in and therefore the creation
>> function command for sepgsql_setcon is missing.
>>
> Thanks for your comments.
>
> I added the definition of sepgsql_setcon function to sepgsql.sql.in file,
> in addition to patch rebasing.

Very brief comments due to must leave keyboard soon:

I read the source code and played a bit with setcon and the debugger, no 
strange things found.

Code comments / questions:

this comment below is a lie, because setcon is set by 
sepgsql_xact_callback()
maybe client_label_committed is a better name for client_label_setcon?

static char *client_label_setcon    = NULL;    /* set by sepgsql_setcon() */

Is the double negation in the sentence below intended?

+ * Neither of them has no special state, the security label being 
initialized
+ * at database-logon time shall be returned.

Is the assert client_label_peer != NULL in sepgsql_get_client_label 
necessary?

sepgsql_set_client_label(), maybe add a comment to !new_label that it is 
reset to the peer label.

new_label == NULL / pending_label->label == NULL means use the peer 
label. Why not use the peer label instead?

set_label: if new_label == current label according to getcon, is it 
necessary to add to the pending list?

sepgsql_subxact_callback(), could this be made easier to read by just 
taking llast(client_label_pending), assert that plabel->subid == mySubId 
and then list_delete on pointer of that listcell?

Some comments contain typos, I can spend some time on this, though I'm 
not a native english speaker so it won't be perfect.

regards,
Yeb Havinga

-- 
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

Re: [v9.2] Add GUC sepgsql.client_label

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-02-28T16:33:38Z

2012/2/24 Yeb Havinga <yebhavinga@gmail.com>:
> On 2012-02-24 15:17, Yeb Havinga wrote:
>>
>> I don't know what's fishy about the mgrid user and root that causes
>> c0.c1023 to be absent.
>
>
> more info:
>
> In shells started in a x environment under Xvnc, id -Z shows the system_u
> and c0.c1023 absent.
>
> In shells started from the ssh daemon, the id -Z matches what it should be
> according to the seusers file: unconfined_u and c0.c1023 present.
>
It seems to me the reason why your security label on bash is different from
the expected default one.
Unlike sshd daemon, vncserver does not assign security label on itself
according to the /etc/selinux/targeted/seusers, thus it inherits the label
of system startup script. It is also the reason why you saw "system_u"
at the head of security context.

I'll report this topic to selinux community to discuss the preferable solution.
Anyway, please use ssh connection for the testing purpose.

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Re: [v9.2] Add GUC sepgsql.client_label

Yeb Havinga <yebhavinga@gmail.com> — 2012-03-03T14:12:33Z

On 2012-02-24 17:25, Yeb Havinga wrote:
> On 2012-02-23 12:17, Kohei KaiGai wrote:
>> 2012/2/20 Yeb Havinga<yebhavinga@gmail.com>:
>>> On 2012-02-05 10:09, Kohei KaiGai wrote:
>>>> The attached part-1 patch moves related routines from hooks.c to 
>>>> label.c
>>>> because of references to static variables. The part-2 patch 
>>>> implements above
>>>> mechanism.
>>>
>>> I took a short look at this patch but am stuck getting the 
>>> regression test
>>> to run properly.
>>>
>>> First, patch 2 misses the file sepgsql.sql.in and therefore the 
>>> creation
>>> function command for sepgsql_setcon is missing.
>>>
>> Thanks for your comments.
>>
>> I added the definition of sepgsql_setcon function to sepgsql.sql.in 
>> file,
>> in addition to patch rebasing.
>
> Very brief comments due to must leave keyboard soon:
>
> I read the source code and played a bit with setcon and the debugger, 
> no strange things found.
>
> Code comments / questions:

I took the liberty to change a few things, mostly comments, in the 
attached patch:
>
> maybe client_label_committed is a better name for client_label_setcon?

this change was made.
>
> Is the double negation in the sentence below intended?

several comments were changed / moved. There is now one place where te 
behaviour of the different client_label variables are explained.

>
> sepgsql_set_client_label(), maybe add a comment to !new_label that it 
> is reset to the peer label.

done.

>
> Is the assert client_label_peer != NULL in sepgsql_get_client_label 
> necessary?
> new_label == NULL / pending_label->label == NULL means use the peer 
> label. Why not use the peer label instead?

It turned out that pending_label->label is invariantly non null. Changed 
code to assume that and added some Asserts.

>
> set_label: if new_label == current label according to getcon, is it 
> necessary to add to the pending list?

this question still remains. Maybe there would be reasons to hit selinux 
with the question: can I change from A->A.
>
> sepgsql_subxact_callback(), could this be made easier to read by just 
> taking llast(client_label_pending), assert that plabel->subid == 
> mySubId and then list_delete on pointer of that listcell?

no this was a naieve suggestion, which fails in any case of a 
subtransaction with not exactly one call to sepgsql_setcon()

> Some comments contain typos, I can spend some time on this, though I'm 
> not a native english speaker so it won't be perfect.

sgml documentation must still be added. If time permits I can spend some 
time on that tomorrow.

regards,
Yeb Havinga


-- 
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

Re: [v9.2] Add GUC sepgsql.client_label

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-03-06T14:14:54Z

Hi, Yeb.

Thanks for your reviewing and patch updates.
(and sorry my delayed response...)

I'd like to point out a case when plabel->label is NULL.

In case of sepgsql_setcon() being invoked with null argument
to reset security label of the client, but not committed yet,
the last item of the client_label_pending has null label.
(It performs as a mark of a security label being reset.)

It is a case when sepgsql_get_client_label() should return
the client_label_peer, not plabel->label.
So, I reverted some of your replacement; that assumes the
pending label is valid with Assert() check to null value.

Most of comments update are quite helpful for me.
So, I merged your revised one in this patch.

Thanks so much!

2012/3/3 Yeb Havinga <yebhavinga@gmail.com>:
> On 2012-02-24 17:25, Yeb Havinga wrote:
>>
>> On 2012-02-23 12:17, Kohei KaiGai wrote:
>>>
>>> 2012/2/20 Yeb Havinga<yebhavinga@gmail.com>:
>>>>
>>>> On 2012-02-05 10:09, Kohei KaiGai wrote:
>>>>>
>>>>> The attached part-1 patch moves related routines from hooks.c to
>>>>> label.c
>>>>> because of references to static variables. The part-2 patch implements
>>>>> above
>>>>> mechanism.
>>>>
>>>>
>>>> I took a short look at this patch but am stuck getting the regression
>>>> test
>>>> to run properly.
>>>>
>>>> First, patch 2 misses the file sepgsql.sql.in and therefore the creation
>>>> function command for sepgsql_setcon is missing.
>>>>
>>> Thanks for your comments.
>>>
>>> I added the definition of sepgsql_setcon function to sepgsql.sql.in file,
>>> in addition to patch rebasing.
>>
>>
>> Very brief comments due to must leave keyboard soon:
>>
>> I read the source code and played a bit with setcon and the debugger, no
>> strange things found.
>>
>> Code comments / questions:
>
>
> I took the liberty to change a few things, mostly comments, in the attached
> patch:
>
>>
>> maybe client_label_committed is a better name for client_label_setcon?
>
>
> this change was made.
>
>>
>> Is the double negation in the sentence below intended?
>
>
> several comments were changed / moved. There is now one place where te
> behaviour of the different client_label variables are explained.
>
>
>>
>> sepgsql_set_client_label(), maybe add a comment to !new_label that it is
>> reset to the peer label.
>
>
> done.
>
>>
>> Is the assert client_label_peer != NULL in sepgsql_get_client_label
>> necessary?
>> new_label == NULL / pending_label->label == NULL means use the peer label.
>> Why not use the peer label instead?
>
>
> It turned out that pending_label->label is invariantly non null. Changed
> code to assume that and added some Asserts.
>
>
>>
>> set_label: if new_label == current label according to getcon, is it
>> necessary to add to the pending list?
>
>
> this question still remains. Maybe there would be reasons to hit selinux
> with the question: can I change from A->A.
>
>>
>> sepgsql_subxact_callback(), could this be made easier to read by just
>> taking llast(client_label_pending), assert that plabel->subid == mySubId and
>> then list_delete on pointer of that listcell?
>
>
> no this was a naieve suggestion, which fails in any case of a subtransaction
> with not exactly one call to sepgsql_setcon()
>
>
>> Some comments contain typos, I can spend some time on this, though I'm not
>> a native english speaker so it won't be perfect.
>
>
> sgml documentation must still be added. If time permits I can spend some
> time on that tomorrow.
>
>
> regards,
> Yeb Havinga
>
>
> --
> Yeb Havinga
> http://www.mgrid.net/
> Mastering Medical Data
>



-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Re: [v9.2] Add GUC sepgsql.client_label

Yeb Havinga <yebhavinga@gmail.com> — 2012-03-09T20:09:43Z

On 2012-03-06 15:14, Kohei KaiGai wrote:
> In case of sepgsql_setcon() being invoked with null argument
> to reset security label of the client, but not committed yet,
> the last item of the client_label_pending has null label.
> (It performs as a mark of a security label being reset.)
Yes, I see that now. Another solution could be to append 
client_label_peer on the pending list instead of NULL, but maybe this is 
not important enough to discuss. I tried to do some testing by first 
transition into a smaller MLS context, then reset to the original again, 
but that is not allowed by the regtest policy. I searched the internet 
for 30 minutes about how to write a allow rule that would allow e.g. the 
transition from c1.c4 back to c1.c1023 but failed.

two other minor nitpicks

-- * contrib/sepgsql/label.c
-+ * contrib/sepgsqlet/label.c

typo here..

-+      /* Append the supplied new_label on the pending list. */
++      /*
++       * Append the supplied new_label on the pending list until
++       * the current transaction is committed.
++       */

the 'until the current transaction is committed' part is something going 
on outside of sepgsql_set_client_label() - this function just appends to 
the list, always. That the list is reset on transaction commit/abort 
time, is done and also already documented elsewhere. A new reader could 
be confused to not find transaction related things in 
sepgsql_set_client_label().

regards,
Yeb

-- 
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

Re: [v9.2] Add GUC sepgsql.client_label

Robert Haas <robertmhaas@gmail.com> — 2012-03-09T20:49:55Z

On Tue, Mar 6, 2012 at 9:14 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> [ new patch ]

Are we absolutely certain that we want the semantics of
sepgsql_setcon() to be transactional?  Because if we made them
non-transactional, this would be a whole lot simpler, and it would
still meet the originally proposed use case, which is to allow the
security context of a connection to be changed on a one-time basis
before handing it off to a client application.

As a separate but related note, the label management here seems to be
excessively complicated.  In particular, it seems to me that the
semantics of sepgsql_get_client_label() become quite muddled under
this patch.  An explicitly-set label overrides the default label, but
a trusted procedure's temporary label overrides that.  So if you enter
a trusted procedure, and it calls sepgsql_setcon(), it'll change the
label, but no actual security transition will occur; then, when you
exit the trusted procedure, you'll get the new label in place of
whatever the caller had before.  That seems like one heck of a
surprising set of semantics.

It seems to me that it would make more sense to view the set of
security labels in effect as a stack.  When we enter a trusted
procedure, it pushes a new label on the stack; when we exit a trusted
procedure, it pops the top label off the stack.  sepgsql_setcon()
changes the top label on the stack.  If we want to retain
transactional semantics, then you can also have a toplevel label that
doesn't participate in the stack; a commit copies the sole item on the
stack into the toplevel label, and transaction start copies the
toplevel label into an empty stack.  In the current coding, I think
client_label_peer is redundant with client_label_committed - once the
latter is set, the former is unused, IIUC - and what I'm proposing is
that client_label_func shouldn't be separate, but rather should mutate
the stack of labels maintained by client_label_pending.

The docs need updating, both to reflect the version bump in
sepgsql-regtest.te and to describe the actual feature.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [v9.2] Add GUC sepgsql.client_label

Yeb Havinga <yebhavinga@gmail.com> — 2012-03-10T09:39:00Z

On 2012-03-09 21:49, Robert Haas wrote:
> On Tue, Mar 6, 2012 at 9:14 AM, Kohei KaiGai<kaigai@kaigai.gr.jp>  wrote:
>> [ new patch ]
> Are we absolutely certain that we want the semantics of
> sepgsql_setcon() to be transactional?  Because if we made them
> non-transactional, this would be a whole lot simpler, and it would
> still meet the originally proposed use case, which is to allow the
> security context of a connection to be changed on a one-time basis
> before handing it off to a client application.

It would meet the original use case, but outside of that use case it 
would be very easy to get POLA violations. Imagine a transaction like
1- do stuff under label A
2- setcon to B
3- do stuff under label B

When that transaction fails due to a serialization error, one would 
expect that when the transaction is replayed, the initial actions are 
executed under label A. If it was B, or any other further label in the 
original transaction, it would be very hard to develop software in user 
space that could cope with this behaviour.
> As a separate but related note, the label management here seems to be
> excessively complicated.  In particular, it seems to me that the
> semantics of sepgsql_get_client_label() become quite muddled under
> this patch.  An explicitly-set label overrides the default label, but
> a trusted procedure's temporary label overrides that.  So if you enter
> a trusted procedure, and it calls sepgsql_setcon(), it'll change the
> label, but no actual security transition will occur; then, when you
> exit the trusted procedure, you'll get the new label in place of
> whatever the caller had before.  That seems like one heck of a
> surprising set of semantics.

I agree that sepgsql_get_*client*_label does not really match with a 
trusted procedure temporarily changing it.
> It seems to me that it would make more sense to view the set of
> security labels in effect as a stack.  When we enter a trusted
> procedure, it pushes a new label on the stack; when we exit a trusted
> procedure, it pops the top label off the stack.  sepgsql_setcon()
> changes the top label on the stack.  If we want to retain
> transactional semantics, then you can also have a toplevel label that
> doesn't participate in the stack; a commit copies the sole item on the
> stack into the toplevel label, and transaction start copies the
> toplevel label into an empty stack.

Yes the additions be sepgsql_setcon look like a stack for pushing. 
However, the current code that rollbacks the pending list to a certain 
savepoint matches code from e.g. StandbyReleaseLocks(), so having a 
concept like this as a list is not unknown to the current codebase.
>    In the current coding, I think
> client_label_peer is redundant with client_label_committed - once the
> latter is set, the former is unused, IIUC

client_label_peer is used on reset of the client label, i.e. calling 
sepgsql_setcon with NULL.

>   - and what I'm proposing is
> that client_label_func shouldn't be separate, but rather should mutate
> the stack of labels maintained by client_label_pending.

The drawback of having trusted procedures push things on this stack is 
that it would add a memory alloc and size overhead when calling trusted 
functions, especially for recursive functions.

> The docs need updating, both to reflect the version bump in
> sepgsql-regtest.te and to describe the actual feature.

I can probably write some docs tomorrow.

regards,
Yeb Havinga

Re: [v9.2] Add GUC sepgsql.client_label

Robert Haas <robertmhaas@gmail.com> — 2012-03-10T13:06:18Z

On Sat, Mar 10, 2012 at 4:39 AM, Yeb Havinga <yebhavinga@gmail.com> wrote:
>> As a separate but related note, the label management here seems to be
>> excessively complicated.  In particular, it seems to me that the
>> semantics of sepgsql_get_client_label() become quite muddled under
>> this patch.  An explicitly-set label overrides the default label, but
>> a trusted procedure's temporary label overrides that.  So if you enter
>> a trusted procedure, and it calls sepgsql_setcon(), it'll change the
>> label, but no actual security transition will occur; then, when you
>> exit the trusted procedure, you'll get the new label in place of
>> whatever the caller had before.  That seems like one heck of a
>> surprising set of semantics.
>
> I agree that sepgsql_get_*client*_label does not really match with a trusted
> procedure temporarily changing it.

I'm not complaining about the name of the function; I'm complaining
about the semantics.

>>   In the current coding, I think
>> client_label_peer is redundant with client_label_committed - once the
>> latter is set, the former is unused, IIUC
>
> client_label_peer is used on reset of the client label, i.e. calling
> sepgsql_setcon with NULL.

Ugh.  What's the point of supporting that?

> The drawback of having trusted procedures push things on this stack is that
> it would add a memory alloc and size overhead when calling trusted
> functions, especially for recursive functions.

Compared to all the other overhead of using sepgsql, that is miniscule
and not worth worrying about.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [v9.2] Add GUC sepgsql.client_label

Yeb Havinga <yebhavinga@gmail.com> — 2012-03-10T21:35:23Z

On 2012-03-10 14:06, Robert Haas wrote:
> On Sat, Mar 10, 2012 at 4:39 AM, Yeb Havinga<yebhavinga@gmail.com>  wrote:
>>> As a separate but related note, the label management here seems to be
>>> excessively complicated.  In particular, it seems to me that the
>>> semantics of sepgsql_get_client_label() become quite muddled under
>>> this patch.  An explicitly-set label overrides the default label, but
>>> a trusted procedure's temporary label overrides that.  So if you enter
>>> a trusted procedure, and it calls sepgsql_setcon(), it'll change the
>>> label, but no actual security transition will occur; then, when you
>>> exit the trusted procedure, you'll get the new label in place of
>>> whatever the caller had before.  That seems like one heck of a
>>> surprising set of semantics.
>> I agree that sepgsql_get_*client*_label does not really match with a trusted
>> procedure temporarily changing it.
> I'm not complaining about the name of the function; I'm complaining
> about the semantics.

The semantics are muddled because the client labels are mixed with 
labels from trusted procedures. The stack you described upthread may 
also exhibit surprising behaviour. If a trusted procedure is called, 
it's label is pushed onto the stack. Suppose it pushes another label by 
calling sepgsql_setcon(). In the stack case, this is now the top of the 
stack and the result of sepgsql_get_client_label(). The procedure 
exists. What should now be the result of sepgsql_get_client_label()? 
Since labels are managed by a stack, we can only pop what's on top and 
need to pop twice, so we've lost the label pushed onto the stack by the 
trusted function, which means that trusted procedures cannot be used to 
change client labels beyond their own scope, but other functions would.

Maybe this semantics muddling of trusted procedures and setting the 
client label can be avoided by denying changing the client label with 
sepgsql_setcon() from a trusted procedure, which would also make some sense:

ERROR: cannot override the client label of a trusted procedure during 
execution.

-- 
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

Re: [v9.2] Add GUC sepgsql.client_label

Yeb Havinga <yebhavinga@gmail.com> — 2012-03-11T10:33:57Z

On 2012-03-10 10:39, I  wrote:
>
> I can probably write some docs tomorrow.
>

Attached is v5 of the patch, with is exactly equal to v4 but with added 
documentation.

Some other notes.

- Robert asked why sepgsql_setcon with NULL to reset the value to the 
initial client label was supported. Maybe this could be a leftover from 
the initial implementation as GUC variable?
- earlier I suggested preventing setting a new client label from a 
trusted procedure, however I just read in the original post that this 
was how the current usecase of Joshua is set up. Suggestion withdrawn.

-- 
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

Re: [v9.2] Add GUC sepgsql.client_label

Yeb Havinga <yebhavinga@gmail.com> — 2012-03-11T10:36:38Z

On 2012-03-11 11:33, Yeb Havinga wrote:
> On 2012-03-10 10:39, I  wrote:
>>
>> I can probably write some docs tomorrow.
>>
>
> Attached is v5 of the patch, with is exactly equal to v4 but with 
> added documentation.

s/<literal>cube(1) == '(1)'</literal>// in that patch please

Re: [v9.2] Add GUC sepgsql.client_label

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-03-12T14:42:24Z

2012/3/9 Robert Haas <robertmhaas@gmail.com>:
> On Tue, Mar 6, 2012 at 9:14 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> [ new patch ]
>
> Are we absolutely certain that we want the semantics of
> sepgsql_setcon() to be transactional?  Because if we made them
> non-transactional, this would be a whole lot simpler, and it would
> still meet the originally proposed use case, which is to allow the
> security context of a connection to be changed on a one-time basis
> before handing it off to a client application.
>
I hesitate to implement sepgsql_setcon() being not-transaction aware,
because any other functionality are all transaction aware, such as
SECURITY LABEL statement.
Even though the original use-case from Joshua does not require it
being transaction-aware, I'd like to keep consistency of behavior
with any other features. Is it really unacceptable complexity?

> It seems to me that it would make more sense to view the set of
> security labels in effect as a stack.  When we enter a trusted
> procedure, it pushes a new label on the stack; when we exit a trusted
> procedure, it pops the top label off the stack.  sepgsql_setcon()
> changes the top label on the stack.  If we want to retain
> transactional semantics, then you can also have a toplevel label that
> doesn't participate in the stack; a commit copies the sole item on the
> stack into the toplevel label, and transaction start copies the
> toplevel label into an empty stack.  In the current coding, I think
> client_label_peer is redundant with client_label_committed - once the
> latter is set, the former is unused, IIUC - and what I'm proposing is
> that client_label_func shouldn't be separate, but rather should mutate
> the stack of labels maintained by client_label_pending.
>
I almost agree with your opinion. A semantics to stack security label
will be more straight-forward.

One reason of the redundant client_label_peer is to support reset
client label using NULL-input on sepgsql_setcon(). We need to save
the original value somewhere.

In case of sepgsql_setcon() being invoked inside of the trusted-
procedure, indeed, the existing behavior is confusing as you pointed
out. If we would changed the semantics using a stack structure,
an invocation of trusted procedure takes a label transition from A to B,
then it invokes sepgsql_setcon() to take a label transition from B to C,
and the label "B" shall be popped from the stack at end of the trusted
procedure. Eventually, the label "C" shall be applied on top of the stack.
Do we share same image to whole of the idea?

> The docs need updating, both to reflect the version bump in
> sepgsql-regtest.te and to describe the actual feature.
>
OK, please wait for a few days.

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Re: [v9.2] Add GUC sepgsql.client_label

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-03-12T14:58:03Z

2012/3/11 Yeb Havinga <yebhavinga@gmail.com>:
> On 2012-03-10 10:39, I  wrote:
>
>>
>> I can probably write some docs tomorrow.
>>
>
> Attached is v5 of the patch, with is exactly equal to v4 but with added
> documentation.
>
Thanks for your dedicated volunteer. I'm under checking of the updates
at documentation.

> Some other notes.
>
> - Robert asked why sepgsql_setcon with NULL to reset the value to the
> initial client label was supported. Maybe this could be a leftover from the
> initial implementation as GUC variable?
>
It is a practical reason. In case when httpd open the connection to PG and
set a suitable security label according to the given credential prior to launch
of user application, then keep this connection for upcoming request, it is
worthwhile to reset security label of the client.

> - earlier I suggested preventing setting a new client label from a trusted
> procedure, however I just read in the original post that this was how the
> current usecase of Joshua is set up. Suggestion withdrawn.
>
In the scenario of Joshua's security policy, it does not allow httpd to issue
SQL commands except for the trusted procedure that calls sepgsql_setcon()
according to the given credential. (Thus, we have no way to set an arbitrary
security label without credentials.) The security label being switched is
allowed to issue SQL commands with restricted privileges, and also allows
to translate into httpd's domain.
If we would not support sepgsql_setcon() in trusted procedure, we have to
allow httpd to translate an arbitrary label thus it also disallow to keep
connection because it should not revert the client label to httpd (it means
"restricted" users enable to switch someone arbitrary!)

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Re: [v9.2] Add GUC sepgsql.client_label

Robert Haas <robertmhaas@gmail.com> — 2012-03-12T14:59:39Z

On Sat, Mar 10, 2012 at 4:35 PM, Yeb Havinga <yebhavinga@gmail.com> wrote:
> The semantics are muddled because the client labels are mixed with labels
> from trusted procedures. The stack you described upthread may also exhibit
> surprising behaviour. If a trusted procedure is called, it's label is pushed
> onto the stack.

What I was proposing is that it would *replace* the label on top of the stack.

> Suppose it pushes another label by calling sepgsql_setcon().
> In the stack case, this is now the top of the stack and the result of
> sepgsql_get_client_label(). The procedure exists. What should now be the
> result of sepgsql_get_client_label()? Since labels are managed by a stack,
> we can only pop what's on top and need to pop twice,

The above avoids the need to pop twice.

> so we've lost the label
> pushed onto the stack by the trusted function, which means that trusted
> procedures cannot be used to change client labels beyond their own scope,
> but other functions would.

That's true, but I'm not convinced it's bad.  I mean, if you instruct
the system to change security labels for the duration of a trusted
procedure, then it shouldn't be surprising that you end up with the
same security label after it exits that you had before entering it.
At the very least, it has the virtue of being consistent with other
things, like how GUCs behave.  The current behavior is that you change
the context and you don't see the results of your own context change,
which seems far worse.

> Maybe this semantics muddling of trusted procedures and setting the client
> label can be avoided by denying changing the client label with
> sepgsql_setcon() from a trusted procedure, which would also make some sense:
>
> ERROR: cannot override the client label of a trusted procedure during
> execution.

That also seems possibly reasonable.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [v9.2] Add GUC sepgsql.client_label

Robert Haas <robertmhaas@gmail.com> — 2012-03-12T15:01:02Z

On Mon, Mar 12, 2012 at 10:58 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> It is a practical reason. In case when httpd open the connection to PG and
> set a suitable security label according to the given credential prior to launch
> of user application, then keep this connection for upcoming request, it is
> worthwhile to reset security label of the client.

But wait a minute - how is that any good?  That allows the client to
pretty trivially circumvent the security restriction that we were
trying to impose by doing sepgsql_setcon() in the first place.  It
doesn't matter how convenient it is if it's flagrantly insecure.

Am I missing something here?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [v9.2] Add GUC sepgsql.client_label

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-03-12T15:13:52Z

2012/3/12 Robert Haas <robertmhaas@gmail.com>:
> On Mon, Mar 12, 2012 at 10:58 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> It is a practical reason. In case when httpd open the connection to PG and
>> set a suitable security label according to the given credential prior to launch
>> of user application, then keep this connection for upcoming request, it is
>> worthwhile to reset security label of the client.
>
> But wait a minute - how is that any good?  That allows the client to
> pretty trivially circumvent the security restriction that we were
> trying to impose by doing sepgsql_setcon() in the first place.  It
> doesn't matter how convenient it is if it's flagrantly insecure.
>
> Am I missing something here?
>
It is a practical reason. If we would not support the reset feature,
the application has to know the security label of itself, as a target
label to be reverted. However, I'm not certain the status of script-
language binding of libselinux feature to obtain the self label,
although it is supported on Perl, Ruby and PHP (with extension
by myself) at least.

It seems to me a reasonable cost to track the original label to
eliminate a restriction of application side that tries to revert
the security label once switched.

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Re: [v9.2] Add GUC sepgsql.client_label

Robert Haas <robertmhaas@gmail.com> — 2012-03-12T15:45:21Z

On Mon, Mar 12, 2012 at 11:13 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> 2012/3/12 Robert Haas <robertmhaas@gmail.com>:
>> On Mon, Mar 12, 2012 at 10:58 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>> It is a practical reason. In case when httpd open the connection to PG and
>>> set a suitable security label according to the given credential prior to launch
>>> of user application, then keep this connection for upcoming request, it is
>>> worthwhile to reset security label of the client.
>>
>> But wait a minute - how is that any good?  That allows the client to
>> pretty trivially circumvent the security restriction that we were
>> trying to impose by doing sepgsql_setcon() in the first place.  It
>> doesn't matter how convenient it is if it's flagrantly insecure.
>>
>> Am I missing something here?
>>
> It is a practical reason. If we would not support the reset feature,
> the application has to know the security label of itself, as a target
> label to be reverted. However, I'm not certain the status of script-
> language binding of libselinux feature to obtain the self label,
> although it is supported on Perl, Ruby and PHP (with extension
> by myself) at least.

You're still missing my point.  The issue isn't the particular choice
of mechanism for reverting to the original security label; it's the
fact that such a thing would be possible at all.

Suppose that the connection starts out in context connection_pooler_t.
 Based on the identity of the user, we transition to foo_t, bar_t, or
baz_t.  If it's possible, by any method, for one of those contexts to
get back to connection_pooler_t, then we've got a problem.  We give a
connection to user foo which is in foo_t; he transitions it back to
connection_pooler_t, then to bar_t, and usurps user bar's privileges.
Unless there's some way to prevent that, the only way to make this
secure is to make the transition to foo_t irreversible.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [v9.2] Add GUC sepgsql.client_label

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-03-12T16:30:00Z

2012/3/12 Robert Haas <robertmhaas@gmail.com>:
> On Mon, Mar 12, 2012 at 11:13 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> 2012/3/12 Robert Haas <robertmhaas@gmail.com>:
>>> On Mon, Mar 12, 2012 at 10:58 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>>> It is a practical reason. In case when httpd open the connection to PG and
>>>> set a suitable security label according to the given credential prior to launch
>>>> of user application, then keep this connection for upcoming request, it is
>>>> worthwhile to reset security label of the client.
>>>
>>> But wait a minute - how is that any good?  That allows the client to
>>> pretty trivially circumvent the security restriction that we were
>>> trying to impose by doing sepgsql_setcon() in the first place.  It
>>> doesn't matter how convenient it is if it's flagrantly insecure.
>>>
>>> Am I missing something here?
>>>
>> It is a practical reason. If we would not support the reset feature,
>> the application has to know the security label of itself, as a target
>> label to be reverted. However, I'm not certain the status of script-
>> language binding of libselinux feature to obtain the self label,
>> although it is supported on Perl, Ruby and PHP (with extension
>> by myself) at least.
>
> You're still missing my point.  The issue isn't the particular choice
> of mechanism for reverting to the original security label; it's the
> fact that such a thing would be possible at all.
>
> Suppose that the connection starts out in context connection_pooler_t.
>  Based on the identity of the user, we transition to foo_t, bar_t, or
> baz_t.  If it's possible, by any method, for one of those contexts to
> get back to connection_pooler_t, then we've got a problem.  We give a
> connection to user foo which is in foo_t; he transitions it back to
> connection_pooler_t, then to bar_t, and usurps user bar's privileges.
> Unless there's some way to prevent that, the only way to make this
> secure is to make the transition to foo_t irreversible.
>
It is the reason why I advocate the idea to allow sepgsql_setcon()
inside of trusted-procedures.

The original use-case of Joshua does not allow connection_pooler_t
to execute any SQL commands except for invocation of a particular
trusted-procedures; that takes a secret credential as an argument,
then it switches the client label to foo_t, bar_t or baz_t according to
the supplied credential.
These labels are allowed to switch back to the original
connection_pooler_t, but it is unavailable to switch arbitrary label
without suitable credential.

Please also note that the reset of security label is handled as
a switch from the current one to the original one; that takes
permission check as normal manner.
So, it is an option to prevent to reset the client label to the original
one; that is allowed to switch arbitrary label, in an environment
without connection pooling.

Do we still have problematic scenario here?

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Re: [v9.2] Add GUC sepgsql.client_label

Robert Haas <robertmhaas@gmail.com> — 2012-03-12T17:39:45Z

On Mon, Mar 12, 2012 at 12:30 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> Suppose that the connection starts out in context connection_pooler_t.
>>  Based on the identity of the user, we transition to foo_t, bar_t, or
>> baz_t.  If it's possible, by any method, for one of those contexts to
>> get back to connection_pooler_t, then we've got a problem.  We give a
>> connection to user foo which is in foo_t; he transitions it back to
>> connection_pooler_t, then to bar_t, and usurps user bar's privileges.
>> Unless there's some way to prevent that, the only way to make this
>> secure is to make the transition to foo_t irreversible.
>>
> It is the reason why I advocate the idea to allow sepgsql_setcon()
> inside of trusted-procedures.
>
> The original use-case of Joshua does not allow connection_pooler_t
> to execute any SQL commands except for invocation of a particular
> trusted-procedures; that takes a secret credential as an argument,
> then it switches the client label to foo_t, bar_t or baz_t according to
> the supplied credential.
> These labels are allowed to switch back to the original
> connection_pooler_t, but it is unavailable to switch arbitrary label
> without suitable credential.

Oh, I get it.

Given that that's the intended use case, the current design does make
sense, but it seems awfully special-purpose.  Not knowing that this is
what you had in mind, I never would have guessed the reason for all
this complexity.  I worry that this is too much of a purpose-built
mechanism, and that nobody will ever be able to use it for much of
anything beyond the extremely specific use case that you've laid out
here.  I think that, at the very least, the comments and documentation
need to make it clear that this is very deliberately intended to
modify only the toplevel security context of the session, which may be
different from the currently active context if a TP is in use; and
also that the change will apply to future transactions only if the
current transaction commits.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [v9.2] Add GUC sepgsql.client_label

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-03-13T14:12:21Z

2012/3/12 Robert Haas <robertmhaas@gmail.com>:
> On Mon, Mar 12, 2012 at 12:30 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>> Suppose that the connection starts out in context connection_pooler_t.
>>>  Based on the identity of the user, we transition to foo_t, bar_t, or
>>> baz_t.  If it's possible, by any method, for one of those contexts to
>>> get back to connection_pooler_t, then we've got a problem.  We give a
>>> connection to user foo which is in foo_t; he transitions it back to
>>> connection_pooler_t, then to bar_t, and usurps user bar's privileges.
>>> Unless there's some way to prevent that, the only way to make this
>>> secure is to make the transition to foo_t irreversible.
>>>
>> It is the reason why I advocate the idea to allow sepgsql_setcon()
>> inside of trusted-procedures.
>>
>> The original use-case of Joshua does not allow connection_pooler_t
>> to execute any SQL commands except for invocation of a particular
>> trusted-procedures; that takes a secret credential as an argument,
>> then it switches the client label to foo_t, bar_t or baz_t according to
>> the supplied credential.
>> These labels are allowed to switch back to the original
>> connection_pooler_t, but it is unavailable to switch arbitrary label
>> without suitable credential.
>
> Oh, I get it.
>
> Given that that's the intended use case, the current design does make
> sense, but it seems awfully special-purpose.  Not knowing that this is
> what you had in mind, I never would have guessed the reason for all
> this complexity.  I worry that this is too much of a purpose-built
> mechanism, and that nobody will ever be able to use it for much of
> anything beyond the extremely specific use case that you've laid out
> here.  I think that, at the very least, the comments and documentation
> need to make it clear that this is very deliberately intended to
> modify only the toplevel security context of the session, which may be
> different from the currently active context if a TP is in use; and
> also that the change will apply to future transactions only if the
> current transaction commits.
>
OK, I try to update the documentation and test cases with related
security policy, rather than the code base itself.

Please wait for a few days to update them.
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Re: [v9.2] Add GUC sepgsql.client_label

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-03-14T15:10:39Z

2012/3/13 Kohei KaiGai <kaigai@kaigai.gr.jp>:
> 2012/3/12 Robert Haas <robertmhaas@gmail.com>:
>> On Mon, Mar 12, 2012 at 12:30 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>>> Suppose that the connection starts out in context connection_pooler_t.
>>>>  Based on the identity of the user, we transition to foo_t, bar_t, or
>>>> baz_t.  If it's possible, by any method, for one of those contexts to
>>>> get back to connection_pooler_t, then we've got a problem.  We give a
>>>> connection to user foo which is in foo_t; he transitions it back to
>>>> connection_pooler_t, then to bar_t, and usurps user bar's privileges.
>>>> Unless there's some way to prevent that, the only way to make this
>>>> secure is to make the transition to foo_t irreversible.
>>>>
>>> It is the reason why I advocate the idea to allow sepgsql_setcon()
>>> inside of trusted-procedures.
>>>
>>> The original use-case of Joshua does not allow connection_pooler_t
>>> to execute any SQL commands except for invocation of a particular
>>> trusted-procedures; that takes a secret credential as an argument,
>>> then it switches the client label to foo_t, bar_t or baz_t according to
>>> the supplied credential.
>>> These labels are allowed to switch back to the original
>>> connection_pooler_t, but it is unavailable to switch arbitrary label
>>> without suitable credential.
>>
>> Oh, I get it.
>>
>> Given that that's the intended use case, the current design does make
>> sense, but it seems awfully special-purpose.  Not knowing that this is
>> what you had in mind, I never would have guessed the reason for all
>> this complexity.  I worry that this is too much of a purpose-built
>> mechanism, and that nobody will ever be able to use it for much of
>> anything beyond the extremely specific use case that you've laid out
>> here.  I think that, at the very least, the comments and documentation
>> need to make it clear that this is very deliberately intended to
>> modify only the toplevel security context of the session, which may be
>> different from the currently active context if a TP is in use; and
>> also that the change will apply to future transactions only if the
>> current transaction commits.
>>
> OK, I try to update the documentation and test cases with related
> security policy, rather than the code base itself.
>
The attached patch contains the documentation updates and test
cases that simulate a typical behavior of connection pooling
software.

In this test case, sepgsql_regtest_pool_t is only allowed to translate
to sepgsql_regtest_(foo|var)_t via trusted procedure, and these
domains are unavailable to reference the tables related to other
domains. It is according to the original explanation I got from Joshua
Brindle. In actual cases, the trusted procedure will take an argument
of the credential being stored within CAC card.

I didn't touch the code portion from the previous version.

If it is ready to commit, please remember the credit to Yeb's volunteer
on this patch.

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Re: [v9.2] Add GUC sepgsql.client_label

Robert Haas <robertmhaas@gmail.com> — 2012-03-15T20:45:58Z

On Wed, Mar 14, 2012 at 11:10 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> If it is ready to commit, please remember the credit to Yeb's volunteer
> on this patch.

Done.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [v9.2] Add GUC sepgsql.client_label

Yeb Havinga <yebhavinga@gmail.com> — 2012-03-16T07:44:17Z

On 2012-03-15 21:45, Robert Haas wrote:
> On Wed, Mar 14, 2012 at 11:10 AM, Kohei KaiGai<kaigai@kaigai.gr.jp>  wrote:
>> If it is ready to commit, please remember the credit to Yeb's volunteer
>> on this patch.
> Done.
>
In the patch with copy-editing documentation following that commit, at 
"in at their option", s/in// ? Also 'rather than .. as mandated by the 
system': I'm having trouble parsing 'as'. It is also unclear to me what 
'system' means: selinux or PostgreSQL, or both? I suspect it is 
PostgreSQL, since selinux is still enforcing / 'mandating' it's policy. 
What about "rather than that the switch is controlled by the PostgreSQL 
server, as in the case of a trusted procedure."

+    Dynamic domain transitions should be considered carefully, because they
+    allow users to switch their label, and therefore their privileges, in
+    at their option, rather than (as in the case of a trusted procedure)
+    as mandated by the system.

-- 
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

Re: [v9.2] Add GUC sepgsql.client_label

Robert Haas <robertmhaas@gmail.com> — 2012-03-20T19:15:46Z

On Fri, Mar 16, 2012 at 3:44 AM, Yeb Havinga <yebhavinga@gmail.com> wrote:
> In the patch with copy-editing documentation following that commit, at "in
> at their option", s/in// ?

Oh, yeah.  Oops.  Thanks.

> Also 'rather than .. as mandated by the system':
> I'm having trouble parsing 'as'. It is also unclear to me what 'system'
> means: selinux or PostgreSQL, or both? I suspect it is PostgreSQL, since
> selinux is still enforcing / 'mandating' it's policy. What about "rather
> than that the switch is controlled by the PostgreSQL server, as in the case
> of a trusted procedure."

Well, I think it's both.  PostgreSQL is responsible for enforcing
privileges on database objects, but it relies on SE-Linux to tell it
whether a given access is allowable.  So, from PostgreSQL's point of
view, it's delegating the decision to SE-Linux.  But SE-Linux views
itself as a mechanism for enforcing a system-wide security policy, so
views PostgreSQL as an instrument for carrying out its access control
goals.  I don't know how to disentangle that.  I'm actually not
entirely sure that I even believe the underlying sentiment that
dynamic transitions are dangerous.  Maybe KaiGai could comment further
on what we should be trying to convey here.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [v9.2] Add GUC sepgsql.client_label

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-03-21T10:07:24Z

2012/3/20 Robert Haas <robertmhaas@gmail.com>:
> On Fri, Mar 16, 2012 at 3:44 AM, Yeb Havinga <yebhavinga@gmail.com> wrote:
>> In the patch with copy-editing documentation following that commit, at "in
>> at their option", s/in// ?
>
> Oh, yeah.  Oops.  Thanks.
>
>> Also 'rather than .. as mandated by the system':
>> I'm having trouble parsing 'as'. It is also unclear to me what 'system'
>> means: selinux or PostgreSQL, or both? I suspect it is PostgreSQL, since
>> selinux is still enforcing / 'mandating' it's policy. What about "rather
>> than that the switch is controlled by the PostgreSQL server, as in the case
>> of a trusted procedure."
>
> Well, I think it's both.  PostgreSQL is responsible for enforcing
> privileges on database objects, but it relies on SE-Linux to tell it
> whether a given access is allowable.  So, from PostgreSQL's point of
> view, it's delegating the decision to SE-Linux.  But SE-Linux views
> itself as a mechanism for enforcing a system-wide security policy, so
> views PostgreSQL as an instrument for carrying out its access control
> goals.  I don't know how to disentangle that.  I'm actually not
> entirely sure that I even believe the underlying sentiment that
> dynamic transitions are dangerous.  Maybe KaiGai could comment further
> on what we should be trying to convey here.
>
The reason why dynamic domain transition should be configured
carefully is that it partially allows users to switch their own privileges
in discretionary way, unlike trusted procedure.

The original model of selinux on operating system assumes all the
domain transition shall happen on execve(2) time, but it made clear
some sort of application is not happy with traditional fork - exec
lifecycle, such as web server, connection pooling software, or others.

Even as they perform according to the operations from users,
it does not fork - exec itself because of some reason, typically
performance. One point we should focus on is these applications
have relatively trustable portion and untrustable one.

The dynamic domain transition was designed to "restrict" privileges
more than the current one on the trustable portion, prior to launch
untrustable one. So, it never intend to switch client domain with
100% arbitrary. Its bottom line is restricted with the security policy;
that explicitly describes the range of domains being allowed to
translate.

So, we will be able to conclude dynamic domain transition is
harmless as long as it works to reduce privileges; that should
be guaranteed with the security policy.
It also means sepgsql_setcon() is harmless as long as it works
according to the decision of SELinux.

The connection pooling software scenario using trusted procedure
might be a bit confusing. In this case, the client domain is once
switched to the trusted one with mandatory way, then it switches
to more restricted domain in arbitrary way; thus, it is not allowed
to promote its privileges in arbitrary way.
We assume the trusted procedure is a enough small portion to
ensure bug or vulnerability free.

Joshua, please add some comments, if you have.

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Re: [v9.2] Add GUC sepgsql.client_label

Robert Haas <robertmhaas@gmail.com> — 2012-03-21T17:02:18Z

On Wed, Mar 21, 2012 at 6:07 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> The reason why dynamic domain transition should be configured
> carefully is that it partially allows users to switch their own privileges
> in discretionary way, unlike trusted procedure.
>
> The original model of selinux on operating system assumes all the
> domain transition shall happen on execve(2) time, but it made clear
> some sort of application is not happy with traditional fork - exec
> lifecycle, such as web server, connection pooling software, or others.
>
> Even as they perform according to the operations from users,
> it does not fork - exec itself because of some reason, typically
> performance. One point we should focus on is these applications
> have relatively trustable portion and untrustable one.
>
> The dynamic domain transition was designed to "restrict" privileges
> more than the current one on the trustable portion, prior to launch
> untrustable one. So, it never intend to switch client domain with
> 100% arbitrary. Its bottom line is restricted with the security policy;
> that explicitly describes the range of domains being allowed to
> translate.
>
> So, we will be able to conclude dynamic domain transition is
> harmless as long as it works to reduce privileges; that should
> be guaranteed with the security policy.
> It also means sepgsql_setcon() is harmless as long as it works
> according to the decision of SELinux.
>
> The connection pooling software scenario using trusted procedure
> might be a bit confusing. In this case, the client domain is once
> switched to the trusted one with mandatory way, then it switches
> to more restricted domain in arbitrary way; thus, it is not allowed
> to promote its privileges in arbitrary way.
> We assume the trusted procedure is a enough small portion to
> ensure bug or vulnerability free.
>
> Joshua, please add some comments, if you have.

I guess my feeling on this is that the warning in the documentation
isn't really helping anything here.  I mean, we don't need to document
that allowing people to give themselves more privileges is a security
hole; that much is obvious.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company