Message-ID: <52758e36-63aa-a8e8-7aa7-e7217b81cbfe@schaufler-ca.com>
Date:   Tue, 14 Dec 2021 10:49:39 -0800
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Christian Göttsche <cgzones@...glemail.com>
Cc:     casey.schaufler@...el.com, jmorris@...ei.org,
        linux-security-module@...r.kernel.org,
        SElinux list <selinux@...r.kernel.org>, linux-audit@...hat.com,
        Kees Cook <keescook@...omium.org>, john.johansen@...onical.com,
        penguin-kernel@...ove.sakura.ne.jp,
        Paul Moore <paul@...l-moore.com>,
        Stephen Smalley <sds@...ho.nsa.gov>,
        linux-kernel@...r.kernel.org,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-api@...r.kernel.org, linux-doc@...r.kernel.org,
        Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v31 14/28] LSM: Specify which LSM to display

On 12/14/2021 10:38 AM, Christian Göttsche wrote:
> On Tue, 14 Dec 2021 at 00:56, Casey Schaufler <casey@...aufler-ca.com> wrote:
>> Create a new entry "interface_lsm" in the procfs attr directory for
>> controlling which LSM security information is displayed for a
>> process. A process can only read or write its own display value.
>>
>> The name of an active LSM that supplies hooks for
>> human readable data may be written to "interface_lsm" to set the
>> value. The name of the LSM currently in use can be read from
>> "interface_lsm". At this point there can only be one LSM capable
>> of display active. A helper function lsm_task_ilsm() is
>> provided to get the interface lsm slot for a task_struct.
>>
>> Setting the "interface_lsm" requires that all security modules using
>> setprocattr hooks allow the action. Each security module is
>> responsible for defining its policy.
>>
>> AppArmor hook provided by John Johansen <john.johansen@...onical.com>
>> SELinux hook provided by Stephen Smalley <stephen.smalley.work@...il.com>
>>
>> Signed-off-by: Casey Schaufler <casey@...aufler-ca.com>
>> Cc: Kees Cook <keescook@...omium.org>
>> Cc: Stephen Smalley <stephen.smalley.work@...il.com>
>> Cc: Paul Moore <paul@...l-moore.com>
>> Cc: John Johansen <john.johansen@...onical.com>
>> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>> Cc: linux-api@...r.kernel.org
>> Cc: linux-doc@...r.kernel.org
>> ---
>>   .../ABI/testing/procfs-attr-lsm_display       |  22 +++
>>   Documentation/security/lsm.rst                |  14 ++
>>   fs/proc/base.c                                |   1 +
>>   include/linux/security.h                      |  17 ++
>>   security/apparmor/include/apparmor.h          |   3 +-
>>   security/apparmor/lsm.c                       |  32 ++++
>>   security/security.c                           | 166 ++++++++++++++++--
>>   security/selinux/hooks.c                      |  11 ++
>>   security/selinux/include/classmap.h           |   2 +-
>>   security/smack/smack_lsm.c                    |   7 +
>>   10 files changed, 256 insertions(+), 19 deletions(-)
>>   create mode 100644 Documentation/ABI/testing/procfs-attr-lsm_display
>>
>> diff --git a/Documentation/ABI/testing/procfs-attr-lsm_display b/Documentation/ABI/testing/procfs-attr-lsm_display
>> new file mode 100644
>> index 000000000000..0f60005c235c
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/procfs-attr-lsm_display
>> @@ -0,0 +1,22 @@
>> +What:          /proc/*/attr/lsm_display
>> +Contact:       linux-security-module@...r.kernel.org,
>> +Description:   The name of the Linux security module (LSM) that will
>> +               provide information in the /proc/*/attr/current,
>> +               /proc/*/attr/prev and /proc/*/attr/exec interfaces.
>> +               The details of permissions required to read from
>> +               this interface are dependent on the LSMs active on the
>> +               system.
>> +               A process cannot write to this interface unless it
>> +               refers to itself.
>> +               The other details of permissions required to write to
>> +               this interface are dependent on the LSMs active on the
>> +               system.
>> +               The format of the data used by this interface is a
>> +               text string identifying the name of an LSM. The values
>> +               accepted are:
>> +                       selinux         - the SELinux LSM
>> +                       smack           - the Smack LSM
>> +                       apparmor        - The AppArmor LSM
>> +               By convention the LSM names are lower case and do not
>> +               contain special characters.
>> +Users:         LSM user-space
>> diff --git a/Documentation/security/lsm.rst b/Documentation/security/lsm.rst
>> index 6a2a2e973080..b77b4a540391 100644
>> --- a/Documentation/security/lsm.rst
>> +++ b/Documentation/security/lsm.rst
>> @@ -129,3 +129,17 @@ to identify it as the first security module to be registered.
>>   The capabilities security module does not use the general security
>>   blobs, unlike other modules. The reasons are historical and are
>>   based on overhead, complexity and performance concerns.
>> +
>> +LSM External Interfaces
>> +=======================
>> +
>> +The LSM infrastructure does not generally provide external interfaces.
>> +The individual security modules provide what external interfaces they
>> +require.
>> +
>> +The file ``/sys/kernel/security/lsm`` provides a comma
>> +separated list of the active security modules.
>> +
>> +The file ``/proc/pid/attr/interface_lsm`` contains the name of the security
>> +module for which the ``/proc/pid/attr/current`` interface will
>> +apply. This interface can be written to.
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index 13eda8de2998..50dbe5612a26 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -2828,6 +2828,7 @@ static const struct pid_entry attr_dir_stuff[] = {
>>          ATTR(NULL, "fscreate",          0666),
>>          ATTR(NULL, "keycreate",         0666),
>>          ATTR(NULL, "sockcreate",        0666),
>> +       ATTR(NULL, "interface_lsm",     0666),
>>   #ifdef CONFIG_SECURITY_SMACK
>>          DIR("smack",                    0555,
>>              proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 3b653fe331dd..872e543d37dd 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -219,6 +219,23 @@ static inline u32 lsmblob_value(const struct lsmblob *blob)
>>          return 0;
>>   }
>>
>> +/**
>> + * lsm_task_ilsm - the "interface_lsm" for this task
>> + * @task: The task to report on
>> + *
>> + * Returns the task's interface LSM slot.
>> + */
>> +static inline int lsm_task_ilsm(struct task_struct *task)
>> +{
>> +#ifdef CONFIG_SECURITY
>> +       int *ilsm = task->security;
>> +
>> +       if (ilsm)
>> +               return *ilsm;
>> +#endif
>> +       return LSMBLOB_INVALID;
>> +}
>> +
>>   /* These functions are in security/commoncap.c */
>>   extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
>>                         int cap, unsigned int opts);
>> diff --git a/security/apparmor/include/apparmor.h b/security/apparmor/include/apparmor.h
>> index 1fbabdb565a8..b1622fcb4394 100644
>> --- a/security/apparmor/include/apparmor.h
>> +++ b/security/apparmor/include/apparmor.h
>> @@ -28,8 +28,9 @@
>>   #define AA_CLASS_SIGNAL                10
>>   #define AA_CLASS_NET           14
>>   #define AA_CLASS_LABEL         16
>> +#define AA_CLASS_DISPLAY_LSM   17
>>
>> -#define AA_CLASS_LAST          AA_CLASS_LABEL
>> +#define AA_CLASS_LAST          AA_CLASS_DISPLAY_LSM
>>
>>   /* Control parameters settable through module/boot flags */
>>   extern enum audit_mode aa_g_audit;
>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
>> index 24241db8ec54..5ed40fd93ce9 100644
>> --- a/security/apparmor/lsm.c
>> +++ b/security/apparmor/lsm.c
>> @@ -621,6 +621,25 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
>>          return error;
>>   }
>>
>> +
>> +static int profile_interface_lsm(struct aa_profile *profile,
>> +                                struct common_audit_data *sa)
>> +{
>> +       struct aa_perms perms = { };
>> +       unsigned int state;
>> +
>> +       state = PROFILE_MEDIATES(profile, AA_CLASS_DISPLAY_LSM);
>> +       if (state) {
>> +               aa_compute_perms(profile->policy.dfa, state, &perms);
>> +               aa_apply_modes_to_perms(profile, &perms);
>> +               aad(sa)->label = &profile->label;
>> +
>> +               return aa_check_perms(profile, &perms, AA_MAY_WRITE, sa, NULL);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>   static int apparmor_setprocattr(const char *name, void *value,
>>                                  size_t size)
>>   {
>> @@ -632,6 +651,19 @@ static int apparmor_setprocattr(const char *name, void *value,
>>          if (size == 0)
>>                  return -EINVAL;
>>
>> +       /* LSM infrastructure does actual setting of interface_lsm if allowed */
>> +       if (!strcmp(name, "interface_lsm")) {
>> +               struct aa_profile *profile;
>> +               struct aa_label *label;
>> +
>> +               aad(&sa)->info = "set interface lsm";
>> +               label = begin_current_label_crit_section();
>> +               error = fn_for_each_confined(label, profile,
>> +                                       profile_interface_lsm(profile, &sa));
>> +               end_current_label_crit_section(label);
>> +               return error;
>> +       }
>> +
>>          /* AppArmor requires that the buffer must be null terminated atm */
>>          if (args[size - 1] != '\0') {
>>                  /* null terminate */
>> diff --git a/security/security.c b/security/security.c
>> index 0e17620a60e2..1d734d9579f1 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -78,7 +78,16 @@ static struct kmem_cache *lsm_file_cache;
>>   static struct kmem_cache *lsm_inode_cache;
>>
>>   char *lsm_names;
>> -static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init;
>> +
>> +/*
>> + * The task blob includes the "interface_lsm" slot used for
>> + * chosing which module presents contexts.
>> + * Using a long to avoid potential alignment issues with
>> + * module assigned task blobs.
>> + */
>> +static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init = {
>> +       .lbs_task = sizeof(long),
>> +};
>>
>>   /* Boot-time LSM user choice */
>>   static __initdata const char *chosen_lsm_order;
>> @@ -672,6 +681,8 @@ int lsm_inode_alloc(struct inode *inode)
>>    */
>>   static int lsm_task_alloc(struct task_struct *task)
>>   {
>> +       int *ilsm;
>> +
>>          if (blob_sizes.lbs_task == 0) {
>>                  task->security = NULL;
>>                  return 0;
>> @@ -680,6 +691,15 @@ static int lsm_task_alloc(struct task_struct *task)
>>          task->security = kzalloc(blob_sizes.lbs_task, GFP_KERNEL);
>>          if (task->security == NULL)
>>                  return -ENOMEM;
>> +
>> +       /*
>> +        * The start of the task blob contains the "interface" LSM slot number.
>> +        * Start with it set to the invalid slot number, indicating that the
>> +        * default first registered LSM be displayed.
>> +        */
>> +       ilsm = task->security;
>> +       *ilsm = LSMBLOB_INVALID;
>> +
>>          return 0;
>>   }
>>
>> @@ -1736,14 +1756,26 @@ int security_file_open(struct file *file)
>>
>>   int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
>>   {
>> +       int *oilsm = current->security;
>> +       int *nilsm;
>>          int rc = lsm_task_alloc(task);
>>
>> -       if (rc)
>> +       if (unlikely(rc))
>>                  return rc;
>> +
>>          rc = call_int_hook(task_alloc, 0, task, clone_flags);
>> -       if (unlikely(rc))
>> +       if (unlikely(rc)) {
>>                  security_task_free(task);
>> -       return rc;
>> +               return rc;
>> +       }
>> +
>> +       if (oilsm) {
>> +               nilsm = task->security;
>> +               if (nilsm)
>> +                       *nilsm = *oilsm;
>> +       }
>> +
>> +       return 0;
>>   }
>>
>>   void security_task_free(struct task_struct *task)
>> @@ -2175,23 +2207,110 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>>                                  char **value)
>>   {
>>          struct security_hook_list *hp;
>> +       int ilsm = lsm_task_ilsm(current);
>> +       int slot = 0;
>> +
>> +       if (!strcmp(name, "interface_lsm")) {
>> +               /*
>> +                * lsm_slot will be 0 if there are no displaying modules.
>> +                */
>> +               if (lsm_slot == 0)
>> +                       return -EINVAL;
>> +
>> +               /*
>> +                * Only allow getting the current process' interface_lsm.
>> +                * There are too few reasons to get another process'
>> +                * interface_lsm and too many LSM policy issues.
>> +                */
>> +               if (current != p)
>> +                       return -EINVAL;
>> +
>> +               ilsm = lsm_task_ilsm(p);
>> +               if (ilsm != LSMBLOB_INVALID)
>> +                       slot = ilsm;
>> +               *value = kstrdup(lsm_slotlist[slot]->lsm, GFP_KERNEL);
>> +               if (*value)
>> +                       return strlen(*value);
>> +               return -ENOMEM;
>> +       }
>>
>>          hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
>>                  if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>>                          continue;
>> +               if (lsm == NULL && ilsm != LSMBLOB_INVALID &&
>> +                   ilsm != hp->lsmid->slot)
>> +                       continue;
>>                  return hp->hook.getprocattr(p, name, value);
>>          }
>>          return LSM_RET_DEFAULT(getprocattr);
>>   }
>>
>> +/**
>> + * security_setprocattr - Set process attributes via /proc
>> + * @lsm: name of module involved, or NULL
>> + * @name: name of the attribute
>> + * @value: value to set the attribute to
>> + * @size: size of the value
>> + *
>> + * Set the process attribute for the specified security module
>> + * to the specified value. Note that this can only be used to set
>> + * the process attributes for the current, or "self" process.
>> + * The /proc code has already done this check.
>> + *
>> + * Returns 0 on success, an appropriate code otherwise.
>> + */
>>   int security_setprocattr(const char *lsm, const char *name, void *value,
>>                           size_t size)
>>   {
>>          struct security_hook_list *hp;
>> +       char *termed;
>> +       char *copy;
>> +       int *ilsm = current->security;
>> +       int rc = -EINVAL;
>> +       int slot = 0;
>> +
>> +       if (!strcmp(name, "interface_lsm")) {
>> +               /*
>> +                * Change the "interface_lsm" value only if all the security
>> +                * modules that support setting a procattr allow it.
>> +                * It is assumed that all such security modules will be
>> +                * cooperative.
>> +                */
>> +               if (size == 0)
>> +                       return -EINVAL;
>> +
>> +               hlist_for_each_entry(hp, &security_hook_heads.setprocattr,
>> +                                    list) {
>> +                       rc = hp->hook.setprocattr(name, value, size);
>> +                       if (rc < 0)
>> +                               return rc;
>> +               }
>> +
>> +               rc = -EINVAL;
>> +
>> +               copy = kmemdup_nul(value, size, GFP_KERNEL);
>> +               if (copy == NULL)
>> +                       return -ENOMEM;
>> +
>> +               termed = strsep(&copy, " \n");
>> +
>> +               for (slot = 0; slot < lsm_slot; slot++)
>> +                       if (!strcmp(termed, lsm_slotlist[slot]->lsm)) {
>> +                               *ilsm = lsm_slotlist[slot]->slot;
>> +                               rc = size;
>> +                               break;
>> +                       }
>> +
>> +               kfree(termed);
>> +               return rc;
>> +       }
>>
>>          hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
>>                  if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>>                          continue;
>> +               if (lsm == NULL && *ilsm != LSMBLOB_INVALID &&
>> +                   *ilsm != hp->lsmid->slot)
>> +                       continue;
>>                  return hp->hook.setprocattr(name, value, size);
>>          }
>>          return LSM_RET_DEFAULT(setprocattr);
>> @@ -2211,15 +2330,15 @@ EXPORT_SYMBOL(security_ismaclabel);
>>   int security_secid_to_secctx(struct lsmblob *blob, char **secdata, u32 *seclen)
>>   {
>>          struct security_hook_list *hp;
>> -       int rc;
>> +       int ilsm = lsm_task_ilsm(current);
>>
>>          hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
>>                  if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>>                          continue;
>> -               rc = hp->hook.secid_to_secctx(blob->secid[hp->lsmid->slot],
>> -                                             secdata, seclen);
>> -               if (rc != LSM_RET_DEFAULT(secid_to_secctx))
>> -                       return rc;
>> +               if (ilsm == LSMBLOB_INVALID || ilsm == hp->lsmid->slot)
>> +                       return hp->hook.secid_to_secctx(
>> +                                       blob->secid[hp->lsmid->slot],
>> +                                       secdata, seclen);
>>          }
>>
>>          return LSM_RET_DEFAULT(secid_to_secctx);
>> @@ -2230,16 +2349,15 @@ int security_secctx_to_secid(const char *secdata, u32 seclen,
>>                               struct lsmblob *blob)
>>   {
>>          struct security_hook_list *hp;
>> -       int rc;
>> +       int ilsm = lsm_task_ilsm(current);
>>
>>          lsmblob_init(blob, 0);
>>          hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid, list) {
>>                  if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
>>                          continue;
>> -               rc = hp->hook.secctx_to_secid(secdata, seclen,
>> -                                             &blob->secid[hp->lsmid->slot]);
>> -               if (rc != 0)
>> -                       return rc;
>> +               if (ilsm == LSMBLOB_INVALID || ilsm == hp->lsmid->slot)
>> +                       return hp->hook.secctx_to_secid(secdata, seclen,
>> +                                               &blob->secid[hp->lsmid->slot]);
>>          }
>>          return 0;
>>   }
>> @@ -2247,7 +2365,14 @@ EXPORT_SYMBOL(security_secctx_to_secid);
>>
>>   void security_release_secctx(char *secdata, u32 seclen)
>>   {
>> -       call_void_hook(release_secctx, secdata, seclen);
>> +       struct security_hook_list *hp;
>> +       int ilsm = lsm_task_ilsm(current);
>> +
>> +       hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list)
>> +               if (ilsm == LSMBLOB_INVALID || ilsm == hp->lsmid->slot) {
>> +                       hp->hook.release_secctx(secdata, seclen);
>> +                       return;
>> +               }
>>   }
>>   EXPORT_SYMBOL(security_release_secctx);
>>
>> @@ -2388,8 +2513,15 @@ EXPORT_SYMBOL(security_sock_rcv_skb);
>>   int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
>>                                        int __user *optlen, unsigned len)
>>   {
>> -       return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock,
>> -                               optval, optlen, len);
>> +       int ilsm = lsm_task_ilsm(current);
>> +       struct security_hook_list *hp;
>> +
>> +       hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream,
>> +                            list)
>> +               if (ilsm == LSMBLOB_INVALID || ilsm == hp->lsmid->slot)
>> +                       return hp->hook.socket_getpeersec_stream(sock, optval,
>> +                                                                optlen, len);
>> +       return -ENOPROTOOPT;
>>   }
>>
>>   int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index c295b1035bc6..824a6e4fb126 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -6441,6 +6441,17 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
>>          /*
>>           * Basic control over ability to set these attributes at all.
>>           */
>> +
>> +       /*
>> +        * For setting interface_lsm, we only perform a permission check;
>> +        * the actual update to the interface_lsm value is handled by the
>> +        * LSM framework.
>> +        */
>> +       if (!strcmp(name, "interface_lsm"))
>> +               return avc_has_perm(&selinux_state,
>> +                                   mysid, mysid, SECCLASS_PROCESS2,
>> +                                   PROCESS2__SETDISPLAY, NULL);
>> +
>>          if (!strcmp(name, "exec"))
>>                  error = avc_has_perm(&selinux_state,
>>                                       mysid, mysid, SECCLASS_PROCESS,
>> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
>> index 35aac62a662e..46a7cfaf392d 100644
>> --- a/security/selinux/include/classmap.h
>> +++ b/security/selinux/include/classmap.h
>> @@ -53,7 +53,7 @@ struct security_class_mapping secclass_map[] = {
>>              "execmem", "execstack", "execheap", "setkeycreate",
>>              "setsockcreate", "getrlimit", NULL } },
>>          { "process2",
>> -         { "nnp_transition", "nosuid_transition", NULL } },
>> +         { "nnp_transition", "nosuid_transition", "setdisplay", NULL } },
> Was the suggestion to use a more descriptive permission verb[1]
> accentdentially forgotten or dicarded for some reason?

The former. I realized I had missed it while the posting was
in progress.

>
> [1]: https://lore.kernel.org/selinux/6cfc262c-1981-edcf-39bf-197a81cdebf3@schaufler-ca.com/T/#m66d868cf250928f0e8fdae6ffd5df775225036b8
>
>>          { "system",
>>            { "ipc_info", "syslog_read", "syslog_mod",
>>              "syslog_console", "module_request", "module_load", NULL } },
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 9c44327d8ea7..1069ba7abf40 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -3517,6 +3517,13 @@ static int smack_setprocattr(const char *name, void *value, size_t size)
>>          struct smack_known_list_elem *sklep;
>>          int rc;
>>
>> +       /*
>> +        * Allow the /proc/.../attr/current and SO_PEERSEC "interface_lsm"
>> +        * to be reset at will.
>> +        */
>> +       if (strcmp(name, "interface_lsm") == 0)
>> +               return 0;
>> +
>>          if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp->smk_relabel))
>>                  return -EPERM;
>>
>> --
>> 2.31.1
>>

Powered by blists - more mailing lists