Received-SPF: pass (google.com: domain of selinux-refpolicy-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220224102417.42268-1-richard_c_haines@btinternet.com> <CAHC9VhT4V=d8LtNT2mbiB6m1eUypi5WxPdRux_YNJ9iCV=WjWg@mail.gmail.com>
In-Reply-To: <CAHC9VhT4V=d8LtNT2mbiB6m1eUypi5WxPdRux_YNJ9iCV=WjWg@mail.gmail.com>
From:   Ondrej Mosnacek <omosnace@redhat.com>
Date:   Fri, 25 Feb 2022 11:01:52 +0100
Message-ID: <CAFqZXNtLw_c1V7MvQ=uVO9da1LH0dP64Vy3eEvL8Por_o6NQ5A@mail.gmail.com>
Subject: Re: [PATCH V3] security/selinux: Always allow FIOCLEX and FIONCLEX
To:     Paul Moore <paul@paul-moore.com>
Cc:     Richard Haines <richard_c_haines@btinternet.com>,
        SElinux list <selinux@vger.kernel.org>,
        Stephen Smalley <stephen.smalley.work@gmail.com>,
        Eric Paris <eparis@parisplace.org>, demiobenour@gmail.com,
        Linux kernel mailing list <linux-kernel@vger.kernel.org>,
        selinux-refpolicy@vger.kernel.org,
        Jeff Vander Stoep <jeffv@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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