Messages in this thread | | | From | Paul Moore <> | Date | Fri, 24 Jun 2016 16:29:27 -0400 | Subject | Re: [PATCH v4 2/3] LSM: module hierarchy in /proc/.../attr |
| |
On Fri, Jun 24, 2016 at 4:08 PM, Kees Cook <keescook@chromium.org> wrote: > On Fri, Jun 24, 2016 at 1:05 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 6/24/2016 12:11 PM, Paul Moore wrote: >>> On Thu, Jun 23, 2016 at 5:11 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>>> Subject: [PATCH v4 2/3] LSM: module hierarchy in /proc/.../attr >>>> >>>> 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. That hasn't >>>> been a problem to date, as you can only have one module >>>> that supports process attributes at a time. We are coming >>>> up on a change to that, where multiple modules with process >>>> attributes can be supported. (Not included here) >>>> >>>> This patch provides a subdirectory in /proc/.../attr for >>>> each of the security modules that use the LSM hooks >>>> getprocattr() and setprocattr(). Each of the interfaces >>>> used by a module are presented in the subdirectory. The >>>> old interfaces remain and work the same as before. >>>> User space code can begin migrating to the subdirectory >>>> interfaces in anticipation of the time when what comes >>>> from /proc/self/attr/current might not be what a runtime >>>> wants. >>>> >>>> The original implementation is by Kees Cook. The code >>>> has been changed a bit to reflect changes in the direction >>>> of the multiple concurrent module work, to be independent >>>> of it, and to bring it up to date with the current tree. >>>> >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>> >>>> --- >>>> Documentation/security/LSM.txt | 26 +++++++++--- >>>> fs/proc/base.c | 91 +++++++++++++++++++++++++++++++++++++----- >>>> fs/proc/internal.h | 1 + >>>> include/linux/security.h | 15 ++++--- >>>> security/security.c | 31 ++++++++++++-- >>>> 5 files changed, 140 insertions(+), 24 deletions(-) >>>> >>>> diff --git a/Documentation/security/LSM.txt b/Documentation/security/LSM.txt >>>> index 3db7e67..125c489 100644 >>>> --- a/Documentation/security/LSM.txt >>>> +++ b/Documentation/security/LSM.txt >>>> @@ -16,11 +16,25 @@ 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. >>>> -For more details on capabilities, see capabilities(7) in the Linux >>>> -man-pages project. >>>> +The Linux capabilities modules will always be included. For more details >>>> +on capabilities, see capabilities(7) in the Linux man-pages project. >>>> +This may be followed by any number of "minor" modules and at most one >>>> +"major" module. >>>> + >>>> +A list of the active security modules can be found by reading >>>> +/sys/kernel/security/lsm. This is a comma separated list, and >>>> +will always include the capability module. The list reflects the >>>> +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. >>> I wouldn't respin it just for this, but it seems like the paragraph >>> above should really be part of patch 1/3, yes? >> >> Yes. I can fix that for v5. >> >>>> +Process attributes associated with "ma >>>> diff --git a/fs/proc/base.c b/fs/proc/base.c >>>> index a11eb71..182bc28 100644 >>>> --- a/fs/proc/base.c >>>> +++ b/fs/proc/base.c >>>> @@ -131,9 +131,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 . >>>> @@ -2433,7 +2437,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); >>>> @@ -2473,7 +2477,7 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, >>>> if (length < 0) >>>> goto out_free; >>>> >>>> - length = security_setprocattr(task, >>>> + length = security_setprocattr(task, PROC_I(inode)->op.lsm, >>>> (char*)file->f_path.dentry->d_name.name, >>>> page, count); >>>> mutex_unlock(&task->signal->cred_guard_mutex); >>>> @@ -2491,13 +2495,82 @@ static const struct file_operations proc_pid_attr_operations = { >>>> .llseek = generic_file_llseek, >>>> }; >>>> >>>> +#define LSM_DIR_OPS(LSM) \ >>>> +static int proc_##LSM##_attr_dir_iterate(struct file *filp, \ >>>> + struct dir_context *ctx) \ >>>> +{ \ >>>> + return proc_pident_readdir(filp, ctx, \ >>>> + LSM##_attr_dir_stuff, \ >>>> + ARRAY_SIZE(LSM##_attr_dir_stuff)); \ >>>> +} \ >>>> +\ >>>> +static const struct file_operations proc_##LSM##_attr_dir_ops = { \ >>>> + .read = generic_read_dir, \ >>>> + .iterate = proc_##LSM##_attr_dir_iterate, \ >>>> + .llseek = default_llseek, \ >>>> +}; \ >>>> +\ >>>> +static struct dentry *proc_##LSM##_attr_dir_lookup(struct inode *dir, \ >>>> + struct dentry *dentry, unsigned int flags) \ >>>> +{ \ >>>> + return proc_pident_lookup(dir, dentry, \ >>>> + LSM##_attr_dir_stuff, \ >>>> + ARRAY_SIZE(LSM##_attr_dir_stuff)); \ >>>> +} \ >>>> +\ >>>> +static const struct inode_operations proc_##LSM##_attr_dir_inode_ops = { \ >>>> + .lookup = proc_##LSM##_attr_dir_lookup, \ >>>> + .getattr = pid_getattr, \ >>>> + .setattr = proc_setattr, \ >>>> +} >>>> + >>>> +#ifdef CONFIG_SECURITY_SELINUX >>>> +static const struct pid_entry selinux_attr_dir_stuff[] = { >>>> + ATTR("selinux", "current", S_IRUGO|S_IWUGO), >>>> + ATTR("selinux", "prev", S_IRUGO), >>>> + ATTR("selinux", "exec", S_IRUGO|S_IWUGO), >>>> + ATTR("selinux", "fscreate", S_IRUGO|S_IWUGO), >>>> + ATTR("selinux", "keycreate", S_IRUGO|S_IWUGO), >>>> + ATTR("selinux", "sockcreate", S_IRUGO|S_IWUGO), >>>> +}; >>>> +LSM_DIR_OPS(selinux); >>>> +#endif >>>> + >>>> +#ifdef CONFIG_SECURITY_SMACK >>>> +static const struct pid_entry smack_attr_dir_stuff[] = { >>>> + ATTR("smack", "current", S_IRUGO|S_IWUGO), >>>> +}; >>>> +LSM_DIR_OPS(smack); >>>> +#endif >>>> + >>>> +#ifdef CONFIG_SECURITY_APPARMOR >>>> +static const struct pid_entry apparmor_attr_dir_stuff[] = { >>>> + ATTR("apparmor", "current", S_IRUGO|S_IWUGO), >>>> + ATTR("apparmor", "prev", S_IRUGO), >>>> + ATTR("apparmor", "exec", S_IRUGO|S_IWUGO), >>>> +}; >>>> +LSM_DIR_OPS(apparmor); >>>> +#endif >>>> + >>>> static const struct pid_entry attr_dir_stuff[] = { >>>> - REG("current", S_IRUGO|S_IWUGO, proc_pid_attr_operations), >>>> - REG("prev", S_IRUGO, proc_pid_attr_operations), >>>> - REG("exec", S_IRUGO|S_IWUGO, proc_pid_attr_operations), >>>> - REG("fscreate", S_IRUGO|S_IWUGO, proc_pid_attr_operations), >>>> - REG("keycreate", S_IRUGO|S_IWUGO, proc_pid_attr_operations), >>>> - REG("sockcreate", S_IRUGO|S_IWUGO, proc_pid_attr_operations), >>>> + ATTR(NULL, "current", S_IRUGO|S_IWUGO), >>>> + ATTR(NULL, "prev", S_IRUGO), >>>> + ATTR(NULL, "exec", S_IRUGO|S_IWUGO), >>>> + ATTR(NULL, "fscreate", S_IRUGO|S_IWUGO), >>>> + ATTR(NULL, "keycreate", S_IRUGO|S_IWUGO), >>>> + ATTR(NULL, "sockcreate", S_IRUGO|S_IWUGO), >>>> +#ifdef CONFIG_SECURITY_SELINUX >>>> + DIR("selinux", S_IRUGO|S_IXUGO, >>>> + proc_selinux_attr_dir_inode_ops, proc_selinux_attr_dir_ops), >>>> +#endif >>>> +#ifdef CONFIG_SECURITY_SMACK >>>> + DIR("smack", S_IRUGO|S_IXUGO, >>>> + proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops), >>>> +#endif >>>> +#ifdef CONFIG_SECURITY_APPARMOR >>>> + DIR("apparmor", S_IRUGO|S_IXUGO, >>>> + proc_apparmor_attr_dir_inode_ops, proc_apparmor_attr_dir_ops), >>>> +#endif >>>> }; >>> With the number of LSMs set to grow, it seems like it might be a lot >>> cleaner, and easier to maintain, if we moved the various LSM pid_entry >>> definitions into the LSMs themselves. Granted, I say this without >>> seriously looking at how one would do that, I'm just mentioning it >>> here; it may prove to be more hassle than it is worth. > > I had the same suggestion, and when I looked at what it would take, I > decided this was just fine. ;) > >> I have looked into doing it that way, but have yet to >> come up with anything that would work. It seems like a >> wonderful challenge for a young, nimble brain. Or maybe >> an old wise one. In neither case, mine. > > I think it would require creating a number of new APIs to the proc > interface, and none of it looked fun.
Okay, I can live with that.
-- paul moore www.paul-moore.com
| |