Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1422219 > unrolled thread
| Started by | Kees Cook <keescook@chromium.org> |
|---|---|
| First post | 2016-06-14 20:50 +0200 |
| Last post | 2016-06-14 23:30 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v3 2/3] LSM: module hierarchy in /proc/.../attr Kees Cook <keescook@chromium.org> - 2016-06-14 20:50 +0200
Re: [PATCH v3 2/3] LSM: module hierarchy in /proc/.../attr Casey Schaufler <casey@schaufler-ca.com> - 2016-06-14 21:30 +0200
Re: [PATCH v3 2/3] LSM: module hierarchy in /proc/.../attr Kees Cook <keescook@chromium.org> - 2016-06-14 23:30 +0200
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2016-06-14 20:50 +0200 |
| Subject | Re: [PATCH v3 2/3] LSM: module hierarchy in /proc/.../attr |
| Message-ID | <rK1zs-bN-29@gated-at.bofh.it> |
On Fri, Jun 10, 2016 at 2:25 PM, Casey Schaufler <casey@schaufler-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@schaufler-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
[toc] | [next] | [standalone]
| From | Casey Schaufler <casey@schaufler-ca.com> |
|---|---|
| Date | 2016-06-14 21:30 +0200 |
| Message-ID | <rK2c9-Fp-25@gated-at.bofh.it> |
| In reply to | #1422219 |
On 6/14/2016 11:43 AM, Kees Cook wrote:
> On Fri, Jun 10, 2016 at 2:25 PM, Casey Schaufler <casey@schaufler-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@schaufler-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?
Space before a tab. checkpatch.pl was moaning about it.
>> 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.
>> +
>> +#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
>
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2016-06-14 23:30 +0200 |
| Message-ID | <rK44i-1Sc-17@gated-at.bofh.it> |
| In reply to | #1422248 |
On Tue, Jun 14, 2016 at 12:27 PM, Casey Schaufler
<casey@schaufler-ca.com> wrote:
> On 6/14/2016 11:43 AM, Kees Cook wrote:
>> On Fri, Jun 10, 2016 at 2:25 PM, Casey Schaufler <casey@schaufler-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@schaufler-ca.com>
>>> [...]
>>> 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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web