| 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
| ||
|
Date: Tue, 14 Jun 2016 11:57:24 -0700
From: Kees Cook <keescook@...omium.org>
To: Casey Schaufler <casey@...aufler-ca.com>
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>,
James Morris <james.l.morris@...cle.com>
Subject: Re: [PATCH v3 3/3] LSM: Add context interface for proc attr
On Fri, Jun 10, 2016 at 2:25 PM, Casey Schaufler <casey@...aufler-ca.com> wrote:
> Subject: [PATCH v3 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'
For humans to read this, could we add a space between each?
>
> 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 | 97 ++++++++++++++++++++++++++++++++++++++++++++++
> security/selinux/hooks.c | 22 ++++++++++-
> security/smack/smack_lsm.c | 21 ++++++----
> 5 files changed, 164 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);
> + 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 41ac80d..6f7ad19 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1187,8 +1187,49 @@ 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 = kzalloc(strlen(*value) + strlen(vp) + 1,
> + GFP_KERNEL);
> + if (cp == NULL) {
> + kfree(*value);
> + kfree(vp);
> + return -ENOMEM;
> + }
> + sprintf(cp, "%s%s", *value, vp);
This is another place kasprinf() would be cleaner.
> + 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))
> @@ -1205,7 +1246,63 @@ 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;
>
> + /*
> + * "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.
> + */
I'd add a note that this is creating a NULL-terminated string. Though
to that end, should NULL bytes be invalid in the input string? It
could confuse things.
> + 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++;
> + }
Is it okay that this loop allows unknown LSMs to be input? (I think
it's okay.) Also, should lsm == NULL be checked early and
rejected/skipped so the lsm != NULL test isn't needed in both loops?
> + 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);
More kasprintf...
> + }
> + }
> +
> 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);
And more kasprintf
> + } 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);
>
Otherwise, looks good to me.
-Kees
--
Kees Cook
Chrome OS & Brillo Security
Powered by blists - more mailing lists