Re: Label switcher function

The attached patch allows the security label provider to switch
security label of the client during execution of certain functions.
I named it as "label switcher function"; also called as "trusted-
procedure" in SELinux community.

This feature is quite similar idea toward security definer function,
or set-uid program on operating system. It allows label providers
to switch its internal state that holds security label of the
client, then restore it.
If and when a label provider said the function being invoked is
a label-switcher, fmgr_security_definer() traps this invocation
and set some states just before actual invocations.

We added three new hooks for security label provider.
The get_client_label and set_client_label allows the PG core to
save and restore security label of the client; which is mostly
just an internal state of plugin module.
And, the get_switched_label shall return NULL or a valid label
if the supplied function is a label switcher. It also informs
the PG core whether the function is switcher or not.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

2010/11/12 KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>:
> The attached patch allows the security label provider to switch
> security label of the client during execution of certain functions.
> I named it as "label switcher function"; also called as "trusted-
> procedure" in SELinux community.
>
> This feature is quite similar idea toward security definer function,
> or set-uid program on operating system. It allows label providers
> to switch its internal state that holds security label of the
> client, then restore it.
> If and when a label provider said the function being invoked is
> a label-switcher, fmgr_security_definer() traps this invocation
> and set some states just before actual invocations.
>
> We added three new hooks for security label provider.
> The get_client_label and set_client_label allows the PG core to
> save and restore security label of the client; which is mostly
> just an internal state of plugin module.
> And, the get_switched_label shall return NULL or a valid label
> if the supplied function is a label switcher. It also informs
> the PG core whether the function is switcher or not.

I don't see why the plugin needs to expose the label stack to core PG.
If the plugin needs a label stack, it can do that all on its own. I
see that we need the hooks to allow the plugin to selectively disable
inlining and to gain control when function execution starts and ends
(or aborts) but I don't think the exact manipulations that the plugin
chooses to do at that point need to be visible to core PG.

For SE-Linux, how do you intend to determine whether or not the
function is a trusted procedure? Will that be a function of the
security label applied to it?

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

(2010/11/14 11:19), Robert Haas wrote:
> 2010/11/12 KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp>:
>> The attached patch allows the security label provider to switch
>> security label of the client during execution of certain functions.
>> I named it as "label switcher function"; also called as "trusted-
>> procedure" in SELinux community.
>>
>> This feature is quite similar idea toward security definer function,
>> or set-uid program on operating system. It allows label providers
>> to switch its internal state that holds security label of the
>> client, then restore it.
>> If and when a label provider said the function being invoked is
>> a label-switcher, fmgr_security_definer() traps this invocation
>> and set some states just before actual invocations.
>>
>> We added three new hooks for security label provider.
>> The get_client_label and set_client_label allows the PG core to
>> save and restore security label of the client; which is mostly
>> just an internal state of plugin module.
>> And, the get_switched_label shall return NULL or a valid label
>> if the supplied function is a label switcher. It also informs
>> the PG core whether the function is switcher or not.
>
> I don't see why the plugin needs to expose the label stack to core PG.
> If the plugin needs a label stack, it can do that all on its own. I
> see that we need the hooks to allow the plugin to selectively disable
> inlining and to gain control when function execution starts and ends
> (or aborts) but I don't think the exact manipulations that the plugin
> chooses to do at that point need to be visible to core PG.
>
Hmm. I designed this patch according to the implementation of existing
security definer function, but it is not a only design.

Does the "label stack" means that this patch touches xact.c, doesn't it?
Yes, if we have above three hooks around function calls, the core PG
does not need to manage a label stack.

However, I want fmgr_security_definer_cache to have a field to save
private opaque data, because it is not a very-light step to ask SE-Linux
whether the function is trusted-procedure and to allocate a string to
be applied during execution, although switching is a very-light step.
So, I want to compute it at first time of the function calls, like as
security definer function checks syscache at once.

Of course, it is a private opaque data, it will be open for other usage.

> For SE-Linux, how do you intend to determine whether or not the
> function is a trusted procedure? Will that be a function of the
> security label applied to it?
>
When the function being invoked has a special security label with
a "type_transition" rule on the current client's label in the
security policy, SE-Linux decides the function is trusted procedure.

In other words, we can know whether or not the function is a trusted
procedure by asking to the security policy. It is a task of the plugin.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

I revised my patch as I attached.

The hook function is modified and consolidated as follows:

typedef enum FunctionCallEventType
{
FCET_BE_HOOKED,
FCET_PREPARE,
FCET_START,
FCET_END,
FCET_ABORT,
} FunctionCallEventType;

typedef Datum (*function_call_event_type)(Oid functionId,
FunctionCallEventType event,
Datum event_arg);
extern PGDLLIMPORT function_call_event_type function_call_event_hook;

Unlike the subject of this e-mail, now it does not focus on only switching
security labels during execution of a certain functions.
For example, we may use this hook to track certain functions for security
auditing, performance tuning, and others.

In the case of SE-PgSQL, it shall return BoolGetDatum(true), if the target
function is configured as a trusted procedure, then, this invocation will
be hooked by fmgr_security_definer. In the first call, it shall compute
the security context to be assigned during execution on FCET_PREPARE event.
Then, it switches to the computed label on the FCET_START event, and
restore it on the FCET_END or ECET_ABORT event.

I also fixed up regression test, dummy_seclabel module and its
documentation as Robert pointed out in another topic.

Thanks,

(2010/11/14 13:16), KaiGai Kohei wrote:
> (2010/11/14 11:19), Robert Haas wrote:
>> 2010/11/12 KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp>:
>>> The attached patch allows the security label provider to switch
>>> security label of the client during execution of certain functions.
>>> I named it as "label switcher function"; also called as "trusted-
>>> procedure" in SELinux community.
>>>
>>> This feature is quite similar idea toward security definer function,
>>> or set-uid program on operating system. It allows label providers
>>> to switch its internal state that holds security label of the
>>> client, then restore it.
>>> If and when a label provider said the function being invoked is
>>> a label-switcher, fmgr_security_definer() traps this invocation
>>> and set some states just before actual invocations.
>>>
>>> We added three new hooks for security label provider.
>>> The get_client_label and set_client_label allows the PG core to
>>> save and restore security label of the client; which is mostly
>>> just an internal state of plugin module.
>>> And, the get_switched_label shall return NULL or a valid label
>>> if the supplied function is a label switcher. It also informs
>>> the PG core whether the function is switcher or not.
>>
>> I don't see why the plugin needs to expose the label stack to core PG.
>> If the plugin needs a label stack, it can do that all on its own. I
>> see that we need the hooks to allow the plugin to selectively disable
>> inlining and to gain control when function execution starts and ends
>> (or aborts) but I don't think the exact manipulations that the plugin
>> chooses to do at that point need to be visible to core PG.
>>
> Hmm. I designed this patch according to the implementation of existing
> security definer function, but it is not a only design.
>
> Does the "label stack" means that this patch touches xact.c, doesn't it?
> Yes, if we have above three hooks around function calls, the core PG
> does not need to manage a label stack.
>
> However, I want fmgr_security_definer_cache to have a field to save
> private opaque data, because it is not a very-light step to ask SE-Linux
> whether the function is trusted-procedure and to allocate a string to
> be applied during execution, although switching is a very-light step.
> So, I want to compute it at first time of the function calls, like as
> security definer function checks syscache at once.
>
> Of course, it is a private opaque data, it will be open for other usage.
>
>> For SE-Linux, how do you intend to determine whether or not the
>> function is a trusted procedure? Will that be a function of the
>> security label applied to it?
>>
> When the function being invoked has a special security label with
> a "type_transition" rule on the current client's label in the
> security policy, SE-Linux decides the function is trusted procedure.
>
> In other words, we can know whether or not the function is a trusted
> procedure by asking to the security policy. It is a task of the plugin.
>
> Thanks,

--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

2010/11/17 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> I also fixed up regression test, dummy_seclabel module and its
> documentation as Robert pointed out in another topic.

I have committed the documentation portion of this patch with some
editing. I also fixed the markup, which was broken, because you used
_ in several places where it isn't valid.

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

2010/11/17 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> I revised my patch as I attached.
>
> The hook function is modified and consolidated as follows:
>
>  typedef enum FunctionCallEventType
>  {
>     FCET_BE_HOOKED,
>     FCET_PREPARE,
>     FCET_START,
>     FCET_END,
>     FCET_ABORT,
>  } FunctionCallEventType;
>
>  typedef Datum (*function_call_event_type)(Oid functionId,
>                                            FunctionCallEventType event,
>                                            Datum event_arg);
>  extern PGDLLIMPORT function_call_event_type function_call_event_hook;
>
> Unlike the subject of this e-mail, now it does not focus on only switching
> security labels during execution of a certain functions.
> For example, we may use this hook to track certain functions for security
> auditing, performance tuning, and others.
>
> In the case of SE-PgSQL, it shall return BoolGetDatum(true), if the target
> function is configured as a trusted procedure, then, this invocation will
> be hooked by fmgr_security_definer. In the first call, it shall compute
> the security context to be assigned during execution on FCET_PREPARE event.
> Then, it switches to the computed label on the FCET_START event, and
> restore it on the FCET_END or ECET_ABORT event.

This seems like it's a lot simpler than before, which is good. It
looks to me as though there should really be two separate hooks,
though, one for what is now FCET_BE_HOOKED and one for everything
else. For FCET_BE_HOOKED, you want a function that takes an Oid and
returns a bool. For the other event types, the functionId and event
arguments are OK, but I think you should forget about the save_datum
stuff and just always pass fcache->flinfo and &fcache->private. The
plugin can get the effect of save_datum by passing around whatever
state it needs to hold on to using fcache->private. So:

bool (*needs_function_call_hook)(Oid fn_oid);
void (*function_call_hook)(Oid fn_oid, FunctionCallEventType event,
FmgrInfo flinfo, Datum *private);

Another general comment is that you've not done a very complete job
updating the comments; there are several of them in fmgr.c that are no
longer accurate. Also, please zap the unnecessary whitespace changes.

Thanks,

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

(2010/11/18 11:30), Robert Haas wrote:
> 2010/11/17 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> I revised my patch as I attached.
>>
>> The hook function is modified and consolidated as follows:
>>
>> typedef enum FunctionCallEventType
>> {
>> FCET_BE_HOOKED,
>> FCET_PREPARE,
>> FCET_START,
>> FCET_END,
>> FCET_ABORT,
>> } FunctionCallEventType;
>>
>> typedef Datum (*function_call_event_type)(Oid functionId,
>> FunctionCallEventType event,
>> Datum event_arg);
>> extern PGDLLIMPORT function_call_event_type function_call_event_hook;
>>
>> Unlike the subject of this e-mail, now it does not focus on only switching
>> security labels during execution of a certain functions.
>> For example, we may use this hook to track certain functions for security
>> auditing, performance tuning, and others.
>>
>> In the case of SE-PgSQL, it shall return BoolGetDatum(true), if the target
>> function is configured as a trusted procedure, then, this invocation will
>> be hooked by fmgr_security_definer. In the first call, it shall compute
>> the security context to be assigned during execution on FCET_PREPARE event.
>> Then, it switches to the computed label on the FCET_START event, and
>> restore it on the FCET_END or ECET_ABORT event.
>
> This seems like it's a lot simpler than before, which is good. It
> looks to me as though there should really be two separate hooks,
> though, one for what is now FCET_BE_HOOKED and one for everything
> else. For FCET_BE_HOOKED, you want a function that takes an Oid and
> returns a bool. For the other event types, the functionId and event
> arguments are OK, but I think you should forget about the save_datum
> stuff and just always pass fcache->flinfo and&fcache->private. The
> plugin can get the effect of save_datum by passing around whatever
> state it needs to hold on to using fcache->private. So:
>
> bool (*needs_function_call_hook)(Oid fn_oid);
> void (*function_call_hook)(Oid fn_oid, FunctionCallEventType event,
> FmgrInfo flinfo, Datum *private);
>
It seems to me a good idea. The characteristic of FCET_BE_HOOKED event
type was a bit different from other three event types.
Please wait for about a week to revise my patch.

> Another general comment is that you've not done a very complete job
> updating the comments; there are several of them in fmgr.c that are no
> longer accurate. Also, please zap the unnecessary whitespace changes.
>

Indeed, the comment at middle of the fmgr_info_cxt_security() and just
above definition of the fmgr_security_definer() are not correct.
Did you notice anything else?

Thanks,
--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

2010/11/19 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> Indeed, the comment at middle of the fmgr_info_cxt_security() and just
> above definition of the fmgr_security_definer() are not correct.
> Did you notice anything else?

I think I noticed a couple of places, but I didn't write down exactly
which ones. Sorry....

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

The attached patch is a revised one.

It provides two hooks; the one informs core PG whether the supplied
function needs to be hooked, or not. the other is an actual hook on
prepare, start, end and abort of function invocations.

typedef bool (*needs_function_call_type)(Oid fn_oid);

typedef void (*function_call_type)(FunctionCallEventType event,
FmgrInfo *flinfo, Datum *private);

The hook prototype was a bit modified since the suggestion from
Robert. Because FmgrInfo structure contain OID of the function,
it might be redundant to deliver OID of the function individually.

Rest of parts are revised according to the comment.

I also fixed up source code comments which might become incorrect.

Thanks,

(2010/11/18 11:30), Robert Haas wrote:
> 2010/11/17 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> I revised my patch as I attached.
>>
>> The hook function is modified and consolidated as follows:
>>
>> typedef enum FunctionCallEventType
>> {
>> FCET_BE_HOOKED,
>> FCET_PREPARE,
>> FCET_START,
>> FCET_END,
>> FCET_ABORT,
>> } FunctionCallEventType;
>>
>> typedef Datum (*function_call_event_type)(Oid functionId,
>> FunctionCallEventType event,
>> Datum event_arg);
>> extern PGDLLIMPORT function_call_event_type function_call_event_hook;
>>
>> Unlike the subject of this e-mail, now it does not focus on only switching
>> security labels during execution of a certain functions.
>> For example, we may use this hook to track certain functions for security
>> auditing, performance tuning, and others.
>>
>> In the case of SE-PgSQL, it shall return BoolGetDatum(true), if the target
>> function is configured as a trusted procedure, then, this invocation will
>> be hooked by fmgr_security_definer. In the first call, it shall compute
>> the security context to be assigned during execution on FCET_PREPARE event.
>> Then, it switches to the computed label on the FCET_START event, and
>> restore it on the FCET_END or ECET_ABORT event.
>
> This seems like it's a lot simpler than before, which is good. It
> looks to me as though there should really be two separate hooks,
> though, one for what is now FCET_BE_HOOKED and one for everything
> else. For FCET_BE_HOOKED, you want a function that takes an Oid and
> returns a bool. For the other event types, the functionId and event
> arguments are OK, but I think you should forget about the save_datum
> stuff and just always pass fcache->flinfo and&fcache->private. The
> plugin can get the effect of save_datum by passing around whatever
> state it needs to hold on to using fcache->private. So:
>
> bool (*needs_function_call_hook)(Oid fn_oid);
> void (*function_call_hook)(Oid fn_oid, FunctionCallEventType event,
> FmgrInfo flinfo, Datum *private);
>
> Another general comment is that you've not done a very complete job
> updating the comments; there are several of them in fmgr.c that are no
> longer accurate. Also, please zap the unnecessary whitespace changes.
>
> Thanks,
>

--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

2010/11/25 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> The attached patch is a revised one.
>
> It provides two hooks; the one informs core PG whether the supplied
> function needs to be hooked, or not. the other is an actual hook on
> prepare, start, end and abort of function invocations.
>
>  typedef bool (*needs_function_call_type)(Oid fn_oid);
>
>  typedef void (*function_call_type)(FunctionCallEventType event,
>                                     FmgrInfo *flinfo, Datum *private);
>
> The hook prototype was a bit modified since the suggestion from
> Robert. Because FmgrInfo structure contain OID of the function,
> it might be redundant to deliver OID of the function individually.
>
> Rest of parts are revised according to the comment.
>
> I also fixed up source code comments which might become incorrect.

FCET_PREPARE looks completely unnecessary to me. Any necessary
one-time work can easily be done at FCET_START time, assuming that the
private-data field is initialized to (Datum) 0.

I'm fairly certain that the following is not portable:

+ ObjectAddress object = { .classId = ProcedureRelationId,
+ .objectId = fn_oid,
+ .objectSubId = 0 };

I'd suggest renaming needs_function_call_type and function_call_type
to needs_fmgr_hook_type and fmgr_hook_type.

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

Thanks for your reviewing.

The attached patch is a revised version.
I don't have any objections to your comments.

(2010/12/07 4:38), Robert Haas wrote:
> 2010/11/25 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> The attached patch is a revised one.
>>
>> It provides two hooks; the one informs core PG whether the supplied
>> function needs to be hooked, or not. the other is an actual hook on
>> prepare, start, end and abort of function invocations.
>>
>> typedef bool (*needs_function_call_type)(Oid fn_oid);
>>
>> typedef void (*function_call_type)(FunctionCallEventType event,
>> FmgrInfo *flinfo, Datum *private);
>>
>> The hook prototype was a bit modified since the suggestion from
>> Robert. Because FmgrInfo structure contain OID of the function,
>> it might be redundant to deliver OID of the function individually.
>>
>> Rest of parts are revised according to the comment.
>>
>> I also fixed up source code comments which might become incorrect.
>
> FCET_PREPARE looks completely unnecessary to me. Any necessary
> one-time work can easily be done at FCET_START time, assuming that the
> private-data field is initialized to (Datum) 0.
>
It seems to me a reasonable assumption.
I revised the code, and modified source code comments to ensure
the private shall be initialized to (Datum) 0.

> I'm fairly certain that the following is not portable:
>
> + ObjectAddress object = { .classId = ProcedureRelationId,
> + .objectId = fn_oid,
> + .objectSubId = 0 };
>
Fixed.

> I'd suggest renaming needs_function_call_type and function_call_type
> to needs_fmgr_hook_type and fmgr_hook_type.
>
I also think the suggested names are better than before.

According to the renaming, FunctionCallEventType was also renamed to
FmgrHookEventType. Perhaps, it is a reasonable change.

Thanks,
--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

2010/12/7 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> Thanks for your reviewing.
>
> The attached patch is a revised version.
> I don't have any objections to your comments.

This failed to update the security_label docs, but I don't think it's
a requirement that a hook have regression testing the way we require
for an SQL statement, so I just committed this without that part.

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

On Mon, Dec 13, 2010 at 7:17 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> 2010/12/7 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> Thanks for your reviewing.
>>
>> The attached patch is a revised version.
>> I don't have any objections to your comments.
>
> This failed to update the security_label docs, but I don't think it's
> a requirement that a hook have regression testing the way we require
> for an SQL statement, so I just committed this without that part.

Err, dummy_seclabel, not security_label.

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