| Message ID | 20211206172600.1495968-1-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ima: Namespace IMA with audit support in IMA-ns | expand |
On Mon, 2021-12-06 at 12:25 -0500, Stefan Berger wrote: [...] > v3: > - Further modifications to virtualized SecurityFS following James's > posted patch > - Dropping of early teardown for user_namespaces since not needed > anymore This is my incremental to this series that moves the namespaced securityfs away from using a vfsmount and on to a root dentry instead, meaning we can call the blocking notifier from fill_super as Christian requested (and thus can remove the securityfs_notifier_sent indicator since it's only called once). James --- From 07b680d5fd59f5d3cea5580be25a2c9e08a01c3b Mon Sep 17 00:00:00 2001 From: James Bottomley <James.Bottomley@HansenPartnership.com> Date: Mon, 6 Dec 2021 20:27:00 +0000 Subject: [PATCH] Incremental for d_root --- include/linux/user_namespace.h | 3 +- security/inode.c | 55 +++++++++++++--------------------- 2 files changed, 22 insertions(+), 36 deletions(-) diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h index 6b8bd060d8c4..03a0879376a0 100644 --- a/include/linux/user_namespace.h +++ b/include/linux/user_namespace.h @@ -104,8 +104,7 @@ struct user_namespace { struct ima_namespace *ima_ns; #endif #ifdef CONFIG_SECURITYFS - struct vfsmount *securityfs_mount; - bool securityfs_notifier_sent; + struct dentry *securityfs_root; #endif } __randomize_layout; diff --git a/security/inode.c b/security/inode.c index 45211845fc31..f8b6cb3dfb87 100644 --- a/security/inode.c +++ b/security/inode.c @@ -24,6 +24,7 @@ #include <linux/magic.h> #include <linux/user_namespace.h> +static struct vfsmount *securityfs_mount; static int securityfs_mount_count; static BLOCKING_NOTIFIER_HEAD(securityfs_ns_notifier); @@ -40,43 +41,24 @@ static const struct super_operations securityfs_super_operations = { .free_inode = securityfs_free_inode, }; -static struct file_system_type fs_type; - -static void securityfs_free_context(struct fs_context *fc) -{ - struct user_namespace *ns = fc->user_ns; - - if (ns == &init_user_ns || - ns->securityfs_notifier_sent) - return; - - ns->securityfs_notifier_sent = true; - - ns->securityfs_mount = vfs_kern_mount(&fs_type, SB_KERNMOUNT, - fs_type.name, NULL); - if (IS_ERR(ns->securityfs_mount)) { - printk(KERN_ERR "kern mount on securityfs ERROR: %ld\n", - PTR_ERR(ns->securityfs_mount)); - ns->securityfs_mount = NULL; - return; - } - - blocking_notifier_call_chain(&securityfs_ns_notifier, - SECURITYFS_NS_ADD, fc->user_ns); - mntput(ns->securityfs_mount); -} - static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc) { static const struct tree_descr files[] = {{""}}; int error; + struct user_namespace *ns = fc->user_ns; error = simple_fill_super(sb, SECURITYFS_MAGIC, files); if (error) return error; + ns->securityfs_root = dget(sb->s_root); + sb->s_op = &securityfs_super_operations; + if (ns != &init_user_ns) + blocking_notifier_call_chain(&securityfs_ns_notifier, + SECURITYFS_NS_ADD, ns); + return 0; } @@ -87,7 +69,6 @@ static int securityfs_get_tree(struct fs_context *fc) static const struct fs_context_operations securityfs_context_ops = { .get_tree = securityfs_get_tree, - .free = securityfs_free_context, }; static int securityfs_init_fs_context(struct fs_context *fc) @@ -104,8 +85,10 @@ static void securityfs_kill_super(struct super_block *sb) blocking_notifier_call_chain(&securityfs_ns_notifier, SECURITYFS_NS_REMOVE, sb->s_fs_info); - ns->securityfs_notifier_sent = false; - ns->securityfs_mount = NULL; + + dput(ns->securityfs_root); + ns->securityfs_root = NULL; + kill_litter_super(sb); } @@ -174,14 +157,18 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode, pr_debug("securityfs: creating file '%s'\n",name); if (ns == &init_user_ns) { - error = simple_pin_fs(&fs_type, &ns->securityfs_mount, + error = simple_pin_fs(&fs_type, &securityfs_mount, &securityfs_mount_count); if (error) return ERR_PTR(error); } - if (!parent) - parent = ns->securityfs_mount->mnt_root; + if (!parent) { + if (ns == &init_user_ns) + parent = securityfs_mount->mnt_root; + else + parent = ns->securityfs_root; + } dir = d_inode(parent); @@ -227,7 +214,7 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode, out: inode_unlock(dir); if (ns == &init_user_ns) - simple_release_fs(&ns->securityfs_mount, + simple_release_fs(&securityfs_mount, &securityfs_mount_count); return dentry; } @@ -371,7 +358,7 @@ void securityfs_remove(struct dentry *dentry) } inode_unlock(dir); if (ns == &init_user_ns) - simple_release_fs(&ns->securityfs_mount, + simple_release_fs(&securityfs_mount, &securityfs_mount_count); } EXPORT_SYMBOL_GPL(securityfs_remove);
On 12/6/21 16:14, James Bottomley wrote: > On Mon, 2021-12-06 at 12:25 -0500, Stefan Berger wrote: > [...] >> v3: >> - Further modifications to virtualized SecurityFS following James's >> posted patch >> - Dropping of early teardown for user_namespaces since not needed >> anymore > This is my incremental to this series that moves the namespaced > securityfs away from using a vfsmount and on to a root dentry instead, > meaning we can call the blocking notifier from fill_super as Christian > requested (and thus can remove the securityfs_notifier_sent indicator > since it's only called once). Thanks. I have this now in a branch for v4. Stefan
On Mon, Dec 06, 2021 at 04:14:15PM -0500, James Bottomley wrote: > On Mon, 2021-12-06 at 12:25 -0500, Stefan Berger wrote: > [...] > > v3: > > - Further modifications to virtualized SecurityFS following James's > > posted patch > > - Dropping of early teardown for user_namespaces since not needed > > anymore > > This is my incremental to this series that moves the namespaced > securityfs away from using a vfsmount and on to a root dentry instead, > meaning we can call the blocking notifier from fill_super as Christian > requested (and thus can remove the securityfs_notifier_sent indicator > since it's only called once). Somehow b4 retrieves your patch out-of-band which makes it weird to reply to so I'm copy-pasting it here and reply inline: On Mon, Dec 06, 2021 at 08:27:00PM +0000, James Bottomley wrote: > --- > include/linux/user_namespace.h | 3 +- > security/inode.c | 55 +++++++++++++--------------------- > 2 files changed, 22 insertions(+), 36 deletions(-) > > diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h > index 6b8bd060d8c4..03a0879376a0 100644 > --- a/include/linux/user_namespace.h > +++ b/include/linux/user_namespace.h > @@ -104,8 +104,7 @@ struct user_namespace { > struct ima_namespace *ima_ns; > #endif > #ifdef CONFIG_SECURITYFS > - struct vfsmount *securityfs_mount; > - bool securityfs_notifier_sent; > + struct dentry *securityfs_root; > #endif > } __randomize_layout; > > diff --git a/security/inode.c b/security/inode.c > index 45211845fc31..f8b6cb3dfb87 100644 > --- a/security/inode.c > +++ b/security/inode.c > @@ -24,6 +24,7 @@ > #include <linux/magic.h> > #include <linux/user_namespace.h> > > +static struct vfsmount *securityfs_mount; > static int securityfs_mount_count; > > static BLOCKING_NOTIFIER_HEAD(securityfs_ns_notifier); > @@ -40,43 +41,24 @@ static const struct super_operations securityfs_super_operations = { > .free_inode = securityfs_free_inode, > }; > > -static struct file_system_type fs_type; > - > -static void securityfs_free_context(struct fs_context *fc) > -{ > - struct user_namespace *ns = fc->user_ns; > - > - if (ns == &init_user_ns || > - ns->securityfs_notifier_sent) > - return; > - > - ns->securityfs_notifier_sent = true; > - > - ns->securityfs_mount = vfs_kern_mount(&fs_type, SB_KERNMOUNT, > - fs_type.name, NULL); > - if (IS_ERR(ns->securityfs_mount)) { > - printk(KERN_ERR "kern mount on securityfs ERROR: %ld\n", > - PTR_ERR(ns->securityfs_mount)); > - ns->securityfs_mount = NULL; > - return; > - } > - > - blocking_notifier_call_chain(&securityfs_ns_notifier, > - SECURITYFS_NS_ADD, fc->user_ns); > - mntput(ns->securityfs_mount); > -} > - > static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc) > { > static const struct tree_descr files[] = {{""}}; > int error; > + struct user_namespace *ns = fc->user_ns; > > error = simple_fill_super(sb, SECURITYFS_MAGIC, files); > if (error) > return error; > > + ns->securityfs_root = dget(sb->s_root); > + > sb->s_op = &securityfs_super_operations; > > + if (ns != &init_user_ns) > + blocking_notifier_call_chain(&securityfs_ns_notifier, > + SECURITYFS_NS_ADD, ns); I would propose not to use the notifier logic. While it might be nifty it's over-engineered in my opinion. The dentry stashing in struct user_namespace currently serves the purpose to make it retrievable in ima_fs_ns_init(). That doesn't justify its existence imho. There is one central place were all users of namespaced securityfs can create the files that they need to and that is in securityfs_fill_super(). (If you want to make that more obvious then give it a subdirectory securityfs and move inode.c in there.) We simply will expect users to add: ima_init_securityfs() mylsm_init_securityfs() that are to be placed in fill_super and ima_kill_securityfs() mylsm_kill_securityfs() that get called in kill_super and the root dentry and other relevant information should be passed explicitly into those functions. Then we can remove the dentry stashing from struct user_namespace altogether and the patch gets smaller too.
On Tue, 2021-12-07 at 15:59 +0100, Christian Brauner wrote: > On Mon, Dec 06, 2021 at 04:14:15PM -0500, James Bottomley wrote: > > On Mon, 2021-12-06 at 12:25 -0500, Stefan Berger wrote: > > [...] > > > v3: > > > - Further modifications to virtualized SecurityFS following > > > James's posted patch > > > - Dropping of early teardown for user_namespaces since not > > > needed anymore > > > > This is my incremental to this series that moves the namespaced > > securityfs away from using a vfsmount and on to a root dentry > > instead, meaning we can call the blocking notifier from fill_super > > as Christian requested (and thus can remove the > > securityfs_notifier_sent indicator since it's only called once). > > Somehow b4 retrieves your patch out-of-band which makes it weird to > reply to so I'm copy-pasting it here and reply inline: > > On Mon, Dec 06, 2021 at 08:27:00PM +0000, James Bottomley wrote: > > --- > > include/linux/user_namespace.h | 3 +- > > security/inode.c | 55 +++++++++++++----------------- > > ---- > > 2 files changed, 22 insertions(+), 36 deletions(-) > > > > diff --git a/include/linux/user_namespace.h > > b/include/linux/user_namespace.h > > index 6b8bd060d8c4..03a0879376a0 100644 > > --- a/include/linux/user_namespace.h > > +++ b/include/linux/user_namespace.h > > @@ -104,8 +104,7 @@ struct user_namespace { > > struct ima_namespace *ima_ns; > > #endif > > #ifdef CONFIG_SECURITYFS > > - struct vfsmount *securityfs_mount; > > - bool securityfs_notifier_sent; > > + struct dentry *securityfs_root; > > #endif > > } __randomize_layout; > > > > diff --git a/security/inode.c b/security/inode.c > > index 45211845fc31..f8b6cb3dfb87 100644 > > --- a/security/inode.c > > +++ b/security/inode.c > > @@ -24,6 +24,7 @@ > > #include <linux/magic.h> > > #include <linux/user_namespace.h> > > > > +static struct vfsmount *securityfs_mount; > > static int securityfs_mount_count; > > > > static BLOCKING_NOTIFIER_HEAD(securityfs_ns_notifier); > > @@ -40,43 +41,24 @@ static const struct super_operations > > securityfs_super_operations = { > > .free_inode = securityfs_free_inode, > > }; > > > > -static struct file_system_type fs_type; > > - > > -static void securityfs_free_context(struct fs_context *fc) > > -{ > > - struct user_namespace *ns = fc->user_ns; > > - > > - if (ns == &init_user_ns || > > - ns->securityfs_notifier_sent) > > - return; > > - > > - ns->securityfs_notifier_sent = true; > > - > > - ns->securityfs_mount = vfs_kern_mount(&fs_type, SB_KERNMOUNT, > > - fs_type.name, NULL); > > - if (IS_ERR(ns->securityfs_mount)) { > > - printk(KERN_ERR "kern mount on securityfs ERROR: > > %ld\n", > > - PTR_ERR(ns->securityfs_mount)); > > - ns->securityfs_mount = NULL; > > - return; > > - } > > - > > - blocking_notifier_call_chain(&securityfs_ns_notifier, > > - SECURITYFS_NS_ADD, fc->user_ns); > > - mntput(ns->securityfs_mount); > > -} > > - > > static int securityfs_fill_super(struct super_block *sb, struct > > fs_context *fc) > > { > > static const struct tree_descr files[] = {{""}}; > > int error; > > + struct user_namespace *ns = fc->user_ns; > > > > error = simple_fill_super(sb, SECURITYFS_MAGIC, files); > > if (error) > > return error; > > > > + ns->securityfs_root = dget(sb->s_root); > > + > > sb->s_op = &securityfs_super_operations; > > > > + if (ns != &init_user_ns) > > + blocking_notifier_call_chain(&securityfs_ns_notifier, > > + SECURITYFS_NS_ADD, ns); > > I would propose not to use the notifier logic. While it might be > nifty it's over-engineered in my opinion. The reason for a notifier is that this current patch set only namespaces ima, but we also have integrity and evm to do. Plus, as Casey said, we might get apparmour and selinux. Since each of those will also want to add entries in fill_super, the notifier mechanism seemed fairly tailor made for this. The alternative is to have a load of #if CONFIG_securityfeature callback() #endif Inside securityfs_fill_super which is a bit inelegant. > The dentry stashing in struct user_namespace currently serves the > purpose to make it retrievable in ima_fs_ns_init(). That doesn't > justify its existence imho. I can thread the root as part of the callback. I think I can still use the standard securityfs calls because the only reason for the dentry in the namespace is so the callee can pass NULL and have the dentry created at the top level. We can insist in the namespaced use case that the callee always pass in the dentry, even for the top level. > There is one central place were all users of namespaced securityfs > can create the files that they need to and that is in > securityfs_fill_super(). (If you want to make that more obvious then > give it a subdirectory securityfs and move inode.c in there.) Right, that's what the patch does. > We simply will expect users to add: > > ima_init_securityfs() > mylsm_init_securityfs() Yes, plus all the #ifdefs because securityfs can exist independently of each of the features. We can hide the ifdefs in the header files and make the functions static do nothing if not defined, but the ifdeffery has to live somewhere. > that are to be placed in fill_super > > and > > ima_kill_securityfs() > mylsm_kill_securityfs() > > that get called in kill_super and the root dentry and other relevant > information should be passed explicitly into those functions. Then we > can remove the dentry stashing from struct user_namespace altogether > and the patch gets smaller too. Removing dentry stashing can be done independently of removing the notifier because the dentry can thread through the notifier (or the callback, of course). Let me have a look at doing the recoding. James
On Mon, Dec 06, 2021 at 12:25:44PM -0500, Stefan Berger wrote: > The goal of this series of patches is to start with the namespacing of > IMA and support auditing within an IMA namespace (IMA-ns) as the first > step. > > In this series the IMA namespace is piggy backing on the user namespace > and therefore an IMA namespace gets created when a user namespace is > created. The advantage of this is that the user namespace can provide > the keys infrastructure that IMA appraisal support will need later on. > > We chose the goal of supporting auditing within an IMA namespace since it > requires the least changes to IMA. Following this series, auditing within > an IMA namespace can be activated by a user running the following lines > that rely on a statically linked busybox to be installed on the host for > execution within the minimal container environment: > > mkdir -p rootfs/{bin,mnt,proc} > cp /sbin/busybox rootfs/bin > PATH=/bin unshare --user --map-root-user --mount-proc --pid --fork \ > --root rootfs busybox sh -c \ > "busybox mount -t securityfs /mnt /mnt; \ > busybox echo 'audit func=BPRM_CHECK mask=MAY_EXEC' > /mnt/ima/policy; \ > busybox cat /mnt/ima/policy" > > Following the audit log on the host the last line cat'ing the IMA policy > inside the namespace would have been audited. Unfortunately the auditing > line is not distinguishable from one stemming from actions on the host. > The hope here is that Richard Brigg's container id support for auditing > would help resolve the problem. > > The following lines added to a suitable IMA policy on the host would > cause the execution of the commands inside the container (by uid 1000) > to be measured and audited as well on the host, thus leading to two > auditing messages for the 'busybox cat' above and log entries in IMA's > system log. > > echo -e "measure func=BPRM_CHECK mask=MAY_EXEC uid=1000\n" \ > "audit func=BPRM_CHECK mask=MAY_EXEC uid=1000\n" \ > > /sys/kernel/security/ima/policy > > The goal of supporting measurement and auditing by the host, of actions > occurring within IMA namespaces, is that users, particularly root, > should not be able to evade the host's IMA policy just by spawning > new IMA namespaces, running programs there, and discarding the namespaces > again. This is achieved through 'hierarchical processing' of file > accesses that are evaluated against the policy of the namespace where > the action occurred and against all namespaces' and their policies leading > back to the root IMA namespace (init_ima_ns). > > The patch series adds support for a virtualized SecurityFS with a few > new API calls that are used by IMA namespacing. Only the data relevant > to the IMA namespace are shown. The files and directories of other > security subsystems (TPM, evm, Tomoyo, safesetid) are not showing > up when secruityfs is mounted inside a user namespace. > > Much of the code leading up to the virtualization of SecurityFS deals > with moving IMA's variables from various files into the IMA namespace > structure called 'ima_namespace'. When it comes to determining the > current IMA namespace I took the approach to get the current IMA > namespace (get_current_ns()) on the top level and pass the pointer all > the way down to those functions that now need access to the ima_namespace > to get to their variables. This later on comes in handy once hierarchical > processing is implemented in this series where we walk the list of > namespaces backwards and again need to pass the pointer into functions. > > This patch also introduces usage of CAP_MAC_ADMIN to allow access to the > IMA policy via reduced capabilities. We would again later on use this > capability to allow users to set file extended attributes for IMA appraisal > support. > > The basis for this series of patches is Linux v5.15. > My tree with these patches is here: > https://github.com/stefanberger/linux-ima-namespaces/tree/v5.15%2Bimans.v3.public I have one small procedural favor to ask. :) I couldn't apply your patch series directly. It if isn't too inconvenient for you could you pass --base with a proper upstream tag, e.g. --base=v5.15. The branch you posted here doesn't exist afaict and I had to peruse your github repo and figured the correct branch might be v5.15+imans.v3.posted. In any case, --base with a proper upstream tag would make this all a bit easier or - if it really is necessary to pull from your tree it would be nice if you could post it in a form directly consumable by git and note url-escaped. So something like git clone https://github.com/stefanberger/linux-ima-namespaces v5.15+imans.v3.posted would already help. Christian
On Tue, 2021-12-07 at 10:16 -0500, James Bottomley wrote: > On Tue, 2021-12-07 at 15:59 +0100, Christian Brauner wrote: > > On Mon, Dec 06, 2021 at 04:14:15PM -0500, James Bottomley wrote: [...] > > > static int securityfs_fill_super(struct super_block *sb, struct > > > fs_context *fc) > > > { > > > static const struct tree_descr files[] = {{""}}; > > > int error; > > > + struct user_namespace *ns = fc->user_ns; > > > > > > error = simple_fill_super(sb, SECURITYFS_MAGIC, files); > > > if (error) > > > return error; > > > > > > + ns->securityfs_root = dget(sb->s_root); > > > + > > > sb->s_op = &securityfs_super_operations; > > > > > > + if (ns != &init_user_ns) > > > + blocking_notifier_call_chain(&securityfs_ns_notifier, > > > + SECURITYFS_NS_ADD, ns); > > > > I would propose not to use the notifier logic. While it might be > > nifty it's over-engineered in my opinion. > > The reason for a notifier is that this current patch set only > namespaces ima, but we also have integrity and evm to do. Plus, as > Casey said, we might get apparmour and selinux. Since each of those > will also want to add entries in fill_super, the notifier mechanism > seemed fairly tailor made for this. The alternative is to have a > load of > > #if CONFIG_securityfeature > callback() > #endif > > Inside securityfs_fill_super which is a bit inelegant. > > > The dentry stashing in struct user_namespace currently serves the > > purpose to make it retrievable in ima_fs_ns_init(). That doesn't > > justify its existence imho. > > I can thread the root as part of the callback. I think I can still > use the standard securityfs calls because the only reason for the > dentry in the namespace is so the callee can pass NULL and have the > dentry created at the top level. We can insist in the namespaced use > case that the callee always pass in the dentry, even for the top > level. > > > There is one central place were all users of namespaced securityfs > > can create the files that they need to and that is in > > securityfs_fill_super(). (If you want to make that more obvious > > then give it a subdirectory securityfs and move inode.c in there.) > > Right, that's what the patch does. > > > We simply will expect users to add: > > > > ima_init_securityfs() > > mylsm_init_securityfs() > > Yes, plus all the #ifdefs because securityfs can exist independently > of each of the features. We can hide the ifdefs in the header files > and make the functions static do nothing if not defined, but the > ifdeffery has to live somewhere. Actually, I've got a much better reason: securityfs is a bool; all the other LSMs and IMA are tristates. We can't call module init functions from core code, it has to be done by something like a notifier. James
On 12/7/2021 7:40 AM, James Bottomley wrote: > On Tue, 2021-12-07 at 10:16 -0500, James Bottomley wrote: >> On Tue, 2021-12-07 at 15:59 +0100, Christian Brauner wrote: >>> On Mon, Dec 06, 2021 at 04:14:15PM -0500, James Bottomley wrote: > [...] >>>> static int securityfs_fill_super(struct super_block *sb, struct >>>> fs_context *fc) >>>> { >>>> static const struct tree_descr files[] = {{""}}; >>>> int error; >>>> + struct user_namespace *ns = fc->user_ns; >>>> >>>> error = simple_fill_super(sb, SECURITYFS_MAGIC, files); >>>> if (error) >>>> return error; >>>> >>>> + ns->securityfs_root = dget(sb->s_root); >>>> + >>>> sb->s_op = &securityfs_super_operations; >>>> >>>> + if (ns != &init_user_ns) >>>> + blocking_notifier_call_chain(&securityfs_ns_notifier, >>>> + SECURITYFS_NS_ADD, ns); >>> I would propose not to use the notifier logic. While it might be >>> nifty it's over-engineered in my opinion. >> The reason for a notifier is that this current patch set only >> namespaces ima, but we also have integrity and evm to do. Plus, as >> Casey said, we might get apparmour and selinux. Since each of those >> will also want to add entries in fill_super, the notifier mechanism >> seemed fairly tailor made for this. The alternative is to have a >> load of >> >> #if CONFIG_securityfeature >> callback() >> #endif >> >> Inside securityfs_fill_super which is a bit inelegant. >> >>> The dentry stashing in struct user_namespace currently serves the >>> purpose to make it retrievable in ima_fs_ns_init(). That doesn't >>> justify its existence imho. >> I can thread the root as part of the callback. I think I can still >> use the standard securityfs calls because the only reason for the >> dentry in the namespace is so the callee can pass NULL and have the >> dentry created at the top level. We can insist in the namespaced use >> case that the callee always pass in the dentry, even for the top >> level. >> >>> There is one central place were all users of namespaced securityfs >>> can create the files that they need to and that is in >>> securityfs_fill_super(). (If you want to make that more obvious >>> then give it a subdirectory securityfs and move inode.c in there.) >> Right, that's what the patch does. >> >>> We simply will expect users to add: >>> >>> ima_init_securityfs() >>> mylsm_init_securityfs() >> Yes, plus all the #ifdefs because securityfs can exist independently >> of each of the features. We can hide the ifdefs in the header files >> and make the functions static do nothing if not defined, but the >> ifdeffery has to live somewhere. > Actually, I've got a much better reason: securityfs is a bool; all the > other LSMs and IMA are tristates. We can't call module init functions > from core code, it has to be done by something like a notifier. Err, no. LSMs are not available as loadable modules.
On 12/7/21 10:17, Christian Brauner wrote: > On Mon, Dec 06, 2021 at 12:25:44PM -0500, Stefan Berger wrote: >> The goal of this series of patches is to start with the namespacing of >> IMA and support auditing within an IMA namespace (IMA-ns) as the first >> step. >> >> In this series the IMA namespace is piggy backing on the user namespace >> and therefore an IMA namespace gets created when a user namespace is >> created. The advantage of this is that the user namespace can provide >> the keys infrastructure that IMA appraisal support will need later on. >> >> We chose the goal of supporting auditing within an IMA namespace since it >> requires the least changes to IMA. Following this series, auditing within >> an IMA namespace can be activated by a user running the following lines >> that rely on a statically linked busybox to be installed on the host for >> execution within the minimal container environment: >> >> mkdir -p rootfs/{bin,mnt,proc} >> cp /sbin/busybox rootfs/bin >> PATH=/bin unshare --user --map-root-user --mount-proc --pid --fork \ >> --root rootfs busybox sh -c \ >> "busybox mount -t securityfs /mnt /mnt; \ >> busybox echo 'audit func=BPRM_CHECK mask=MAY_EXEC' > /mnt/ima/policy; \ >> busybox cat /mnt/ima/policy" >> >> Following the audit log on the host the last line cat'ing the IMA policy >> inside the namespace would have been audited. Unfortunately the auditing >> line is not distinguishable from one stemming from actions on the host. >> The hope here is that Richard Brigg's container id support for auditing >> would help resolve the problem. >> >> The following lines added to a suitable IMA policy on the host would >> cause the execution of the commands inside the container (by uid 1000) >> to be measured and audited as well on the host, thus leading to two >> auditing messages for the 'busybox cat' above and log entries in IMA's >> system log. >> >> echo -e "measure func=BPRM_CHECK mask=MAY_EXEC uid=1000\n" \ >> "audit func=BPRM_CHECK mask=MAY_EXEC uid=1000\n" \ >> > /sys/kernel/security/ima/policy >> >> The goal of supporting measurement and auditing by the host, of actions >> occurring within IMA namespaces, is that users, particularly root, >> should not be able to evade the host's IMA policy just by spawning >> new IMA namespaces, running programs there, and discarding the namespaces >> again. This is achieved through 'hierarchical processing' of file >> accesses that are evaluated against the policy of the namespace where >> the action occurred and against all namespaces' and their policies leading >> back to the root IMA namespace (init_ima_ns). >> >> The patch series adds support for a virtualized SecurityFS with a few >> new API calls that are used by IMA namespacing. Only the data relevant >> to the IMA namespace are shown. The files and directories of other >> security subsystems (TPM, evm, Tomoyo, safesetid) are not showing >> up when secruityfs is mounted inside a user namespace. >> >> Much of the code leading up to the virtualization of SecurityFS deals >> with moving IMA's variables from various files into the IMA namespace >> structure called 'ima_namespace'. When it comes to determining the >> current IMA namespace I took the approach to get the current IMA >> namespace (get_current_ns()) on the top level and pass the pointer all >> the way down to those functions that now need access to the ima_namespace >> to get to their variables. This later on comes in handy once hierarchical >> processing is implemented in this series where we walk the list of >> namespaces backwards and again need to pass the pointer into functions. >> >> This patch also introduces usage of CAP_MAC_ADMIN to allow access to the >> IMA policy via reduced capabilities. We would again later on use this >> capability to allow users to set file extended attributes for IMA appraisal >> support. >> >> The basis for this series of patches is Linux v5.15. >> My tree with these patches is here: >> https://github.com/stefanberger/linux-ima-namespaces/tree/v5.15%2Bimans.v3.public > I have one small procedural favor to ask. :) > > I couldn't apply your patch series directly. It if isn't too > inconvenient for you could you pass --base with a proper upstream tag, > e.g. --base=v5.15. > > The branch you posted here doesn't exist afaict and I had to peruse your > github repo and figured the correct branch might be v5.15+imans.v3.posted. > > In any case, --base with a proper upstream tag would make this all a bit > easier or - if it really is necessary to pull from your tree it would be > nice if you could post it in a form directly consumable by git and note > url-escaped. So something like > > git clone https://github.com/stefanberger/linux-ima-namespaces v5.15+imans.v3.posted > > would already help. Sure, will do. Stefan > > Christian
On Tue, 2021-12-07 at 07:48 -0800, Casey Schaufler wrote: > On 12/7/2021 7:40 AM, James Bottomley wrote: > > On Tue, 2021-12-07 at 10:16 -0500, James Bottomley wrote: > > > On Tue, 2021-12-07 at 15:59 +0100, Christian Brauner wrote: > > > > On Mon, Dec 06, 2021 at 04:14:15PM -0500, James Bottomley > > > > wrote: > > [...] > > > > > static int securityfs_fill_super(struct super_block *sb, > > > > > struct > > > > > fs_context *fc) > > > > > { > > > > > static const struct tree_descr files[] = {{""}}; > > > > > int error; > > > > > + struct user_namespace *ns = fc->user_ns; > > > > > > > > > > error = simple_fill_super(sb, SECURITYFS_MAGIC, files); > > > > > if (error) > > > > > return error; > > > > > > > > > > + ns->securityfs_root = dget(sb->s_root); > > > > > + > > > > > sb->s_op = &securityfs_super_operations; > > > > > > > > > > + if (ns != &init_user_ns) > > > > > + blocking_notifier_call_chain(&securityfs_ns_not > > > > > ifier, > > > > > + SECURITYFS_NS_ADD, > > > > > ns); > > > > > > > > I would propose not to use the notifier logic. While it might > > > > be nifty it's over-engineered in my opinion. > > > > > > The reason for a notifier is that this current patch set only > > > namespaces ima, but we also have integrity and evm to do. Plus, > > > as Casey said, we might get apparmour and selinux. Since each of > > > those will also want to add entries in fill_super, the notifier > > > mechanism seemed fairly tailor made for this. The alternative is > > > to have a load of > > > > > > #if CONFIG_securityfeature > > > callback() > > > #endif > > > > > > Inside securityfs_fill_super which is a bit inelegant. > > > > > > > The dentry stashing in struct user_namespace currently serves > > > > the purpose to make it retrievable in ima_fs_ns_init(). That > > > > doesn't justify its existence imho. > > > > > > I can thread the root as part of the callback. I think I can > > > still use the standard securityfs calls because the only reason > > > for the dentry in the namespace is so the callee can pass NULL > > > and have the dentry created at the top level. We can insist in > > > the namespaced use case that the callee always pass in the > > > dentry, even for the top level. > > > > > > > There is one central place were all users of namespaced > > > > securityfs can create the files that they need to and that is > > > > in securityfs_fill_super(). (If you want to make that more > > > > obvious then give it a subdirectory securityfs and move inode.c > > > > in there.) > > > > > > > Right, that's what the patch does. > > > > > > > We simply will expect users to add: > > > > > > > > ima_init_securityfs() > > > > mylsm_init_securityfs() > > > > > > Yes, plus all the #ifdefs because securityfs can exist > > > independently of each of the features. We can hide the ifdefs in > > > the header files and make the functions static do nothing if not > > > defined, but the ifdeffery has to live somewhere. > > > > Actually, I've got a much better reason: securityfs is a bool; all > > the other LSMs and IMA are tristates. We can't call module init > > functions from core code, it has to be done by something like a > > notifier. > > Err, no. LSMs are not available as loadable modules. Well securityfs has EXPORT_MODULE_GPL() across all its dentry creation functions ... that does mean it expects to be called by a module. However, it does appear to be it's only TPM that may use it as a module ... this is still going to cause a problem eventually because now we'll have to require some of the TPM code be built in once we want to attach vTPMs to containers. James
On Tue, 2021-12-07 at 15:59 +0100, Christian Brauner wrote: [...] > I would propose not to use the notifier logic. While it might be > nifty it's over-engineered in my opinion. The dentry stashing in > struct user_namespace currently serves the purpose to make it > retrievable in ima_fs_ns_init(). That doesn't justify its existence > imho. This is the incremental to Stefan's set with the notifier removed and the root dentry threaded. James --- From d9322270157531f4772fe862fa1655993a0c387d Mon Sep 17 00:00:00 2001 From: James Bottomley <James.Bottomley@HansenPartnership.com> Date: Mon, 6 Dec 2021 20:27:00 +0000 Subject: [PATCH] Incremental for root dentry --- include/linux/ima.h | 2 + include/linux/security.h | 8 ---- include/linux/user_namespace.h | 4 -- security/inode.c | 71 ++++++++++----------------------- security/integrity/ima/ima_fs.c | 40 ++++--------------- 5 files changed, 29 insertions(+), 96 deletions(-) diff --git a/include/linux/ima.h b/include/linux/ima.h index cab5fc6caeb3..a6e93bb5ce8a 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -40,6 +40,8 @@ extern int ima_measure_critical_data(const char *event_label, const char *event_name, const void *buf, size_t buf_len, bool hash, u8 *digest, size_t digest_len); +extern int ima_fs_ns_init(struct user_namespace *ns, struct dentry *root); +extern void ima_fs_ns_free_dentries(struct user_namespace *ns); #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM extern void ima_appraise_parse_cmdline(void); diff --git a/include/linux/security.h b/include/linux/security.h index a34109c5e3ed..bbf44a466832 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -1929,14 +1929,6 @@ struct dentry *securityfs_create_symlink(const char *name, const struct inode_operations *iops); extern void securityfs_remove(struct dentry *dentry); -enum { - SECURITYFS_NS_ADD, - SECURITYFS_NS_REMOVE, -}; - -extern int securityfs_register_ns_notifier(struct notifier_block *nb); -extern int securityfs_unregister_ns_notifier(struct notifier_block *nb); - #else /* CONFIG_SECURITYFS */ static inline struct dentry *securityfs_create_dir(const char *name, diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h index 6b8bd060d8c4..5249db04d62b 100644 --- a/include/linux/user_namespace.h +++ b/include/linux/user_namespace.h @@ -103,10 +103,6 @@ struct user_namespace { #ifdef CONFIG_IMA struct ima_namespace *ima_ns; #endif -#ifdef CONFIG_SECURITYFS - struct vfsmount *securityfs_mount; - bool securityfs_notifier_sent; -#endif } __randomize_layout; struct ucounts { diff --git a/security/inode.c b/security/inode.c index 45211845fc31..0b173792fbd3 100644 --- a/security/inode.c +++ b/security/inode.c @@ -16,18 +16,17 @@ #include <linux/fs_context.h> #include <linux/mount.h> #include <linux/pagemap.h> +#include <linux/ima.h> #include <linux/init.h> #include <linux/namei.h> -#include <linux/notifier.h> #include <linux/security.h> #include <linux/lsm_hooks.h> #include <linux/magic.h> #include <linux/user_namespace.h> +static struct vfsmount *securityfs_mount; static int securityfs_mount_count; -static BLOCKING_NOTIFIER_HEAD(securityfs_ns_notifier); - static void securityfs_free_inode(struct inode *inode) { if (S_ISLNK(inode->i_mode)) @@ -40,36 +39,11 @@ static const struct super_operations securityfs_super_operations = { .free_inode = securityfs_free_inode, }; -static struct file_system_type fs_type; - -static void securityfs_free_context(struct fs_context *fc) -{ - struct user_namespace *ns = fc->user_ns; - - if (ns == &init_user_ns || - ns->securityfs_notifier_sent) - return; - - ns->securityfs_notifier_sent = true; - - ns->securityfs_mount = vfs_kern_mount(&fs_type, SB_KERNMOUNT, - fs_type.name, NULL); - if (IS_ERR(ns->securityfs_mount)) { - printk(KERN_ERR "kern mount on securityfs ERROR: %ld\n", - PTR_ERR(ns->securityfs_mount)); - ns->securityfs_mount = NULL; - return; - } - - blocking_notifier_call_chain(&securityfs_ns_notifier, - SECURITYFS_NS_ADD, fc->user_ns); - mntput(ns->securityfs_mount); -} - static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc) { static const struct tree_descr files[] = {{""}}; int error; + struct user_namespace *ns = fc->user_ns; error = simple_fill_super(sb, SECURITYFS_MAGIC, files); if (error) @@ -77,6 +51,10 @@ static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc) sb->s_op = &securityfs_super_operations; + if (ns != &init_user_ns) { + ima_fs_ns_init(ns, sb->s_root); + } + return 0; } @@ -87,7 +65,6 @@ static int securityfs_get_tree(struct fs_context *fc) static const struct fs_context_operations securityfs_context_ops = { .get_tree = securityfs_get_tree, - .free = securityfs_free_context, }; static int securityfs_init_fs_context(struct fs_context *fc) @@ -100,12 +77,10 @@ static void securityfs_kill_super(struct super_block *sb) { struct user_namespace *ns = sb->s_fs_info; - if (ns != &init_user_ns) - blocking_notifier_call_chain(&securityfs_ns_notifier, - SECURITYFS_NS_REMOVE, - sb->s_fs_info); - ns->securityfs_notifier_sent = false; - ns->securityfs_mount = NULL; + if (ns != &init_user_ns) { + ima_fs_ns_free_dentries(ns); + } + kill_litter_super(sb); } @@ -117,16 +92,6 @@ static struct file_system_type fs_type = { .fs_flags = FS_USERNS_MOUNT, }; -int securityfs_register_ns_notifier(struct notifier_block *nb) -{ - return blocking_notifier_chain_register(&securityfs_ns_notifier, nb); -} - -int securityfs_unregister_ns_notifier(struct notifier_block *nb) -{ - return blocking_notifier_chain_unregister(&securityfs_ns_notifier, nb); -} - /** * securityfs_create_dentry - create a dentry in the securityfs filesystem * @@ -174,14 +139,18 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode, pr_debug("securityfs: creating file '%s'\n",name); if (ns == &init_user_ns) { - error = simple_pin_fs(&fs_type, &ns->securityfs_mount, + error = simple_pin_fs(&fs_type, &securityfs_mount, &securityfs_mount_count); if (error) return ERR_PTR(error); } - if (!parent) - parent = ns->securityfs_mount->mnt_root; + if (!parent) { + if (ns == &init_user_ns) + parent = securityfs_mount->mnt_root; + else + return ERR_PTR(-EINVAL); + } dir = d_inode(parent); @@ -227,7 +196,7 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode, out: inode_unlock(dir); if (ns == &init_user_ns) - simple_release_fs(&ns->securityfs_mount, + simple_release_fs(&securityfs_mount, &securityfs_mount_count); return dentry; } @@ -371,7 +340,7 @@ void securityfs_remove(struct dentry *dentry) } inode_unlock(dir); if (ns == &init_user_ns) - simple_release_fs(&ns->securityfs_mount, + simple_release_fs(&securityfs_mount, &securityfs_mount_count); } EXPORT_SYMBOL_GPL(securityfs_remove); diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index c17a6b7eeb95..fb29cb7b0384 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -446,9 +446,10 @@ static const struct file_operations ima_measure_policy_ops = { .llseek = generic_file_llseek, }; -static void ima_fs_ns_free_dentries(struct ima_namespace *ns) +void ima_fs_ns_free_dentries(struct user_namespace *user_ns) { int i; + struct ima_namespace *ns = user_ns->ima_ns; for (i = IMAFS_DENTRY_LAST - 1; i >= 0; i--) securityfs_remove(ns->dentry[i]); @@ -456,7 +457,7 @@ static void ima_fs_ns_free_dentries(struct ima_namespace *ns) memset(ns->dentry, 0, sizeof(ns->dentry)); } -static int ima_fs_ns_init(struct user_namespace *user_ns) +int ima_fs_ns_init(struct user_namespace *user_ns, struct dentry *root) { struct ima_namespace *ns = user_ns->ima_ns; struct dentry *ima_dir; @@ -468,7 +469,7 @@ static int ima_fs_ns_init(struct user_namespace *user_ns) /* FIXME: update when evm and integrity are namespaced */ if (user_ns != &init_user_ns) ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR] = - securityfs_create_dir("integrity", NULL); + securityfs_create_dir("integrity", root); else ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR] = integrity_dir; @@ -480,7 +481,7 @@ static int ima_fs_ns_init(struct user_namespace *user_ns) ima_dir = ns->dentry[IMAFS_DENTRY_DIR]; ns->dentry[IMAFS_DENTRY_SYMLINK] = - securityfs_create_symlink("ima", NULL, "integrity/ima", NULL); + securityfs_create_symlink("ima", root, "integrity/ima", NULL); if (IS_ERR(ns->dentry[IMAFS_DENTRY_SYMLINK])) goto out; @@ -520,38 +521,11 @@ static int ima_fs_ns_init(struct user_namespace *user_ns) return 0; out: - ima_fs_ns_free_dentries(ns); + ima_fs_ns_free_dentries(user_ns); return -1; } -static int ima_ns_notify(struct notifier_block *this, unsigned long msg, - void *data) -{ - int rc = 0; - struct user_namespace *user_ns = data; - - switch (msg) { - case SECURITYFS_NS_ADD: - rc = ima_fs_ns_init(user_ns); - break; - case SECURITYFS_NS_REMOVE: - ima_fs_ns_free_dentries(user_ns->ima_ns); - break; - } - return rc; -} - -static struct notifier_block ima_ns_notifier = { - .notifier_call = ima_ns_notify, -}; - int ima_fs_init() { - int rc; - - rc = securityfs_register_ns_notifier(&ima_ns_notifier); - if (rc) - return rc; - - return ima_fs_ns_init(&init_user_ns); + return ima_fs_ns_init(&init_user_ns, NULL); }
The goal of this series of patches is to start with the namespacing of IMA and support auditing within an IMA namespace (IMA-ns) as the first step. In this series the IMA namespace is piggy backing on the user namespace and therefore an IMA namespace gets created when a user namespace is created. The advantage of this is that the user namespace can provide the keys infrastructure that IMA appraisal support will need later on. We chose the goal of supporting auditing within an IMA namespace since it requires the least changes to IMA. Following this series, auditing within an IMA namespace can be activated by a user running the following lines that rely on a statically linked busybox to be installed on the host for execution within the minimal container environment: mkdir -p rootfs/{bin,mnt,proc} cp /sbin/busybox rootfs/bin PATH=/bin unshare --user --map-root-user --mount-proc --pid --fork \ --root rootfs busybox sh -c \ "busybox mount -t securityfs /mnt /mnt; \ busybox echo 'audit func=BPRM_CHECK mask=MAY_EXEC' > /mnt/ima/policy; \ busybox cat /mnt/ima/policy" Following the audit log on the host the last line cat'ing the IMA policy inside the namespace would have been audited. Unfortunately the auditing line is not distinguishable from one stemming from actions on the host. The hope here is that Richard Brigg's container id support for auditing would help resolve the problem. The following lines added to a suitable IMA policy on the host would cause the execution of the commands inside the container (by uid 1000) to be measured and audited as well on the host, thus leading to two auditing messages for the 'busybox cat' above and log entries in IMA's system log. echo -e "measure func=BPRM_CHECK mask=MAY_EXEC uid=1000\n" \ "audit func=BPRM_CHECK mask=MAY_EXEC uid=1000\n" \ > /sys/kernel/security/ima/policy The goal of supporting measurement and auditing by the host, of actions occurring within IMA namespaces, is that users, particularly root, should not be able to evade the host's IMA policy just by spawning new IMA namespaces, running programs there, and discarding the namespaces again. This is achieved through 'hierarchical processing' of file accesses that are evaluated against the policy of the namespace where the action occurred and against all namespaces' and their policies leading back to the root IMA namespace (init_ima_ns). The patch series adds support for a virtualized SecurityFS with a few new API calls that are used by IMA namespacing. Only the data relevant to the IMA namespace are shown. The files and directories of other security subsystems (TPM, evm, Tomoyo, safesetid) are not showing up when secruityfs is mounted inside a user namespace. Much of the code leading up to the virtualization of SecurityFS deals with moving IMA's variables from various files into the IMA namespace structure called 'ima_namespace'. When it comes to determining the current IMA namespace I took the approach to get the current IMA namespace (get_current_ns()) on the top level and pass the pointer all the way down to those functions that now need access to the ima_namespace to get to their variables. This later on comes in handy once hierarchical processing is implemented in this series where we walk the list of namespaces backwards and again need to pass the pointer into functions. This patch also introduces usage of CAP_MAC_ADMIN to allow access to the IMA policy via reduced capabilities. We would again later on use this capability to allow users to set file extended attributes for IMA appraisal support. The basis for this series of patches is Linux v5.15. My tree with these patches is here: https://github.com/stefanberger/linux-ima-namespaces/tree/v5.15%2Bimans.v3.public Regards, Stefan v3: - Further modifications to virtualized SecurityFS following James's posted patch - Dropping of early teardown for user_namespaces since not needed anymore v2: - Folllwed Christian's suggestion to virtualize securitytfs; no more securityfs_ns - Followed James's advice for late 'population' of securityfs for IMA namespaces - Squashed 2 patches dealing with capabilities - Added missing 'depends on USER_NS' to Kconfig - Added missing 'static' to several functions *** BLURB HERE *** Mehmet Kayaalp (2): ima: Define ns_status for storing namespaced iint data ima: Namespace audit status flags Stefan Berger (14): ima: Add IMA namespace support ima: Move delayed work queue and variables into ima_namespace ima: Move IMA's keys queue related variables into ima_namespace ima: Move policy related variables into ima_namespace ima: Move ima_htable into ima_namespace ima: Move measurement list related variables into ima_namespace ima: Only accept AUDIT rules for IMA non-init_ima_ns namespaces for now ima: Implement hierarchical processing of file accesses securityfs: Move vfsmount into user_namespace securityfs: Extend securityfs with namespacing support ima: Move some IMA policy and filesystem related variables into ima_namespace ima: Use mac_admin_ns_capable() to check corresponding capability ima: Move dentries into ima_namespace ima: Setup securityfs for IMA namespace include/linux/capability.h | 6 + include/linux/ima.h | 126 ++++++++++++++ include/linux/security.h | 8 + include/linux/user_namespace.h | 8 + init/Kconfig | 13 ++ kernel/user.c | 9 +- kernel/user_namespace.c | 16 ++ security/inode.c | 83 +++++++-- security/integrity/ima/Makefile | 4 +- security/integrity/ima/ima.h | 145 ++++++++++------ security/integrity/ima/ima_api.c | 33 ++-- security/integrity/ima/ima_appraise.c | 26 +-- security/integrity/ima/ima_asymmetric_keys.c | 8 +- security/integrity/ima/ima_fs.c | 170 ++++++++++++------- security/integrity/ima/ima_init.c | 22 +-- security/integrity/ima/ima_init_ima_ns.c | 73 ++++++++ security/integrity/ima/ima_main.c | 128 +++++++++----- security/integrity/ima/ima_ns.c | 111 ++++++++++++ security/integrity/ima/ima_ns_status.c | 132 ++++++++++++++ security/integrity/ima/ima_policy.c | 142 +++++++++------- security/integrity/ima/ima_queue.c | 75 ++++---- security/integrity/ima/ima_queue_keys.c | 73 +++----- security/integrity/ima/ima_template.c | 4 +- 23 files changed, 1075 insertions(+), 340 deletions(-) create mode 100644 security/integrity/ima/ima_init_ima_ns.c create mode 100644 security/integrity/ima/ima_ns.c create mode 100644 security/integrity/ima/ima_ns_status.c