Re: [PATCH v26 14/25] LSM: Specify which LSM to display

On 5/14/2021 12:23 PM, Kees Cook wrote: > On Thu, May 13, 2021 at 01:07:56PM -0700, Casey Schaufler 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(a)canonical.com&gt; >> SELinux hook provided by Stephen Smalley <stephen.smalley.work(a)gmail.com&gt; >> >> Signed-off-by: Casey Schaufler <casey(a)schaufler-ca.com&gt; >> Cc: Kees Cook <keescook(a)chromium.org&gt; >> Cc: Stephen Smalley <stephen.smalley.work(a)gmail.com&gt; >> Cc: Paul Moore <paul(a)paul-moore.com&gt; >> Cc: John Johansen <john.johansen(a)canonical.com&gt; >> Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org&gt; >> Cc: linux-api(a)vger.kernel.org >> Cc: linux-doc(a)vger.kernel.org >> --- >> .../ABI/testing/procfs-attr-lsm_display | 22 +++ >> Documentation/security/lsm.rst | 14 ++ >> fs/proc/base.c | 1 + >> include/linux/lsm_hooks.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

>> @@ -2171,23 +2203,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; > ... but context isn't established by just checking "current", as this > file handle may have been given to another process. > > I suspect the security_get/setprocattr needs to gain a pointer to "file" > so that the f_cred struct can be examined[1] (i.e. compare opener > against reader/writer). > > [1] https://www.kernel.org/doc/html/latest/security/credentials.html#open-fil... It's not credentials being checked here. The check is whether the task that would be affected is "current". Process A can't open /proc/B/attr/interface_lsm with write access. The only process that can open it for write access is B. If process B opens /proc/B/attr/interface_lsm for write access it could send the file handle to process A, but process A can't write to the file because (current != p) that is, (A != B).