| Message ID | 20220420140633.753772-14-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: Namespace IMA with audit support in IMA-ns | expand |
On Wed, Apr 20, 2022 at 10:06:20AM -0400, Stefan Berger wrote: > Add a pointer to ima_namespace to the user_namespace and initialize > the init_user_ns with a pointer to init_ima_ns. We need a pointer from > the user namespace to its associated IMA namespace since IMA namespaces > are piggybacking on user namespaces. > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > Acked-by: Christian Brauner <brauner@kernel.org> > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> > > --- > v11: > - Added lost A-b from Christian back > - Added sentence to patch description explaining why we need the pointer > > v9: > - Deferred implementation of ima_ns_from_user_ns() to later patch > --- > include/linux/ima.h | 2 ++ > include/linux/user_namespace.h | 4 ++++ > kernel/user.c | 4 ++++ > 3 files changed, 10 insertions(+) > > diff --git a/include/linux/ima.h b/include/linux/ima.h > index 426b1744215e..fcb60a44e05f 100644 > --- a/include/linux/ima.h > +++ b/include/linux/ima.h > @@ -14,6 +14,8 @@ > #include <crypto/hash_info.h> > struct linux_binprm; > > +extern struct ima_namespace init_ima_ns; > + > #ifdef CONFIG_IMA > extern enum hash_algo ima_get_current_hash_algo(void); > extern int ima_bprm_check(struct linux_binprm *bprm); > diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h > index 33a4240e6a6f..019e8cf7b633 100644 > --- a/include/linux/user_namespace.h > +++ b/include/linux/user_namespace.h > @@ -36,6 +36,7 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */ > #define USERNS_INIT_FLAGS USERNS_SETGROUPS_ALLOWED > > struct ucounts; > +struct ima_namespace; > > enum ucount_type { > UCOUNT_USER_NAMESPACES, > @@ -99,6 +100,9 @@ struct user_namespace { > #endif > struct ucounts *ucounts; > long ucount_max[UCOUNT_COUNTS]; > +#ifdef CONFIG_IMA_NS It's probably worth putting a comment here saying that user_ns does not pin ima_ns. That the only time the ima_ns will be freed is when user_ns is freed, and only time it will be changed is when user_ns is freed, or during ima_fs_ns_init() (under smp_load_acquire) during a new mount. > + struct ima_namespace *ima_ns; So, if I create a new user_ns with a new ima_ns, and in there I create a new user_ns again, it looks like ima_ns will be NULL in the new user_ns? Should it not be set to the parent->ima_ns? (which would cause trouble for the way it's currently being freed...) > +#endif > } __randomize_layout; > > struct ucounts { > diff --git a/kernel/user.c b/kernel/user.c > index e2cf8c22b539..e5d1f4b9b8ba 100644 > --- a/kernel/user.c > +++ b/kernel/user.c > @@ -19,6 +19,7 @@ > #include <linux/export.h> > #include <linux/user_namespace.h> > #include <linux/proc_ns.h> > +#include <linux/ima.h> > > /* > * userns count is 1 for root user, 1 for init_uts_ns, > @@ -67,6 +68,9 @@ struct user_namespace init_user_ns = { > .keyring_name_list = LIST_HEAD_INIT(init_user_ns.keyring_name_list), > .keyring_sem = __RWSEM_INITIALIZER(init_user_ns.keyring_sem), > #endif > +#ifdef CONFIG_IMA_NS > + .ima_ns = &init_ima_ns, > +#endif > }; > EXPORT_SYMBOL_GPL(init_user_ns); > > -- > 2.34.1
On Sun, May 22, 2022 at 01:24:26PM -0500, Serge Hallyn wrote: > On Wed, Apr 20, 2022 at 10:06:20AM -0400, Stefan Berger wrote: > > Add a pointer to ima_namespace to the user_namespace and initialize > > the init_user_ns with a pointer to init_ima_ns. We need a pointer from > > the user namespace to its associated IMA namespace since IMA namespaces > > are piggybacking on user namespaces. > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > > Acked-by: Christian Brauner <brauner@kernel.org> > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> > > > > --- > > v11: > > - Added lost A-b from Christian back > > - Added sentence to patch description explaining why we need the pointer > > > > v9: > > - Deferred implementation of ima_ns_from_user_ns() to later patch > > --- > > include/linux/ima.h | 2 ++ > > include/linux/user_namespace.h | 4 ++++ > > kernel/user.c | 4 ++++ > > 3 files changed, 10 insertions(+) > > > > diff --git a/include/linux/ima.h b/include/linux/ima.h > > index 426b1744215e..fcb60a44e05f 100644 > > --- a/include/linux/ima.h > > +++ b/include/linux/ima.h > > @@ -14,6 +14,8 @@ > > #include <crypto/hash_info.h> > > struct linux_binprm; > > > > +extern struct ima_namespace init_ima_ns; > > + > > #ifdef CONFIG_IMA > > extern enum hash_algo ima_get_current_hash_algo(void); > > extern int ima_bprm_check(struct linux_binprm *bprm); > > diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h > > index 33a4240e6a6f..019e8cf7b633 100644 > > --- a/include/linux/user_namespace.h > > +++ b/include/linux/user_namespace.h > > @@ -36,6 +36,7 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */ > > #define USERNS_INIT_FLAGS USERNS_SETGROUPS_ALLOWED > > > > struct ucounts; > > +struct ima_namespace; > > > > enum ucount_type { > > UCOUNT_USER_NAMESPACES, > > @@ -99,6 +100,9 @@ struct user_namespace { > > #endif > > struct ucounts *ucounts; > > long ucount_max[UCOUNT_COUNTS]; > > +#ifdef CONFIG_IMA_NS > > It's probably worth putting a comment here saying that user_ns does not > pin ima_ns. > > That the only time the ima_ns will be freed is when user_ns is freed, > and only time it will be changed is when user_ns is freed, or during > ima_fs_ns_init() (under smp_load_acquire) during a new mount. > > > + struct ima_namespace *ima_ns; > > So, if I create a new user_ns with a new ima_ns, and in there I > create a new user_ns again, it looks like ima_ns will be NULL in > the new user_ns? Should it not be set to the parent->ima_ns? > (which would cause trouble for the way it's currently being > freed...) Would also work and wouldn't be difficult to do imho.
On 5/23/22 05:59, Christian Brauner wrote: > On Sun, May 22, 2022 at 01:24:26PM -0500, Serge Hallyn wrote: >> On Wed, Apr 20, 2022 at 10:06:20AM -0400, Stefan Berger wrote: >>> Add a pointer to ima_namespace to the user_namespace and initialize >>> the init_user_ns with a pointer to init_ima_ns. We need a pointer from >>> the user namespace to its associated IMA namespace since IMA namespaces >>> are piggybacking on user namespaces. >>> >>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >>> Acked-by: Christian Brauner <brauner@kernel.org> >>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> >>> >>> --- >>> v11: >>> - Added lost A-b from Christian back >>> - Added sentence to patch description explaining why we need the pointer >>> >>> v9: >>> - Deferred implementation of ima_ns_from_user_ns() to later patch >>> --- >>> include/linux/ima.h | 2 ++ >>> include/linux/user_namespace.h | 4 ++++ >>> kernel/user.c | 4 ++++ >>> 3 files changed, 10 insertions(+) >>> >>> diff --git a/include/linux/ima.h b/include/linux/ima.h >>> index 426b1744215e..fcb60a44e05f 100644 >>> --- a/include/linux/ima.h >>> +++ b/include/linux/ima.h >>> @@ -14,6 +14,8 @@ >>> #include <crypto/hash_info.h> >>> struct linux_binprm; >>> >>> +extern struct ima_namespace init_ima_ns; >>> + >>> #ifdef CONFIG_IMA >>> extern enum hash_algo ima_get_current_hash_algo(void); >>> extern int ima_bprm_check(struct linux_binprm *bprm); >>> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h >>> index 33a4240e6a6f..019e8cf7b633 100644 >>> --- a/include/linux/user_namespace.h >>> +++ b/include/linux/user_namespace.h >>> @@ -36,6 +36,7 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */ >>> #define USERNS_INIT_FLAGS USERNS_SETGROUPS_ALLOWED >>> >>> struct ucounts; >>> +struct ima_namespace; >>> >>> enum ucount_type { >>> UCOUNT_USER_NAMESPACES, >>> @@ -99,6 +100,9 @@ struct user_namespace { >>> #endif >>> struct ucounts *ucounts; >>> long ucount_max[UCOUNT_COUNTS]; >>> +#ifdef CONFIG_IMA_NS >> >> It's probably worth putting a comment here saying that user_ns does not >> pin ima_ns. >> >> That the only time the ima_ns will be freed is when user_ns is freed, >> and only time it will be changed is when user_ns is freed, or during >> ima_fs_ns_init() (under smp_load_acquire) during a new mount. >> >>> + struct ima_namespace *ima_ns; >> >> So, if I create a new user_ns with a new ima_ns, and in there I >> create a new user_ns again, it looks like ima_ns will be NULL in >> the new user_ns? Should it not be set to the parent->ima_ns? >> (which would cause trouble for the way it's currently being >> freed...) > > Would also work and wouldn't be difficult to do imho. We previously decide to create an ima_namespace when securityfs is mounted. This now also applies to nested containers where an IMA namespace is first configured with the hash and template to use in a particular container and then activated. If no configuration is done it will inherit the hash and template from the first ancestor that has been configure when it is activated. So the same steps and behavior applies to *all* containers, no difference at any depth of nesting. Besides that, we don't want nested containers to share policy and logs but keep them isolated from each other, or do we not? Further, how should it work if we were to apply this even to the first container? Should it just inherit the &init_ima_namespace and we'd have no isolation at all? Why would we start treating containers at deeper nesting levels differently?
On Mon, May 23, 2022 at 07:31:29AM -0400, Stefan Berger wrote: > > > On 5/23/22 05:59, Christian Brauner wrote: > > On Sun, May 22, 2022 at 01:24:26PM -0500, Serge Hallyn wrote: > > > On Wed, Apr 20, 2022 at 10:06:20AM -0400, Stefan Berger wrote: > > > > Add a pointer to ima_namespace to the user_namespace and initialize > > > > the init_user_ns with a pointer to init_ima_ns. We need a pointer from > > > > the user namespace to its associated IMA namespace since IMA namespaces > > > > are piggybacking on user namespaces. > > > > > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > > > > Acked-by: Christian Brauner <brauner@kernel.org> > > > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> > > > > > > > > --- > > > > v11: > > > > - Added lost A-b from Christian back > > > > - Added sentence to patch description explaining why we need the pointer > > > > > > > > v9: > > > > - Deferred implementation of ima_ns_from_user_ns() to later patch > > > > --- > > > > include/linux/ima.h | 2 ++ > > > > include/linux/user_namespace.h | 4 ++++ > > > > kernel/user.c | 4 ++++ > > > > 3 files changed, 10 insertions(+) > > > > > > > > diff --git a/include/linux/ima.h b/include/linux/ima.h > > > > index 426b1744215e..fcb60a44e05f 100644 > > > > --- a/include/linux/ima.h > > > > +++ b/include/linux/ima.h > > > > @@ -14,6 +14,8 @@ > > > > #include <crypto/hash_info.h> > > > > struct linux_binprm; > > > > +extern struct ima_namespace init_ima_ns; > > > > + > > > > #ifdef CONFIG_IMA > > > > extern enum hash_algo ima_get_current_hash_algo(void); > > > > extern int ima_bprm_check(struct linux_binprm *bprm); > > > > diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h > > > > index 33a4240e6a6f..019e8cf7b633 100644 > > > > --- a/include/linux/user_namespace.h > > > > +++ b/include/linux/user_namespace.h > > > > @@ -36,6 +36,7 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */ > > > > #define USERNS_INIT_FLAGS USERNS_SETGROUPS_ALLOWED > > > > struct ucounts; > > > > +struct ima_namespace; > > > > enum ucount_type { > > > > UCOUNT_USER_NAMESPACES, > > > > @@ -99,6 +100,9 @@ struct user_namespace { > > > > #endif > > > > struct ucounts *ucounts; > > > > long ucount_max[UCOUNT_COUNTS]; > > > > +#ifdef CONFIG_IMA_NS > > > > > > It's probably worth putting a comment here saying that user_ns does not > > > pin ima_ns. > > > > > > That the only time the ima_ns will be freed is when user_ns is freed, > > > and only time it will be changed is when user_ns is freed, or during > > > ima_fs_ns_init() (under smp_load_acquire) during a new mount. > > > > > > > + struct ima_namespace *ima_ns; > > > > > > So, if I create a new user_ns with a new ima_ns, and in there I > > > create a new user_ns again, it looks like ima_ns will be NULL in > > > the new user_ns? Should it not be set to the parent->ima_ns? > > > (which would cause trouble for the way it's currently being > > > freed...) > > > > Would also work and wouldn't be difficult to do imho. > > We previously decide to create an ima_namespace when securityfs is mounted. > This now also applies to nested containers where an IMA namespace is first > configured with the hash and template to use in a particular container and > then activated. If no configuration is done it will inherit the hash and > template from the first ancestor that has been configure when it is > activated. So the same steps and behavior applies to *all* containers, no > difference at any depth of nesting. Besides that, we don't want nested > containers to share policy and logs but keep them isolated from each other, > or do we not? > > Further, how should it work if we were to apply this even to the first > container? Should it just inherit the &init_ima_namespace and we'd have no > isolation at all? Why would we start treating containers at deeper nesting > levels differently? Valid points. I understood Serge as suggesting an implementation detail change not a design change but might misunderstand him here. # Currently 1. create new userns -> imans set to NULL 2. mount securityfs and configure imans -> set imans to &new_ima_ns When 2. hasn't been done then we find the relevant imans by walking the userns hierarchy upwards finding the first parent userns that has a non-NULL imans. # Serge's suggestion 1. create new userns -> imans is set to parent imans 2. mount securityfs and configure imans -> replace parent with &new_ima_ns So when 2. hasn't been done we don't need to walk the userns hierarchy upwards. We always find the relevant imans directly. Some massaging would be needed in process_measurement() probably but it wouldn't need to change semantics per se. But I think I misunderstood something in any case. So looking at an example like ima_post_path_mknod(). You seem to not call into ima_must_appraise() if the caller's userns doesn't have an imans enabled. I somehow had thought that the same logic applied as in process_measurement. But if it isn't then it might make sense to keep the current implementation.
On 5/23/22 08:41, Christian Brauner wrote: > On Mon, May 23, 2022 at 07:31:29AM -0400, Stefan Berger wrote: >> >> >> On 5/23/22 05:59, Christian Brauner wrote: >>> On Sun, May 22, 2022 at 01:24:26PM -0500, Serge Hallyn wrote: >>>> On Wed, Apr 20, 2022 at 10:06:20AM -0400, Stefan Berger wrote: >>>>> Add a pointer to ima_namespace to the user_namespace and initialize >>>>> the init_user_ns with a pointer to init_ima_ns. We need a pointer from >>>>> the user namespace to its associated IMA namespace since IMA namespaces >>>>> are piggybacking on user namespaces. >>>>> >>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >>>>> Acked-by: Christian Brauner <brauner@kernel.org> >>>>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> >>>>> >>>>> --- >>>>> v11: >>>>> - Added lost A-b from Christian back >>>>> - Added sentence to patch description explaining why we need the pointer >>>>> >>>>> v9: >>>>> - Deferred implementation of ima_ns_from_user_ns() to later patch >>>>> --- >>>>> include/linux/ima.h | 2 ++ >>>>> include/linux/user_namespace.h | 4 ++++ >>>>> kernel/user.c | 4 ++++ >>>>> 3 files changed, 10 insertions(+) >>>>> >>>>> diff --git a/include/linux/ima.h b/include/linux/ima.h >>>>> index 426b1744215e..fcb60a44e05f 100644 >>>>> --- a/include/linux/ima.h >>>>> +++ b/include/linux/ima.h >>>>> @@ -14,6 +14,8 @@ >>>>> #include <crypto/hash_info.h> >>>>> struct linux_binprm; >>>>> +extern struct ima_namespace init_ima_ns; >>>>> + >>>>> #ifdef CONFIG_IMA >>>>> extern enum hash_algo ima_get_current_hash_algo(void); >>>>> extern int ima_bprm_check(struct linux_binprm *bprm); >>>>> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h >>>>> index 33a4240e6a6f..019e8cf7b633 100644 >>>>> --- a/include/linux/user_namespace.h >>>>> +++ b/include/linux/user_namespace.h >>>>> @@ -36,6 +36,7 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */ >>>>> #define USERNS_INIT_FLAGS USERNS_SETGROUPS_ALLOWED >>>>> struct ucounts; >>>>> +struct ima_namespace; >>>>> enum ucount_type { >>>>> UCOUNT_USER_NAMESPACES, >>>>> @@ -99,6 +100,9 @@ struct user_namespace { >>>>> #endif >>>>> struct ucounts *ucounts; >>>>> long ucount_max[UCOUNT_COUNTS]; >>>>> +#ifdef CONFIG_IMA_NS >>>> >>>> It's probably worth putting a comment here saying that user_ns does not >>>> pin ima_ns. >>>> >>>> That the only time the ima_ns will be freed is when user_ns is freed, >>>> and only time it will be changed is when user_ns is freed, or during >>>> ima_fs_ns_init() (under smp_load_acquire) during a new mount. >>>> >>>>> + struct ima_namespace *ima_ns; >>>> >>>> So, if I create a new user_ns with a new ima_ns, and in there I >>>> create a new user_ns again, it looks like ima_ns will be NULL in >>>> the new user_ns? Should it not be set to the parent->ima_ns? >>>> (which would cause trouble for the way it's currently being >>>> freed...) >>> >>> Would also work and wouldn't be difficult to do imho. >> >> We previously decide to create an ima_namespace when securityfs is mounted. >> This now also applies to nested containers where an IMA namespace is first >> configured with the hash and template to use in a particular container and >> then activated. If no configuration is done it will inherit the hash and >> template from the first ancestor that has been configure when it is >> activated. So the same steps and behavior applies to *all* containers, no >> difference at any depth of nesting. Besides that, we don't want nested >> containers to share policy and logs but keep them isolated from each other, >> or do we not? >> >> Further, how should it work if we were to apply this even to the first >> container? Should it just inherit the &init_ima_namespace and we'd have no >> isolation at all? Why would we start treating containers at deeper nesting >> levels differently? > > Valid points. I understood Serge as suggesting an implementation detail > change not a design change but might misunderstand him here. > > # Currently > > 1. create new userns -> imans set to NULL > 2. mount securityfs and configure imans -> set imans to &new_ima_ns > > When 2. hasn't been done then we find the relevant imans by walking > the userns hierarchy upwards finding the first parent userns that has a > non-NULL imans. > > # Serge's suggestion > > 1. create new userns -> imans is set to parent imans > 2. mount securityfs and configure imans -> replace parent with &new_ima_ns > > So when 2. hasn't been done we don't need to walk the userns hierarchy > upwards. We always find the relevant imans directly. Some massaging In my understanding we *always* have to walk the hierarchy upwards independent of nesting depth and visit all the parent namespaces so we don't miss anything that has happened in a child namespace and can log the action in the parent namespaces depending on that namespace's policy. It's this command line that I am concerned about that must not lead to concealing of running of programs in nested user namespaces from the init_ima_ns for example: unshare --user --map-root-user \ unshare --user --map-root-user \ unshare --user --map-root-user \ ... \ <malware> The malware would obviously run in the init_(pid|mnt|...)_namespace's. Similarly this applies to running nested user namespaces that root could nsenter into to try to conceal the execution of programs. So I would rather want to prevent having to walk namespaces backwards and encountering an ima_namespace pointer multiple times and having to figure out whether the evaluation of the policy of that namespace already occurred. I'd rather skip over user namespaces with NULL pointers to IMA namespaces because those have not been configured and/or activated, yet. > would be needed in process_measurement() probably but it wouldn't need > to change semantics per se. > > But I think I misunderstood something in any case. So looking at an > example like ima_post_path_mknod(). You seem to not call into > ima_must_appraise() if the caller's userns doesn't have an imans > enabled. I somehow had thought that the same logic applied as in > process_measurement. But if it isn't then it might make sense to keep > the current implementation.
On Mon, May 23, 2022 at 02:41:59PM +0200, Christian Brauner wrote: > On Mon, May 23, 2022 at 07:31:29AM -0400, Stefan Berger wrote: > > > > > > On 5/23/22 05:59, Christian Brauner wrote: > > > On Sun, May 22, 2022 at 01:24:26PM -0500, Serge Hallyn wrote: > > > > On Wed, Apr 20, 2022 at 10:06:20AM -0400, Stefan Berger wrote: > > > > > Add a pointer to ima_namespace to the user_namespace and initialize > > > > > the init_user_ns with a pointer to init_ima_ns. We need a pointer from > > > > > the user namespace to its associated IMA namespace since IMA namespaces > > > > > are piggybacking on user namespaces. > > > > > > > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > > > > > Acked-by: Christian Brauner <brauner@kernel.org> > > > > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> > > > > > > > > > > --- > > > > > v11: > > > > > - Added lost A-b from Christian back > > > > > - Added sentence to patch description explaining why we need the pointer > > > > > > > > > > v9: > > > > > - Deferred implementation of ima_ns_from_user_ns() to later patch > > > > > --- > > > > > include/linux/ima.h | 2 ++ > > > > > include/linux/user_namespace.h | 4 ++++ > > > > > kernel/user.c | 4 ++++ > > > > > 3 files changed, 10 insertions(+) > > > > > > > > > > diff --git a/include/linux/ima.h b/include/linux/ima.h > > > > > index 426b1744215e..fcb60a44e05f 100644 > > > > > --- a/include/linux/ima.h > > > > > +++ b/include/linux/ima.h > > > > > @@ -14,6 +14,8 @@ > > > > > #include <crypto/hash_info.h> > > > > > struct linux_binprm; > > > > > +extern struct ima_namespace init_ima_ns; > > > > > + > > > > > #ifdef CONFIG_IMA > > > > > extern enum hash_algo ima_get_current_hash_algo(void); > > > > > extern int ima_bprm_check(struct linux_binprm *bprm); > > > > > diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h > > > > > index 33a4240e6a6f..019e8cf7b633 100644 > > > > > --- a/include/linux/user_namespace.h > > > > > +++ b/include/linux/user_namespace.h > > > > > @@ -36,6 +36,7 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */ > > > > > #define USERNS_INIT_FLAGS USERNS_SETGROUPS_ALLOWED > > > > > struct ucounts; > > > > > +struct ima_namespace; > > > > > enum ucount_type { > > > > > UCOUNT_USER_NAMESPACES, > > > > > @@ -99,6 +100,9 @@ struct user_namespace { > > > > > #endif > > > > > struct ucounts *ucounts; > > > > > long ucount_max[UCOUNT_COUNTS]; > > > > > +#ifdef CONFIG_IMA_NS > > > > > > > > It's probably worth putting a comment here saying that user_ns does not > > > > pin ima_ns. > > > > > > > > That the only time the ima_ns will be freed is when user_ns is freed, > > > > and only time it will be changed is when user_ns is freed, or during > > > > ima_fs_ns_init() (under smp_load_acquire) during a new mount. > > > > > > > > > + struct ima_namespace *ima_ns; > > > > > > > > So, if I create a new user_ns with a new ima_ns, and in there I > > > > create a new user_ns again, it looks like ima_ns will be NULL in > > > > the new user_ns? Should it not be set to the parent->ima_ns? > > > > (which would cause trouble for the way it's currently being > > > > freed...) > > > > > > Would also work and wouldn't be difficult to do imho. > > > > We previously decide to create an ima_namespace when securityfs is mounted. > > This now also applies to nested containers where an IMA namespace is first > > configured with the hash and template to use in a particular container and > > then activated. If no configuration is done it will inherit the hash and > > template from the first ancestor that has been configure when it is > > activated. So the same steps and behavior applies to *all* containers, no > > difference at any depth of nesting. Besides that, we don't want nested > > containers to share policy and logs but keep them isolated from each other, > > or do we not? > > > > Further, how should it work if we were to apply this even to the first > > container? Should it just inherit the &init_ima_namespace and we'd have no > > isolation at all? Why would we start treating containers at deeper nesting > > levels differently? > > Valid points. I understood Serge as suggesting an implementation detail > change not a design change but might misunderstand him here. > > # Currently > > 1. create new userns -> imans set to NULL > 2. mount securityfs and configure imans -> set imans to &new_ima_ns > > When 2. hasn't been done then we find the relevant imans by walking > the userns hierarchy upwards finding the first parent userns that has a > non-NULL imans. Ah, right, thanks Christian. But so the code - I think where the ima_ns is defined in the user_ns struct, needs to make this clear. Just something like // Pointer to ima_ns which this user_ns created. Can be null. // Access checks will walk the userns->parent chain and check // against all active ima_ns's. Note that when the user_ns is // freed, the ima_ns is guaranteed to be free-able. struct ima_namespace *ima_ns; > # Serge's suggestion > > 1. create new userns -> imans is set to parent imans > 2. mount securityfs and configure imans -> replace parent with &new_ima_ns > > So when 2. hasn't been done we don't need to walk the userns hierarchy > upwards. We always find the relevant imans directly. Some massaging > would be needed in process_measurement() probably but it wouldn't need > to change semantics per se. > > But I think I misunderstood something in any case. So looking at an > example like ima_post_path_mknod(). You seem to not call into > ima_must_appraise() if the caller's userns doesn't have an imans > enabled. I somehow had thought that the same logic applied as in > process_measurement. But if it isn't then it might make sense to keep > the current implementation.
On 5/23/22 10:25, Serge E. Hallyn wrote: > On Mon, May 23, 2022 at 02:41:59PM +0200, Christian Brauner wrote: >> On Mon, May 23, 2022 at 07:31:29AM -0400, Stefan Berger wrote: >>> >>> >>> On 5/23/22 05:59, Christian Brauner wrote: >>>> On Sun, May 22, 2022 at 01:24:26PM -0500, Serge Hallyn wrote: >>>>> On Wed, Apr 20, 2022 at 10:06:20AM -0400, Stefan Berger wrote: >>>>>> Add a pointer to ima_namespace to the user_namespace and initialize >>>>>> the init_user_ns with a pointer to init_ima_ns. We need a pointer from >>>>>> the user namespace to its associated IMA namespace since IMA namespaces >>>>>> are piggybacking on user namespaces. >>>>>> >>>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >>>>>> Acked-by: Christian Brauner <brauner@kernel.org> >>>>>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> >>>>>> >>>>>> --- >>>>>> v11: >>>>>> - Added lost A-b from Christian back >>>>>> - Added sentence to patch description explaining why we need the pointer >>>>>> >>>>>> v9: >>>>>> - Deferred implementation of ima_ns_from_user_ns() to later patch >>>>>> --- >>>>>> include/linux/ima.h | 2 ++ >>>>>> include/linux/user_namespace.h | 4 ++++ >>>>>> kernel/user.c | 4 ++++ >>>>>> 3 files changed, 10 insertions(+) >>>>>> >>>>>> diff --git a/include/linux/ima.h b/include/linux/ima.h >>>>>> index 426b1744215e..fcb60a44e05f 100644 >>>>>> --- a/include/linux/ima.h >>>>>> +++ b/include/linux/ima.h >>>>>> @@ -14,6 +14,8 @@ >>>>>> #include <crypto/hash_info.h> >>>>>> struct linux_binprm; >>>>>> +extern struct ima_namespace init_ima_ns; >>>>>> + >>>>>> #ifdef CONFIG_IMA >>>>>> extern enum hash_algo ima_get_current_hash_algo(void); >>>>>> extern int ima_bprm_check(struct linux_binprm *bprm); >>>>>> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h >>>>>> index 33a4240e6a6f..019e8cf7b633 100644 >>>>>> --- a/include/linux/user_namespace.h >>>>>> +++ b/include/linux/user_namespace.h >>>>>> @@ -36,6 +36,7 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */ >>>>>> #define USERNS_INIT_FLAGS USERNS_SETGROUPS_ALLOWED >>>>>> struct ucounts; >>>>>> +struct ima_namespace; >>>>>> enum ucount_type { >>>>>> UCOUNT_USER_NAMESPACES, >>>>>> @@ -99,6 +100,9 @@ struct user_namespace { >>>>>> #endif >>>>>> struct ucounts *ucounts; >>>>>> long ucount_max[UCOUNT_COUNTS]; >>>>>> +#ifdef CONFIG_IMA_NS >>>>> >>>>> It's probably worth putting a comment here saying that user_ns does not >>>>> pin ima_ns. >>>>> >>>>> That the only time the ima_ns will be freed is when user_ns is freed, >>>>> and only time it will be changed is when user_ns is freed, or during >>>>> ima_fs_ns_init() (under smp_load_acquire) during a new mount. >>>>> >>>>>> + struct ima_namespace *ima_ns; >>>>> >>>>> So, if I create a new user_ns with a new ima_ns, and in there I >>>>> create a new user_ns again, it looks like ima_ns will be NULL in >>>>> the new user_ns? Should it not be set to the parent->ima_ns? >>>>> (which would cause trouble for the way it's currently being >>>>> freed...) >>>> >>>> Would also work and wouldn't be difficult to do imho. >>> >>> We previously decide to create an ima_namespace when securityfs is mounted. >>> This now also applies to nested containers where an IMA namespace is first >>> configured with the hash and template to use in a particular container and >>> then activated. If no configuration is done it will inherit the hash and >>> template from the first ancestor that has been configure when it is >>> activated. So the same steps and behavior applies to *all* containers, no >>> difference at any depth of nesting. Besides that, we don't want nested >>> containers to share policy and logs but keep them isolated from each other, >>> or do we not? >>> >>> Further, how should it work if we were to apply this even to the first >>> container? Should it just inherit the &init_ima_namespace and we'd have no >>> isolation at all? Why would we start treating containers at deeper nesting >>> levels differently? >> >> Valid points. I understood Serge as suggesting an implementation detail >> change not a design change but might misunderstand him here. >> >> # Currently >> >> 1. create new userns -> imans set to NULL >> 2. mount securityfs and configure imans -> set imans to &new_ima_ns >> >> When 2. hasn't been done then we find the relevant imans by walking >> the userns hierarchy upwards finding the first parent userns that has a >> non-NULL imans. > > Ah, right, thanks Christian. > > But so the code - I think where the ima_ns is defined in the user_ns > struct, needs to make this clear. Just something like > > // Pointer to ima_ns which this user_ns created. Can be null. > // Access checks will walk the userns->parent chain and check > // against all active ima_ns's. Note that when the user_ns is > // freed, the ima_ns is guaranteed to be free-able. > struct ima_namespace *ima_ns; I added this comment now. Thanks. > >> # Serge's suggestion >> >> 1. create new userns -> imans is set to parent imans >> 2. mount securityfs and configure imans -> replace parent with &new_ima_ns >> >> So when 2. hasn't been done we don't need to walk the userns hierarchy >> upwards. We always find the relevant imans directly. Some massaging >> would be needed in process_measurement() probably but it wouldn't need >> to change semantics per se. >> >> But I think I misunderstood something in any case. So looking at an >> example like ima_post_path_mknod(). You seem to not call into >> ima_must_appraise() if the caller's userns doesn't have an imans >> enabled. I somehow had thought that the same logic applied as in >> process_measurement. But if it isn't then it might make sense to keep >> the current implementation.
diff --git a/include/linux/ima.h b/include/linux/ima.h index 426b1744215e..fcb60a44e05f 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -14,6 +14,8 @@ #include <crypto/hash_info.h> struct linux_binprm; +extern struct ima_namespace init_ima_ns; + #ifdef CONFIG_IMA extern enum hash_algo ima_get_current_hash_algo(void); extern int ima_bprm_check(struct linux_binprm *bprm); diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h index 33a4240e6a6f..019e8cf7b633 100644 --- a/include/linux/user_namespace.h +++ b/include/linux/user_namespace.h @@ -36,6 +36,7 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */ #define USERNS_INIT_FLAGS USERNS_SETGROUPS_ALLOWED struct ucounts; +struct ima_namespace; enum ucount_type { UCOUNT_USER_NAMESPACES, @@ -99,6 +100,9 @@ struct user_namespace { #endif struct ucounts *ucounts; long ucount_max[UCOUNT_COUNTS]; +#ifdef CONFIG_IMA_NS + struct ima_namespace *ima_ns; +#endif } __randomize_layout; struct ucounts { diff --git a/kernel/user.c b/kernel/user.c index e2cf8c22b539..e5d1f4b9b8ba 100644 --- a/kernel/user.c +++ b/kernel/user.c @@ -19,6 +19,7 @@ #include <linux/export.h> #include <linux/user_namespace.h> #include <linux/proc_ns.h> +#include <linux/ima.h> /* * userns count is 1 for root user, 1 for init_uts_ns, @@ -67,6 +68,9 @@ struct user_namespace init_user_ns = { .keyring_name_list = LIST_HEAD_INIT(init_user_ns.keyring_name_list), .keyring_sem = __RWSEM_INITIALIZER(init_user_ns.keyring_sem), #endif +#ifdef CONFIG_IMA_NS + .ima_ns = &init_ima_ns, +#endif }; EXPORT_SYMBOL_GPL(init_user_ns);