| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <98e0171e-b4fa-a80d-5227-53d917c58343@schaufler-ca.com>
Date: Thu, 23 Jun 2016 15:10:29 -0700
From: Casey Schaufler <casey@...aufler-ca.com>
To: Kees Cook <keescook@...omium.org>
Cc: LSM <linux-security-module@...r.kernel.org>,
James Morris <jmorris@...ei.org>,
John Johansen <john.johansen@...onical.com>,
Stephen Smalley <sds@...ho.nsa.gov>,
Paul Moore <paul@...l-moore.com>,
Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
LKLM <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 3/3] LSM: Add context interface for proc attrs
On 6/23/2016 2:49 PM, Kees Cook wrote:
> On Thu, Jun 23, 2016 at 2:11 PM, Casey Schaufler <casey@...aufler-ca.com> 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 <casey@...aufler-ca.com>
>>
>> ---
>> 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);
>>
>
>
Powered by blists - more mailing lists