On Thu, Sep 16, 2021 at 4:59 AM Paul Moore [off-list ref] wrote:

On Mon, Sep 13, 2021 at 5:05 PM Paul Moore [off-list ref] wrote:

quoted

On Mon, Sep 13, 2021 at 10:02 AM Ondrej Mosnacek [off-list ref] wrote:

quoted

Commit 59438b46471a ("security,lockdown,selinux: implement SELinux
lockdown") added an implementation of the locked_down LSM hook to
SELinux, with the aim to restrict which domains are allowed to perform
operations that would breach lockdown.

However, in several places the security_locked_down() hook is called in
situations where the current task isn't doing any action that would
directly breach lockdown, leading to SELinux checks that are basically
bogus.

To fix this, add an explicit struct cred pointer argument to
security_lockdown() and define NULL as a special value to pass instead
of current_cred() in such situations. LSMs that take the subject
credentials into account can then fall back to some default or ignore
such calls altogether. In the SELinux lockdown hook implementation, use
SECINITSID_KERNEL in case the cred argument is NULL.

Most of the callers are updated to pass current_cred() as the cred
pointer, thus maintaining the same behavior. The following callers are
modified to pass NULL as the cred pointer instead:
1. arch/powerpc/xmon/xmon.c
     Seems to be some interactive debugging facility. It appears that
     the lockdown hook is called from interrupt context here, so it
     should be more appropriate to request a global lockdown decision.
2. fs/tracefs/inode.c:tracefs_create_file()
     Here the call is used to prevent creating new tracefs entries when
     the kernel is locked down. Assumes that locking down is one-way -
     i.e. if the hook returns non-zero once, it will never return zero
     again, thus no point in creating these files. Also, the hook is
     often called by a module's init function when it is loaded by
     userspace, where it doesn't make much sense to do a check against
     the current task's creds, since the task itself doesn't actually
     use the tracing functionality (i.e. doesn't breach lockdown), just
     indirectly makes some new tracepoints available to whoever is
     authorized to use them.
3. net/xfrm/xfrm_user.c:copy_to_user_*()
     Here a cryptographic secret is redacted based on the value returned
     from the hook. There are two possible actions that may lead here:
     a) A netlink message XFRM_MSG_GETSA with NLM_F_DUMP set - here the
        task context is relevant, since the dumped data is sent back to
        the current task.
     b) When adding/deleting/updating an SA via XFRM_MSG_xxxSA, the
        dumped SA is broadcasted to tasks subscribed to XFRM events -
        here the current task context is not relevant as it doesn't
        represent the tasks that could potentially see the secret.
     It doesn't seem worth it to try to keep using the current task's
     context in the a) case, since the eventual data leak can be
     circumvented anyway via b), plus there is no way for the task to
     indicate that it doesn't care about the actual key value, so the
     check could generate a lot of "false alert" denials with SELinux.
     Thus, let's pass NULL instead of current_cred() here faute de
     mieux.

Improvements-suggested-by: Casey Schaufler [off-list ref]
Improvements-suggested-by: Paul Moore [off-list ref]
Fixes: 59438b46471a ("security,lockdown,selinux: implement SELinux lockdown")
Acked-by: Dan Williams <redacted>         [cxl]
Acked-by: Steffen Klassert <steffen.klassert@secunet.com> [xfrm]
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---

v4:
- rebase on top of TODO
- fix rebase conflicts:
  * drivers/cxl/pci.c
    - trivial: the lockdown reason was corrected in mainline
  * kernel/bpf/helpers.c, kernel/trace/bpf_trace.c
    - trivial: LOCKDOWN_BPF_READ was renamed to LOCKDOWN_BPF_READ_KERNEL
      in mainline
  * kernel/power/hibernate.c
    - trivial: !secretmem_active() was added to the condition in
      hibernation_available()
- cover new security_locked_down() call in kernel/bpf/helpers.c
  (LOCKDOWN_BPF_WRITE_USER in BPF_FUNC_probe_write_user case)

v3: https://lore.kernel.org/lkml/20210616085118.1141101-1-omosnace@redhat.com/ (local)
- add the cred argument to security_locked_down() and adapt all callers
- keep using current_cred() in BPF, as the hook calls have been shifted
  to program load time (commit ff40e51043af ("bpf, lockdown, audit: Fix
  buggy SELinux lockdown permission checks"))
- in SELinux, don't ignore hook calls where cred == NULL, but use
  SECINITSID_KERNEL as the subject instead
- update explanations in the commit message

v2: https://lore.kernel.org/lkml/20210517092006.803332-1-omosnace@redhat.com/ (local)
- change to a single hook based on suggestions by Casey Schaufler

v1: https://lore.kernel.org/lkml/20210507114048.138933-1-omosnace@redhat.com/ (local)

The changes between v3 and v4 all seem sane to me, but I'm going to
let this sit for a few days in hopes that we can collect a few more
Reviewed-bys and ACKs.  If I don't see any objections I'll merge it
mid-week(ish) into selinux/stable-5.15 and plan on sending it to Linus
after it goes through a build/test cycle.

Time's up, I just merged this into selinux/stable-5.15 and I'll send
this to Linus once it passes testing.

Thanks!

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.