Date:	Tue, 14 Jun 2016 11:43:44 -0700
From:	Kees Cook <keescook@...omium.org>
To:	Casey Schaufler <casey@...aufler-ca.com>
Cc:	LSM <linux-security-module@...r.kernel.org>,
	James Morris <jmorris@...ei.org>,
	John Johansen <john.johansen@...onical.com>,
	Stephen Smalley <sds@...ho.nsa.gov>,
	Paul Moore <paul@...l-moore.com>,
	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
	LKLM <linux-kernel@...r.kernel.org>,
	James Morris <james.l.morris@...cle.com>
Subject: Re: [PATCH v3 2/3] LSM: module hierarchy in /proc/.../attr

On Fri, Jun 10, 2016 at 2:25 PM, Casey Schaufler <casey@...aufler-ca.com> 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@...aufler-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.
> +
> +Process attributes associated with "major" security modules should
> +be accessed and maintained using the special files in the module
> +specific subdirectories in /proc/.../attr. The attributes related
> +to Smack would be found in /proc/.../attr/smack while the attributes
> +for SELinux would be in /proc/.../attr/selinux. Using the files
> +found directly in /proc/.../attr (e.g. current) should be avoided.
> +These files remain as legacy interfaces.
>
>  Based on https://lkml.org/lkml/2007/10/26/215,
>  a new LSM is accepted into the kernel when its intent (a description of
> @@ -31,4 +45,4 @@ that end users and distros can make a more informed decision about which
>  LSMs suit their requirements.
>
>  For extensive documentation on the available LSM hook interfaces, please
> -see include/linux/security.h.
> +see include/linux/lsm_hooks.h.
> 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?

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

> +
> +#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
>  };
>
>  static int proc_attr_dir_readdir(struct file *file, struct dir_context *ctx)
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index aa27810..b607cd5 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -56,6 +56,7 @@ union proc_op {
>         int (*proc_show)(struct seq_file *m,
>                 struct pid_namespace *ns, struct pid *pid,
>                 struct task_struct *task);
> +       const char *lsm;
>  };
>
>  struct proc_inode {
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 14df373..383fcb0 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -355,8 +355,10 @@ int security_sem_semctl(struct sem_array *sma, int cmd);
>  int security_sem_semop(struct sem_array *sma, struct sembuf *sops,
>                         unsigned nsops, int alter);
>  void security_d_instantiate(struct dentry *dentry, struct inode *inode);
> -int security_getprocattr(struct task_struct *p, char *name, char **value);
> -int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size);
> +int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> +                               char **value);
> +int security_setprocattr(struct task_struct *p, const char *lsm, char *name,
> +                               void *value, size_t size);
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
>  int security_ismaclabel(const char *name);
>  int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> @@ -1075,15 +1077,18 @@ static inline int security_sem_semop(struct sem_array *sma,
>         return 0;
>  }
>
> -static inline void security_d_instantiate(struct dentry *dentry, struct inode *inode)
> +static inline void security_d_instantiate(struct dentry *dentry,
> +                                               struct inode *inode)
>  { }
>
> -static inline int security_getprocattr(struct task_struct *p, char *name, char **value)
> +static inline int security_getprocattr(struct task_struct *p, const char *lsm,
> +                                       char *name, char **value)
>  {
>         return -EINVAL;
>  }
>
> -static inline int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size)
> +static inline int security_setprocattr(struct task_struct *p, const char *lsm,
> +                                       char *name, void *value, size_t size)
>  {
>         return -EINVAL;
>  }
> diff --git a/security/security.c b/security/security.c
> index dd792d5..41ac80d 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1183,14 +1183,37 @@ void security_d_instantiate(struct dentry *dentry, struct inode *inode)
>  }
>  EXPORT_SYMBOL(security_d_instantiate);
>
> -int security_getprocattr(struct task_struct *p, char *name, char **value)
> +int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> +                               char **value)
>  {
> -       return call_int_hook(getprocattr, -EINVAL, p, name, value);
> +       struct security_hook_list *hp;
> +       int rc = -EINVAL;
> +
> +
> +       list_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
> +               if (lsm != NULL && strcmp(lsm, hp->lsm))
> +                       continue;
> +               rc = hp->hook.getprocattr(p, name, value);
> +               if (rc != -ENOENT)
> +                       return rc;
> +       }
> +       return -EINVAL;
>  }
>
> -int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size)
> +int security_setprocattr(struct task_struct *p, const char *lsm, char *name,
> +                               void *value, size_t size)
>  {
> -       return call_int_hook(setprocattr, -EINVAL, p, name, value, size);
> +       struct security_hook_list *hp;
> +       int rc = -EINVAL;
> +
> +       list_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
> +               if (lsm != NULL && strcmp(lsm, hp->lsm))
> +                       continue;
> +               rc = hp->hook.setprocattr(p, name, value, size);
> +               if (rc != -ENOENT)
> +                       break;
> +       }
> +       return rc;
>  }
>
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb)
>

With those changes, I'd like it. :)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Powered by blists - more mailing lists