Re: [RFC 01/10] selinux: introduce a selinux namespace
On Thu, Feb 8, 2018 at 10:02 AM, Stephen Smalley wrote:
> On Wed, 2018-02-07 at 14:56 -0500, Paul Moore wrote:
>> On Wed, Feb 7, 2018 at 12:48 PM, Stephen Smalley
>> wrote:
>> > On Tue, 2018-02-06 at 17:18 -0500, Paul Moore wrote:
>>
>> ...
>>
>> > > While I don't think we need to tackle this as part of the
>> > > encapsulation work, this is another reminder that we should look
>> > > into
>> > > breaking the separation between the security server and the
>> > > Linux/hooks code. I understand there were historical reasons for
>> > > this
>> > > split, but I think all of those reasons are now gone, and further
>> > > I
>> > > think enough Linux'isms have crept into the security server that
>> > > the
>> > > separation is no longer as meaningful as it may have been in the
>> > > past.
>> >
>> > I think we want to retain some degree of encapsulation of the
>> > policy
>> > logic and data structures, which is what the security server is
>> > supposed to provide. That allows us to evolve that logic and
>> > structures without impacting the object label management and
>> > permission
>> > enforcement code. Separation of policy from mechanism. I don't
>> > mind
>> > nativizing the security server for Linux, and where appropriate,
>> > allowing some optimization of the interfaces between it and the
>> > rest of
>> > the SELinux code, but I wouldn't want to e.g. directly expose the
>> > policydb to the rest of the code. There has been some leakage of
>> > policy awareness outside the security server in the past but I view
>> > that as a mistake that ought to be corrected over time.
>>
>> I agree that a level of abstraction between the policydb code and the
>> enforcement code is a good thing, but I think there are some
>> boundaries between the hook code and the security server that we
>> could
>> do without. Once again, not really part of this work, just popped up
>> in my head again while looking at these patches.
>
> I wanted to clarify that point because it motivates keeping selinux_ss
> as a separate struct from selinux_state (previously selinux_ns), and
> not exposing the selinux_ss struct definition outside of the security
> server. So the selinux_state struct would still be something like:
> struct selinux_state {
> bool disabled;
> #ifdef CONFIG_SECURITY_SELINUX_DEVELOP
> bool enforcing;
> #endif
> bool checkreqprot;
> bool initialized;
> bool policycap[__POLICYDB_CAPABILITY_MAX];
> struct selinux_avc *avc;
> struct selinux_ss *ss;
> }
>
> (dropping the reference count, work struct, and parent pointers that
> were part of selinux_ns as being specific to the namespace work).
I think keeping the selinux_avc and selinux_ss structs separate is
fine right now, as I mentioned earlier, I really don't want to
entangle the encapsulation work with any potential hooks/ss
consolidation work.
Directly embedding the selinux_avc and selinux_ss structures inside
selinux_state would be nice as we could just do one allocation, but
that isn't worth breaking the abstraction at this point. Maybe we
will get there at some point, but I think it's important to stay
focused on the task at hand.
> hooks.c would declare struct selinux_state selinux_state;.
> ss/services.c would declare static struct selinux_ss selinux_ss; and
> provide a selinux_ss_init() function (instead of selinux_ss_create)
> that would set the ->ss field to &selinux_ss. Likewise for avc.c and
> the selinux_avc. hooks.c:selinux_init() would call
> selinux_ss_init(&selinux_state.ss) and
> selinux_avc_init(&selinux_state.avc) to set those pointers to the
> appropriate structures private to the ss and avc code. The _create and
> _free functions would go away and none of the structures would be
> dynamically allocated/freed.
>
> This is in contrast to exposing the selinux_ss struct definition
> outside the security server and directly embedding it in the
> selinux_state, since that would require exposing the policydb and
> sidtab structures as well (unless we were to make those opaque pointers
> within the selinux_ss; currently the policydb and sidtab are directly
> embedded within it). Likewise for the selinux_avc and its embedded
> avc_cache structure.
>
> Trying to make sure we're in agreement on the data structures before I
> rewrite since I don't want to have to rewrite it twice ;)
--
paul moore
www.paul-moore.com
Re: [RFC 01/10] selinux: introduce a selinux namespace
On Wed, 2018-02-07 at 14:56 -0500, Paul Moore wrote:
> On Wed, Feb 7, 2018 at 12:48 PM, Stephen Smalley
> wrote:
> > On Tue, 2018-02-06 at 17:18 -0500, Paul Moore wrote:
>
> ...
>
> > > While I don't think we need to tackle this as part of the
> > > encapsulation work, this is another reminder that we should look
> > > into
> > > breaking the separation between the security server and the
> > > Linux/hooks code. I understand there were historical reasons for
> > > this
> > > split, but I think all of those reasons are now gone, and further
> > > I
> > > think enough Linux'isms have crept into the security server that
> > > the
> > > separation is no longer as meaningful as it may have been in the
> > > past.
> >
> > I think we want to retain some degree of encapsulation of the
> > policy
> > logic and data structures, which is what the security server is
> > supposed to provide. That allows us to evolve that logic and
> > structures without impacting the object label management and
> > permission
> > enforcement code. Separation of policy from mechanism. I don't
> > mind
> > nativizing the security server for Linux, and where appropriate,
> > allowing some optimization of the interfaces between it and the
> > rest of
> > the SELinux code, but I wouldn't want to e.g. directly expose the
> > policydb to the rest of the code. There has been some leakage of
> > policy awareness outside the security server in the past but I view
> > that as a mistake that ought to be corrected over time.
>
> I agree that a level of abstraction between the policydb code and the
> enforcement code is a good thing, but I think there are some
> boundaries between the hook code and the security server that we
> could
> do without. Once again, not really part of this work, just popped up
> in my head again while looking at these patches.
I wanted to clarify that point because it motivates keeping selinux_ss
as a separate struct from selinux_state (previously selinux_ns), and
not exposing the selinux_ss struct definition outside of the security
server. So the selinux_state struct would still be something like:
struct selinux_state {
bool disabled;
#ifdef CONFIG_SECURITY_SELINUX_DEVELOP
bool enforcing;
#endif
bool checkreqprot;
bool initialized;
bool policycap[__POLICYDB_CAPABILITY_MAX];
struct selinux_avc *avc;
struct selinux_ss *ss;
}
(dropping the reference count, work struct, and parent pointers that
were part of selinux_ns as being specific to the namespace work).
hooks.c would declare struct selinux_state selinux_state;.
ss/services.c would declare static struct selinux_ss selinux_ss; and
provide a selinux_ss_init() function (instead of selinux_ss_create)
that would set the ->ss field to &selinux_ss. Likewise for avc.c and
the selinux_avc. hooks.c:selinux_init() would call
selinux_ss_init(&selinux_state.ss) and
selinux_avc_init(&selinux_state.avc) to set those pointers to the
appropriate structures private to the ss and avc code. The _create and
_free functions would go away and none of the structures would be
dynamically allocated/freed.
This is in contrast to exposing the selinux_ss struct definition
outside the security server and directly embedding it in the
selinux_state, since that would require exposing the policydb and
sidtab structures as well (unless we were to make those opaque pointers
within the selinux_ss; currently the policydb and sidtab are directly
embedded within it). Likewise for the selinux_avc and its embedded
avc_cache structure.
Trying to make sure we're in agreement on the data structures before I
rewrite since I don't want to have to rewrite it twice ;)
Re: [RFC 01/10] selinux: introduce a selinux namespace
On Wed, Feb 7, 2018 at 12:48 PM, Stephen Smalley wrote: > On Tue, 2018-02-06 at 17:18 -0500, Paul Moore wrote: ... >> While I don't think we need to tackle this as part of the >> encapsulation work, this is another reminder that we should look into >> breaking the separation between the security server and the >> Linux/hooks code. I understand there were historical reasons for >> this >> split, but I think all of those reasons are now gone, and further I >> think enough Linux'isms have crept into the security server that the >> separation is no longer as meaningful as it may have been in the >> past. > > I think we want to retain some degree of encapsulation of the policy > logic and data structures, which is what the security server is > supposed to provide. That allows us to evolve that logic and > structures without impacting the object label management and permission > enforcement code. Separation of policy from mechanism. I don't mind > nativizing the security server for Linux, and where appropriate, > allowing some optimization of the interfaces between it and the rest of > the SELinux code, but I wouldn't want to e.g. directly expose the > policydb to the rest of the code. There has been some leakage of > policy awareness outside the security server in the past but I view > that as a mistake that ought to be corrected over time. I agree that a level of abstraction between the policydb code and the enforcement code is a good thing, but I think there are some boundaries between the hook code and the security server that we could do without. Once again, not really part of this work, just popped up in my head again while looking at these patches. -- paul moore www.paul-moore.com
Re: [RFC 01/10] selinux: introduce a selinux namespace
On Tue, 2018-02-06 at 17:18 -0500, Paul Moore wrote:
> On Mon, Oct 2, 2017 at 11:58 AM, Stephen Smalley
> wrote:
> > Define a selinux namespace structure (struct selinux_ns)
> > for SELinux state and pass it explicitly to all security server
> > functions. The public portion of the structure contains state
> > that is used throughout the SELinux code, such as the enforcing
> > mode.
> > The structure also contains a pointer to a selinux_ss structure
> > whose
> > definition is private to the security server and contains security
> > server specific state such as the policy database and SID table.
> >
> > This change allocates a single selinux namespace, the
> > init_selinux_ns.
> > It defines and passes a symbol for the current selinux namespace
> > (current_selinux_ns) as a placeholder for future changes where
> > multiple selinux namespaces will be supported, but in this change
> > the current selinux namespace is always the init selinux namespace.
> > Note that passing the current selinux namespace is not correct for
> > all hooks; some hooks will need to be adjusted to pass the selinux
> > namespace associated with an open file, a network namespace or
> > socket,
> > etc, since not all hooks are invoked in process context and some
> > hooks operate in the context of a cred that may differ from
> > current's
> > cred. Fixing all of these cases is left to future changes, once
> > we introduce the support for multiple selinux namespaces.
> >
> > This change by itself should have no effect on SELinux behavior or
> > APIs (userspace or LSM). It merely wraps SELinux state and passes
> > it
> > explicitly as needed.
>
> To put things in context, I'm looking at this patch not really with
> namespacing in mind, but rather with the idea of encapsulating the
> SELinux global state. Regardless of what we do with namespacing, I
> believe the encapsulation work still has value.
>
> My comments are inline below. I'm going to try and trim out a lot of
> this patch in my reply as most of are trivial changes needed to pass
> around the new state pointers.
>
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index f5d3047..9eb48a1 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -97,20 +97,24 @@
> > #include "audit.h"
> > #include "avc_ss.h"
> >
> > +struct selinux_ns *init_selinux_ns;
>
> This comment applies to more than just the particular line above, it
> really applies to the entire patch.
>
> If we are going to merge this patch, and presumably a few others,
> independent of the namespacing work, I would strongly prefer if we
> used more general names instead of ones tied so closely to
> namespacing. For example, something along the lines of "struct
> selinux_state" would be more desirable than "struct selinux_ns"; it
> doesn't have to be "selinux_state", that was just the first thing I
> could come up with.
>
> > +static void selinux_ns_free(struct work_struct *work);
> > +
> > +int selinux_ns_create(struct selinux_ns *parent, struct selinux_ns
> > **ns)
> > +{
> > + struct selinux_ns *newns;
> > + int rc;
> > +
> > + newns = kzalloc(sizeof(*newns), GFP_KERNEL);
> > + if (!newns)
> > + return -ENOMEM;
> > +
> > + refcount_set(&newns->count, 1);
> > + INIT_WORK(&newns->work, selinux_ns_free);
> > +
> > + rc = selinux_ss_create(&newns->ss);
> > + if (rc)
> > + goto err;
> > +
> > + if (parent)
> > + newns->parent = get_selinux_ns(parent);
> > +
> > + *ns = newns;
> > + return 0;
> > +err:
> > + kfree(newns);
> > + return rc;
> > +}
> > +
> > +static void selinux_ns_free(struct work_struct *work)
> > +{
> > + struct selinux_ns *parent, *ns =
> > + container_of(work, struct selinux_ns, work);
> > +
> > + do {
> > + parent = ns->parent;
> > + selinux_ss_free(ns->ss);
> > + kfree(ns);
> > + ns = parent;
> > + } while (ns && refcount_dec_and_test(&ns->count));
> > +}
> > +
> > +void __put_selinux_ns(struct selinux_ns *ns)
> > +{
> > + schedule_work(&ns->work);
> > +}
>
> While we would obviously need something like this for proper
> namespacing, with only a single state struct we don't need all the
> state/namespace management.
>
> However, I would still be willing to accept all the changes to pass
> around a reference to the global state struct, even if in this
> particular case it was always going to be single/init struct.
>
> > @@ -6487,6 +6585,11 @@ static __init int selinux_init(void)
> >
> > printk(KERN_INFO "SELinux: Initializing.\n");
> >
> > + if (selinux_ns_create(NULL, &init_selinux_ns))
> > + panic("SELinux: Could not create initial
> > namespace\n");
>
> Not yet :) If we stick with selinux_ns_create(), see my comments
> above, at the very least we need to pick an error message that
> doesn't
Re: [RFC 01/10] selinux: introduce a selinux namespace
On Tue, Feb 6, 2018 at 5:18 PM, Paul Moore wrote:
> On Mon, Oct 2, 2017 at 11:58 AM, Stephen Smalley wrote:
>> Define a selinux namespace structure (struct selinux_ns)
>> for SELinux state and pass it explicitly to all security server
>> functions. The public portion of the structure contains state
>> that is used throughout the SELinux code, such as the enforcing mode.
>> The structure also contains a pointer to a selinux_ss structure whose
>> definition is private to the security server and contains security
>> server specific state such as the policy database and SID table.
>>
>> This change allocates a single selinux namespace, the init_selinux_ns.
>> It defines and passes a symbol for the current selinux namespace
>> (current_selinux_ns) as a placeholder for future changes where
>> multiple selinux namespaces will be supported, but in this change
>> the current selinux namespace is always the init selinux namespace.
>> Note that passing the current selinux namespace is not correct for
>> all hooks; some hooks will need to be adjusted to pass the selinux
>> namespace associated with an open file, a network namespace or socket,
>> etc, since not all hooks are invoked in process context and some
>> hooks operate in the context of a cred that may differ from current's
>> cred. Fixing all of these cases is left to future changes, once
>> we introduce the support for multiple selinux namespaces.
>>
>> This change by itself should have no effect on SELinux behavior or
>> APIs (userspace or LSM). It merely wraps SELinux state and passes it
>> explicitly as needed.
>
> To put things in context, I'm looking at this patch not really with
> namespacing in mind, but rather with the idea of encapsulating the
> SELinux global state. Regardless of what we do with namespacing, I
> believe the encapsulation work still has value.
With the above comment in mind, I've started looking at the remaining
patches in the patchset to see what might be worth merging,
independent of the greater namespacing effort.
* 02/10
My comments are very similar to 01/10; I think the encapsulation is a
good thing, although perhaps not as important as the changes in
01/100, but there are some namespace specific things that should
probably be dropped.
* 03/10
Once again, similar comments to the previous two patches, although
this patch is perhaps the least "namespace-y" of the three. While I
agree with James comments regarding namespaced stats, that isn't a
major concern at this point.
* {04..10}/10
Changes specific to namespacing, not generally useful in other contexts.
> My comments are inline below. I'm going to try and trim out a lot of
> this patch in my reply as most of are trivial changes needed to pass
> around the new state pointers.
>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index f5d3047..9eb48a1 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -97,20 +97,24 @@
>> #include "audit.h"
>> #include "avc_ss.h"
>>
>> +struct selinux_ns *init_selinux_ns;
>
> This comment applies to more than just the particular line above, it
> really applies to the entire patch.
>
> If we are going to merge this patch, and presumably a few others,
> independent of the namespacing work, I would strongly prefer if we
> used more general names instead of ones tied so closely to
> namespacing. For example, something along the lines of "struct
> selinux_state" would be more desirable than "struct selinux_ns"; it
> doesn't have to be "selinux_state", that was just the first thing I
> could come up with.
>
>> +static void selinux_ns_free(struct work_struct *work);
>> +
>> +int selinux_ns_create(struct selinux_ns *parent, struct selinux_ns **ns)
>> +{
>> + struct selinux_ns *newns;
>> + int rc;
>> +
>> + newns = kzalloc(sizeof(*newns), GFP_KERNEL);
>> + if (!newns)
>> + return -ENOMEM;
>> +
>> + refcount_set(&newns->count, 1);
>> + INIT_WORK(&newns->work, selinux_ns_free);
>> +
>> + rc = selinux_ss_create(&newns->ss);
>> + if (rc)
>> + goto err;
>> +
>> + if (parent)
>> + newns->parent = get_selinux_ns(parent);
>> +
>> + *ns = newns;
>> + return 0;
>> +err:
>> + kfree(newns);
>> + return rc;
>> +}
>> +
>> +static void selinux_ns_free(struct work_struct *work)
>> +{
>> + struct selinux_ns *parent, *ns =
>> + container_of(work, struct selinux_ns, work);
>> +
>> + do {
>> + parent = ns->parent;
>> + selinux_ss_free(ns->ss);
>> + kfree(ns);
>> + ns = parent;
>> + } while (ns && refcount_dec_and_test(&ns->count));
>> +}
>> +
>> +void __put_selinux_ns(struct selinux_ns *ns)
>> +{
>> + schedule_work(&ns->work);
>> +}
>
> While we would obviously need something like this for proper
> namespacing, with only a single state struct we don't need all the
> state/namespace management.
>
> Ho
Re: [RFC 01/10] selinux: introduce a selinux namespace
On Mon, Oct 2, 2017 at 11:58 AM, Stephen Smalley wrote:
> Define a selinux namespace structure (struct selinux_ns)
> for SELinux state and pass it explicitly to all security server
> functions. The public portion of the structure contains state
> that is used throughout the SELinux code, such as the enforcing mode.
> The structure also contains a pointer to a selinux_ss structure whose
> definition is private to the security server and contains security
> server specific state such as the policy database and SID table.
>
> This change allocates a single selinux namespace, the init_selinux_ns.
> It defines and passes a symbol for the current selinux namespace
> (current_selinux_ns) as a placeholder for future changes where
> multiple selinux namespaces will be supported, but in this change
> the current selinux namespace is always the init selinux namespace.
> Note that passing the current selinux namespace is not correct for
> all hooks; some hooks will need to be adjusted to pass the selinux
> namespace associated with an open file, a network namespace or socket,
> etc, since not all hooks are invoked in process context and some
> hooks operate in the context of a cred that may differ from current's
> cred. Fixing all of these cases is left to future changes, once
> we introduce the support for multiple selinux namespaces.
>
> This change by itself should have no effect on SELinux behavior or
> APIs (userspace or LSM). It merely wraps SELinux state and passes it
> explicitly as needed.
To put things in context, I'm looking at this patch not really with
namespacing in mind, but rather with the idea of encapsulating the
SELinux global state. Regardless of what we do with namespacing, I
believe the encapsulation work still has value.
My comments are inline below. I'm going to try and trim out a lot of
this patch in my reply as most of are trivial changes needed to pass
around the new state pointers.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f5d3047..9eb48a1 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -97,20 +97,24 @@
> #include "audit.h"
> #include "avc_ss.h"
>
> +struct selinux_ns *init_selinux_ns;
This comment applies to more than just the particular line above, it
really applies to the entire patch.
If we are going to merge this patch, and presumably a few others,
independent of the namespacing work, I would strongly prefer if we
used more general names instead of ones tied so closely to
namespacing. For example, something along the lines of "struct
selinux_state" would be more desirable than "struct selinux_ns"; it
doesn't have to be "selinux_state", that was just the first thing I
could come up with.
> +static void selinux_ns_free(struct work_struct *work);
> +
> +int selinux_ns_create(struct selinux_ns *parent, struct selinux_ns **ns)
> +{
> + struct selinux_ns *newns;
> + int rc;
> +
> + newns = kzalloc(sizeof(*newns), GFP_KERNEL);
> + if (!newns)
> + return -ENOMEM;
> +
> + refcount_set(&newns->count, 1);
> + INIT_WORK(&newns->work, selinux_ns_free);
> +
> + rc = selinux_ss_create(&newns->ss);
> + if (rc)
> + goto err;
> +
> + if (parent)
> + newns->parent = get_selinux_ns(parent);
> +
> + *ns = newns;
> + return 0;
> +err:
> + kfree(newns);
> + return rc;
> +}
> +
> +static void selinux_ns_free(struct work_struct *work)
> +{
> + struct selinux_ns *parent, *ns =
> + container_of(work, struct selinux_ns, work);
> +
> + do {
> + parent = ns->parent;
> + selinux_ss_free(ns->ss);
> + kfree(ns);
> + ns = parent;
> + } while (ns && refcount_dec_and_test(&ns->count));
> +}
> +
> +void __put_selinux_ns(struct selinux_ns *ns)
> +{
> + schedule_work(&ns->work);
> +}
While we would obviously need something like this for proper
namespacing, with only a single state struct we don't need all the
state/namespace management.
However, I would still be willing to accept all the changes to pass
around a reference to the global state struct, even if in this
particular case it was always going to be single/init struct.
> @@ -6487,6 +6585,11 @@ static __init int selinux_init(void)
>
> printk(KERN_INFO "SELinux: Initializing.\n");
>
> + if (selinux_ns_create(NULL, &init_selinux_ns))
> + panic("SELinux: Could not create initial namespace\n");
Not yet :) If we stick with selinux_ns_create(), see my comments
above, at the very least we need to pick an error message that doesn't
hint at namespacing.
> @@ -6629,23 +6738,32 @@ static void selinux_nf_ip_exit(void)
> #endif /* CONFIG_NETFILTER */
>
> #ifdef CONFIG_SECURITY_SELINUX_DISABLE
> -static int selinux_disabled;
> -
> -int selinux_disable(void)
> +int selinux_disable(struct selinux_ns *ns)
> {
> - if (ss_initialized) {
> + if (ns->in
