On 10/31/19 5:59 AM, Paul Moore wrote:
On Wed, Oct 30, 2019 at 9:16 AM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:Add SELinux access control hooks for lockdown integrity and confidentiality. This effectively mimics the current implementation of lockdown (caveat noted below). If lockdown is enabled alongside SELinux, then the lockdown access control will take precedence over the SELinux lockdown implementation. Note that this SELinux implementation allows the integrity and confidentiality reasons to be controlled independently from one another. Thus, in an SELinux policy, one could allow integrity operations while blocking confidentiality operations. (original patch authored by an intern who wishes to remain anonymous; I am signing off on his behalf) Signed-off-by: Stephen Smalley <sds@xxxxxxxxxxxxx> --- security/selinux/hooks.c | 22 ++++++++++++++++++++++ security/selinux/include/classmap.h | 2 ++ 2 files changed, 24 insertions(+) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 36e531b91df2..6722c6b4ae74 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -91,6 +91,7 @@ #include <uapi/linux/mount.h> #include <linux/fsnotify.h> #include <linux/fanotify.h> +#include <linux/security.h> #include "avc.h" #include "objsec.h" @@ -6799,6 +6800,25 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux) } #endif +static int selinux_lockdown(enum lockdown_reason what) +{ + u32 sid = current_sid(); + + if (what <= LOCKDOWN_INTEGRITY_MAX) + return avc_has_perm(&selinux_state, + sid, sid, + SECCLASS_LOCKDOWN, LOCKDOWN__INTEGRITY, NULL); + else if (what <= LOCKDOWN_CONFIDENTIALITY_MAX) + return avc_has_perm(&selinux_state, + sid, sid, + SECCLASS_LOCKDOWN, LOCKDOWN__CONFIDENTIALITY, + NULL); + + /* invalid reason */ + pr_warn("SELinux: invalid lockdown reason\n"); + return -EPERM; +}I don't have any objections to adding a hook to control access to the lockdown functionality (I think it's a good idea), but I am a little nervous about the granularity of the control. Sticking with just an integrity and a confidentiality permission seems okay, but I worry about adding additional permissions until we have a better idea of how the lockdown functionality is adopted by developers and we see how the lockdown_reason evolves.
Ok, so let's discuss what if anything else is needed for a final non-RFC version of this patch.
One thing that I wondered about was whether we ought to include the reason information in the audit record as supplemental data via a new LSM_AUDIT_DATA_* type for help in policy debugging / development. However, the lockdown_reasons[] string array is presently private to the lockdown module so we would have to export that or replicate it for creating a string representation of the reason if we were to do so. That would expose the reasons in terms of audit data but not as a basis for the permission check. Note that the lockdown module logs these reason strings via pr_notice() when it denies access, so it appears that exposing the strings as part of audit data would not introduce any extra kernel stable ABI guarantees?
I also wasn't sure about the pr_warn() above. If we reach it, it is effectively a kernel bug. We could mirror what the lockdown module does in lockdown_is_locked_down(), i.e. use WARN() instead. Of course, the SELinux hook won't even be reached in this case if the lockdown module is enabled, but the lockdown module could be disabled so I guess we need to check it too.
If we take the lockdown class with just integrity and confidentiality permissions now and later introduce finer granularity, we'll presumably need a policy capability to select whether the coarse-grained or fine-grained permissions are used.
