Re: [PATCH v4 2/3] LSM: module hierarchy in /proc/.../attr

From: Paul Moore
Date: Fri Jun 24 2016 - 16:29:35 EST

Next message: Salah Triki: "[PATCH] fs: befs: Replace befs_bread by sb_bread"
Previous message: Jason Gunthorpe: "Re: [PATCH v8 1/5] tpm_tis: Improve reporting of IO errors"
In reply to: Kees Cook: "Re: [PATCH v4 2/3] LSM: module hierarchy in /proc/.../attr"
Next in thread: Casey Schaufler: "[PATCH v5 0/3] LSM: security module information improvements"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Jun 24, 2016 at 4:08 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Fri, Jun 24, 2016 at 1:05 PM, Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>> On 6/24/2016 12:11 PM, Paul Moore wrote:
>>> On Thu, Jun 23, 2016 at 5:11 PM, Casey Schaufler <casey@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
>>>>
>>>> ---
>>>> 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

Next message: Salah Triki: "[PATCH] fs: befs: Replace befs_bread by sb_bread"
Previous message: Jason Gunthorpe: "Re: [PATCH v8 1/5] tpm_tis: Improve reporting of IO errors"
In reply to: Kees Cook: "Re: [PATCH v4 2/3] LSM: module hierarchy in /proc/.../attr"
Next in thread: Casey Schaufler: "[PATCH v5 0/3] LSM: security module information improvements"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]