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

[Kees Cook]

On Tue, Jun 14, 2016 at 12:27 PM, Casey Schaufler
<casey@xxxxxxxxxxxxxxxx> wrote:
> On 6/14/2016 11:43 AM, Kees Cook wrote:
>> On Fri, Jun 10, 2016 at 2:25 PM, Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>>> Subject: [PATCH v3 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>
>>> [...]
>>> 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)),                     \
>> Accidental whitespace change?
>
> Space before a tab. checkpatch.pl was moaning about it.

Gotcha.

>
>>>                 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
>> I would still prefer these be defined in the LSM instead of in a common header.
>
> I am open to suggestions on how to accomplish that.

Okay, after reading through this code more, I'm convinced. Doing this
dynamically looks needlessly hard right now. :)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security