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

[Casey Schaufler]

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 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.