Обсуждение: 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@kaigai.gr.jp>
Вложения
2010/11/12 KaiGai Kohei <kaigai@kaigai.gr.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 allon 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@kaigai.gr.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@kaigai.gr.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@kaigai.gr.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@ak.jp.nec.com>
Вложения
2010/11/17 KaiGai Kohei <kaigai@ak.jp.nec.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@ak.jp.nec.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@ak.jp.nec.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@ak.jp.nec.com>
2010/11/19 KaiGai Kohei <kaigai@ak.jp.nec.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@ak.jp.nec.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@ak.jp.nec.com>
Вложения
2010/11/25 KaiGai Kohei <kaigai@ak.jp.nec.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@ak.jp.nec.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@ak.jp.nec.com>
Вложения
2010/12/7 KaiGai Kohei <kaigai@ak.jp.nec.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@gmail.com> wrote: > 2010/12/7 KaiGai Kohei <kaigai@ak.jp.nec.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