On Mon, May 17, 2021 at 3:53 PM Casey Schaufler [off-list ref] wrote:

On 5/14/2021 12:23 PM, Kees Cook wrote:

quoted

On Thu, May 13, 2021 at 01:07:56PM -0700, Casey Schaufler wrote:

quoted

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 [off-list ref]
SELinux hook provided by Stephen Smalley [off-list ref]

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Cc: Kees Cook <redacted>
Cc: Stephen Smalley <stephen.smalley.work@gmail.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: John Johansen <john.johansen@canonical.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-api@vger.kernel.org
Cc: linux-doc@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

...

quoted

quoted

@@ -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@paul-moore.com>


--
paul moore
www.paul-moore.com