From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752047AbcFWWKa (ORCPT ); Thu, 23 Jun 2016 18:10:30 -0400 Received: from nm33-vm7.bullet.mail.bf1.yahoo.com ([72.30.239.207]:55896 "EHLO nm33-vm7.bullet.mail.bf1.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750876AbcFWWK3 (ORCPT ); Thu, 23 Jun 2016 18:10:29 -0400 X-Yahoo-Newman-Id: 734063.86838.bm@smtp213.mail.bf1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: TEk1hkYVM1n8WsI2SjD_UxAaTZGcwIA5Qd54wl3QUZsnBnL LKoYOdWxazCOED0iYN1ZRXXpXapJuEDwVQFKy7XT2lSBFVDTDVTtHb7QBtn5 fSGUTPZs2GcTxMUXSkyt3bUeKSD5dk3XVnXli5PzdAvlKGeDGiZB05VWfvGQ 3DHmQbXhGG6gVoucEpyFwpv9vVeeCWGx5Q9nQ4tYe0BZx.NGyzqXtO2f_o6R VMxUJ0FHLCTSSnAhn2eUeo1At8mMh3kFP6_MZ5NH1bQSioWnD1xdx7xlupT1 5fiAXB0iN6QpeislOeWWbXKpBuaxvUjMVPobyqvYib0QGhjkSHouYiIYf7DX Qnk2yXu8azvtOA4eP0BLksdsx_HM7Qn6rrlaG8ZOFtW9DFgUH_TdO_eXGVwA Oq7S5cIlsKNDUF67mUiz.ZC29T5bLPZiBRv0n.2kDEpjbJZysulgwbCLioEn jrh9Vul.jstKK8A_cnp0VWTdWuomMF7m3EpWjgNCZWA1kcsXhSCT4SRuLbul _PqUN9ECZrwLOuE1rlYu9kntVBpJ7yAS3VI9WdUKw.H.NW7md24gNCkCK9dc - X-Yahoo-SMTP: OIJXglSswBDfgLtXluJ6wiAYv6_cnw-- Subject: Re: [PATCH v4 3/3] LSM: Add context interface for proc attrs To: Kees Cook References: <599d0a80-0838-2baa-8ee2-7eefafc10cec@schaufler-ca.com> <5767eed4-78ec-cc4c-2ece-c1fec4d752af@schaufler-ca.com> Cc: LSM , James Morris , John Johansen , Stephen Smalley , Paul Moore , Tetsuo Handa , LKLM From: Casey Schaufler Message-ID: <98e0171e-b4fa-a80d-5227-53d917c58343@schaufler-ca.com> Date: Thu, 23 Jun 2016 15:10:29 -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/23/2016 2:49 PM, Kees Cook wrote: > On Thu, Jun 23, 2016 at 2:11 PM, Casey Schaufler wrote: >> Subject: [PATCH v4 3/3] LSM: Add context interface for proc attrs >> >> The /proc/.../attr/current interface is used by all three >> Linux security modules (SELinux, Smack and AppArmor) to >> report and modify the process security attribute. This is >> all fine when there is exactly one of these modules active >> and the userspace code knows which it module it is. >> It would require a major change to the "current" interface >> to provide information about more than one set of process >> security attributes. Instead, a "context" attribute is >> added, which identifies the security module that the >> information applies to. The format is: >> >> lsmname='context-value' >> >> When multiple concurrent modules are supported the >> /proc/.../attr/context interface will include the data >> for all of the active modules. >> >> lsmname1='context-value1'lsmname2='context-value2' >> >> The module specific subdirectories under attr contain context >> entries that report the information for that specific module >> in the same format. >> >> Signed-off-by: Casey Schaufler >> >> --- >> fs/proc/base.c | 4 ++ >> security/apparmor/lsm.c | 34 +++++++++++++-- >> security/security.c | 100 +++++++++++++++++++++++++++++++++++++++++++++ >> security/selinux/hooks.c | 22 +++++++++- >> security/smack/smack_lsm.c | 21 ++++++---- >> 5 files changed, 167 insertions(+), 14 deletions(-) >> >> diff --git a/fs/proc/base.c b/fs/proc/base.c >> index 182bc28..df94f26 100644 >> --- a/fs/proc/base.c >> +++ b/fs/proc/base.c >> @@ -2532,6 +2532,7 @@ static const struct pid_entry selinux_attr_dir_stuff[] = { >> ATTR("selinux", "fscreate", S_IRUGO|S_IWUGO), >> ATTR("selinux", "keycreate", S_IRUGO|S_IWUGO), >> ATTR("selinux", "sockcreate", S_IRUGO|S_IWUGO), >> + ATTR("selinux", "context", S_IRUGO|S_IWUGO), >> }; >> LSM_DIR_OPS(selinux); >> #endif >> @@ -2539,6 +2540,7 @@ LSM_DIR_OPS(selinux); >> #ifdef CONFIG_SECURITY_SMACK >> static const struct pid_entry smack_attr_dir_stuff[] = { >> ATTR("smack", "current", S_IRUGO|S_IWUGO), >> + ATTR("smack", "context", S_IRUGO|S_IWUGO), >> }; >> LSM_DIR_OPS(smack); >> #endif >> @@ -2548,6 +2550,7 @@ 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), >> + ATTR("apparmor", "context", S_IRUGO|S_IWUGO), >> }; >> LSM_DIR_OPS(apparmor); >> #endif >> @@ -2559,6 +2562,7 @@ static const struct pid_entry attr_dir_stuff[] = { >> ATTR(NULL, "fscreate", S_IRUGO|S_IWUGO), >> ATTR(NULL, "keycreate", S_IRUGO|S_IWUGO), >> ATTR(NULL, "sockcreate", S_IRUGO|S_IWUGO), >> + ATTR(NULL, "context", 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), >> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c >> index fb0fb03..3790a7d 100644 >> --- a/security/apparmor/lsm.c >> +++ b/security/apparmor/lsm.c >> @@ -479,6 +479,8 @@ static int apparmor_getprocattr(struct task_struct *task, char *name, >> >> if (strcmp(name, "current") == 0) >> profile = aa_get_newest_profile(cxt->profile); >> + else if (strcmp(name, "context") == 0) >> + profile = aa_get_newest_profile(cxt->profile); >> else if (strcmp(name, "prev") == 0 && cxt->previous) >> profile = aa_get_newest_profile(cxt->previous); >> else if (strcmp(name, "exec") == 0 && cxt->onexec) >> @@ -486,8 +488,29 @@ static int apparmor_getprocattr(struct task_struct *task, char *name, >> else >> error = -EINVAL; >> >> - if (profile) >> - error = aa_getprocattr(profile, value); >> + if (profile) { >> + if (strcmp(name, "context") == 0) { >> + char *vp; >> + char *np; >> + >> + error = aa_getprocattr(profile, &vp); >> + if (error > 0) { >> + error += 12; >> + *value = kzalloc(error, GFP_KERNEL); >> + if (*value == NULL) >> + error = -ENOMEM; >> + else { >> + sprintf(*value, "apparmor='%s'", vp); > This and the others seem to still not be using kasprintf()? I got the one in security.c but missed the ones in the modules. Sigh. It'll be in v4. Thank you. > > -Kees > >> + np = strchr(*value, '\n'); >> + if (np != NULL) { >> + np[0] = '\''; >> + np[1] = '\0'; >> + } >> + } >> + } >> + } else >> + error = aa_getprocattr(profile, value); >> + } >> >> aa_put_profile(profile); >> put_cred(cred); >> @@ -530,7 +553,7 @@ static int apparmor_setprocattr(struct task_struct *task, char *name, >> return -EINVAL; >> >> arg_size = size - (args - (char *) value); >> - if (strcmp(name, "current") == 0) { >> + if (strcmp(name, "current") == 0 || strcmp(name, "context") == 0) { >> if (strcmp(command, "changehat") == 0) { >> error = aa_setprocattr_changehat(args, arg_size, >> !AA_DO_TEST); >> @@ -552,7 +575,10 @@ static int apparmor_setprocattr(struct task_struct *task, char *name, >> else >> goto fail; >> } else >> - /* only support the "current" and "exec" process attributes */ >> + /* >> + * only support the "current", context and "exec" >> + * process attributes >> + */ >> return -EINVAL; >> >> if (!error) >> diff --git a/security/security.c b/security/security.c >> index 1e9cb55..fec70b4 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -1186,8 +1186,47 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name, >> char **value) >> { >> struct security_hook_list *hp; >> + char *vp; >> + char *cp = NULL; >> int rc = -EINVAL; >> + int trc; >> >> + /* >> + * "context" requires work here in addition to what >> + * the modules provide. >> + */ >> + if (strcmp(name, "context") == 0) { >> + *value = NULL; >> + list_for_each_entry(hp, >> + &security_hook_heads.getprocattr, list) { >> + if (lsm != NULL && strcmp(lsm, hp->lsm)) >> + continue; >> + trc = hp->hook.getprocattr(p, "context", &vp); >> + if (trc == -ENOENT) >> + continue; >> + if (trc <= 0) { >> + kfree(*value); >> + return trc; >> + } >> + rc = trc; >> + if (*value == NULL) { >> + *value = vp; >> + } else { >> + cp = kasprintf(GFP_KERNEL, "%s%s", *value, vp); >> + if (cp == NULL) { >> + kfree(*value); >> + kfree(vp); >> + return -ENOMEM; >> + } >> + kfree(*value); >> + kfree(vp); >> + *value = cp; >> + } >> + } >> + if (rc > 0) >> + return strlen(*value); >> + return rc; >> + } >> >> list_for_each_entry(hp, &security_hook_heads.getprocattr, list) { >> if (lsm != NULL && strcmp(lsm, hp->lsm)) >> @@ -1204,7 +1243,68 @@ int security_setprocattr(struct task_struct *p, const char *lsm, char *name, >> { >> struct security_hook_list *hp; >> int rc = -EINVAL; >> + char *local; >> + char *cp; >> + int slen; >> + int failed = 0; >> >> + /* >> + * If lsm is NULL look at all the modules to find one >> + * that processes name. If lsm is not NULL only look at >> + * that module. >> + * >> + * "context" is handled directly here. >> + */ >> + if (strcmp(name, "context") == 0) { >> + /* >> + * First verify that the input is acceptable. >> + * lsm1='v1'lsm2='v2'lsm3='v3' >> + * >> + * A note on the use of strncmp() below. >> + * The check is for the substring at the beginning of cp. >> + * The kzalloc of size + 1 ensures a terminated string. >> + */ >> + local = kzalloc(size + 1, GFP_KERNEL); >> + memcpy(local, value, size); >> + cp = local; >> + list_for_each_entry(hp, &security_hook_heads.setprocattr, >> + list) { >> + if (lsm != NULL && strcmp(lsm, hp->lsm)) >> + continue; >> + slen = strlen(hp->lsm); >> + if (strncmp(cp, hp->lsm, slen)) >> + goto free_out; >> + cp += slen; >> + if (cp[0] != '=' || cp[1] != '\'' || cp[2] == '\'') >> + goto free_out; >> + for (cp += 2; cp[0] != '\''; cp++) >> + if (cp[0] == '\0') >> + goto free_out; >> + cp++; >> + } >> + cp = local; >> + list_for_each_entry(hp, &security_hook_heads.setprocattr, >> + list) { >> + if (lsm != NULL && strcmp(lsm, hp->lsm)) >> + continue; >> + cp += strlen(hp->lsm) + 2; >> + for (slen = 0; cp[slen] != '\''; slen++) >> + ; >> + cp[slen] = '\0'; >> + >> + rc = hp->hook.setprocattr(p, "context", cp, slen); >> + if (rc < 0) >> + failed = rc; >> + cp += slen + 1; >> + } >> + if (failed != 0) >> + rc = failed; >> + else >> + rc = size; >> +free_out: >> + kfree(local); >> + return rc; >> + } >> list_for_each_entry(hp, &security_hook_heads.setprocattr, list) { >> if (lsm != NULL && strcmp(lsm, hp->lsm)) >> continue; >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> index ed3a757..3a21c2b 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -5711,6 +5711,8 @@ static int selinux_getprocattr(struct task_struct *p, >> >> if (!strcmp(name, "current")) >> sid = __tsec->sid; >> + else if (!strcmp(name, "context")) >> + sid = __tsec->sid; >> else if (!strcmp(name, "prev")) >> sid = __tsec->osid; >> else if (!strcmp(name, "exec")) >> @@ -5728,7 +5730,21 @@ static int selinux_getprocattr(struct task_struct *p, >> if (!sid) >> return 0; >> >> - error = security_sid_to_context(sid, value, &len); >> + if (strcmp(name, "context")) { >> + error = security_sid_to_context(sid, value, &len); >> + } else { >> + char *vp; >> + >> + error = security_sid_to_context(sid, &vp, &len); >> + if (!error) { >> + *value = kzalloc(len + 10, GFP_KERNEL); >> + if (*value == NULL) >> + error = -ENOMEM; >> + else >> + sprintf(*value, "selinux='%s'", vp); >> + } >> + } >> + >> if (error) >> return error; >> return len; >> @@ -5768,6 +5784,8 @@ static int selinux_setprocattr(struct task_struct *p, >> error = current_has_perm(p, PROCESS__SETSOCKCREATE); >> else if (!strcmp(name, "current")) >> error = current_has_perm(p, PROCESS__SETCURRENT); >> + else if (!strcmp(name, "context")) >> + error = current_has_perm(p, PROCESS__SETCURRENT); >> else >> error = -EINVAL; >> if (error) >> @@ -5827,7 +5845,7 @@ static int selinux_setprocattr(struct task_struct *p, >> tsec->keycreate_sid = sid; >> } else if (!strcmp(name, "sockcreate")) { >> tsec->sockcreate_sid = sid; >> - } else if (!strcmp(name, "current")) { >> + } else if (!strcmp(name, "current") || !strcmp(name, "context")) { >> error = -EINVAL; >> if (sid == 0) >> goto abort_change; >> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c >> index 3577009..d2d8624 100644 >> --- a/security/smack/smack_lsm.c >> +++ b/security/smack/smack_lsm.c >> @@ -3576,16 +3576,21 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value) >> char *cp; >> int slen; >> >> - if (strcmp(name, "current") != 0) >> + if (strcmp(name, "current") == 0) { >> + cp = kstrdup(skp->smk_known, GFP_KERNEL); >> + if (cp == NULL) >> + return -ENOMEM; >> + } else if (strcmp(name, "context") == 0) { >> + slen = strlen(skp->smk_known) + 9; >> + cp = kzalloc(slen, GFP_KERNEL); >> + if (cp == NULL) >> + return -ENOMEM; >> + sprintf(cp, "smack='%s'", skp->smk_known); >> + } else >> return -EINVAL; >> >> - cp = kstrdup(skp->smk_known, GFP_KERNEL); >> - if (cp == NULL) >> - return -ENOMEM; >> - >> - slen = strlen(cp); >> *value = cp; >> - return slen; >> + return strlen(cp); >> } >> >> /** >> @@ -3622,7 +3627,7 @@ static int smack_setprocattr(struct task_struct *p, char *name, >> if (value == NULL || size == 0 || size >= SMK_LONGLABEL) >> return -EINVAL; >> >> - if (strcmp(name, "current") != 0) >> + if (strcmp(name, "current") != 0 && strcmp(name, "context") != 0) >> return -EINVAL; >> >> skp = smk_import_entry(value, size); >> > >