Re: [PATCH 01/10] procfs: add smack subdir to attrs

From: Kees Cook
Date: Wed Sep 12 2018 - 18:57:31 EST

Next message: Alexey Dobriyan: "Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information"
Previous message: Ard Biesheuvel: "Re: [PATCH net-next v3 02/17] zinc: introduce minimal cryptography library"
In reply to: Casey Schaufler: "Re: [PATCH 01/10] procfs: add smack subdir to attrs"
Next in thread: Casey Schaufler: "[PATCH 02/10] Smack: Abstract use of cred security blob"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Sep 11, 2018 at 9:41 AM, Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
> 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.
>
> This patch provides a subdirectory in /proc/.../attr for
> Smack. Smack user space can use the "current" file in
> this subdirectory and never have to worry about getting
> SELinux attributes by mistake. Programs that use the
> old interface will continue to work (or fail, as the case
> may be) as before.
>
> The proposed S.A.R.A security module is dependent on
> the mechanism to create its own attr subdirectory.
>
> The original implementation is by Kees Cook.
>
> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> ---
> Documentation/admin-guide/LSM/index.rst | 13 +++--
> fs/proc/base.c | 64 +++++++++++++++++++++----
> fs/proc/internal.h | 1 +
> include/linux/security.h | 15 ++++--
> security/security.c | 24 ++++++++--
> 5 files changed, 96 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/admin-guide/LSM/index.rst b/Documentation/admin-guide/LSM/index.rst
> index c980dfe9abf1..9842e21afd4a 100644
> --- a/Documentation/admin-guide/LSM/index.rst
> +++ b/Documentation/admin-guide/LSM/index.rst
> @@ -17,9 +17,8 @@ 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.
> +The Linux capabilities modules will always be included. This may be
> +followed by any number of "minor" modules and at most one "major" module.
> For more details on capabilities, see ``capabilities(7)`` in the Linux
> man-pages project.
>
> @@ -30,6 +29,14 @@ 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 ``/proc/.../attr``.
> +A security module may maintain a module specific subdirectory there,
> +named after the module. ``/proc/.../attr/smack`` is provided by the Smack
> +security module and contains all its special files. The files directly
> +in ``/proc/.../attr`` remain as legacy interfaces for modules that provide
> +subdirectories.
> +
> .. toctree::
> :maxdepth: 1
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index ccf86f16d9f0..bd2dd85310fe 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -140,9 +140,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 .
> @@ -2503,7 +2507,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);
> @@ -2552,7 +2556,9 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
> if (rv < 0)
> goto out_free;
>
> - rv = security_setprocattr(file->f_path.dentry->d_name.name, page, count);
> + rv = security_setprocattr(PROC_I(inode)->op.lsm,
> + file->f_path.dentry->d_name.name, page,
> + count);
> mutex_unlock(&current->signal->cred_guard_mutex);
> out_free:
> kfree(page);
> @@ -2566,13 +2572,53 @@ 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_SMACK
> +static const struct pid_entry smack_attr_dir_stuff[] = {
> + ATTR("smack", "current", 0666),
> +};
> +LSM_DIR_OPS(smack);
> +#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", 0666),
> + ATTR(NULL, "prev", 0444),
> + ATTR(NULL, "exec", 0666),
> + ATTR(NULL, "fscreate", 0666),
> + ATTR(NULL, "keycreate", 0666),
> + ATTR(NULL, "sockcreate", 0666),
> +#ifdef CONFIG_SECURITY_SMACK
> + DIR("smack", 0555,
> + proc_smack_attr_dir_inode_ops, proc_smack_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 5185d7f6a51e..d4f9989063d0 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -81,6 +81,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 75f4156c84d7..418de5d20ffb 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -390,8 +390,10 @@ int security_sem_semctl(struct kern_ipc_perm *sma, int cmd);
> int security_sem_semop(struct kern_ipc_perm *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(const char *name, void *value, size_t size);
> +int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> + char **value);
> +int security_setprocattr(const char *lsm, const 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);
> @@ -1139,15 +1141,18 @@ static inline int security_sem_semop(struct kern_ipc_perm *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(char *name, void *value, size_t size)
> +static inline int security_setprocattr(const char *lsm, char *name,
> + void *value, size_t size)
> {
> return -EINVAL;
> }
> diff --git a/security/security.c b/security/security.c
> index 736e78da1ab9..3dfe75d0d373 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1288,14 +1288,30 @@ 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;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
> + if (lsm != NULL && strcmp(lsm, hp->lsm))
> + continue;
> + return hp->hook.getprocattr(p, name, value);
> + }

Walking this list to do strcmp() makes my eye twitch. ;) I see that
procfs doesn't let us do a late evaluation and attr_dir_stuff is
const. We already have security_initcall() exporting a per-LSM
function, why not expand this slightly to include enough information
that we could resolve all this at build time?

Anyway, I view that as a potential improvement, and not something that
should block this. So:

Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>

> + return -EINVAL;
> }
>
> -int security_setprocattr(const char *name, void *value, size_t size)
> +int security_setprocattr(const char *lsm, const char *name, void *value,
> + size_t size)
> {
> - return call_int_hook(setprocattr, -EINVAL, name, value, size);
> + struct security_hook_list *hp;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
> + if (lsm != NULL && strcmp(lsm, hp->lsm))
> + continue;
> + return hp->hook.setprocattr(name, value, size);
> + }
> + return -EINVAL;
> }
>
> int security_netlink_send(struct sock *sk, struct sk_buff *skb)
> --
> 2.17.1
>
>

--
Kees Cook
Pixel Security

Next message: Alexey Dobriyan: "Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information"
Previous message: Ard Biesheuvel: "Re: [PATCH net-next v3 02/17] zinc: introduce minimal cryptography library"
In reply to: Casey Schaufler: "Re: [PATCH 01/10] procfs: add smack subdir to attrs"
Next in thread: Casey Schaufler: "[PATCH 02/10] Smack: Abstract use of cred security blob"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]