Re: [PATCH v4 3/3] LSM: Add context interface for proc attrs

[Paul Moore <paul@paul-moore.com>]

On Fri, Jun 24, 2016 at 7:29 PM, Casey Schaufler <casey@schaufler-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@schaufler-ca.com>
>
> ---
>  Documentation/security/LSM.txt |   8 +++
>  fs/proc/base.c                 |   4 ++
>  security/apparmor/lsm.c        |  35 +++++++++++--
>  security/security.c            | 108 +++++++++++++++++++++++++++++++++++++++++
>  security/selinux/hooks.c       |  20 +++++++-
>  security/smack/smack_lsm.c     |  20 ++++----
>  6 files changed, 180 insertions(+), 15 deletions(-)

Acked-by: Paul Moore <paul@paul-moore.com>

> diff --git a/Documentation/security/LSM.txt b/Documentation/security/LSM.txt
> index 125c489..af3eb11 100644
> --- a/Documentation/security/LSM.txt
> +++ b/Documentation/security/LSM.txt
> @@ -36,6 +36,14 @@ 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.
>
> +The files named "context" in the attr directories contain the
> +same information as the "current" files, but formatted to
> +identify the module it comes from.
> +
> +if selinux is the active security module:
> +       /proc/self/attr/context could contain selinux='unconfined_t'
> +       /proc/self/attr/selinux/context could contain selinux='unconfined_t'
> +
>  Based on https://lkml.org/lkml/2007/10/26/215,
>  a new LSM is accepted into the kernel when its intent (a description of
>  what it tries to protect against and in what cases one would expect to
> 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..5cac15f 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -476,9 +476,13 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
>         const struct cred *cred = get_task_cred(task);
>         struct aa_task_cxt *cxt = cred_cxt(cred);
>         struct aa_profile *profile = NULL;
> +       char *vp;
> +       char *np;
>
>         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,9 +490,29 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
>         else
>                 error = -EINVAL;
>
> -       if (profile)
> -               error = aa_getprocattr(profile, value);
> +       if (profile == NULL)
> +               goto put_out;
> +
> +       error = aa_getprocattr(profile, &vp);
> +       if (error < 0)
> +               goto put_out;
> +
> +       if (strcmp(name, "context") == 0) {
> +               *value = kasprintf(GFP_KERNEL, "apparmor='%s'", vp);
> +               if (*value == NULL) {
> +                       error = -ENOMEM;
> +                       goto put_out;
> +               }
> +               np = strchr(*value, '\n');
> +               if (np != NULL) {
> +                       np[0] = '\'';
> +                       np[1] = '\0';
> +               }
> +               error = strlen(*value);
> +       } else
> +               *value = vp;
>
> +put_out:
>         aa_put_profile(profile);
>         put_cred(cred);
>
> @@ -530,7 +554,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 +576,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..f97f0d9 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,76 @@ 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;
> +                       if (cp[0] == ',') {
> +                               if (cp == local)
> +                                       goto free_out;
> +                               cp++;
> +                       }
> +                       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;
> +                       if (cp[0] == ',')
> +                               cp++;
> +                       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..6397721 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,19 @@ 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 = kasprintf(GFP_KERNEL, "selinux='%s'", vp);
> +                       if (*value == NULL)
> +                               error = -ENOMEM;
> +               }
> +       }
> +
>         if (error)
>                 return error;
>         return len;
> @@ -5768,6 +5782,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 +5843,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..92e66f8 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3574,18 +3574,20 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>  {
>         struct smack_known *skp = smk_of_task_struct(p);
>         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) {
> +               cp = kasprintf(GFP_KERNEL, "smack='%s'", skp->smk_known);
> +               if (cp == NULL)
> +                       return -ENOMEM;
> +       } 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 +3624,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);
>



-- 
paul moore
www.paul-moore.com