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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 25 Feb 2022 11:01:52 +0100
From:   Ondrej Mosnacek <omosnace@...hat.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     Richard Haines <richard_c_haines@...nternet.com>,
        SElinux list <selinux@...r.kernel.org>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Eric Paris <eparis@...isplace.org>, demiobenour@...il.com,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>,
        selinux-refpolicy@...r.kernel.org,
        Jeff Vander Stoep <jeffv@...gle.com>
Subject: Re: [PATCH V3] security/selinux: Always allow FIOCLEX and FIONCLEX

On Fri, Feb 25, 2022 at 1:34 AM Paul Moore <paul@...l-moore.com> wrote:
> On Thu, Feb 24, 2022 at 5:24 AM Richard Haines
> <richard_c_haines@...nternet.com> 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 <demiobenour@...il.com>
> > Signed-off-by: Richard Haines <richard_c_haines@...nternet.com>
> > ---
> > 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.