From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932131AbcFNT2H (ORCPT ); Tue, 14 Jun 2016 15:28:07 -0400 Received: from nm16.bullet.mail.bf1.yahoo.com ([98.139.212.175]:51937 "EHLO nm16.bullet.mail.bf1.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751153AbcFNT2E (ORCPT ); Tue, 14 Jun 2016 15:28:04 -0400 X-Yahoo-Newman-Id: 778734.92312.bm@smtp115.mail.bf1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: 9UBQdusVM1kvTm_oHzQmwrj.6Lg6tNMZ80.E_18fXp6Oeck bPi5EJrNHva8Mq7dDd4hqH3SiC9mRalkoSkHOKk6sVsWyp8nZDGnzzArTdvg 06PMzlLv6AKtLewnGk1HE4Z8dhxpHFqzCxr19_mTDJjJHs_TVHSmpDyeNFTG TWRFX..FumN8N80ojlEhL5U892kqKTNwSVyDr.aX201WcBfiNeNr_pZgfSNH XonaCW19BGc1jqLf21drW8HcVVTlgdWyAw8cLxk9czb7tStZDGKjQONKDPxY ZfOTsYPwMkQvApmAoEWp7DjZM.Pw73r1Utdj6eSwFqDzzz0W.Avuc4nFc8dP C_LguKcX8rOsIAmE2FCSqV22T7_JuS6dGp7IEDLjV7v.Jm0SSxZ3oDzi.mJB VDzH52kjtij.CxmrezmmTHZnMQn1x8mi7Ev.OAZw7j3t.QUJhSBFw9il1uSe wZXx9FxLdygnKr7CW0Ik6siwKUUT9qurf0fJ3lZ3fwuzSZVx87SEx.I8JAtD GKhx9nSU.AWRwIBr7Cr2ZJxpi83dURyyFa4l7DIoz9tRBqeY- X-Yahoo-SMTP: OIJXglSswBDfgLtXluJ6wiAYv6_cnw-- Subject: Re: [PATCH v3 2/3] LSM: module hierarchy in /proc/.../attr To: Kees Cook References: <3dd03d7d-7c6f-5b6d-062b-6e5fb7e2367b@schaufler-ca.com> Cc: LSM , James Morris , John Johansen , Stephen Smalley , Paul Moore , Tetsuo Handa , LKLM , James Morris From: Casey Schaufler Message-ID: <2f88b40f-e8f4-2ccc-122c-96615db402fa@schaufler-ca.com> Date: Tue, 14 Jun 2016 12:27:59 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 6/14/2016 11:43 AM, Kees Cook wrote: > On Fri, Jun 10, 2016 at 2:25 PM, Casey Schaufler 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 >> >> --- >> 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 >