Re: [PATCH v5 00/38] LSM: Module stacking for SARA and Landlock
On Tue, Jan 8, 2019 at 1:37 PM Casey Schaufler wrote: > > On 1/8/2019 1:05 PM, James Morris wrote: > > On Mon, 7 Jan 2019, Kees Cook wrote: > > > >> On Tue, Dec 11, 2018 at 1:19 PM Kees Cook wrote: > >>> On Tue, Dec 11, 2018 at 10:57 AM James Morris wrote: > >>>> On Tue, 4 Dec 2018, Kees Cook wrote: > >>>> > >>>>> On Mon, Nov 26, 2018 at 3:22 PM Casey Schaufler > >>>>> wrote: > >>>>>> v5: Include Kees Cook's rework of the lsm command > >>>>>> line interface. Stacking is not conditional. > >>>>> Can you resend this series with corrected "From:" lines in the body, > >>>>> etc? > >>>>> > >>>>> Beyond that, I obviously like it. James, what's needed for this to move > >>>>> forward? > >>>> If there are no outstanding issues, I plan to merge this for 4.21. > >>> Yeah, it looks good to me. (Excepting getting the authorship sorted.) > >> I didn't see this actually get merged? Was there something that needed > >> fixing? Should I send you a direct pull request for v5.1? > > Yep, please send a new pull request. > > Do you want it as is or rebased on 5.0-rc1? I've rebased to 5.0-rc1, did some light (re)testing, and sent a pull request... -- Kees Cook ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v5 00/38] LSM: Module stacking for SARA and Landlock
On Tue, Dec 11, 2018 at 1:19 PM Kees Cook wrote: > > On Tue, Dec 11, 2018 at 10:57 AM James Morris wrote: > > > > On Tue, 4 Dec 2018, Kees Cook wrote: > > > > > On Mon, Nov 26, 2018 at 3:22 PM Casey Schaufler > > > wrote: > > > > v5: Include Kees Cook's rework of the lsm command > > > > line interface. Stacking is not conditional. > > > > > > Can you resend this series with corrected "From:" lines in the body, etc? > > > > > > Beyond that, I obviously like it. James, what's needed for this to move > > > forward? > > > > If there are no outstanding issues, I plan to merge this for 4.21. > > Yeah, it looks good to me. (Excepting getting the authorship sorted.) I didn't see this actually get merged? Was there something that needed fixing? Should I send you a direct pull request for v5.1? -- Kees Cook ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v5 00/38] LSM: Module stacking for SARA and Landlock
On Tue, Dec 11, 2018 at 10:57 AM James Morris wrote: > > On Tue, 4 Dec 2018, Kees Cook wrote: > > > On Mon, Nov 26, 2018 at 3:22 PM Casey Schaufler > > wrote: > > > v5: Include Kees Cook's rework of the lsm command > > > line interface. Stacking is not conditional. > > > > Can you resend this series with corrected "From:" lines in the body, etc? > > > > Beyond that, I obviously like it. James, what's needed for this to move > > forward? > > If there are no outstanding issues, I plan to merge this for 4.21. Yeah, it looks good to me. (Excepting getting the authorship sorted.) Thanks! -- Kees Cook ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v5 01/38] LSM: Introduce LSM_FLAG_LEGACY_MAJOR
On Mon, Nov 26, 2018 at 3:26 PM Casey Schaufler wrote:
Hmmm... the "From: Kees..." in the body is missing. Are you using "git
send-email"?
>
> This adds a flag for the current "major" LSMs to distinguish them when
> we have a universal method for ordering all LSMs. It's called "legacy"
> since the distinction of "major" will go away in the blob-sharing world.
>
> Signed-off-by: Kees Cook
> Reviewed-by: Casey Schaufler
> Reviewed-by: John Johansen
> ---
> include/linux/lsm_hooks.h | 3 +++
> security/apparmor/lsm.c| 1 +
> security/selinux/hooks.c | 1 +
> security/smack/smack_lsm.c | 1 +
> security/tomoyo/tomoyo.c | 1 +
> 5 files changed, 7 insertions(+)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index aaeb7fa24dc4..63c0e102de20 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2039,8 +2039,11 @@ extern char *lsm_names;
> extern void security_add_hooks(struct security_hook_list *hooks, int count,
> char *lsm);
>
> +#define LSM_FLAG_LEGACY_MAJOR BIT(0)
> +
> struct lsm_info {
> const char *name; /* Required. */
> + unsigned long flags;/* Optional: flags describing LSM */
> int (*init)(void); /* Required. */
> };
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 42446a216f3b..2edd35ca5044 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1728,5 +1728,6 @@ static int __init apparmor_init(void)
>
> DEFINE_LSM(apparmor) = {
> .name = "apparmor",
> + .flags = LSM_FLAG_LEGACY_MAJOR,
> .init = apparmor_init,
> };
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 7ce683259357..56c6f1849c80 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -7209,6 +7209,7 @@ void selinux_complete_init(void)
> all processes and objects when they are created. */
> DEFINE_LSM(selinux) = {
> .name = "selinux",
> + .flags = LSM_FLAG_LEGACY_MAJOR,
> .init = selinux_init,
> };
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 81fb4c1631e9..3639e55b1f4b 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4891,5 +4891,6 @@ static __init int smack_init(void)
> */
> DEFINE_LSM(smack) = {
> .name = "smack",
> + .flags = LSM_FLAG_LEGACY_MAJOR,
> .init = smack_init,
> };
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 1b5b5097efd7..09f7af130d3a 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -552,5 +552,6 @@ static int __init tomoyo_init(void)
>
> DEFINE_LSM(tomoyo) = {
> .name = "tomoyo",
> + .flags = LSM_FLAG_LEGACY_MAJOR,
> .init = tomoyo_init,
> };
> --
> 2.14.5
>
>
--
Kees Cook
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 21/19] LSM: Cleanup and fixes from Tetsuo Handa
On Wed, Sep 26, 2018 at 2:57 PM, Casey Schaufler wrote: > lsm_early_cred()/lsm_early_task() are called from only __init functions. > > lsm_cred_alloc()/lsm_file_alloc() are called from only security/security.c . > > lsm_early_inode() should be avoided because it is not appropriate to > call panic() when lsm_early_inode() is called after __init phase. > > Since all free hooks are called when one of init hooks failed, each > free hook needs to check whether init hook was called. > > The original changes are from Tetsuo Handa. I have made minor > changes in some places, but this is mostly his code. > > Signed-off-by: Casey Schaufler > --- > include/linux/lsm_hooks.h | 6 ++ > security/security.c | 27 --- > security/selinux/hooks.c | 5 - > security/selinux/include/objsec.h | 2 ++ > security/smack/smack_lsm.c| 8 +++- > 5 files changed, 19 insertions(+), 29 deletions(-) I've split this across the various commits they touch: Infrastructure management of the cred security blob LSM: Infrastructure management of the file security LSM: Infrastructure management of the inode security LSM: Infrastructure management of the task security LSM: Blob sharing support for S.A.R.A and LandLock Based on these changes, I've uploaded the "v4.1", or "Casey is on vacation", tree here: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=lsm/blob-sharing-v4.1 I'm going to work on a merged series for the "arbitrary ordering" and "blob-sharing" trees next... -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v4 20/19] LSM: Correct file blob free empty blob check
On Wed, Sep 26, 2018 at 2:57 PM, Casey Schaufler wrote:
> Instead of checking if the kmem_cache for file blobs
> has been initialized check if the blob is NULL. This
> allows non-blob using modules to do other kinds of
> clean up in the security_file_free hooks.
>
> Signed-off-by: Casey Schaufler
Reviewed-by: Kees Cook
This looks like it should get folded into "LSM: Infrastructure
management of the file security".
-Kees
> ---
> security/security.c | 9 -
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index e7c8506041f1..76f7dc49b63c 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1202,14 +1202,13 @@ void security_file_free(struct file *file)
> {
> void *blob;
>
> - if (!lsm_file_cache)
> - return;
> -
> call_void_hook(file_free_security, file);
>
> blob = file->f_security;
> - file->f_security = NULL;
> - kmem_cache_free(lsm_file_cache, blob);
> + if (blob) {
> + file->f_security = NULL;
> + kmem_cache_free(lsm_file_cache, blob);
> + }
> }
>
> int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long
> arg)
> --
> 2.17.1
>
>
--
Kees Cook
Pixel Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
[PATCH security-next v3 07/29] LSM: Convert security_initcall() into DEFINE_LSM()
Instead of using argument-based initializers, switch to defining the
contents of struct lsm_info on a per-LSM basis. This also drops
the final use of the now inaccurate "initcall" naming.
Cc: John Johansen
Cc: James Morris
Cc: "Serge E. Hallyn"
Cc: Paul Moore
Cc: Stephen Smalley
Cc: Casey Schaufler
Cc: Tetsuo Handa
Cc: Mimi Zohar
Cc: linux-security-mod...@vger.kernel.org
Cc: selinux@tycho.nsa.gov
Signed-off-by: Kees Cook
---
include/linux/lsm_hooks.h | 6 --
security/apparmor/lsm.c| 4 +++-
security/integrity/iint.c | 4 +++-
security/selinux/hooks.c | 4 +++-
security/smack/smack_lsm.c | 4 +++-
security/tomoyo/tomoyo.c | 4 +++-
6 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index ad04761e5587..02ec717189f9 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2045,11 +2045,13 @@ struct lsm_info {
extern struct lsm_info __start_lsm_info[], __end_lsm_info[];
-#define security_initcall(lsm) \
+#define DEFINE_LSM(lsm)
\
static struct lsm_info __lsm_##lsm \
__used __section(.lsm_info.init)\
__aligned(sizeof(unsigned long))\
- = { .init = lsm, }
+ = { \
+
+#define END_LSM }
#ifdef CONFIG_SECURITY_SELINUX_DISABLE
/*
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 8b8b70620bbe..7fa7b4464cf4 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1606,4 +1606,6 @@ static int __init apparmor_init(void)
return error;
}
-security_initcall(apparmor_init);
+DEFINE_LSM(apparmor)
+ .init = apparmor_init,
+END_LSM;
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 70d21b566955..20e60df929a3 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -175,7 +175,9 @@ static int __init integrity_iintcache_init(void)
0, SLAB_PANIC, init_once);
return 0;
}
-security_initcall(integrity_iintcache_init);
+DEFINE_LSM(integrity)
+ .init = integrity_iintcache_init,
+END_LSM;
/*
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ad9a9b8e9979..469a90806bc6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -7202,7 +7202,9 @@ void selinux_complete_init(void)
/* SELinux requires early initialization in order to label
all processes and objects when they are created. */
-security_initcall(selinux_init);
+DEFINE_LSM(selinux)
+ .init = selinux_init,
+END_LSM;
#if defined(CONFIG_NETFILTER)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 340fc30ad85d..1e1ace718e75 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4882,4 +4882,6 @@ static __init int smack_init(void)
* Smack requires early initialization in order to label
* all processes and objects when they are created.
*/
-security_initcall(smack_init);
+DEFINE_LSM(smack)
+ .init = smack_init,
+END_LSM;
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 9f932e2d6852..a280d4eab456 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -550,4 +550,6 @@ static int __init tomoyo_init(void)
return 0;
}
-security_initcall(tomoyo_init);
+DEFINE_LSM(tomoyo)
+ .init = tomoyo_init,
+END_LSM;
--
2.17.1
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v4 00/19] LSM: Module stacking for SARA and Landlock
On Sat, Sep 22, 2018 at 9:38 AM, Casey Schaufler wrote: > On 9/21/2018 8:02 PM, Kees Cook wrote: >> On Fri, Sep 21, 2018 at 4:59 PM, Casey Schaufler >> wrote: >>> v4: Finer granularity in the patches and other >>> cleanups suggested by Kees Cook. >>> Removed dead code created by the removal of SELinux >>> credential blob poisoning. >> Thanks for the splitting, this really does make it easier to review >> (at least for me). I think this looks really good, though obviously >> I'd like to refactor it slightly on top of my series. :) > > Whichever goes on top is fine with me. What's one > more patch set merge, after all? > >> One additional thought I had was about the blobs allocations: some are >> separate kmem caches, and some are kmalloc. I'm thinking it might make >> sense to use separate kmem caches for two reasons: > > I had seriously considered doing that. I can't see any reason > not to. It's something that could be done at any time, and with > all the other things that had to change it just didn't get in. Yup; that is an easy future change. Not needed now! > >> - they're going to always be the same size and are regularly >> allocated/freed, so it may offer a performance benefit. >> >> - they're explicitly not supposed to be exposed to userspace, so >> hardened usercopy would protect them if they were not kmalloc()ed. >> >> I'm excited about getting this landed! > > Soon. Real soon. I hope. I would very much like for > someone from the SELinux camp to chime in, especially on > the selinux_is_enabled() removal. Agreed. > On a somewhat related note, I will be out for the first three > weeks of October, returning just in time for the Linux Security > Summit in Edinburgh. My connectivity will be severely limited. > I don't expect to accomplish anything while I'm out. If you're okay with it, I can help with changes while you're out -- I want to try to rebase it on my tree and see how it looks anyway. :) -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v4 00/19] LSM: Module stacking for SARA and Landlock
On Fri, Sep 21, 2018 at 4:59 PM, Casey Schaufler wrote: > v4: Finer granularity in the patches and other > cleanups suggested by Kees Cook. > Removed dead code created by the removal of SELinux > credential blob poisoning. Thanks for the splitting, this really does make it easier to review (at least for me). I think this looks really good, though obviously I'd like to refactor it slightly on top of my series. :) One additional thought I had was about the blobs allocations: some are separate kmem caches, and some are kmalloc. I'm thinking it might make sense to use separate kmem caches for two reasons: - they're going to always be the same size and are regularly allocated/freed, so it may offer a performance benefit. - they're explicitly not supposed to be exposed to userspace, so hardened usercopy would protect them if they were not kmalloc()ed. I'm excited about getting this landed! -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v4 17/19] Smack: Abstract use of ipc security blobs
On Fri, Sep 21, 2018 at 5:19 PM, Casey Schaufler wrote: > Don't use the ipc->security pointer directly. > Don't use the msg_msg->security pointer directly. > Provide helper functions that provides the security blob pointers. > > Signed-off-by: Casey Schaufler Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v4 10/19] Smack: Abstract use of file security blob
On Fri, Sep 21, 2018 at 5:18 PM, Casey Schaufler wrote: > Don't use the file->f_security pointer directly. > Provide a helper function that provides the security blob pointer. > > Signed-off-by: Casey Schaufler Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v4 08/19] Infrastructure management of the cred security blob
On Fri, Sep 21, 2018 at 5:18 PM, Casey Schaufler wrote: > Move management of the cred security blob out of the > security modules and into the security infrastructre. > Instead of allocating and freeing space the security > modules tell the infrastructure how much space they > require. > > Signed-off-by: Casey Schaufler When combined with my series, this gets slightly simpler: - the double init call and the "finished" stuff goes away - debugging output is controlled by "lsm.debug" param instead of a CONFIG Regardless, for the overall logic, calculating the sizes, etc: Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v4 07/19] TOMOYO: Abstract use of cred security blob
On Fri, Sep 21, 2018 at 5:18 PM, Casey Schaufler wrote: > Don't use the cred->security pointer directly. > Provide helper functions that provide the security blob pointer. > > Signed-off-by: Casey Schaufler Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v4 18/19] LSM: Infrastructure management of the ipc security blob
On Fri, Sep 21, 2018 at 5:20 PM, Casey Schaufler wrote: > Move management of the kern_ipc_perm->security and > msg_msg->security blobs out of the individual security > modules and into the security infrastructure. Instead > of allocating the blobs from within the modules the modules > tell the infrastructure how much space is required, and > the space is allocated there. > > Signed-off-by: Casey Schaufler Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v4 16/19] SELinux: Abstract use of ipc security blobs
On Fri, Sep 21, 2018 at 5:19 PM, Casey Schaufler wrote: > Don't use the ipc->security pointer directly. > Don't use the msg_msg->security pointer directly. > Provide helper functions that provides the security blob pointers. > > Signed-off-by: Casey Schaufler Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v4 11/19] LSM: Infrastructure management of the file security
On Fri, Sep 21, 2018 at 5:19 PM, Casey Schaufler wrote: > Move management of the file->f_security blob out of the > individual security modules and into the infrastructure. > The modules no longer allocate or free the data, instead > they tell the infrastructure how much space they require. > > Signed-off-by: Casey Schaufler Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v4 14/19] LSM: Infrastructure management of the inode security
On Fri, Sep 21, 2018 at 5:19 PM, Casey Schaufler wrote:
> Move management of the inode->i_security blob out
> of the individual security modules and into the security
> infrastructure. Instead of allocating the blobs from within
> the modules the modules tell the infrastructure how much
> space is required, and the space is allocated there.
>
> Signed-off-by: Casey Schaufler
> ---
> include/linux/lsm_hooks.h | 3 ++
> security/security.c | 83 ++-
> security/selinux/hooks.c | 32 +---
> security/selinux/include/objsec.h | 5 +-
> security/smack/smack_lsm.c| 70 --
> 5 files changed, 98 insertions(+), 95 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 167ffbd4d0c0..416b20c3795b 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2030,6 +2030,7 @@ struct security_hook_list {
> struct lsm_blob_sizes {
> int lbs_cred;
> int lbs_file;
> + int lbs_inode;
> };
>
> /*
> @@ -2092,9 +2093,11 @@ static inline void loadpin_add_hooks(void) { };
> #endif
>
> extern int lsm_cred_alloc(struct cred *cred, gfp_t gfp);
> +extern int lsm_inode_alloc(struct inode *inode);
>
> #ifdef CONFIG_SECURITY
> void lsm_early_cred(struct cred *cred);
> +void lsm_early_inode(struct inode *inode);
> #endif
>
> #endif /* ! __LINUX_LSM_HOOKS_H */
> diff --git a/security/security.c b/security/security.c
> index 5430cae73cf6..a8f00fdff4d8 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -41,6 +41,7 @@ struct security_hook_heads security_hook_heads
> __lsm_ro_after_init;
> static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
>
> static struct kmem_cache *lsm_file_cache;
> +static struct kmem_cache *lsm_inode_cache;
>
> char *lsm_names;
> static struct lsm_blob_sizes blob_sizes;
> @@ -101,6 +102,10 @@ int __init security_init(void)
> lsm_file_cache = kmem_cache_create("lsm_file_cache",
>blob_sizes.lbs_file, 0,
>SLAB_PANIC, NULL);
> + if (blob_sizes.lbs_inode)
> + lsm_inode_cache = kmem_cache_create("lsm_inode_cache",
> + blob_sizes.lbs_inode, 0,
> + SLAB_PANIC, NULL);
> /*
> * The second call to a module specific init function
> * adds hooks to the hook lists and does any other early
> @@ -111,6 +116,7 @@ int __init security_init(void)
> #ifdef CONFIG_SECURITY_LSM_DEBUG
> pr_info("LSM: cred blob size = %d\n", blob_sizes.lbs_cred);
> pr_info("LSM: file blob size = %d\n", blob_sizes.lbs_file);
> + pr_info("LSM: inode blob size = %d\n", blob_sizes.lbs_inode);
> #endif
>
> return 0;
> @@ -288,6 +294,13 @@ void __init security_add_blobs(struct lsm_blob_sizes
> *needed)
> {
> lsm_set_size(&needed->lbs_cred, &blob_sizes.lbs_cred);
> lsm_set_size(&needed->lbs_file, &blob_sizes.lbs_file);
> + /*
> +* The inode blob gets an rcu_head in addition to
> +* what the modules might need.
> +*/
> + if (needed->lbs_inode && blob_sizes.lbs_inode == 0)
> + blob_sizes.lbs_inode = sizeof(struct rcu_head);
> + lsm_set_size(&needed->lbs_inode, &blob_sizes.lbs_inode);
> }
>
> /**
> @@ -311,6 +324,46 @@ int lsm_file_alloc(struct file *file)
> return 0;
> }
>
> +/**
> + * lsm_inode_alloc - allocate a composite inode blob
> + * @inode: the inode that needs a blob
> + *
> + * Allocate the inode blob for all the modules
> + *
> + * Returns 0, or -ENOMEM if memory can't be allocated.
> + */
> +int lsm_inode_alloc(struct inode *inode)
> +{
> + if (!lsm_inode_cache) {
> + inode->i_security = NULL;
> + return 0;
> + }
> +
> + inode->i_security = kmem_cache_zalloc(lsm_inode_cache, GFP_NOFS);
> + if (inode->i_security == NULL)
> + return -ENOMEM;
> + return 0;
> +}
> +
> +/**
> + * lsm_early_inode - during initialization allocate a composite inode blob
> + * @inode: the inode that needs a blob
> + *
> + * Allocate the inode blob for all the modules if it's not already there
> + */
> +void lsm_early_inode(struct inode *inode)
> +{
> + int rc;
> +
> + if (inode == NULL)
> + panic(&q
Re: [PATCH v4 15/19] LSM: Infrastructure management of the task security
On Fri, Sep 21, 2018 at 5:19 PM, Casey Schaufler wrote: > Move management of the task_struct->security blob out > of the individual security modules and into the security > infrastructure. Instead of allocating the blobs from within > the modules the modules tell the infrastructure how much > space is required, and the space is allocated there. > The only user of this blob is AppArmor. The AppArmor use > is abstracted to avoid future conflict. > > Signed-off-by: Casey Schaufler Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v4 02/19] Smack: Abstract use of cred security blob
On Fri, Sep 21, 2018 at 5:17 PM, Casey Schaufler wrote: > Don't use the cred->security pointer directly. > Provide a helper function that provides the security blob pointer. > > Signed-off-by: Casey Schaufler Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v4 06/19] AppArmor: Abstract use of cred security blob
On Fri, Sep 21, 2018 at 5:17 PM, Casey Schaufler wrote: > Don't use the cred->security pointer directly. > Provide a helper function that provides the security blob pointer. > > Signed-off-by: Casey Schaufler Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v4 04/19] SELinux: Remove cred security blob poisoning
On Fri, Sep 21, 2018 at 5:17 PM, Casey Schaufler wrote:
> The SELinux specific credential poisioning only makes sense
> if SELinux is managing the credentials. As the intent of this
> patch set is to move the blob management out of the modules
> and into the infrastructure, the SELinux specific code has
> to go. The poisioning could be introduced into the infrastructure
> at some later date.
>
> Signed-off-by: Casey Schaufler
Reviewed-by: Kees Cook
-Kees
> ---
> kernel/cred.c| 13 -
> security/selinux/hooks.c | 6 --
> 2 files changed, 19 deletions(-)
>
> diff --git a/kernel/cred.c b/kernel/cred.c
> index ecf03657e71c..fa2061ee4955 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -704,19 +704,6 @@ bool creds_are_invalid(const struct cred *cred)
> {
> if (cred->magic != CRED_MAGIC)
> return true;
> -#ifdef CONFIG_SECURITY_SELINUX
> - /*
> -* cred->security == NULL if security_cred_alloc_blank() or
> -* security_prepare_creds() returned an error.
> -*/
> - if (selinux_is_enabled() && cred->security) {
> - if ((unsigned long) cred->security < PAGE_SIZE)
> - return true;
> - if ((*(u32 *)cred->security & 0xff00) ==
> - (POISON_FREE << 24 | POISON_FREE << 16 | POISON_FREE <<
> 8))
> - return true;
> - }
> -#endif
> return false;
> }
> EXPORT_SYMBOL(creds_are_invalid);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 9d6cdd21acb6..80614ca25a2b 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3920,12 +3920,6 @@ static void selinux_cred_free(struct cred *cred)
> {
> struct task_security_struct *tsec = selinux_cred(cred);
>
> - /*
> -* cred->security == NULL if security_cred_alloc_blank() or
> -* security_prepare_creds() returned an error.
> -*/
> - BUG_ON(cred->security && (unsigned long) cred->security < PAGE_SIZE);
> - cred->security = (void *) 0x7UL;
> kfree(tsec);
> }
>
> --
> 2.17.1
>
>
--
Kees Cook
Pixel Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v4 05/19] SELinux: Remove unused selinux_is_enabled
On Fri, Sep 21, 2018 at 5:17 PM, Casey Schaufler wrote: > There are no longer users of selinux_is_enabled(). > Remove it. As selinux_is_enabled() is the only reason > for include/linux/selinux.h remove that as well. > > Signed-off-by: Casey Schaufler Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v3 16/16] LSM: Blob sharing support for S.A.R.A and LandLock
On Wed, Sep 19, 2018 at 5:21 PM, Casey Schaufler wrote:
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 8f3b809d7c26..0156ffea7f8c 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3475,18 +3475,16 @@ static int smack_getprocattr(struct task_struct *p,
> char *name, char **value)
> {
> struct smack_known *skp = smk_of_task_struct(p);
> char *cp;
> - int slen;
>
> - if (strcmp(name, "current") != 0)
> + if (strcmp(name, "current") == 0) {
> + cp = kstrdup(skp->smk_known, GFP_KERNEL);
> + if (cp == NULL)
> + return -ENOMEM;
> + } else
> return -EINVAL;
>
> - cp = kstrdup(skp->smk_known, GFP_KERNEL);
> - if (cp == NULL)
> - return -ENOMEM;
> -
> - slen = strlen(cp);
> *value = cp;
> - return slen;
> + return strlen(cp);
> }
This should be separate: it looks like unrelated refactoring? (I
mentioned this before, I think?)
> diff --git a/security/tomoyo/common.h b/security/tomoyo/common.h
> index 0110bebe86e2..f386f92c57c5 100644
> --- a/security/tomoyo/common.h
> +++ b/security/tomoyo/common.h
> @@ -1216,8 +1221,13 @@ static inline struct tomoyo_domain_info
> **tomoyo_cred(const struct cred *cred)
> */
> static inline struct tomoyo_domain_info *tomoyo_domain(void)
> {
> - struct tomoyo_domain_info **blob = tomoyo_cred(current_cred());
> + const struct cred *cred = current_cred();
> + struct tomoyo_domain_info **blob;
> +
> + if (cred->security == NULL)
> + return NULL;
>
> + blob = tomoyo_cred(cred);
> return *blob;
> }
I think this is another lost hunk?
-Kees
--
Kees Cook
Pixel Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v3 15/16] LSM: Infrastructure management of the ipc security blob
On Wed, Sep 19, 2018 at 5:21 PM, Casey Schaufler wrote: > LSM: Infrastructure management of the ipc security blob > > Move management of the kern_ipc_perm->security and > msg_msg->security blobs out of the individual security > modules and into the security infrastructure. Instead > of allocating the blobs from within the modules the modules > tell the infrastructure how much space is required, and > the space is allocated there. Maybe split this up too? (SELinux and Smack need tweaks?) -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v3 14/16] LSM: Infrastructure management of the task security blob
On Wed, Sep 19, 2018 at 5:21 PM, Casey Schaufler wrote:
> diff --git a/security/security.c b/security/security.c
> index 2501cdcbebff..7e11de7eec21 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -116,7 +116,8 @@ int __init security_init(void)
> #ifdef CONFIG_SECURITY_LSM_DEBUG
> pr_info("LSM: cred blob size = %d\n", blob_sizes.lbs_cred);
> pr_info("LSM: file blob size = %d\n", blob_sizes.lbs_file);
> - pr_info("LSM: inode blob size = %d\n", blob_sizes.lbs_inode);
> + pr_info("LSM: inode blob size = %d\n", blob_sizes.lbs_inode);
> + pr_info("LSM: task blob size = %d\n", blob_sizes.lbs_task);
> #endif
This alignment adjustment should be moved to the patch that adds the inode line.
(And this patch might want to mention it is changing the only task
blob user, AppArmor, or split that out into a separate patch.)
-Kees
--
Kees Cook
Pixel Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v3 10/16] LSM: Infrastructure management of the file security blob
On Wed, Sep 19, 2018 at 5:21 PM, Casey Schaufler wrote:
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 01a922856eba..62a22ad8ce92 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -361,6 +361,11 @@ static inline struct task_smack *smack_cred(const struct
> cred *cred)
> return cred->security;
> }
>
> +static inline struct smack_known **smack_file(const struct file *file)
> +{
> + return file->f_security;
> +}
> +
This (and related) still needs splitting out into it's own refactoring
patch for Smack.
-Kees
--
Kees Cook
Pixel Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v3 07/16] TOMOYO: Abstract use of cred security blob
On Wed, Sep 19, 2018 at 5:20 PM, Casey Schaufler wrote:
> @@ -539,13 +557,16 @@ DEFINE_SRCU(tomoyo_ss);
> static int __init tomoyo_init(void)
> {
> struct cred *cred = (struct cred *) current_cred();
> + struct tomoyo_domain_info **blob;
>
> if (!security_module_enable("tomoyo"))
> return 0;
> +
> /* register ourselves with the security framework */
> security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo");
> printk(KERN_INFO "TOMOYO Linux initialized\n");
> - cred->security = &tomoyo_kernel_domain;
> + blob = tomoyo_cred(cred);
> + *blob = &tomoyo_kernel_domain;
> tomoyo_mm_init();
> return 0;
> }
This is missing "tomoyo_enabled = true;" which is included in the next
patch but should be here.
-Kees
--
Kees Cook
Pixel Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Fri, Sep 14, 2018 at 8:57 AM, Casey Schaufler wrote: > On 9/13/2018 5:19 PM, Kees Cook wrote: >> We already have the minor LSMs that cannot change order. > > Are you saying that we don't have a mechanism to change > the order, or that they wouldn't work right in a different > order? Well, there's the capability module that has to be > first. I just meant their order is explicit in security.c. >> They aren't >> part of security= parsing either. > > True, but there's no reason now that we couldn't change that. > Except for capability. Hmm. Right, we have at least one that MUST be first (and must not be disabled). >> Should "blob-sharing" LSMs be like major LSMs or minor LSMs? > > I like the idea of changing the minor modules to do the full > registration process. That would make them all the same. > Except for capability. In any case, the "blob-sharing" LSMs > need to do the full registration process to account for their > blobs sizes, and that brings the "major" behavior along with it. I agree. I'm working on some clean-ups that I'll send out soon, though I'm worried about some of the various boot-time options... >> If someone is booting with "security=selinux,tomoyo" and then SARA >> lands upstream, does that person have to explicitly add "sara" to >> their boot args, since they're doing a non-default list of LSMs? > > Yes. security= is explicit. > >> (I actually prefer the answer being "yes" here, FWIW, I just want to >> nail down the expectations.) > > For now let's leave the minor (capability, yama, loadpin) as they are, > and require all new modules of any flavor to use full registration. I would even be fine to convert yama and loadpin. > We could consider something like > > security=$lsm # Stack with $lsm at priority 2 - Existing behavior > $lsm.stacked=N # Add $lsm to the stack at priority N. Delete if N == > 0 > > It's OK to specify "selinux.stacked=2" and "sara.stacked=2". Which gets > called first is left up to the system to decide. Whatever the behavior is > gets documented. Capability will always be first and have priority 1. > It's OK to specify "smack.stacked=1". I'm less excited about this kind of stacking priority, but, whatever the case, I think my cleanups may help with whatever we decide. > The default stack is determined by CONFIG_SECURITY_$lsm_STACKED at > build time. CONFIG_SECURITY_$lsm_STACKED changes from a boolean to > an integer value to establish the default hook order. > > /sys/kernel/security/lsm reports the modules in hook call order. Didn't I send a patch to new-line terminate this list? I always get annoyed when I "cat" it. ;) > /sys/kernel/security/lsm-stack reports the list with the hook call priority > > capability:1,yama:1,selinux:1,sara:5,landlack:17 > > If stacking is not configured $lsm.stacked=0 is treated as security=none. > For other values of N $lsm.stacked=N is treated as security=$lsm. I feel like "order" is bad enough. Can we avoid adding "priority"? -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Thu, Sep 13, 2018 at 4:51 PM, Casey Schaufler wrote: > On 9/13/2018 4:06 PM, Kees Cook wrote: >> If security= is >> specified, all other major LSMs are disabled (i.e. it is not possible >> to switch between SELinux/AppArmor/SMACK without also disabling >> TOMOYO). > > Correct. If we assume patch 10 is the way forward, how could we go about fixing this specific problem? -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Thu, Sep 13, 2018 at 5:08 PM, Casey Schaufler wrote: > On 9/13/2018 4:57 PM, Kees Cook wrote: >> On Thu, Sep 13, 2018 at 4:51 PM, Casey Schaufler >> wrote: >>> On 9/13/2018 4:06 PM, Kees Cook wrote: >>>> - what order should any stacking happen? Makefile? security=? >>> Makefile by default. >> Okay, if ordering is by Makefile and everyone dislikes my >> $lsm.enabled=0/1 thing, then these mean the same thing: >> >> security=selinux,tomoyo >> security=tomoyo,selinux >> >> i.e. order of security= is _ignored_ in favor of the Makefile ordering. > > No, I think that the two lines above should have a different > execution order. If we really need to specify multiple modules > at boot time that is what makes the most sense. > > It's a matter of mechanics and probably another pass during the > init process, but it's doable. If we determine it's necessary for > this stage it is just work. We already have the minor LSMs that cannot change order. They aren't part of security= parsing either. To enable/disable LoadPin, you do "loadpin.enabled=1/0" separate from "security=". Should "blob-sharing" LSMs be like major LSMs or minor LSMs? If someone is booting with "security=selinux,tomoyo" and then SARA lands upstream, does that person have to explicitly add "sara" to their boot args, since they're doing a non-default list of LSMs? (I actually prefer the answer being "yes" here, FWIW, I just want to nail down the expectations.) -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Thu, Sep 13, 2018 at 5:03 PM, Casey Schaufler wrote: > On 9/13/2018 4:51 PM, Kees Cook wrote: >> So, before we can really make a decision, I think we have to decide: >> should ordering be arbitrary for even this level of stacking? > > Do we have a case where it matters? I know that I could write a > module that would have issues if one hook got called and another > didn't because because a precursor module hook failed. I don't > think that any of the existing modules have this problem. FWIW, I prefer having explicit ordering that cannot be changed at runtime. I'm just concerned about painting ourselves (further) into a corner with security= suddenly gaining ordering semantics, but maybe I can just ignore this and we can point and laugh at anyone who gets burned by some future change to making it order-sensitive. :) -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Thu, Sep 13, 2018 at 4:32 PM, John Johansen wrote: > On 09/13/2018 04:06 PM, Kees Cook wrote: >> - what order should any stacking happen? Makefile? security=? >> > Preferably not. For the single LSM we have the ability to choose the default > LSM, ideally we let the distro decide in the Kconfig and the user with > security=... I can't find a non-crazy way to do this in Kconfig. Right now, if I threw out all the _DEFAULT stuff, I could do: config SECURITY_SELINUX_ENABLED bool "SELinux LSM enabled at boot time" depends on SECURITY_SELINUX depends on !SECURITY_APPARMOR_ENABLED && !SECURITY_SMACK_ENABLED default SECURITY_SELINUX config SECURITY_SMACK_ENABLED bool "SMACK LSM enabled at boot time" depends on SECURITY_SMACK depends on !SECURITY_APPARMOR_ENABLED && !SECURITY_SELINUX_ENABLED default SECURITY_SMACK config SECURITY_APPARMOR_ENABLED bool "AppArmor LSM enabled at boot time" depends on SECURITY_APPARMOR depends on !SECURITY_SMACK_ENABLED && !SECURITY_SELINUX_ENABLED default SECURITY_APPARMOR config SECURITY_TOMOYO_ENABLED bool "TOMOYO LSM enabled at boot time" depends on SECURITY_TOMOYO default y if !SECURITY_SELINUX_ENABLED && !SECURITY_SMACK_ENABLED && !SECURITY_APPARMOR_ENABLED config DEFAULT_SECURITY string default "selinux" if SECURITY_SELINUX_ENABLED default "smack" if SECURITY_SMACK_ENABLED default "apparmor" if SECURITY_APPARMOR_ENABLED default "tomoyo" if SECURITY_TOMOYO_ENABLED (As before CONFIG_DEFAULT_SECURITY basically means the effective "security=" contents. Reminder than Kconfig default are "first match", so tomoyo would only happen if all others are not enabled by default.) But this doesn't provide a way for Kconfig to declare the ordering of TOMOYO followed by SELinux. If we just declare ordering is a function of the Makefile, then the above would work as expected. The "conflicting major LSM" could be specified on "security=" and stacked could be enabled with $lsm.enable=1 (or disabled). So, before we can really make a decision, I think we have to decide: should ordering be arbitrary for even this level of stacking? -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Thu, Sep 13, 2018 at 4:51 PM, Casey Schaufler wrote: > On 9/13/2018 4:06 PM, Kees Cook wrote: >> - what order should any stacking happen? Makefile? security=? > Makefile by default. Okay, if ordering is by Makefile and everyone dislikes my $lsm.enabled=0/1 thing, then these mean the same thing: security=selinux,tomoyo security=tomoyo,selinux i.e. order of security= is _ignored_ in favor of the Makefile ordering. That seems dangerous to accept input that is "out of order", though, if we ever DO want to make order definable. -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Thu, Sep 13, 2018 at 2:51 PM, Kees Cook wrote:
> At the very least, to avoid stacking now (i.e. TOMOYO being enabled
> with another major LSM), we just do nothing. The existing code already
> makes the existing major LSMs exclusive. Adding a stackable LSM would
> need to just have its own "enable" flag (i.e. ignore
> security_module_enable()), and then either check a runtime "is
> stacking allowed?" flag or have new "depends on SECURITY_STACKING". (I
> think the CONFIG will force distros into enabling it without any
> runtime opt-out.)
Before stacking, we have:
- major LSM, pick one
- all CONFIG minor LSMs, in security.c order
There are two minor LSMs: Yama and LoadPin. If built, Yama is always
on (though it has sysctl knobs). If built, LoadPin is controlled by a
boot param.
Picking the major LSM happens via "security=$LSM" and a per-LSM check
of security_module_enable("$LSM").
Ordering is major, then per security.c for minors.
Under stacking, we have:
The minor LSMs remain unchanged.
We don't have SARA and Landlock yet, but we do have TOMOYO, which we
can use as an example to future "compatible blob-using LSMs".
I see two issues:
- how to determine which set of LSMs are enabled at boot
- how to determine the ORDER of the LSMs
Casey's implementation does this (correct me if I'm wrong):
The minor LSMs remain unchanged.
SECURITY_$lsm_STACKED determines which major is enabled, with
SECURITY_TOMOYO_STACKED allowed in addition. If security= is
specified, all other major LSMs are disabled (i.e. it is not possible
to switch between SELinux/AppArmor/SMACK without also disabling
TOMOYO).
Ordering is per security/Makefile modulo enabled-ness for majors (i.e.
TOMOYO is always _before_ AppArmor if stacked together, otherwise
after SELinux and SMACK), and per security.c for minors.
I don't think this is how we want it to work. For example, Ubuntu
builds in all LSMs, and default-enables AppArmor. If an Ubuntu user
wants TOMOYO, the boot with "security=tomoyo". If Ubuntu wants to make
stacking available to users but off by default, what CONFIGs do they
pick? They could try SECURITY_APPARMOR_STACKED=y and
SECURITY_TOMOYO_STACKED=n, but then how does an end user choose
"apparmor and tomoyo" (or more meaningfully, for the future:
"apparmor, sara, and landlock")? They can still pick
"security=tomoyo", but there isn't a runtime way to opt into stacking.
What about leaving SECURITY_$lsm_DEFAULT as-is, and then...
In the past I'd suggested using "security=" to determine both enabled
and order: "security=tomoyo,smack" would mean stacked LSMs, with
tomoyo going first.
Currently I'm leaning towards "security=" to select ONLY the
incompatible LSM, and per-LSM "enable" flags to determine stacking:
tomoyo.enabled=1 security=smack
This doesn't explicitly address ordering, though. If we made param
_position_ meaningful, then we could get ordering (i.e. above would
mean "tomoyo first").
Note, ordering matters because call_int_hook() will _stop_ on a
non-zero return value: potentially hiding events from later LSMs. Do
we need to revisit this? I seem to remember if being a very dead
horse, and we needed to quick-abort otherwise we ended up in
nonsensical states.
The reason for the new approach is because I can't find a meaningful
way to provide CONFIGs that make sense. We want to provide a few
things:
- is an LSM built into the kernel at all? (CONFIG_SECURITY_$lsm)
- is an LSM enabled by default? (CONFIG_SECURITY_$lsm_ENABLED?)
- has an LSM been enable for this boot? $lsm.enabled=1 or security=$lsm,$lsm ?
- what order should any stacking happen? Makefile? security=?
And for the incompatible-major, do we stick with CONFIG_SECURITY_$lsm_DEFAULT ?
Anyway, if the concern is with exposed behavior for distros, what do
we want? i.e. what should be done for patch 10. Everything else looks
good.
-Kees
--
Kees Cook
Pixel Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Thu, Sep 13, 2018 at 2:38 PM, Paul Moore wrote: > The infrastructure bits aren't really my concern; in fact I *like* > that the infrastructure is always exercised, it makes > testing/debugging easier. I also like the ability to limit the > user/admin to one LSM at boot time to make support easier; my goal is > to allow a distro to build support for multiple LSMs without also > requiring that distro to support *stacked* LSMs I see your point, but as soon as SARA and Landlock appear, they'll have: depends on SECURITY_STACKING and then all distros will enable it and there will be no sensible runtime way to manage it. If, instead, we make it entirely runtime now, then a CONFIG can control the default state and we can provide guidance to how SARA and Landlock should expose their "enable"ness. At the very least, to avoid stacking now (i.e. TOMOYO being enabled with another major LSM), we just do nothing. The existing code already makes the existing major LSMs exclusive. Adding a stackable LSM would need to just have its own "enable" flag (i.e. ignore security_module_enable()), and then either check a runtime "is stacking allowed?" flag or have new "depends on SECURITY_STACKING". (I think the CONFIG will force distros into enabling it without any runtime opt-out.) > (see my earlier > comments about the difficulty in determining the source of a failed > operation). Agreed. I would hope that audit could help for that case. *stare at blue sky* -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 04/10] LSM: Infrastructure management of the cred security blob
On Thu, Sep 13, 2018 at 12:01 PM, Casey Schaufler
wrote:
> On 9/12/2018 4:53 PM, Kees Cook wrote:
>> On Tue, Sep 11, 2018 at 9:41 AM, Casey Schaufler
>> wrote:
>>> Move management of the cred security blob out of the
>>> security modules and into the security infrastructure.
>>> Instead of allocating and freeing space the security
>>> modules tell the infrastructure how much space they
>>> require.
>> There's a lot of changes here that I think deserve some longer
>> discussion in the changelog. Notably, now we run _two_ cycles of init
>> calls? That seems... odd. Notes below...
>
> The first pass adds up the blob sizes. This has to be done because
> some modules allocate blobs during the init phase. I have investigated
> alternatives, including blobs that include information about what they
> contain, but they're all significantly more complicated.
Agreed: I liked the idea of not burdening each LSM with a state
machine, but I guess it's not much. Note that "finished" then should
be __initdata.
>>> - if (selinux_is_enabled() && cred->security) {
>>> - if ((unsigned long) cred->security < PAGE_SIZE)
>>> - return true;
>>> - if ((*(u32 *)cred->security & 0xff00) ==
>>> - (POISON_FREE << 24 | POISON_FREE << 16 | POISON_FREE <<
>>> 8))
>>> - return true;
>> These aren't unreasonable checks -- can we add them back in later?
>> (They don't need to be selinux specific, in fact: the LSM could do the
>> poison...)
>
> I had asked the maintainers about the value of these checks, and
> they said that they were primarily there for debugging during the
> original cred breakout development. I'd have not problem making them
> infrastructure managed if there's a strong desire to keep them.
Since it was only SELinux doing this, and it was from old debugging,
then I concur: drop it. It would be easy to restore (and for all LSMs)
in the future.
>>> +static inline void set_cred_label(const struct cred *cred,
>>> + struct aa_label *label)
>>> +{
>>> + struct aa_label **blob = cred->security;
>>> +
>>> + AA_BUG(!blob);
>>> + *blob = label;
>>> +}
>> This feels like it should be a separate patch? Shouldn't AA not be
>> playing with cred->security directly before we get to this "sizing and
>> allocation" patch?
>
> You're correct. This is a harmless patch break-up mistake. The end
> result of the set is correct, and the interim state works as intended,
> albeit with unnecessary code.
I much prefer lots of small easy-to-understand patches than trying to
piece together many separate things. Let's please break out each LSM's
changes and any refactoring into separate patches. Complexity is
harder to review than quantity, IMO.
>>> + */
>>> +void lsm_early_cred(struct cred *cred)
>>> +{
>>> + int rc;
>>> +
>>> + if (cred == NULL)
>>> + panic("%s: NULL cred.\n", __func__);
>> I have been given strongly worded advice to never BUG nor panic. I would:
>>
>> if (WARN_ON(!cred || !cred->security))
>>return;
>>
>>> + if (cred->security != NULL)
>>> + return;
>>> + rc = lsm_cred_alloc(cred, GFP_KERNEL);
>>> + if (rc)
>>> + panic("%s: Early cred alloc failed.\n", __func__);
>> And:
>>
>> WARN_ON(lsm_cred_alloc(cred, GFP_KERNEL));
>
> This duplicates the previous behavior from the SELinux and Smack code.
> A WARN_ON will result in an immediate panic when the calling code tries
> to dereference the blob.
I guess what I'm trying to say is, if you have a patch that does:
+ panic()
or
+ BUG()
and it has "security" anywhere in the topic, you run an extremely high
risk of Linus NAKing the entire series.
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 9d6cdd21acb6..9b49698754a7 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -213,12 +213,9 @@ static void cred_init_security(void)
>>> struct cred *cred = (struct cred *) current->real_cred;
>>> struct task_security_struct *tsec;
>>>
>>> - tsec = kzalloc(sizeof(struct task_security_struct), GFP_KERNEL);
>>> - if (!tsec)
>>> - panic(&qu
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Thu, Sep 13, 2018 at 12:12 PM, Paul Moore wrote:
> None of the above deals with the user experience or support burden a
> distro would have by forcing stacking on. If we make it an option the
Just to make sure we're clear here: this series does not provide
"extreme" stacking: SELinux, AppArmor, and SMACK remain boot-exclusive
no matter what the CONFIGs.
> distros can choose for themselves; picking a kernel build config is
> not something new to distros, and I think Casey's text adequately
> explains CONFIG_SECURITY_STACKING in terms that would be sufficient.
I absolutely want stacking to be configurable, but I want to point out
that there is no operational difference between
CONFIG_SECURITY_STACKING=n and CONFIG_SECURITY_STACKING=y in the code
here:
- all the new accessor and allocation code is exercised in both cases
- with stacking enabled: selinux, apparmor, and smack have an offset
of 0 into blobs (and only one can be enabled at a time)
- with stacking disabled: selinux, apparmor, and smack have an offset
of 0 into blobs (and only one can be enabled at a time)
The only behavioral difference is TOMOYO:
1- with stacking disabled and TOMOYO as the only major LSM, it will
have a 0 offset into blobs (like above)
2- with stacking enabled and TOMOYO as the only major LSM, it will
have a 0 offset into blobs (like above)
3- with stacking disabled and another major LSM is enabled, TOMOYO
will be disabled (like always)
4- with stacking enabled and another major LSM is enabled, TOMOYO will
have a non-0 offset into blobs and will run after selinux or smack or
run before apparmor (based on link ordering defined by the Makefile).
Note that cases 1, 2, and 3 are identical in behavior to before this
series. Only case 4 is different, which is why I'm saying that instead
of creating a redundant and needlessly complex config, or reinventing
the "enable" wheel, we should simply drop the no-op
CONFIG_SECURITY_STACKING config and provide TOMOYO with an "enable"
parameter (and CONFIG). And it should be _separate_ from the
"security=" line.
This will be the SAME outcome for distros: if they want stacking, they
choose the "enable TOMOYO by default" CONFIG. If they don't want
stacking, they don't.
> I currently have a neutral stance on stacking, making it mandatory
> pushes me more towards a "no".
This is why I'm trying to explain myself: the infrastructure proposed
here is always exercised, no matter the CONFIG. From that sense it is
"mandatory" no matter what the config is. There isn't a reality where
you could "turn off stacking", because it's not stacking until you
actually stack something, and that will be disabled by default as I've
proposed it.
Let me put this another way: if we simply leave off patch 10, we can
take the other 9 patches (modulo feedback), and we only have to decide
how to expose "stacking"; all the infrastructure work for supporting
it is done.
I'm arguing that "security=" is likely insufficient to describe what
we want, and instead we should focus on individual LSM enablement via
parameters ("tomoyo.enabled=1"). If _ordering_ becomes an issue, we
could either use parameter order, or use "security=" again maybe, but
for now, ordering is already defined by the Makefile (and
security/security.c).
"Stacking" only exists if you try to enable one of [selinux, apparmor,
or smack] AND tomoyo. CONFIG_SECURITY_STACKING is redundant: if you
want to disable stacking, you just disable tomoyo if you have another
LSM (which we can already enforce in the Kconfig).
If you want something more explicit than per-LSM config, then a simple
"security.lsm_stack=1/0" with a CONFIG for the default would be fine.
I'm trying to argue against what appears to be needless complexity
around CONFIG_SECURITY_STACK as it was proposed, since it doesn't
provide a meaningful operational change, since exclusivity of major
LSMs is already handled.
> As far as the cpp ifdef's, and other conditionals are concerned, I
> remain unconvinced this is any worse than any other significant
> feature that is a build time option.
That's fine. That's just a code style issue. What I'm trying to show
is that by lifting the allocation logic up out of the LSMs, we've
actually simplified the logic. The "stacking" part will only become a
distro-choice once they knowingly enable TOMOYO or build in SARA
and/or Landlock in the future.
-Kees
--
Kees Cook
Pixel Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
On Thu, Sep 13, 2018 at 6:16 AM, Paul Moore wrote: > On Thu, Sep 13, 2018 at 12:19 AM Kees Cook wrote: >> On Tue, Sep 11, 2018 at 9:42 AM, Casey Schaufler >> wrote: >> > Two proposed security modules require the ability to >> > share security blobs with existing "major" security modules. >> > These modules, S.A.R.A and LandLock, provide significantly >> > different services than SELinux, Smack or AppArmor. Using >> > either in conjunction with the existing modules is quite >> > reasonable. S.A.R.A requires access to the cred blob, while >> > LandLock uses the cred, file and inode blobs. >> > >> > The use of the cred, file and inode blobs has been >> > abstracted in preceding patches in the series. This >> > patch teaches the affected security modules how to access >> > the part of the blob set aside for their use in the case >> > where blobs are shared. The configuration option >> > CONFIG_SECURITY_STACKING identifies systems where the >> > blobs may be shared. >> > >> > The mechanism for selecting which security modules are >> > active has been changed to allow non-conflicting "major" >> > security modules to be used together. At this time the >> > TOMOYO module can safely be used with any of the others. >> > The two new modules would be non-conflicting as well. >> > >> > Signed-off-by: Casey Schaufler >> > --- >> > Documentation/admin-guide/LSM/index.rst | 14 +++-- >> > include/linux/lsm_hooks.h | 2 +- >> > security/Kconfig| 81 + >> > security/apparmor/include/cred.h| 8 +++ >> > security/apparmor/include/file.h| 9 ++- >> > security/apparmor/include/lib.h | 4 ++ >> > security/apparmor/lsm.c | 8 ++- >> > security/security.c | 30 - >> > security/selinux/hooks.c| 3 +- >> > security/selinux/include/objsec.h | 18 +- >> > security/smack/smack.h | 19 +- >> > security/smack/smack_lsm.c | 17 +++--- >> > security/tomoyo/common.h| 12 +++- >> > security/tomoyo/tomoyo.c| 3 +- >> > 14 files changed, 200 insertions(+), 28 deletions(-) > > ... > >> > diff --git a/security/Kconfig b/security/Kconfig >> > index 22f7664c4977..ed48025ae9e0 100644 >> > --- a/security/Kconfig >> > +++ b/security/Kconfig >> > @@ -36,6 +36,28 @@ config SECURITY_WRITABLE_HOOKS >> > bool >> > default n >> > >> > +config SECURITY_STACKING >> > + bool "Security module stacking" >> > + depends on SECURITY >> > + help >> > + Allows multiple major security modules to be stacked. >> > + Modules are invoked in the order registered with a >> > + "bail on fail" policy, in which the infrastructure >> > + will stop processing once a denial is detected. Not >> > + all modules can be stacked. SELinux, Smack and AppArmor are >> > + known to be incompatible. User space components may >> > + have trouble identifying the security module providing >> > + data in some cases. >> > + >> > + If you select this option you will have to select which >> > + of the stackable modules you wish to be active. The >> > + "Default security module" will be ignored. The boot line >> > + "security=" option can be used to specify that one of >> > + the modules identifed for stacking should be used instead >> > + of the entire stack. >> > + >> > + If you are unsure how to answer this question, answer N. >> >> I don't see a good reason to make this a config. Why shouldn't this >> always be enabled? > > I do. From a user perspective it is sometimes difficult to determine > the reason behind a failed operation; its is a DAC based denial, the > LSM, or some other failure? Stacking additional LSMs has the > potential to make this worse. The boot time configuration adds to the > complexity. Let me try to convince you otherwise. :) The reason I think there's no need for this is because the only functional change here is how _TOMOYO_ gets stacked. And in my proposal, we can convert TOMOYO to be enabled/disabled like LoadPin. Given the configs I showed, stack
Re: [PATCH 06/10] LSM: Infrastructure management of the file security blob
ty_cache = kmem_cache_create("selinux_file_security",
> - sizeof(struct
> file_security_struct),
> - 0, SLAB_PANIC, NULL);
> avc_init();
>
> avtab_cache_init();
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 0c6dce446825..043525a52e94 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -362,6 +362,11 @@ static inline struct task_smack *smack_cred(const struct
> cred *cred)
> return cred->security;
> }
>
> +static inline struct smack_known **smack_file(const struct file *file)
> +{
> + return file->f_security;
> +}
> +
> /*
> * Is the directory transmuting?
> */
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index a06ea8aa89c4..d1430341798f 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1571,24 +1571,12 @@ static void smack_inode_getsecid(struct inode *inode,
> u32 *secid)
> */
> static int smack_file_alloc_security(struct file *file)
> {
> - struct smack_known *skp = smk_of_current();
> + struct smack_known **blob = smack_file(file);
>
> - file->f_security = skp;
> + *blob = smk_of_current();
> return 0;
> }
>
> -/**
> - * smack_file_free_security - clear a file security blob
> - * @file: the object
> - *
> - * The security blob for a file is a pointer to the master
> - * label list, so no memory is freed.
> - */
> -static void smack_file_free_security(struct file *file)
> -{
> - file->f_security = NULL;
> -}
> -
> /**
> * smack_file_ioctl - Smack check on ioctls
> * @file: the object
> @@ -1813,7 +1801,9 @@ static int smack_mmap_file(struct file *file,
> */
> static void smack_file_set_fowner(struct file *file)
> {
> - file->f_security = smk_of_current();
> + struct smack_known **blob = smack_file(file);
> +
> + *blob = smk_of_current();
> }
Is this supposed to be part of a separate patch to prepare smack for
the infrastructure change?
Otherwise, seems good.
-Kees
>
> /**
> @@ -1830,6 +1820,7 @@ static void smack_file_set_fowner(struct file *file)
> static int smack_file_send_sigiotask(struct task_struct *tsk,
> struct fown_struct *fown, int signum)
> {
> + struct smack_known **blob;
> struct smack_known *skp;
> struct smack_known *tkp = smk_of_task(smack_cred(tsk->cred));
> struct file *file;
> @@ -1842,7 +1833,8 @@ static int smack_file_send_sigiotask(struct task_struct
> *tsk,
> file = container_of(fown, struct file, f_owner);
>
> /* we don't log here as rc can be overriden */
> - skp = file->f_security;
> + blob = smack_file(file);
> + skp = *blob;
> rc = smk_access(skp, tkp, MAY_DELIVER, NULL);
> rc = smk_bu_note("sigiotask", skp, tkp, MAY_DELIVER, rc);
> if (rc != 0 && has_capability(tsk, CAP_MAC_OVERRIDE))
> @@ -4626,6 +4618,7 @@ static int smack_dentry_create_files_as(struct dentry
> *dentry, int mode,
>
> struct lsm_blob_sizes smack_blob_sizes = {
> .lbs_cred = sizeof(struct task_smack),
> + .lbs_file = sizeof(struct smack_known *),
> };
>
> static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
> @@ -4663,7 +4656,6 @@ static struct security_hook_list smack_hooks[]
> __lsm_ro_after_init = {
> LSM_HOOK_INIT(inode_getsecid, smack_inode_getsecid),
>
> LSM_HOOK_INIT(file_alloc_security, smack_file_alloc_security),
> - LSM_HOOK_INIT(file_free_security, smack_file_free_security),
> LSM_HOOK_INIT(file_ioctl, smack_file_ioctl),
> LSM_HOOK_INIT(file_lock, smack_file_lock),
> LSM_HOOK_INIT(file_fcntl, smack_file_fcntl),
> --
> 2.17.1
>
>
--
Kees Cook
Pixel Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 08/10] Smack: Abstract use of inode security blob
On Tue, Sep 11, 2018 at 9:42 AM, Casey Schaufler wrote: > Don't use the inode->i_security pointer directly. > Provide a helper function that provides the security blob pointer. > > Signed-off-by: Casey Schaufler Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock
+542,7 @@ static int __init tomoyo_init(void)
{
struct cred *cred = (struct cred *) current_cred();
- if (!security_module_enable("tomoyo"))
+ if (!enabled)
return 0;
/* register ourselves with the security framework */
security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo");
@@ -550,4 +552,8 @@ static int __init tomoyo_init(void)
return 0;
}
+/* Should not be mutable after boot, so not listed in sysfs (perm == 0). */
+module_param(enabled, int, 0);
+MODULE_PARM_DESC(enabled, "Tomoyo enabled at boot");
+
security_initcall(tomoyo_init);
(I prefer LSMs do enablement with module params so that they leave
their namespace open for other runtime configuration. I think
"apparmor=" and "selinux=" for enable/disable isn't good to
replicate.)
We will quickly encounter "LSM ordering" as an issue, but not in this
case yet: there's no new LSM. Ordering is preserved even with my
suggestion: major order is controlled by Makefile link ordering, and
minor is controlled by explicit ordering in security/security.c's
add_hooks() calls.
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 6617abb51732..be14540ce09c 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3493,18 +3493,16 @@ static int smack_getprocattr(struct task_struct *p,
> char *name, char **value)
> {
> struct smack_known *skp = smk_of_task_struct(p);
> char *cp;
> - int slen;
>
> - if (strcmp(name, "current") != 0)
> + if (strcmp(name, "current") == 0) {
> + cp = kstrdup(skp->smk_known, GFP_KERNEL);
> + if (cp == NULL)
> + return -ENOMEM;
> + } else
> return -EINVAL;
>
> - cp = kstrdup(skp->smk_known, GFP_KERNEL);
> - if (cp == NULL)
> - return -ENOMEM;
> -
> - slen = strlen(cp);
> *value = cp;
> - return slen;
> + return strlen(cp);
> }
This refactoring seems out of place?
-Kees
--
Kees Cook
Pixel Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 07/10] SELinux: Abstract use of inode security blob
On Tue, Sep 11, 2018 at 9:42 AM, Casey Schaufler wrote: > Don't use the inode->i_security pointer directly. > Provide a helper function that provides the security blob pointer. > > Signed-off-by: Casey Schaufler Happily mechanical! :) Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 05/10] SELinux: Abstract use of file security blob
On Tue, Sep 11, 2018 at 9:41 AM, Casey Schaufler wrote: > Don't use the file->f_security pointer directly. > Provide a helper function that provides the security blob pointer. > > Signed-off-by: Casey Schaufler Seems delightfully mechanical. Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 09/10] LSM: Infrastructure management of the inode security
> - struct inode_smack *issp = smack_inode(inode);
> -
> - /*
> -* The inode may still be referenced in a path walk and
> -* a call to smack_inode_permission() can be made
> -* after smack_inode_free_security() is called.
> -* To avoid race condition free the i_security via RCU
> -* and leave the current inode->i_security pointer intact.
> -* The inode will be freed after the RCU grace period too.
> -*/
> - call_rcu(&issp->smk_rcu, smack_inode_free_rcu);
> -}
> -
> /**
> * smack_inode_init_security - copy out the smack from an inode
> * @inode: the newly created inode
> @@ -4619,6 +4571,7 @@ static int smack_dentry_create_files_as(struct dentry
> *dentry, int mode,
> struct lsm_blob_sizes smack_blob_sizes = {
> .lbs_cred = sizeof(struct task_smack),
> .lbs_file = sizeof(struct smack_known *),
> + .lbs_inode = sizeof(struct inode_smack),
> };
>
> static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
> @@ -4637,7 +4590,6 @@ static struct security_hook_list smack_hooks[]
> __lsm_ro_after_init = {
> LSM_HOOK_INIT(bprm_set_creds, smack_bprm_set_creds),
>
> LSM_HOOK_INIT(inode_alloc_security, smack_inode_alloc_security),
> - LSM_HOOK_INIT(inode_free_security, smack_inode_free_security),
> LSM_HOOK_INIT(inode_init_security, smack_inode_init_security),
> LSM_HOOK_INIT(inode_link, smack_inode_link),
> LSM_HOOK_INIT(inode_unlink, smack_inode_unlink),
> --
> 2.17.1
>
>
I continue to really like this series: I think pulling the memory
management up into the core LSM makes things much cleaner.
-Kees
--
Kees Cook
Pixel Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 04/10] LSM: Infrastructure management of the cred security blob
return 0;
>
> tomoyo_dir = securityfs_create_dir("tomoyo", NULL);
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 9f932e2d6852..bb84e6ec3886 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -18,7 +18,9 @@
> */
> static int tomoyo_cred_alloc_blank(struct cred *new, gfp_t gfp)
> {
> - new->security = NULL;
> + struct tomoyo_domain_info **blob = tomoyo_cred(new);
> +
> + *blob = NULL;
> return 0;
> }
>
> @@ -34,8 +36,13 @@ static int tomoyo_cred_alloc_blank(struct cred *new, gfp_t
> gfp)
> static int tomoyo_cred_prepare(struct cred *new, const struct cred *old,
>gfp_t gfp)
> {
> - struct tomoyo_domain_info *domain = old->security;
> - new->security = domain;
> + struct tomoyo_domain_info **old_blob = tomoyo_cred(old);
> + struct tomoyo_domain_info **new_blob = tomoyo_cred(new);
> + struct tomoyo_domain_info *domain;
> +
> + domain = *old_blob;
> + *new_blob = domain;
> +
> if (domain)
> atomic_inc(&domain->users);
> return 0;
> @@ -59,7 +66,9 @@ static void tomoyo_cred_transfer(struct cred *new, const
> struct cred *old)
> */
> static void tomoyo_cred_free(struct cred *cred)
> {
> - struct tomoyo_domain_info *domain = cred->security;
> + struct tomoyo_domain_info **blob = tomoyo_cred(cred);
> + struct tomoyo_domain_info *domain = *blob;
> +
> if (domain)
> atomic_dec(&domain->users);
> }
> @@ -73,6 +82,9 @@ static void tomoyo_cred_free(struct cred *cred)
> */
> static int tomoyo_bprm_set_creds(struct linux_binprm *bprm)
> {
> + struct tomoyo_domain_info **blob;
> + struct tomoyo_domain_info *domain;
> +
> /*
> * Do only if this function is called for the first time of an execve
> * operation.
> @@ -93,13 +105,14 @@ static int tomoyo_bprm_set_creds(struct linux_binprm
> *bprm)
> * stored inside "bprm->cred->security" will be acquired later inside
> * tomoyo_find_next_domain().
> */
> - atomic_dec(&((struct tomoyo_domain_info *)
> -bprm->cred->security)->users);
> + blob = tomoyo_cred(bprm->cred);
> + domain = *blob;
> + atomic_dec(&domain->users);
> /*
> * Tell tomoyo_bprm_check_security() is called for the first time of
> an
> * execve operation.
> */
> - bprm->cred->security = NULL;
> + *blob = NULL;
> return 0;
> }
>
> @@ -112,8 +125,11 @@ static int tomoyo_bprm_set_creds(struct linux_binprm
> *bprm)
> */
> static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
> {
> - struct tomoyo_domain_info *domain = bprm->cred->security;
> + struct tomoyo_domain_info **blob;
> + struct tomoyo_domain_info *domain;
>
> + blob = tomoyo_cred(bprm->cred);
> + domain = *blob;
> /*
> * Execute permission is checked against pathname passed to
> do_execve()
> * using current domain.
> @@ -493,6 +509,10 @@ static int tomoyo_socket_sendmsg(struct socket *sock,
> struct msghdr *msg,
> return tomoyo_socket_sendmsg_permission(sock, msg, size);
> }
>
> +struct lsm_blob_sizes tomoyo_blob_sizes = {
> + .lbs_cred = sizeof(struct tomoyo_domain_info *),
> +};
> +
> /*
> * tomoyo_security_ops is a "struct security_operations" which is used for
> * registering TOMOYO.
> @@ -531,6 +551,8 @@ static struct security_hook_list tomoyo_hooks[]
> __lsm_ro_after_init = {
> /* Lock for GC. */
> DEFINE_SRCU(tomoyo_ss);
>
> +bool tomoyo_enabled;
> +
> /**
> * tomoyo_init - Register TOMOYO Linux as a LSM module.
> *
> @@ -538,14 +560,28 @@ DEFINE_SRCU(tomoyo_ss);
> */
> static int __init tomoyo_init(void)
> {
> + static int finish;
> struct cred *cred = (struct cred *) current_cred();
> + struct tomoyo_domain_info **blob;
> +
> + if (!security_module_enable("tomoyo")) {
> + tomoyo_enabled = false;
> + return 0;
> + }
> + tomoyo_enabled = true;
>
> - if (!security_module_enable("tomoyo"))
> + if (!finish) {
> + security_add_blobs(&tomoyo_blob_sizes);
> + finish = 1;
> return 0;
> + }
> +
> /* register ourselves with the security framework */
> security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo");
> printk(KERN_INFO "TOMOYO Linux initialized\n");
> - cred->security = &tomoyo_kernel_domain;
> + lsm_early_cred(cred);
> + blob = tomoyo_cred(cred);
> + *blob = &tomoyo_kernel_domain;
> tomoyo_mm_init();
> return 0;
> }
> --
> 2.17.1
>
>
-Kees
--
Kees Cook
Pixel Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 01/10] procfs: add smack subdir to attrs
On Tue, Sep 11, 2018 at 9:41 AM, Casey Schaufler wrote:
> Back in 2007 I made what turned out to be a rather serious
> mistake in the implementation of the Smack security module.
> The SELinux module used an interface in /proc to manipulate
> the security context on processes. Rather than use a similar
> interface, I used the same interface. The AppArmor team did
> likewise. Now /proc/.../attr/current will tell you the
> security "context" of the process, but it will be different
> depending on the security module you're using.
>
> This patch provides a subdirectory in /proc/.../attr for
> Smack. Smack user space can use the "current" file in
> this subdirectory and never have to worry about getting
> SELinux attributes by mistake. Programs that use the
> old interface will continue to work (or fail, as the case
> may be) as before.
>
> The proposed S.A.R.A security module is dependent on
> the mechanism to create its own attr subdirectory.
>
> The original implementation is by Kees Cook.
>
> Signed-off-by: Casey Schaufler
> ---
> Documentation/admin-guide/LSM/index.rst | 13 +++--
> fs/proc/base.c | 64 +
> fs/proc/internal.h | 1 +
> include/linux/security.h| 15 --
> security/security.c | 24 --
> 5 files changed, 96 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/admin-guide/LSM/index.rst
> b/Documentation/admin-guide/LSM/index.rst
> index c980dfe9abf1..9842e21afd4a 100644
> --- a/Documentation/admin-guide/LSM/index.rst
> +++ b/Documentation/admin-guide/LSM/index.rst
> @@ -17,9 +17,8 @@ MAC extensions, other extensions can be built using the LSM
> to provide
> specific changes to system operation when these tweaks are not available
> in the core functionality of Linux itself.
>
> -Without a specific LSM built into the kernel, the default LSM will be the
> -Linux capabilities system. Most LSMs choose to extend the capabilities
> -system, building their checks on top of the defined capability hooks.
> +The Linux capabilities modules will always be included. This may be
> +followed by any number of "minor" modules and at most one "major" module.
> For more details on capabilities, see ``capabilities(7)`` in the Linux
> man-pages project.
>
> @@ -30,6 +29,14 @@ order in which checks are made. The capability module will
> always
> be first, followed by any "minor" modules (e.g. Yama) and then
> the one "major" module (e.g. SELinux) if there is one configured.
>
> +Process attributes associated with "major" security modules should
> +be accessed and maintained using the special files in ``/proc/.../attr``.
> +A security module may maintain a module specific subdirectory there,
> +named after the module. ``/proc/.../attr/smack`` is provided by the Smack
> +security module and contains all its special files. The files directly
> +in ``/proc/.../attr`` remain as legacy interfaces for modules that provide
> +subdirectories.
> +
> .. toctree::
> :maxdepth: 1
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index ccf86f16d9f0..bd2dd85310fe 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -140,9 +140,13 @@ struct pid_entry {
> #define REG(NAME, MODE, fops) \
> NOD(NAME, (S_IFREG|(MODE)), NULL, &fops, {})
> #define ONE(NAME, MODE, show) \
> - NOD(NAME, (S_IFREG|(MODE)), \
> + NOD(NAME, (S_IFREG|(MODE)), \
> NULL, &proc_single_file_operations, \
> { .proc_show = show } )
> +#define ATTR(LSM, NAME, MODE) \
> + NOD(NAME, (S_IFREG|(MODE)), \
> + NULL, &proc_pid_attr_operations,\
> + { .lsm = LSM })
>
> /*
> * Count the number of hardlinks for the pid_entry table, excluding the .
> @@ -2503,7 +2507,7 @@ static ssize_t proc_pid_attr_read(struct file * file,
> char __user * buf,
> if (!task)
> return -ESRCH;
>
> - length = security_getprocattr(task,
> + length = security_getprocattr(task, PROC_I(inode)->op.lsm,
> (char*)file->f_path.dentry->d_name.name,
> &p);
> put_task_struct(task);
> @@ -2552,7 +2556,9 @@ static ssize_t proc_pid_attr_write(struct file * file,
> const char __user * buf,
> if (rv < 0)
> goto out_free;
>
> - rv = security_setprocattr(file->f_path.dentry->d_na
Re: [PATCH 02/10] Smack: Abstract use of cred security blob
On Tue, Sep 11, 2018 at 9:41 AM, Casey Schaufler wrote:
> Don't use the cred->security pointer directly.
> Provide a helper function that provides the security blob pointer.
>
> Signed-off-by: Casey Schaufler
> ---
> security/smack/smack.h| 14 +++--
> security/smack/smack_access.c | 4 +--
> security/smack/smack_lsm.c| 57 +--
> security/smack/smackfs.c | 18 +--
> 4 files changed, 50 insertions(+), 43 deletions(-)
>
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index f7db791fb566..0b55d6a55b26 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -356,6 +356,11 @@ extern struct list_head smack_onlycap_list;
> #define SMACK_HASH_SLOTS 16
> extern struct hlist_head smack_known_hash[SMACK_HASH_SLOTS];
>
> +static inline struct task_smack *smack_cred(const struct cred *cred)
> +{
> + return cred->security;
> +}
> +
> /*
> * Is the directory transmuting?
> */
> @@ -382,13 +387,16 @@ static inline struct smack_known *smk_of_task(const
> struct task_smack *tsp)
> return tsp->smk_task;
> }
>
> -static inline struct smack_known *smk_of_task_struct(const struct
> task_struct *t)
> +static inline struct smack_known *smk_of_task_struct(
> + const struct task_struct *t)
> {
> struct smack_known *skp;
> + const struct cred *cred;
>
> rcu_read_lock();
> - skp = smk_of_task(__task_cred(t)->security);
> + cred = __task_cred(t);
> rcu_read_unlock();
> + skp = smk_of_task(smack_cred(cred));
Hm, why is this safe? (i.e. what is pinning the cred?) I would expect
get_cred()/put_cred() since this is not for "current"? And then what
controls the skp lifetime?
Everything else looks to be mechanical replacement, so that's fine.
Did you use some tooling to do the mechanical replacement or was it
done by hand?
-Kees
--
Kees Cook
Pixel Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 03/10] SELinux: Abstract use of cred security blob
On Tue, Sep 11, 2018 at 9:41 AM, Casey Schaufler wrote: > Don't use the cred->security pointer directly. > Provide a helper function that provides the security blob pointer. > > Signed-off-by: Casey Schaufler Like smack, this seems to be largely: s/$identifier->security/selinux_cred($identifier)/ s/current_security()/selinux_cred(current_cred())/ Is that right? The one __task_cred() use seemed to be fully contained under rcu read lock. Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: WARNING in apparmor_secid_to_secctx
On Tue, Sep 4, 2018 at 7:53 AM, Dmitry Vyukov wrote: > Again, if you change wheezy to stretch here, it should reproduce the problem: > https://github.com/google/syzkaller/blob/master/tools/create-image.sh FWIW, I sent an update for the image version here: https://github.com/google/syzkaller/pull/708 -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v1 00/22] LSM: Full security module stacking
On Tue, Aug 14, 2018 at 4:50 PM, Casey Schaufler wrote: > On 8/14/2018 4:22 PM, Jordan Glover wrote: >> On August 14, 2018 8:28 PM, Casey Schaufler wrote: >> >>> >>>>> The blob management part (through "LSM: Sharing of security blobs") >>>>> is ready for prime-time. These changes move the management of >>>>> security blobs out of the security modules and into the security >>>>> module infrastructure. With this change the proposed S.A.R.A, >>>>> LandLock and PTAGS security modules could co-exist with any of >>>>> the existing "major" security modules. The changes reduce some >>>>> code duplication. >>>>> Beyond the blob management there's a bit of clean-up. >>>>> Mounting filesystems had to be changed so that options >>>>> a security module doesn't recognize won't be considered >>>>> a fatal error. The mount infrastructure is somewhat >>>>> more complex than one might assume. >>>> Casey, >>>> Do you think you can break out 1 into its own patch? It seems like >>>> that'd be valuable to everyone. >>> Yes, I think that is a good idea. Landlock, S.A.R.A. and a couple >>> other security modules could be added upstream if this part of the >>> work was available. It would not provide everything needed to stack >>> all the existing modules. I believe there is concern that if this >>> much went upstream the work on finishing what's required to make >>> everything work might be abandoned. >>> >> On the other hand there is concern that those security modules might >> be abandoned if they have to wait until everything is finished :) > > There is some truth to that. If we can get commitment from the developers > of those security module to push for getting upstream, a statement of > intent to support additional modules (e.g. Landlock, S.A.R.A.) from a > significant distribution (e.g. Fedora, Ubuntu, SuSE) and ACKs from the > maintainers of the existing modules we should be able to breeze right in. > > Yeah, I think that's about all it would take. I would strongly recommend Landlock and SARA for every distro. They're opt-in, and provide much-needed missing userspace defenses (and attack surface reduction). -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v2 2/8] exec: Move security_bprm_secureexec() earlier
On Mon, Jul 10, 2017 at 7:07 PM, Kees Cook wrote: > On Mon, Jul 10, 2017 at 10:18 AM, Eric W. Biederman > wrote: >> Kees Cook writes: >> >>> On Mon, Jul 10, 2017 at 1:57 AM, Eric W. Biederman >>> wrote: >>>> Kees Cook writes: >>>> >>>>> There are several places where exec needs to know if a privilege-gain has >>>>> happened. These should be using the results of security_bprm_secureexec() >>>>> but it is getting (needlessly) called very late. >>>> >>>> It is hard to tell at a glance but I believe this introduces a >>>> regression. >>>> >>>> cap_bprm_set_creds is currently called before cap_bprm_secureexec and >>>> it has a number of cases such as no_new_privs and ptrace that can result >>>> in some of the precomputed credential changes not happening. >>>> >>>> Without accounting for that I believe your cap_bprm_securexec now >>>> returns a postive value too early. >>> >>> It's still before cap_bprm_secureexec. cap_brpm_set_creds() is in >>> prepare_binprm(), which is well before exec_binprm() and it's eventual >>> call to setup_new_exec(). >> >> Good point. I didn't double check and the set in the name had me >> thinking it was setting the creds on current. >> >> Is there any reason we need a second security hook? It feels like we >> should be able to just fold the secureexec hook into the set_creds hook. >> >> The two are so interrelated I fear that having them separate only >> encourages them to diverge in trivial ways as it is easy to forget about >> some detail or other. >> >> Certainly having them called from different functions seems wrong. If >> we know enough in prepare_binprm we know enough. > > Hmmm, yes. That would let us have the secureexec-ness knowledge before > copy_strings(), in case we ever need to make that logic > secureexec-aware. > > I'll dig through the LSMs to examine the set_creds hooks to see if > this could be possible. So, yes, after digging, this is very possible. In fact, it's highly desirable. Both commoncaps and AppArmor save state into the bprm struct between bprm_set_creds and bprm_secureexec explicitly to return a sane value from bprm_secureexec. (And Smack and SELinux both have trivial tests that just repeat from bprm_set_creds.) I've reworked the series to just remove bprm_secureexec entirely. It comes out nicely, removing more than it adds: 14 files changed, 54 insertions(+), 144 deletions(-) I'll send the patches in the morning (perhaps to go through -mm since it touches fs/exec.c, binfmt_elf*.c, and security/). -Kees -- Kees Cook Pixel Security
Re: [PATCH v2 2/8] exec: Move security_bprm_secureexec() earlier
On Mon, Jul 10, 2017 at 10:18 AM, Eric W. Biederman wrote: > Kees Cook writes: > >> On Mon, Jul 10, 2017 at 1:57 AM, Eric W. Biederman >> wrote: >>> Kees Cook writes: >>> >>>> There are several places where exec needs to know if a privilege-gain has >>>> happened. These should be using the results of security_bprm_secureexec() >>>> but it is getting (needlessly) called very late. >>> >>> It is hard to tell at a glance but I believe this introduces a >>> regression. >>> >>> cap_bprm_set_creds is currently called before cap_bprm_secureexec and >>> it has a number of cases such as no_new_privs and ptrace that can result >>> in some of the precomputed credential changes not happening. >>> >>> Without accounting for that I believe your cap_bprm_securexec now >>> returns a postive value too early. >> >> It's still before cap_bprm_secureexec. cap_brpm_set_creds() is in >> prepare_binprm(), which is well before exec_binprm() and it's eventual >> call to setup_new_exec(). > > Good point. I didn't double check and the set in the name had me > thinking it was setting the creds on current. > > Is there any reason we need a second security hook? It feels like we > should be able to just fold the secureexec hook into the set_creds hook. > > The two are so interrelated I fear that having them separate only > encourages them to diverge in trivial ways as it is easy to forget about > some detail or other. > > Certainly having them called from different functions seems wrong. If > we know enough in prepare_binprm we know enough. Hmmm, yes. That would let us have the secureexec-ness knowledge before copy_strings(), in case we ever need to make that logic secureexec-aware. I'll dig through the LSMs to examine the set_creds hooks to see if this could be possible. -Kees -- Kees Cook Pixel Security
Re: [PATCH v2 2/8] exec: Move security_bprm_secureexec() earlier
On Mon, Jul 10, 2017 at 1:57 AM, Eric W. Biederman wrote: > Kees Cook writes: > >> There are several places where exec needs to know if a privilege-gain has >> happened. These should be using the results of security_bprm_secureexec() >> but it is getting (needlessly) called very late. > > It is hard to tell at a glance but I believe this introduces a > regression. > > cap_bprm_set_creds is currently called before cap_bprm_secureexec and > it has a number of cases such as no_new_privs and ptrace that can result > in some of the precomputed credential changes not happening. > > Without accounting for that I believe your cap_bprm_securexec now > returns a postive value too early. It's still before cap_bprm_secureexec. cap_brpm_set_creds() is in prepare_binprm(), which is well before exec_binprm() and it's eventual call to setup_new_exec(). -Kees -- Kees Cook Pixel Security
Re: [PATCH v2 1/8] exec: Correct comments about "point of no return"
On Mon, Jul 10, 2017 at 1:46 AM, Eric W. Biederman wrote: > > But you miss it. > > The "point of no return" is the call to de_thread. Or aguably anything in > flush_old_exec. Once anything in the current task is modified you can't > return an error. > > It very much does not have anything to do with brpm.It has > everything to do with current. Yes, but the thing that actually enforces this is the test of bprm->mm and the SIGSEGV in search_binary_handlers(). -Kees > > >> diff --git a/fs/exec.c b/fs/exec.c >> index 904199086490..7842ae661e34 100644 >> --- a/fs/exec.c >> +++ b/fs/exec.c >> @@ -1285,7 +1285,14 @@ int flush_old_exec(struct linux_binprm * bprm) >> if (retval) >> goto out; >> >> - bprm->mm = NULL;/* We're using it now */ >> + /* >> + * After clearing bprm->mm (to mark that current is using the >> + * prepared mm now), we are at the point of no return. If >> + * anything from here on returns an error, the check in >> + * search_binary_handler() will kill current (since the mm has >> + * been replaced). >> + */ >> + bprm->mm = NULL; >> >> set_fs(USER_DS); >> current->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD | > > Eric -- Kees Cook Pixel Security
[PATCH v2 7/8] exec: Consolidate pdeath_signal clearing
Instead of an additional secureexec check for pdeath_signal, just move it
up into the initial secureexec test. Neither perf nor arch code touches
pdeath_signal, so the relocation shouldn't change anything.
Signed-off-by: Kees Cook
---
fs/exec.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index eeb8323977d1..e0186db02f90 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1340,6 +1340,9 @@ void setup_new_exec(struct linux_binprm * bprm)
if (security_bprm_secureexec(bprm)) {
/* Record for AT_SECURE. */
bprm->secureexec = 1;
+
+ /* Make sure parent cannot signal privileged process. */
+ current->pdeath_signal = 0;
}
arch_pick_mmap_layout(current->mm);
@@ -1363,10 +1366,6 @@ void setup_new_exec(struct linux_binprm * bprm)
*/
current->mm->task_size = TASK_SIZE;
- if (bprm->secureexec) {
- current->pdeath_signal = 0;
- }
-
/* An exec changes our domain. We are no longer part of the thread
group */
current->self_exec_id++;
--
2.7.4
[PATCH v2 2/8] exec: Move security_bprm_secureexec() earlier
There are several places where exec needs to know if a privilege-gain has
happened. These should be using the results of security_bprm_secureexec()
but it is getting (needlessly) called very late.
Instead, move this earlier in the exec code, to the start of the point
of no return in setup_new_exec(). Here, the new creds have already
been calculated (and stored in bprm->cred), which is normally what
security_bprm_secureexec() wants to examine. Since it's moved earlier,
LSMs hooking bprm_secureexec() need to be adjusted to use the creds in
bprm:
$ git grep LSM_HOOK_INIT.*bprm_secureexec
apparmor/lsm.c:LSM_HOOK_INIT(bprm_secureexec, apparmor_bprm_secureexec),
commoncap.c: LSM_HOOK_INIT(bprm_secureexec, cap_bprm_secureexec),
selinux/hooks.c: LSM_HOOK_INIT(bprm_secureexec, selinux_bprm_secureexec),
smack/smack_lsm.c: LSM_HOOK_INIT(bprm_secureexec, smack_bprm_secureexec),
AppArmor does not access creds in apparmor_bprm_secureexec.
Capabilities needed to be adjusted to use bprm creds.
SELinux needed to be adjusted to use bprm creds for the security structure.
Smack needed to be adjusted to use bprm creds for the security structure.
The result of the bprm_secureexec() hook is saved in a new bprm field
"secureexec" so it can be queried later (just AT_SECURE currently).
Signed-off-by: Kees Cook
---
fs/binfmt_elf.c| 2 +-
fs/binfmt_elf_fdpic.c | 2 +-
fs/exec.c | 5 +
include/linux/binfmts.h| 3 ++-
include/linux/lsm_hooks.h | 3 ++-
security/commoncap.c | 4 +---
security/selinux/hooks.c | 2 +-
security/smack/smack_lsm.c | 2 +-
8 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 5075fd5c62c8..7f6ec4dac13d 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -254,7 +254,7 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr
*exec,
NEW_AUX_ENT(AT_EUID, from_kuid_munged(cred->user_ns, cred->euid));
NEW_AUX_ENT(AT_GID, from_kgid_munged(cred->user_ns, cred->gid));
NEW_AUX_ENT(AT_EGID, from_kgid_munged(cred->user_ns, cred->egid));
- NEW_AUX_ENT(AT_SECURE, security_bprm_secureexec(bprm));
+ NEW_AUX_ENT(AT_SECURE, bprm->secureexec);
NEW_AUX_ENT(AT_RANDOM, (elf_addr_t)(unsigned long)u_rand_bytes);
#ifdef ELF_HWCAP2
NEW_AUX_ENT(AT_HWCAP2, ELF_HWCAP2);
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index cf93a4fad012..5aa9199dfb13 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -650,7 +650,7 @@ static int create_elf_fdpic_tables(struct linux_binprm
*bprm,
NEW_AUX_ENT(AT_EUID,(elf_addr_t) from_kuid_munged(cred->user_ns,
cred->euid));
NEW_AUX_ENT(AT_GID, (elf_addr_t) from_kgid_munged(cred->user_ns,
cred->gid));
NEW_AUX_ENT(AT_EGID,(elf_addr_t) from_kgid_munged(cred->user_ns,
cred->egid));
- NEW_AUX_ENT(AT_SECURE, security_bprm_secureexec(bprm));
+ NEW_AUX_ENT(AT_SECURE, bprm->secureexec);
NEW_AUX_ENT(AT_EXECFN, bprm->exec);
#ifdef ARCH_DLINFO
diff --git a/fs/exec.c b/fs/exec.c
index 7842ae661e34..b92e37fb53aa 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1337,6 +1337,11 @@ EXPORT_SYMBOL(would_dump);
void setup_new_exec(struct linux_binprm * bprm)
{
+ if (security_bprm_secureexec(bprm)) {
+ /* Record for AT_SECURE. */
+ bprm->secureexec = 1;
+ }
+
arch_pick_mmap_layout(current->mm);
current->sas_ss_sp = current->sas_ss_size = 0;
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 05488da3aee9..1afaa303cad0 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -27,9 +27,10 @@ struct linux_binprm {
unsigned int
cred_prepared:1,/* true if creds already prepared (multiple
* preps happen for interpreters) */
- cap_effective:1;/* true if has elevated effective capabilities,
+ cap_effective:1,/* true if has elevated effective capabilities,
* false if not; except for init which inherits
* its parent's caps anyway */
+ secureexec:1; /* true when gaining privileges */
#ifdef __alpha__
unsigned int taso:1;
#endif
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 080f34e66017..d1bd24fb4a33 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -72,7 +72,8 @@
* Return a boolean value (0 or 1) indicating whether a "secure exec"
* is required. The flag is passed in the auxiliary table
* on the initial stack to the ELF interpreter to indicate whether libc
- * should enable secure mode.
+ * should enable secure mode. Called before bprm_committing_creds(),
+ * so pending credentials are in @bprm->cred.
* @bprm contains the li
[PATCH v2 6/8] exec: Consolidate dumpability logic
Since it's already valid to set dumpability in the early part of
setup_new_exec(), we can consolidate the logic into a single place.
The BINPRM_FLAGS_ENFORCE_NONDUMP is set during would_dump() calls
before setup_new_exec(), so its test is safe to move as well.
Signed-off-by: Kees Cook
---
fs/exec.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index d7bda5b60e7b..eeb8323977d1 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1346,10 +1346,12 @@ void setup_new_exec(struct linux_binprm * bprm)
current->sas_ss_sp = current->sas_ss_size = 0;
- if (!bprm->secureexec)
- set_dumpable(current->mm, SUID_DUMP_USER);
- else
+ /* Figure out dumpability. */
+ if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP ||
+ bprm->secureexec)
set_dumpable(current->mm, suid_dumpable);
+ else
+ set_dumpable(current->mm, SUID_DUMP_USER);
arch_setup_new_exec();
perf_event_exec();
@@ -1363,9 +1365,6 @@ void setup_new_exec(struct linux_binprm * bprm)
if (bprm->secureexec) {
current->pdeath_signal = 0;
- } else {
- if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP)
- set_dumpable(current->mm, suid_dumpable);
}
/* An exec changes our domain. We are no longer part of the thread
--
2.7.4
[PATCH v2 1/8] exec: Correct comments about "point of no return"
In commit 221af7f87b97 ("Split 'flush_old_exec' into two functions"),
the comment about the point of no return should have stayed in
flush_old_exec() since it refers to "bprm->mm = NULL;" line, but prior
changes in commits c89681ed7d0e ("remove steal_locks()"), and
fd8328be874f ("sanitize handling of shared descriptor tables in failing
execve()") made it look like it meant the current->sas_ss_sp line instead.
The comment is referring to the fact that once bprm->mm is NULL, all
failures from a binfmt load_binary hook (e.g. load_elf_binary), will
get SEGV raised against current. Move this comment and expand the
explanation a bit, putting it above the assignment this time.
This also removes an erroneous commet about when credentials are being
installed. That has its own dedicated function, install_exec_creds(),
which carries a similar (and correct) comment, so remove the bogus comment
where installation is not actually happening.
Signed-off-by: Kees Cook
---
fs/exec.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 904199086490..7842ae661e34 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1285,7 +1285,14 @@ int flush_old_exec(struct linux_binprm * bprm)
if (retval)
goto out;
- bprm->mm = NULL;/* We're using it now */
+ /*
+* After clearing bprm->mm (to mark that current is using the
+* prepared mm now), we are at the point of no return. If
+* anything from here on returns an error, the check in
+* search_binary_handler() will kill current (since the mm has
+* been replaced).
+*/
+ bprm->mm = NULL;
set_fs(USER_DS);
current->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
@@ -1332,7 +1339,6 @@ void setup_new_exec(struct linux_binprm * bprm)
{
arch_pick_mmap_layout(current->mm);
- /* This is the point of no return */
current->sas_ss_sp = current->sas_ss_size = 0;
if (uid_eq(current_euid(), current_uid()) && gid_eq(current_egid(),
current_gid()))
@@ -1350,7 +1356,6 @@ void setup_new_exec(struct linux_binprm * bprm)
*/
current->mm->task_size = TASK_SIZE;
- /* install the new credentials */
if (!uid_eq(bprm->cred->uid, current_euid()) ||
!gid_eq(bprm->cred->gid, current_egid())) {
current->pdeath_signal = 0;
--
2.7.4
[PATCH v2 8/8] exec: Use sane stack rlimit under secureexec
For a secureexec, before memory layout selection has happened, reset the stack rlimit to something sane to avoid the caller having control over the resulting layouts. $ ulimit -s 8192 $ ulimit -s unlimited $ /bin/sh -c 'ulimit -s' unlimited $ sudo /bin/sh -c 'ulimit -s' 8192 Signed-off-by: Kees Cook --- fs/exec.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/fs/exec.c b/fs/exec.c index e0186db02f90..1e3463854a16 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1343,6 +1343,16 @@ void setup_new_exec(struct linux_binprm * bprm) /* Make sure parent cannot signal privileged process. */ current->pdeath_signal = 0; + + /* +* For secureexec, reset the stack limit to sane default to +* avoid bad behavior from the prior rlimits. This has to +* happen before arch_pick_mmap_layout(), which examines +* RLIMIT_STACK, but after the point of no return to avoid +* needing to clean up the change on failure. +*/ + if (current->signal->rlim[RLIMIT_STACK].rlim_cur > _STK_LIM) + current->signal->rlim[RLIMIT_STACK].rlim_cur = _STK_LIM; } arch_pick_mmap_layout(current->mm); -- 2.7.4
[PATCH v2 5/8] smack: Remove redundant pdeath_signal clearing
This removes the redundant pdeath_signal clearing in Smack: the check in
smack_bprm_committing_creds() matches the check in smack_bprm_secureexec(),
and since secureexec is now being checked for clearing pdeath_signal, this
is redundant to the common exec code.
Signed-off-by: Kees Cook
---
security/smack/smack_lsm.c | 15 ---
1 file changed, 15 deletions(-)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 13cf9e66d5fe..4b10b782aecc 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -954,20 +954,6 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
}
/**
- * smack_bprm_committing_creds - Prepare to install the new credentials
- * from bprm.
- *
- * @bprm: binprm for exec
- */
-static void smack_bprm_committing_creds(struct linux_binprm *bprm)
-{
- struct task_smack *bsp = bprm->cred->security;
-
- if (bsp->smk_task != bsp->smk_forked)
- current->pdeath_signal = 0;
-}
-
-/**
* smack_bprm_secureexec - Return the decision to use secureexec.
* @bprm: binprm for exec
*
@@ -4645,7 +4631,6 @@ static struct security_hook_list smack_hooks[]
__lsm_ro_after_init = {
LSM_HOOK_INIT(sb_parse_opts_str, smack_parse_opts_str),
LSM_HOOK_INIT(bprm_set_creds, smack_bprm_set_creds),
- LSM_HOOK_INIT(bprm_committing_creds, smack_bprm_committing_creds),
LSM_HOOK_INIT(bprm_secureexec, smack_bprm_secureexec),
LSM_HOOK_INIT(inode_alloc_security, smack_inode_alloc_security),
--
2.7.4
[PATCH v2 4/8] exec: Use secureexec for clearing pdeath_signal
Like dumpability, clearing pdeath_signal happens both in setup_new_exec()
and later in commit_creds(). The test in setup_new_exec() is different
from all other privilege comparisons, though: it is checking the new cred
(bprm) uid vs the old cred (current) euid. This appears to be a bug,
introduced by commit a6f76f23d297 ("CRED: Make execve() take advantage of
copy-on-write credentials"):
- if (bprm->e_uid != current_euid() ||
- bprm->e_gid != current_egid()) {
- set_dumpable(current->mm, suid_dumpable);
+ /* install the new credentials */
+ if (bprm->cred->uid != current_euid() ||
+ bprm->cred->gid != current_egid()) {
It was bprm euid vs current euid (and egids), but the effective got
dropped. Nothing in the exec flow changes bprm->cred->uid (nor gid).
The call traces are:
prepare_bprm_creds()
prepare_exec_creds()
prepare_creds()
memcpy(new_creds, old_creds, ...)
security_prepare_creds() (unimplemented by commoncap)
...
prepare_binprm()
bprm_fill_uid()
resets euid/egid to current euid/egid
sets euid/egid on bprm based on set*id file bits
security_bprm_set_creds()
cap_bprm_set_creds()
handle all caps-based manipulations
so this test is effectively a test of current_uid() vs current_euid(),
which is wrong, just like the prior dumpability tests were wrong.
The commit log says "Clear pdeath_signal and set dumpable on
certain circumstances that may not be covered by commit_creds()." This
may be meaning the earlier old euid vs new euid (and egid) test that
got changed.
Luckily, as with dumpability, this is all masked by commit_creds()
which performs old/new euid and egid tests and clears pdeath_signal.
And again, like dumpability, we should include LSM secureexec logic for
pdeath_signal clearing. For example, Smack goes out of its way to clear
pdeath_signal when it finds a secureexec condition.
Signed-off-by: Kees Cook
---
fs/exec.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 3e519d4f0bd3..d7bda5b60e7b 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1361,8 +1361,7 @@ void setup_new_exec(struct linux_binprm * bprm)
*/
current->mm->task_size = TASK_SIZE;
- if (!uid_eq(bprm->cred->uid, current_euid()) ||
- !gid_eq(bprm->cred->gid, current_egid())) {
+ if (bprm->secureexec) {
current->pdeath_signal = 0;
} else {
if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP)
--
2.7.4
[PATCH v2 0/8] exec: Use sane stack rlimit under secureexec
As discussed with Linus and Andy, we need to reset the stack rlimit before we do memory layouts when execing a privilege-gaining (e.g. setuid) program. This moves security_bprm_secureexec() earlier (with required changes), and then lowers the stack limit when appropriate. As a side-effect, dumpability and pdeath_signal clearing is expanded to cover LSM definitions of secureexec (and Smack can drop its special handler for pdeath_signal clearing). I'd appreciate some extra eyes on this to make sure this isn't broken in some special way. I couldn't find anything that _depended_ on security_bprm_secureexec() being called late. Thanks! -Kees v2: - fix missed current_security() uses in LSMs. - research/consolidate dumpability setting logic - research/consolidate pdeath_signal clearing logic - split up logical steps a little more for easier review (and bisection) - fix some old broken comments
[PATCH v2 3/8] exec: Use secureexec for setting dumpability
The examination of "current" to decide dumpability is wrong. This was a check of and euid/uid (or egid/gid) mismatch in the existing process, not the newly created one. This appears to stretch back into even the "history.git" tree. Luckily, dumpability is later set in commit_creds(). In earlier kernel versions before creds existed, similar checks also existed late in the exec flow, covering up the mistake as far back as I could find. The commit_creds() check examines differences of euid, uid, egid, gid, and capabilities between the old and new creds. It would look like the setup_new_exec() dumpability test could be entirely removed, but strictly speaking, the secureexec test covers a different set of tests than what commit_creds() checks for. So, fix this test to use secureexec, which includes the same logical check (euid != uid || egid != gid), but checks bprm->cred, not current->cred. One would wonder if we need a security_commit_creds() LSM hook and to move the existing checks in commit_creds() into commoncaps.c, which would allow expanding the logic to all LSMs. Currently this doesn't seem needed, though. Signed-off-by: Kees Cook --- fs/exec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/exec.c b/fs/exec.c index b92e37fb53aa..3e519d4f0bd3 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1346,7 +1346,7 @@ void setup_new_exec(struct linux_binprm * bprm) current->sas_ss_sp = current->sas_ss_size = 0; - if (uid_eq(current_euid(), current_uid()) && gid_eq(current_egid(), current_gid())) + if (!bprm->secureexec) set_dumpable(current->mm, SUID_DUMP_USER); else set_dumpable(current->mm, suid_dumpable); -- 2.7.4
Re: [kernel-hardening] [RFC v2 PATCH 0/2] security: mark LSM hooks with __ro_after_init
On Tue, Feb 14, 2017 at 5:15 AM, James Morris wrote: > Updated and simplified down to two patches. > > Following feedback from the list, I've added a new config option to handle > the case where SELinux still needs to disable its hooks at runtime (and > thus the hooks must be writable in that case). > > I've dropped the Netfilter hooks patch as I realized that the hook ops > list structures could be modified after init by the core NF code. > > The SELinux Netlink message patch has been merged, and Mimi is reviewing > the IMA default policy patch (it's not affected by LSM hook requirements > and can be merged separately). > > --- > > James Morris (2): > security: introduce CONFIG_SECURITY_WRITABLE_HOOKS > security: mark LSM hooks as __ro_after_init Please consider these both: Acked-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook
On Mon, Jul 18, 2016 at 1:11 PM, John Stultz wrote:
> As requested, this patch implements a task_settimerslack and
> task_gettimerslack LSM hooks so that the /proc//timerslack_ns
> interface can have finer grained security policies applied to it.
>
> I've kept the CAP_SYS_NICE check in the timerslack_ns_write/show
> functions, as hiding it in the LSM hook seems too opaque, and doesn't
> seem like a widely enough adopted practice.
Yeah, I think this does make it more readable in the end.
>
> Don't really know what I'm doing here, so close review would be
> appreciated!
>
> Cc: Kees Cook
> Cc: "Serge E. Hallyn"
> Cc: Andrew Morton
> Cc: Thomas Gleixner
> CC: Arjan van de Ven
> Cc: Oren Laadan
> Cc: Ruchi Kandoi
> Cc: Rom Lemarchand
> Cc: Todd Kjos
> Cc: Colin Cross
> Cc: Nick Kralevich
> Cc: Dmitry Shmidt
> Cc: Elliott Hughes
> Cc: Android Kernel Team
> Cc: linux-security-mod...@vger.kernel.org
> Cc: selinux@tycho.nsa.gov
> Signed-off-by: John Stultz
Acked-by: Kees Cook
-Kees
> ---
> v2:
> * Initial swing at adding settimerslack LSM hook
> v3:
> * Fix current/p switchup bug noted by NickK
> * Add gettimerslack hook suggested by NickK
>
> fs/proc/base.c| 10 ++
> include/linux/lsm_hooks.h | 13 +
> include/linux/security.h | 12
> security/security.c | 14 ++
> security/selinux/hooks.c | 12
> 5 files changed, 61 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index c94abae..cc66aa8 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2286,6 +2286,12 @@ static ssize_t timerslack_ns_write(struct file *file,
> const char __user *buf,
> goto out;
> }
>
> + err = security_task_settimerslack(p, slack_ns);
> + if (err) {
> + count = err;
> + goto out;
> + }
> +
> task_lock(p);
> if (slack_ns == 0)
> p->timer_slack_ns = p->default_timer_slack_ns;
> @@ -2314,6 +2320,10 @@ static int timerslack_ns_show(struct seq_file *m, void
> *v)
> goto out;
> }
>
> + ret = security_task_gettimerslack(p);
> + if (ret)
> + goto out;
> +
> task_lock(p);
> seq_printf(m, "%llu\n", p->timer_slack_ns);
> task_unlock(p);
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 7ae3976..290483e 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -627,6 +627,15 @@
> * Check permission before moving memory owned by process @p.
> * @p contains the task_struct for process.
> * Return 0 if permission is granted.
> + * @task_settimerslack:
> + * Check permission before setting timerslack value of @p to @slack.
> + * @p contains the task_struct of a process.
> + * @slack contains the new slack value.
> + * Return 0 if permission is granted.
> + * @task_gettimerslack:
> + * Check permission before returning the timerslack value of @p.
> + * @p contains the task_struct of a process.
> + * Return 0 if permission is granted.
> * @task_kill:
> * Check permission before sending signal @sig to @p. @info can be NULL,
> * the constant 1, or a pointer to a siginfo structure. If @info is 1 or
> @@ -1473,6 +1482,8 @@ union security_list_options {
> int (*task_setscheduler)(struct task_struct *p);
> int (*task_getscheduler)(struct task_struct *p);
> int (*task_movememory)(struct task_struct *p);
> + int (*task_settimerslack)(struct task_struct *p, u64 slack);
> + int (*task_gettimerslack)(struct task_struct *p);
> int (*task_kill)(struct task_struct *p, struct siginfo *info,
> int sig, u32 secid);
> int (*task_wait)(struct task_struct *p);
> @@ -1732,6 +1743,8 @@ struct security_hook_heads {
> struct list_head task_setscheduler;
> struct list_head task_getscheduler;
> struct list_head task_movememory;
> + struct list_head task_settimerslack;
> + struct list_head task_gettimerslack;
> struct list_head task_kill;
> struct list_head task_wait;
> struct list_head task_prctl;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 14df373..ab70f47 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -325,6 +325,8 @@ int security_task_setrlimit(struct task_struct *p,
> unsigned int resource,
> int security_task_setscheduler(struct task_struct *p);
> int security_task_getsch
Re: [RFC][PATCH 1/2 v2] proc: Relax /proc//timerslack_ns capability requirements
On Fri, Jul 15, 2016 at 11:42 AM, John Stultz wrote:
> On Fri, Jul 15, 2016 at 10:51 AM, Nick Kralevich wrote:
>> On Fri, Jul 15, 2016 at 10:24 AM, John Stultz wrote:
>>> + if (!capable(CAP_SYS_NICE))
>>> + return -EPERM;
>>> +
>>
>> Since you're going the LSM route (from your second patch of this
>
> Well, you suggested it, so I sent out an RFC. I'm not married to it yet. :)
>
>
>> series), the capability check above should be moved to the LSM hook in
>> security/commoncap.c. Only one security call to
>> security_task_settimerslack is needed, which will cover the standard
>> capabilities check as well as the SELinux check.
>
> Huh. Ok. I was looking at the implementation of nice(), which does:
>
> if (increment < 0 && !can_nice(current, nice))
> return -EPERM;
> retval = security_task_setnice(current, nice);
> if (retval)
> ...
>
> Which made it seem like standard checks are done first, then finer
> grain lsm checks second.
I'm on the fence about this: it can be argued that if it's a cap check
it should live in the commoncap.c checks, but most of our cap checks
for these kinds of access controls are directly in the function, prior
the the security_* calls. I've added James and Casey who may have a
more well constructed rationale for doing this one way or the other.
> (...and now you can guess where my accidental "current" usage in the
> next patch came from :)
>
>
>>
>>> p = get_proc_task(inode);
>>> if (!p)
>>> return -ESRCH;
>>>
>>
>> Per your patch #2, you'd call security_task_settimerslack here. This
>> would call into the capability LSM hook you added in
>> security/commoncap.c
>
> Though I was hoping to keep the CAP_SYS_PTRACE -> CAP_SYS_NICE change
> first, then add the LSM hooks, as it makes the needed ABI change more
> obvious. I worry swapping it around with the LSM hook being added
> first makes it significantly less obvious, as (at least for me) the
> security_task_* functions get indirect and difficult to follow quickly
> ("wait, why are we checking SETSCHED for nice?").
>
> A side curiosity: why does "git grep PROCESS__SETSCHED" miss the
> definition? Is the av_permissions.h file somehow caught by .gitignore?
>
> thanks
> -john
-Kees
--
Kees Cook
Chrome OS & Brillo Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: Linux Firmware Signing
[removed bounced email addresses]
On Wed, Sep 2, 2015 at 2:37 PM, Luis R. Rodriguez wrote:
> On Wed, Sep 02, 2015 at 01:54:43PM -0700, Kees Cook wrote:
>> On Wed, Sep 2, 2015 at 11:46 AM, Luis R. Rodriguez wrote:
>> > On Tue, Sep 01, 2015 at 11:35:05PM -0400, Mimi Zohar wrote:
>> >> > OK great, I think that instead of passing the actual routine name we
>> >> > should
>> >> > instead pass an enum type for to the LSM, that'd be easier to parse and
>> >> > we'd
>> >> > then have each case well documented. Each LSM then could add its own
>> >> > documetnation for this and can switch on it. If we went with a name
>> >> > we'd have
>> >> > to to use something like __func__ and then parse that, its not clear if
>> >> > we need
>> >> > to get that specific.
>> >>
>> >> Agreed. IMA already defines an enumeration.
>> >>
>> >> /* IMA policy related functions */
>> >> enum ima_hooks { FILE_CHECK = 1, MMAP_CHECK, BPRM_CHECK, MODULE_CHECK,
>> >> FIRMWARE_CHECK, POLICY_CHECK, POST_SETATTR };
>> >>
>> >
>> > We want something that is not only useful for IMA but any other LSM,
>> > and FILE_CHECK seems very broad, not sure what BPRM_CHECK is even upon
>> > inspecting kernel code. Likewise for POST_SETATTR. POLICY_CHECK might
>> > be broad, perhaps its best we define then a generic set of enums to
>> > which IMA can map them to then and let it decide. This would ensure
>> > that the kernel defines each use caes for file inspection carefully,
>> > documents and defines them and if an LSM wants to bunch a set together
>> > it can do so easily with a switch statement to map set of generic
>> > file checks in kernel to a group it already handles.
>> >
>> > For instance at least in the short term we'd try to unify:
>> >
>> > security_kernel_fw_from_file()
>> > security_kernel_module_from_file()
>> >
>> > to perhaps:
>> >
>> > security_kernel_from_file()
>> >
>> > As far, as far as I can tell, the only ones we'd be ready to start
>> > grouping immediately or with small amount of work rather soon:
>> >
>> > /**
>> > *
>> > * enum security_filecheck - known kernel security file checks types
>> > *
>> > * @__SECURITY_FILECHECK_UNSPEC: attribute 0 reserved
>> > * @SECURITY_FILECHECK_MODULE: the file being processed is a Linux kernel
>> > module
>> > * @SECURITY_FILECHECK_SYSDATA: the file being processed is either a
>> > firmware
>> > * file or a system data file read from /lib/firmware/* by
>> > firmware_class
>>
>> I'd prefer a distinct category for firmware, as it carries an
>> implication that it is an executable blob of some sort (I know not all
>> are, though).
>
> The ship has sailed in terms of folks using frimrware API for things
> that are not-firmware per se. The first one I am aware of was the
> EEPROM override for the p54 driver. The other similar one was CPU
> microcode, but that's a bit more close to home with "firmware". We
> could ask users on the new system data request API I am building
> to describe the type of file being used, as I agree differentiating
> this for security purposes might be important. So other than just
> file type we could have sub type category, then we could have,
>
> SECURITY_FILECHECK_SYSDATA, and then:
I object to executable code being called data. :)
> SECURITY_FILE_SYSDATA_FW
> SECURITY_FILE_SYSDATA_MICROCODE
> SECURITY_FILE_SYSDATA_EEPROM
> SECURITY_FILE_SYSDATA_POLICY (for 802.11 regulatory I suppose)
The exception to the firmware loading is data, so the primary name
should be firmware. Regardless, if we want distinct objects, just name
them:
SECURITY_FILE_FIRMWARE
SECURITY_FILE_SYSDATA
Do we need finer-grain sub types?
>
> If we do this then we could juse have:
>
> SECURITY_FILECHECK_KEXEC and on that have substypes:
>
> SECURITY_FILE_KEXEC_KERNEL
> SECURITY_FILE_KEXEC_INITRAMFS
>
> Would that be desirable and help grow this to be easily extensible?
>
> Luis
-Kees
--
Kees Cook
Chrome OS Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: Linux Firmware Signing
On Wed, Sep 2, 2015 at 11:46 AM, Luis R. Rodriguez wrote:
> On Tue, Sep 01, 2015 at 11:35:05PM -0400, Mimi Zohar wrote:
>> > OK great, I think that instead of passing the actual routine name we should
>> > instead pass an enum type for to the LSM, that'd be easier to parse and
>> > we'd
>> > then have each case well documented. Each LSM then could add its own
>> > documetnation for this and can switch on it. If we went with a name we'd
>> > have
>> > to to use something like __func__ and then parse that, its not clear if we
>> > need
>> > to get that specific.
>>
>> Agreed. IMA already defines an enumeration.
>>
>> /* IMA policy related functions */
>> enum ima_hooks { FILE_CHECK = 1, MMAP_CHECK, BPRM_CHECK, MODULE_CHECK,
>> FIRMWARE_CHECK, POLICY_CHECK, POST_SETATTR };
>>
>
> We want something that is not only useful for IMA but any other LSM,
> and FILE_CHECK seems very broad, not sure what BPRM_CHECK is even upon
> inspecting kernel code. Likewise for POST_SETATTR. POLICY_CHECK might
> be broad, perhaps its best we define then a generic set of enums to
> which IMA can map them to then and let it decide. This would ensure
> that the kernel defines each use caes for file inspection carefully,
> documents and defines them and if an LSM wants to bunch a set together
> it can do so easily with a switch statement to map set of generic
> file checks in kernel to a group it already handles.
>
> For instance at least in the short term we'd try to unify:
>
> security_kernel_fw_from_file()
> security_kernel_module_from_file()
>
> to perhaps:
>
> security_kernel_from_file()
>
> As far, as far as I can tell, the only ones we'd be ready to start
> grouping immediately or with small amount of work rather soon:
>
> /**
> *
> * enum security_filecheck - known kernel security file checks types
> *
> * @__SECURITY_FILECHECK_UNSPEC: attribute 0 reserved
> * @SECURITY_FILECHECK_MODULE: the file being processed is a Linux kernel
> module
> * @SECURITY_FILECHECK_SYSDATA: the file being processed is either a firmware
> * file or a system data file read from /lib/firmware/* by firmware_class
I'd prefer a distinct category for firmware, as it carries an
implication that it is an executable blob of some sort (I know not all
are, though).
-Kees
> * @SECURITY_FILECHECK_KEXEC_KERNEL: the file being processed is a kernel file
> * used by kexec
> * @SECURITY_FILECHECK_KEXEC_INITRAMFS: the file being processed is an
> initramfs
> * used by kexec
>
> * The kernel reads files directly from the filesystem for a series of
> * operations. The list of files the kernel reads from the filesystem are
> * limited and each type of file consumed may have a different format and
> * security vetting procedures. The kernel enables LSMs to vet for these files
> * through a shared LSM hook prior to consumption. This list documents the
> * different special kernel file types read by the kernel, it enables LSMs
> * to vet for each differently if needed.
> enum security_filecheck {
> SECURITY_FILECHECK_UNSPEC,
> SECURITY_FILECHECK_MODULE,
> SECURITY_FILECHECK_SYSDATA,
> SECURITY_FILECHECK_KEXEC_KERNEL,
> SECURITY_FILECHECK_KEXEC_INITRAMFS,
> };
>
> Provided the MOK thing or alternative gets addressed we could also soon add
> something for SELinux policy files but that needs to be discussed further
> it seems. If MOK is used would SECURITY_FILECHECK_POLICY_MOK be OK? Again
> this would likely need further discussion, its why I didn't list it above.
>
> Luis
--
Kees Cook
Chrome OS Security
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: Linux Firmware Signing
On Tue, Sep 1, 2015 at 8:44 PM, Mimi Zohar wrote: > On Tue, 2015-09-01 at 20:08 -0700, Kees Cook wrote: >> On Tue, Sep 1, 2015 at 4:43 PM, Luis R. Rodriguez wrote: >> > On Mon, Aug 31, 2015 at 10:18:55AM -0400, Mimi Zohar wrote: >> >> > > eBPF/seccomp >> > >> > OK I knew nothing about this but I just looked into it, here are my notes: >> > >> > * old BPF - how far do we want to go? This goes so far as to parsing >> > user passed void __user *arg data through ioctls which typically >> > gets copy_from_user()'d and eventually gets BPF_PROG_RUN(). >> > >> > * eBPF: >> > seccomp() & prctl_set_seccomp() >> > | >> > V >> > do_seccomp() >> > | >> > V >> > seccomp_set_mode_filter() >> > | >> > V >> > seccomp_prepare_user_filter() >> > | >> > V >> > bpf_prog_create_from_user() (seccomp) \ >> > bpf_prog_create() > bpf_prepare_filter() >> > sk_attach_filter()/ >> > >> > All approaches come from user passed data, nothing fd based. >> > >> > For both old BPF and eBPF then: >> > >> > If we wanted to be paranoid I suppose the Machine Owner Key (MOK) >> > Paul had mentioned up could be used to vet for passed filters, or >> > a new interface to enable fd based filters. This really would limit >> > the dynamic nature of these features though. >> > >> > eBPF / secccomp would not be the only place in the kernel that would >> > have >> > issues with user passed data, we have tons of places the same applies >> > so >> > implicating the old BPF / eBPF / seccomp approaches can easily >> > implicate >> > many other areas of the kernel, that's pretty huge but from the looks >> > of >> > it below you seem to enable that to be a possibility for us to >> > consider. >> >> At the time (LSS 2014?) I argued that seccomp policies come from >> binaries, which are already being measured. And that policies only >> further restrict a process, so there seems to be to be little risk in >> continuing to leave them unmeasured. > > What do you mean by "measured"? Who is doing the measurement? Could > someone detect a change in measurement? I meant from the perspective of IMA. The binary would have already been evaluated when it executed, and it's what's installing the seccomp filter. And since seccomp filters can only reduce privilege, it seems like they're not worth getting processed by IMA. But I might not understand the requirements! :) -Kees -- Kees Cook Chrome OS Security ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
