On Wed, Feb 15, 2023 at 9:17 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>
> There is currently a very suspicious condition in sock_has_perm() that
> basically skips any permission checking in sock_has_perm() if the target
> socket is labeled with SECINITSID_KERNEL. This seems bad for several
> reasons:
> 1. If a user-space process somehow gets its hands on such a socket, it
> will be able to don any socket operation on it without any SELinux
> restriction, even if it is not allowed to do such operations by the
> policy.
> 2. The exception is inconsistent, since one can e.g. write to a stream
> socket not only via send(2)/sendto(2)/..., but also via write(2),
> which doesn't go through sock_has_perm() and isn't subject to the
> SECINITSID_KERNEL exception.
> 3. The exception also allows operations on sockets that were created
> before the SELinux policy was loaded (even by user space), since
> these will always inherit the SECINITSID_KERNEL label.
>
> Additionally, it's unclear what is the rationale behind this exception.
> For sockets created by the kernel that are expected to be passed to
> user space, it seems better to let them undergo normal access checks to
> avoid misuse. A possible rationale is to skip checking on operations on
> sockets created with kern=1 passed to __sock_create(), which can happen
> under user-space credentials even thogh executed internally by the
> kernel - notice that such sockets are always labeled with
> SECINITSID_KERNEL. However, the operations on these sockets already
> normally bypass LSM checks entirely, so arguably this not necessary. On
> the contrary, it's better if actual user-space operations on these
> sockets go through SELinux checks, since there may be a possibility that
> such a socket accidentally leaks into user space and we definitely want
> SELinux to detect that and prevent privilege escalation.
>
> Since removing this condition could lead to regressions (notably due to
> bullet point (3.) above), add a new policy capability that allows the
> policy to opt-out from the condition. This allows policy writers or
> distributors to test for impact, add any missing rules, and then enable
> the capability.
>
> I tested a kernel with the condition removed on my Fedora workstation
> and noted only one new denial, related to a user-space socket created by
> systemd-journald before the policy is loaded, which is then continued to
> be used by systemd-journald while the system is running.
>
> Also selinux-testsuite is passing without new denials when the check is
> removed.
I'll have to dig a bit in history to fully recover the original
motivation/background but IIRC, this had to do with kernel-internal
sockets created and used by the kernel on behalf of userspace. For
example, sockets associated with NFS mounting and subsequent RPC. In
these cases, we don't necessarily want to allow the userspace process
to directly create/use such sockets and from the userspace
perspective, it is just acting on NFS files and not performing any
socket I/O. Open to doing it in a better way, or finding out that it
is no longer necessary.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> ---
> security/selinux/hooks.c | 4 +++-
> security/selinux/include/policycap.h | 1 +
> security/selinux/include/policycap_names.h | 3 ++-
> security/selinux/include/security.h | 7 +++++++
> 4 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3c5be76a91991..cf7418df3d4c0 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4561,7 +4561,9 @@ static int sock_has_perm(struct sock *sk, u32 perms)
> struct common_audit_data ad;
> struct lsm_network_audit net = {0,};
>
> - if (sksec->sid == SECINITSID_KERNEL)
> + /* legacy behavior was to not perform checks on kernel sockets */
> + if (!selinux_policycap_check_kernel_sockets() &&
> + sksec->sid == SECINITSID_KERNEL)
> return 0;
>
> ad.type = LSM_AUDIT_DATA_NET;
> diff --git a/security/selinux/include/policycap.h b/security/selinux/include/policycap.h
> index f35d3458e71de..814db520b9d8b 100644
> --- a/security/selinux/include/policycap.h
> +++ b/security/selinux/include/policycap.h
> @@ -12,6 +12,7 @@ enum {
> POLICYDB_CAP_NNP_NOSUID_TRANSITION,
> POLICYDB_CAP_GENFS_SECLABEL_SYMLINKS,
> POLICYDB_CAP_IOCTL_SKIP_CLOEXEC,
> + POLICYDB_CAP_CHECK_KERNEL_SOCKETS,
> __POLICYDB_CAP_MAX
> };
> #define POLICYDB_CAP_MAX (__POLICYDB_CAP_MAX - 1)
> diff --git a/security/selinux/include/policycap_names.h b/security/selinux/include/policycap_names.h
> index 2a87fc3702b81..62de8262f90fe 100644
> --- a/security/selinux/include/policycap_names.h
> +++ b/security/selinux/include/policycap_names.h
> @@ -13,7 +13,8 @@ const char *const selinux_policycap_names[__POLICYDB_CAP_MAX] = {
> "cgroup_seclabel",
> "nnp_nosuid_transition",
> "genfs_seclabel_symlinks",
> - "ioctl_skip_cloexec"
> + "ioctl_skip_cloexec",
> + "check_kernel_sockets",
> };
>
> #endif /* _SELINUX_POLICYCAP_NAMES_H_ */
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index 393aff41d3ef8..1e57c71d067fb 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -230,6 +230,13 @@ static inline bool selinux_policycap_ioctl_skip_cloexec(void)
> return READ_ONCE(state->policycap[POLICYDB_CAP_IOCTL_SKIP_CLOEXEC]);
> }
>
> +static inline bool selinux_policycap_check_kernel_sockets(void)
> +{
> + struct selinux_state *state = &selinux_state;
> +
> + return READ_ONCE(state->policycap[POLICYDB_CAP_CHECK_KERNEL_SOCKETS]);
> +}
> +
> struct selinux_policy_convert_data;
>
> struct selinux_load_state {
> --
> 2.39.1
>
