From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932069AbcFWVuF (ORCPT ); Thu, 23 Jun 2016 17:50:05 -0400 Received: from mail-wm0-f43.google.com ([74.125.82.43]:38591 "EHLO mail-wm0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750980AbcFWVuD (ORCPT ); Thu, 23 Jun 2016 17:50:03 -0400 MIME-Version: 1.0 In-Reply-To: <5767eed4-78ec-cc4c-2ece-c1fec4d752af@schaufler-ca.com> References: <599d0a80-0838-2baa-8ee2-7eefafc10cec@schaufler-ca.com> <5767eed4-78ec-cc4c-2ece-c1fec4d752af@schaufler-ca.com> From: Kees Cook Date: Thu, 23 Jun 2016 14:49:59 -0700 X-Google-Sender-Auth: SZzLsjQ_wsQcM8Nhlt8NWgwZNNc Message-ID: Subject: Re: [PATCH v4 3/3] LSM: Add context interface for proc attrs To: Casey Schaufler Cc: LSM , James Morris , John Johansen , Stephen Smalley , Paul Moore , Tetsuo Handa , LKLM Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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()? -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); > -- Kees Cook Chrome OS & Brillo Security