Thread

1. Label switcher function

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2010-11-13T03:36:58Z

   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>
   

2. Re: Label switcher function

   Robert Haas <robertmhaas@gmail.com> — 2010-11-14T02:19:46Z

   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.
   
   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
   
   

3. Re: Label switcher function

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2010-11-14T04:16:30Z

   (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>
   
   

4. Re: Label switcher function

   Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-11-17T07:15:37Z

   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>
   

5. Re: Label switcher function

   Robert Haas <robertmhaas@gmail.com> — 2010-11-18T01:53:17Z

   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
   
   

6. Re: Label switcher function

   Robert Haas <robertmhaas@gmail.com> — 2010-11-18T02:30:56Z

   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
   
   

7. Re: Label switcher function

   Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-11-19T08:33:32Z

   (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>
   
   

8. Re: Label switcher function

   Robert Haas <robertmhaas@gmail.com> — 2010-11-19T14:50:26Z

   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
   
   

9. Re: Label switcher function

   Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-11-25T05:19:17Z

   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>
   

10. Re: Label switcher function

    Robert Haas <robertmhaas@gmail.com> — 2010-12-06T19:38:22Z

    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
    
    

11. Re: Label switcher function

    Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-12-07T09:14:08Z

    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>
    

12. Re: Label switcher function

    Robert Haas <robertmhaas@gmail.com> — 2010-12-14T00:17:41Z

    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
    
    

13. Re: Label switcher function

    Robert Haas <robertmhaas@gmail.com> — 2010-12-14T00:18:34Z

    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