On Mon, May 17, 2021 at 3:53 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
> 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@xxxxxxxxxxxxx>
> >> SELinux hook provided by Stephen Smalley <stephen.smalley.work@xxxxxxxxx>
> >>
> >> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> >> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> >> Cc: Stephen Smalley <stephen.smalley.work@xxxxxxxxx>
> >> Cc: Paul Moore <paul@xxxxxxxxxxxxxx>
> >> Cc: John Johansen <john.johansen@xxxxxxxxxxxxx>
> >> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >> Cc: linux-api@xxxxxxxxxxxxxxx
> >> Cc: linux-doc@xxxxxxxxxxxxxxx
> >> ---
> >> .../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-file-credentials
>
> 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).
Agreed.
Acked-by: Paul Moore <paul@xxxxxxxxxxxxxx>
--
paul moore
www.paul-moore.com
