Re: [PATCH V3] security/selinux: Always allow FIOCLEX and FIONCLEX

On Fri, Feb 25, 2022 at 1:34 AM Paul Moore <[email protected]> wrote:
> On Thu, Feb 24, 2022 at 5:24 AM Richard Haines
> <[email protected]> wrote:
> >
> > These ioctls are equivalent to fcntl(fd, F_SETFD, flags), which SELinux
> > always allows too.  Furthermore, a failed FIOCLEX could result in a file
> > descriptor being leaked to a process that should not have access to it.
> >
> > As this patch removes access controls, a policy capability needs to be
> > enabled in policy to always allow these ioctls.
> >
> > Based-on-patch-by: Demi Marie Obenour <[email protected]>
> > Signed-off-by: Richard Haines <[email protected]>
> > ---
> > V2 Change: Control via a policy capability.
> > V3 Change: Update switch check.
> >
> >  security/selinux/hooks.c                   | 6 ++++++
> >  security/selinux/include/policycap.h       | 1 +
> >  security/selinux/include/policycap_names.h | 3 ++-
> >  security/selinux/include/security.h        | 7 +++++++
> >  4 files changed, 16 insertions(+), 1 deletion(-)
>
> This looks good to me, but before I merge this are the SELinux
> userspace folks okay with the policy capability's name and enum value?

Since you mention it... I would suggest naming the enum
POLICYDB_CAPABILITY_IOCTL_SKIP_CLOEXEC to match the display name. Yes,
it becomes awkwardly long, but e.g.
POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS is already longer than
that, so I'd prefer more descriptiveness over brevity.

(IMHO the POLICYDB_CAPABILITY_ prefix is ridiculously long for no
reason and we should simply shorten it (just POLCAP_ would be
perfectly fine, IMHO) instead of trying to abbreviate the rest. Of
course, this doesn't have to be done now - I'm taking a note to myself
to splice in such rename next time I add a new capability, if not
earlier.)

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