| Message ID | 1491988018-4120-1-git-send-email-sbuisson@ddn.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Wed, Apr 12, 2017 at 5:06 AM, Sebastien Buisson <sbuisson.ddn@gmail.com> wrote: > Add selinux_is_enforced() function to give access to SELinux > enforcement to the rest of the kernel. > > Signed-off-by: Sebastien Buisson <sbuisson@ddn.com> > --- > include/linux/selinux.h | 5 +++++ > security/selinux/exports.c | 6 ++++++ > security/selinux/hooks.c | 2 ++ > security/selinux/include/avc.h | 6 ------ > security/selinux/include/security.h | 1 + > 5 files changed, 14 insertions(+), 6 deletions(-) As currently written this code isn't something we would want to merge upstream for two important reasons: * No clear user of this functionality. There needs to be a well defined user of this functionality in the kernel. * No abstraction layer at the LSM interface. The core kernel code should not call directly into any specific LSM, all interaction should go through the LSM hooks. > diff --git a/include/linux/selinux.h b/include/linux/selinux.h > index 44f4596..1007321 100644 > --- a/include/linux/selinux.h > +++ b/include/linux/selinux.h > @@ -24,12 +24,17 @@ > * selinux_is_enabled - is SELinux enabled? > */ > bool selinux_is_enabled(void); > +bool selinux_is_enforced(void); > #else > > static inline bool selinux_is_enabled(void) > { > return false; > } > +static inline bool selinux_is_enforced(void) > +{ > + return false; > +} > #endif /* CONFIG_SECURITY_SELINUX */ > > #endif /* _LINUX_SELINUX_H */ > diff --git a/security/selinux/exports.c b/security/selinux/exports.c > index e75dd94..016f1e2 100644 > --- a/security/selinux/exports.c > +++ b/security/selinux/exports.c > @@ -21,3 +21,9 @@ bool selinux_is_enabled(void) > return selinux_enabled; > } > EXPORT_SYMBOL_GPL(selinux_is_enabled); > + > +bool selinux_is_enforced(void) > +{ > + return selinux_enforcing; > +} > +EXPORT_SYMBOL_GPL(selinux_is_enforced); > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index e67a526..da2baeb 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -109,6 +109,8 @@ static int __init enforcing_setup(char *str) > return 1; > } > __setup("enforcing=", enforcing_setup); > +#else > +int selinux_enforcing; > #endif > > #ifdef CONFIG_SECURITY_SELINUX_BOOTPARAM > diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h > index 0999df0..ff98351 100644 > --- a/security/selinux/include/avc.h > +++ b/security/selinux/include/avc.h > @@ -19,12 +19,6 @@ > #include "av_permissions.h" > #include "security.h" > > -#ifdef CONFIG_SECURITY_SELINUX_DEVELOP > -extern int selinux_enforcing; > -#else > -#define selinux_enforcing 1 > -#endif > - > /* > * An entry in the AVC. > */ > diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h > index f979c35..1e67e268 100644 > --- a/security/selinux/include/security.h > +++ b/security/selinux/include/security.h > @@ -64,6 +64,7 @@ > struct netlbl_lsm_secattr; > > extern int selinux_enabled; > +extern int selinux_enforcing; > > /* Policy capabilities */ > enum { > -- > 1.8.3.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2017-04-12 at 18:06 +0900, Sebastien Buisson wrote: > Add selinux_is_enforced() function to give access to SELinux > enforcement to the rest of the kernel. > > Signed-off-by: Sebastien Buisson <sbuisson@ddn.com> > --- > include/linux/selinux.h | 5 +++++ > security/selinux/exports.c | 6 ++++++ > security/selinux/hooks.c | 2 ++ > security/selinux/include/avc.h | 6 ------ > security/selinux/include/security.h | 1 + > 5 files changed, 14 insertions(+), 6 deletions(-) > > diff --git a/include/linux/selinux.h b/include/linux/selinux.h > index 44f4596..1007321 100644 > --- a/include/linux/selinux.h > +++ b/include/linux/selinux.h > @@ -24,12 +24,17 @@ > * selinux_is_enabled - is SELinux enabled? > */ > bool selinux_is_enabled(void); > +bool selinux_is_enforced(void); > #else > > static inline bool selinux_is_enabled(void) > { > return false; > } > +static inline bool selinux_is_enforced(void) > +{ > + return false; > +} > #endif /* CONFIG_SECURITY_SELINUX */ > > #endif /* _LINUX_SELINUX_H */ > diff --git a/security/selinux/exports.c b/security/selinux/exports.c > index e75dd94..016f1e2 100644 > --- a/security/selinux/exports.c > +++ b/security/selinux/exports.c > @@ -21,3 +21,9 @@ bool selinux_is_enabled(void) > return selinux_enabled; > } > EXPORT_SYMBOL_GPL(selinux_is_enabled); > + > +bool selinux_is_enforced(void) > +{ > + return selinux_enforcing; > +} > +EXPORT_SYMBOL_GPL(selinux_is_enforced); > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index e67a526..da2baeb 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -109,6 +109,8 @@ static int __init enforcing_setup(char *str) > return 1; > } > __setup("enforcing=", enforcing_setup); > +#else > +int selinux_enforcing; > #endif > > #ifdef CONFIG_SECURITY_SELINUX_BOOTPARAM > diff --git a/security/selinux/include/avc.h > b/security/selinux/include/avc.h > index 0999df0..ff98351 100644 > --- a/security/selinux/include/avc.h > +++ b/security/selinux/include/avc.h > @@ -19,12 +19,6 @@ > #include "av_permissions.h" > #include "security.h" > > -#ifdef CONFIG_SECURITY_SELINUX_DEVELOP > -extern int selinux_enforcing; > -#else > -#define selinux_enforcing 1 > -#endif If CONFIG_SECURITY_SELINUX_DEVELOP=n, then selinux_enforcing is supposed to always be 1, i.e. global permissive mode is not supported. Your patch breaks that. That's in addition to the points raised by others about needing an in-tree user and use of a LSM hook interface instead. > - > /* > * An entry in the AVC. > */ > diff --git a/security/selinux/include/security.h > b/security/selinux/include/security.h > index f979c35..1e67e268 100644 > --- a/security/selinux/include/security.h > +++ b/security/selinux/include/security.h > @@ -64,6 +64,7 @@ > struct netlbl_lsm_secattr; > > extern int selinux_enabled; > +extern int selinux_enforcing; > > /* Policy capabilities */ > enum {
2017-04-12 13:55 GMT+02:00 Paul Moore <pmoore@redhat.com>: > As currently written this code isn't something we would want to merge > upstream for two important reasons: > > * No abstraction layer at the LSM interface. The core kernel code > should not call directly into any specific LSM, all interaction should > go through the LSM hooks. The idea behind this patch and the other one was to replicate what is done with selinux_is_enabled(). As I understand it now, selinux_is_enabled() should remain the only exception to the LSM hooks. So do you agree if I propose a new security_is_enforced() function at the LSM abstraction layer, which will be hooked to a selinux_is_enforced() function defined inside the SELinux LSM?
2017-04-12 13:55 GMT+02:00 Paul Moore <pmoore@redhat.com>: > As currently written this code isn't something we would want to merge > upstream for two important reasons: > > * No clear user of this functionality. There needs to be a well > defined user of this functionality in the kernel. The use case for this new functionality (and the other one) is getting SELinux information from the Lustre client code in kernel space. Latest patch can be accessed at: https://review.whamcloud.com/24421 Actual user is sptlrpc_get_sepol() function in lustre/lustre/ptlrpc/sec.c file. This code will be pushed to the upstream kernel as soon as it is landed into Lustre master branch.
On Wed, 2017-04-12 at 15:30 +0200, Sebastien Buisson wrote: > 2017-04-12 13:55 GMT+02:00 Paul Moore <pmoore@redhat.com>: > > As currently written this code isn't something we would want to > > merge > > upstream for two important reasons: > > > > * No abstraction layer at the LSM interface. The core kernel code > > should not call directly into any specific LSM, all interaction > > should > > go through the LSM hooks. > > The idea behind this patch and the other one was to replicate what is > done with selinux_is_enabled(). As I understand it now, > selinux_is_enabled() should remain the only exception to the LSM > hooks. > So do you agree if I propose a new security_is_enforced() function at > the LSM abstraction layer, which will be hooked to a > selinux_is_enforced() function defined inside the SELinux LSM? Even your usage of selinux_is_enabled() looks suspect; that should probably go away. Only other user of it seems to be some cred validity checking that could be dropped as well. The include/linux/selinux.h interfaces were originally for use by audit and secmark when there were no other LSMs and have gradually been removed.
On Wed, 2017-04-12 at 15:30 +0200, Sebastien Buisson wrote: > 2017-04-12 13:55 GMT+02:00 Paul Moore <pmoore@redhat.com>: > > As currently written this code isn't something we would want to > > merge > > upstream for two important reasons: > > > > * No clear user of this functionality. There needs to be a well > > defined user of this functionality in the kernel. > > The use case for this new functionality (and the other one) is > getting > SELinux information from the Lustre client code in kernel space. > Latest patch can be accessed at: > https://review.whamcloud.com/24421 > Actual user is sptlrpc_get_sepol() function in > lustre/lustre/ptlrpc/sec.c file. > This code will be pushed to the upstream kernel as soon as it is > landed into Lustre master branch. How are you using this SELinux information in the kernel and/or in userspace? What's the purpose of it? What are you comparing it against? Why do you care if it changes? Note btw that the notion of a policy name/type and the policy file path is purely a userspace construct and shouldn't be embedded in your kernel code. Android for example doesn't follow that convention at all; their SELinux policy file is simply /sepolicy. On modern kernels, you can always read the currently loaded policy from the kernel itself via /sys/fs/selinux/policy (formerly just /selinux/policy).
2017-04-12 16:35 GMT+02:00 Stephen Smalley <sds@tycho.nsa.gov>: > How are you using this SELinux information in the kernel and/or in > userspace? What's the purpose of it? What are you comparing it > against? Why do you care if it changes? Enforcement status and policy version are compared to their previously stored value. If they differ, then it means we need to call a userland helper from Lustre client kernelspace to read the currently loaded policy (reading it will let us know if the Lustre client node is conforming to the Lustre-wide security policy). As calling the userland helper is costly, we do it only when it is necessary by retrieving some SELinux key information directly from kernelspace. > Note btw that the notion of a policy name/type and the policy file path > is purely a userspace construct and shouldn't be embedded in your > kernel code. Android for example doesn't follow that convention at > all; their SELinux policy file is simply /sepolicy. On modern kernels, > you can always read the currently loaded policy from the kernel itself > via /sys/fs/selinux/policy (formerly just /selinux/policy). As I understand it, a userspace program can directly read the policy info exposed by the kernel by reading this file. But how about reading it from kernelspace?
2017-04-12 15:58 GMT+02:00 Stephen Smalley <sds@tycho.nsa.gov>: > Even your usage of selinux_is_enabled() looks suspect; that should > probably go away. Only other user of it seems to be some cred validity > checking that could be dropped as well. Well the main reason for calling selinux_is_enabled() is performance optimization. Should I propose a patch to add a new security_is_enabled() function at the LSM abstraction layer? Or do you consider we should not test security enabled at all?
On Wed, 2017-04-12 at 17:11 +0200, Sebastien Buisson wrote: > 2017-04-12 16:35 GMT+02:00 Stephen Smalley <sds@tycho.nsa.gov>: > > How are you using this SELinux information in the kernel and/or in > > userspace? What's the purpose of it? What are you comparing it > > against? Why do you care if it changes? > > Enforcement status and policy version are compared to their > previously > stored value. If they differ, then it means we need to call a > userland > helper from Lustre client kernelspace to read the currently loaded > policy (reading it will let us know if the Lustre client node is > conforming to the Lustre-wide security policy). > As calling the userland helper is costly, we do it only when it is > necessary by retrieving some SELinux key information directly from > kernelspace. Maybe you want to register a notifier callback on policy reload? See the archives for the SELinux support for Infiniband RDMA patches (which seem to have stalled), which included LSM hooks and SELinux implementation to support notifications on policy reloads. > > > Note btw that the notion of a policy name/type and the policy file > > path > > is purely a userspace construct and shouldn't be embedded in your > > kernel code. Android for example doesn't follow that convention at > > all; their SELinux policy file is simply /sepolicy. On modern > > kernels, > > you can always read the currently loaded policy from the kernel > > itself > > via /sys/fs/selinux/policy (formerly just /selinux/policy). > > As I understand it, a userspace program can directly read the policy > info exposed by the kernel by reading this file. But how about > reading > it from kernelspace? In SELinux, the underlying kernel function is security_read_policy(); you'd have to wrap it with a LSM hook interface, and generalize it a bit wrt whether to use vmalloc_user() or not. This seems very inefficient though for your purposes. Wouldn't it be better to just extend SELinux to compute the checksum from the original image when the policy is loaded, save that checksum in the policydb, and provide you with a way to fetch the already computed checksum? The computation would be done in security_load_policy() and saved in the policydb. Then you could introduce a function and a LSM hook to export it to your code. We would probably want to also expose it via a selinuxfs node to userspace. This however only works for checking that you have a completely identical policy built in exactly the same way. You could have semantically identical policies that still differ in the binary policy file, or policies with minor local customizations that aren't significant. But perhaps that isn't an issue for Lustre environments.
On Wed, 2017-04-12 at 17:19 +0200, Sebastien Buisson wrote: > 2017-04-12 15:58 GMT+02:00 Stephen Smalley <sds@tycho.nsa.gov>: > > Even your usage of selinux_is_enabled() looks suspect; that should > > probably go away. Only other user of it seems to be some cred > > validity > > checking that could be dropped as well. > > Well the main reason for calling selinux_is_enabled() is performance > optimization. > Should I propose a patch to add a new security_is_enabled() function > at the LSM abstraction layer? Or do you consider we should not test > security enabled at all? It isn't clear what "is enabled" means in general, particularly with stacking. I would either drop it or replace it with a LSM hook that is more precise. For example, NFSv4 introduced a security_ismaclabel() hook so that it could test whether a given security.* xattr is a MAC label.
2017-04-12 18:24 GMT+02:00 Stephen Smalley <sds@tycho.nsa.gov>: > Maybe you want to register a notifier callback on policy reload? See > the archives for the SELinux support for Infiniband RDMA patches (which > seem to have stalled), which included LSM hooks and SELinux > implementation to support notifications on policy reloads. I need to have a look indeed. So it is a callback in kernelspace? >> As I understand it, a userspace program can directly read the policy >> info exposed by the kernel by reading this file. But how about >> reading it from kernelspace? > > This seems very inefficient though for your purposes. Wouldn't it be > better to just extend SELinux to compute the checksum from the original > image when the policy is loaded, save that checksum in the policydb, > and provide you with a way to fetch the already computed checksum? The > computation would be done in security_load_policy() and saved in the > policydb. Then you could introduce a function and a LSM hook to export > it to your code. We would probably want to also expose it via a > selinuxfs node to userspace. This is an excellent suggestion. It makes much more sense to have the checksum computed on SELinux side when a policy is loaded. And then just read this checksum when needed, both from kernel and userspace. > This however only works for checking that you have a completely > identical policy built in exactly the same way. You could have > semantically identical policies that still differ in the binary policy > file, or policies with minor local customizations that aren't > significant. But perhaps that isn't an issue for Lustre environments. If we can protect against local customizations this is great. What could be the other scenario leading to different binary policies while being semantically identical?
On Wed, 2017-04-12 at 19:07 +0200, Sebastien Buisson wrote: > 2017-04-12 18:24 GMT+02:00 Stephen Smalley <sds@tycho.nsa.gov>: > > Maybe you want to register a notifier callback on policy reload? > > See > > the archives for the SELinux support for Infiniband RDMA patches > > (which > > seem to have stalled), which included LSM hooks and SELinux > > implementation to support notifications on policy reloads. > > I need to have a look indeed. So it is a callback in kernelspace? Yes, see: https://patchwork.kernel.org/patch/9443417/ > > > > As I understand it, a userspace program can directly read the > > > policy > > > info exposed by the kernel by reading this file. But how about > > > reading it from kernelspace? > > > > This seems very inefficient though for your purposes. Wouldn't it > > be > > better to just extend SELinux to compute the checksum from the > > original > > image when the policy is loaded, save that checksum in the > > policydb, > > and provide you with a way to fetch the already computed > > checksum? The > > computation would be done in security_load_policy() and saved in > > the > > policydb. Then you could introduce a function and a LSM hook to > > export > > it to your code. We would probably want to also expose it via a > > selinuxfs node to userspace. > > This is an excellent suggestion. It makes much more sense to have the > checksum computed on SELinux side when a policy is loaded. And then > just read this checksum when needed, both from kernel and userspace. > > > This however only works for checking that you have a completely > > identical policy built in exactly the same way. You could have > > semantically identical policies that still differ in the binary > > policy > > file, or policies with minor local customizations that aren't > > significant. But perhaps that isn't an issue for Lustre > > environments. > > If we can protect against local customizations this is great. What > could be the other scenario leading to different binary policies > while > being semantically identical? There can be ordering or optimization differences, depending on the policy compiler toolchain and build process. Probably not a concern if they are all running the same distro with the same policy package, built in the same build environment.
On 4/12/2017 9:33 AM, Stephen Smalley wrote: > On Wed, 2017-04-12 at 17:19 +0200, Sebastien Buisson wrote: >> 2017-04-12 15:58 GMT+02:00 Stephen Smalley <sds@tycho.nsa.gov>: >>> Even your usage of selinux_is_enabled() looks suspect; that should >>> probably go away. Only other user of it seems to be some cred >>> validity >>> checking that could be dropped as well. >> Well the main reason for calling selinux_is_enabled() is performance >> optimization. >> Should I propose a patch to add a new security_is_enabled() function >> at the LSM abstraction layer? Or do you consider we should not test >> security enabled at all? > It isn't clear what "is enabled" means in general, particularly with > stacking. I would either drop it or replace it with a LSM hook that is > more precise. For example, NFSv4 introduced a security_ismaclabel() > hook so that it could test whether a given security.* xattr is a MAC > label. You can determine what security modules are enabled by reading /sys/kernel/security/lsm in userspace (4.11 feature). Your kernel code *really* shouldn't care what security modules are enabled. If you do care, you've designed poorly. If you're trying to ensure consistent policy between members of your cluster you're better off doing that in user space than in the kernel. > > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
diff --git a/include/linux/selinux.h b/include/linux/selinux.h index 44f4596..1007321 100644 --- a/include/linux/selinux.h +++ b/include/linux/selinux.h @@ -24,12 +24,17 @@ * selinux_is_enabled - is SELinux enabled? */ bool selinux_is_enabled(void); +bool selinux_is_enforced(void); #else static inline bool selinux_is_enabled(void) { return false; } +static inline bool selinux_is_enforced(void) +{ + return false; +} #endif /* CONFIG_SECURITY_SELINUX */ #endif /* _LINUX_SELINUX_H */ diff --git a/security/selinux/exports.c b/security/selinux/exports.c index e75dd94..016f1e2 100644 --- a/security/selinux/exports.c +++ b/security/selinux/exports.c @@ -21,3 +21,9 @@ bool selinux_is_enabled(void) return selinux_enabled; } EXPORT_SYMBOL_GPL(selinux_is_enabled); + +bool selinux_is_enforced(void) +{ + return selinux_enforcing; +} +EXPORT_SYMBOL_GPL(selinux_is_enforced); diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index e67a526..da2baeb 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -109,6 +109,8 @@ static int __init enforcing_setup(char *str) return 1; } __setup("enforcing=", enforcing_setup); +#else +int selinux_enforcing; #endif #ifdef CONFIG_SECURITY_SELINUX_BOOTPARAM diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h index 0999df0..ff98351 100644 --- a/security/selinux/include/avc.h +++ b/security/selinux/include/avc.h @@ -19,12 +19,6 @@ #include "av_permissions.h" #include "security.h" -#ifdef CONFIG_SECURITY_SELINUX_DEVELOP -extern int selinux_enforcing; -#else -#define selinux_enforcing 1 -#endif - /* * An entry in the AVC. */ diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index f979c35..1e67e268 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -64,6 +64,7 @@ struct netlbl_lsm_secattr; extern int selinux_enabled; +extern int selinux_enforcing; /* Policy capabilities */ enum {
Add selinux_is_enforced() function to give access to SELinux enforcement to the rest of the kernel. Signed-off-by: Sebastien Buisson <sbuisson@ddn.com> --- include/linux/selinux.h | 5 +++++ security/selinux/exports.c | 6 ++++++ security/selinux/hooks.c | 2 ++ security/selinux/include/avc.h | 6 ------ security/selinux/include/security.h | 1 + 5 files changed, 14 insertions(+), 6 deletions(-)