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

[Casey Schaufler]

On 6/23/2016 2:49 PM, Kees Cook wrote:
> On Thu, Jun 23, 2016 at 2:11 PM, Casey Schaufler <casey@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
>>
>> ---
>>  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);
>>
>
>