Re: [PATCH] SELinux: Always allow FIOCLEX and FIONCLEX

[Paul Moore]

On Mon, Feb 7, 2022 at 4:51 PM William Roberts <bill.c.roberts@xxxxxxxxx> wrote:
>
> On Mon, Feb 7, 2022 at 3:42 PM William Roberts <bill.c.roberts@xxxxxxxxx> wrote:
> >
> > + NNK and Dan
> - nnk and Dan.
> + Jeff
> Let me try again, looks like Nick left, not sure about Dan.
> Jeff, can you look this over?

FWIW, I'm still not convinced merging this kernel patch is something
we want to do, so please don't assume that it's a done deal on the
kernel side.

> > On Mon, Feb 7, 2022 at 3:12 PM Demi Marie Obenour <demiobenour@xxxxxxxxx> wrote:
> > >
> > > On 2/7/22 13:35, William Roberts wrote:
> > > > On Mon, Feb 7, 2022 at 11:09 AM Demi Marie Obenour
> > > > <demiobenour@xxxxxxxxx> wrote:
> > > >>
> > > >> On 2/7/22 12:00, William Roberts wrote:
> > > >>> On Mon, Feb 7, 2022 at 9:08 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > >>>>
> > > >>>> On Wed, Feb 2, 2022 at 5:13 AM Demi Marie Obenour <demiobenour@xxxxxxxxx> wrote:
> > > >>>>> On 2/1/22 12:26, Paul Moore wrote:
> > > >>>>>> On Sat, Jan 29, 2022 at 10:40 PM Demi Marie Obenour
> > > >>>>>> <demiobenour@xxxxxxxxx> wrote:
> > > >>>>>>> On 1/26/22 17:41, Paul Moore wrote:
> > > >>>>>>>> On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
> > > >>>>>>>> <demiobenour@xxxxxxxxx> wrote:
> > > >>>>>>>>> On 1/25/22 17:27, Paul Moore wrote:
> > > >>>>>>>>>> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
> > > >>>>>>>>>> <demiobenour@xxxxxxxxx> 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.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> Signed-off-by: Demi Marie Obenour <demiobenour@xxxxxxxxx>
> > > >>>>>>>>>>> ---
> > > >>>>>>>>>>>  security/selinux/hooks.c | 5 +++++
> > > >>>>>>>>>>>  1 file changed, 5 insertions(+)
> > > >>>>>>>>>>
> > > >>>>>>>>>> I'm not convinced that these two ioctls should be exempt from SELinux
> > > >>>>>>>>>> policy control, can you explain why allowing these ioctls with the
> > > >>>>>>>>>> file:ioctl permission is not sufficient for your use case?  Is it a
> > > >>>>>>>>>> matter of granularity?
> > > >>>>>>>>>
> > > >>>>>>>>> FIOCLEX and FIONCLEX are applicable to *all* file descriptors, not just
> > > >>>>>>>>> files.  If I want to allow them with SELinux policy, I have to grant
> > > >>>>>>>>> *:ioctl to all processes and use xperm rules to determine what ioctls
> > > >>>>>>>>> are actually allowed.  That is incompatible with existing policies and
> > > >>>>>>>>> needs frequent maintenance when new ioctls are added.
> > > >>>>>>>>>
> > > >>>>>>>>> Furthermore, these ioctls do not allow one to do anything that cannot
> > > >>>>>>>>> already be done by fcntl(F_SETFD), and (unless I have missed something)
> > > >>>>>>>>> SELinux unconditionally allows that.  Therefore, blocking these ioctls
> > > >>>>>>>>> does not improve security, but does risk breaking userspace programs.
> > > >>>>>>>>> The risk is especially great because in the absence of SELinux, I
> > > >>>>>>>>> believe FIOCLEX and FIONCLEX *will* always succeed, and userspace
> > > >>>>>>>>> programs may rely on this.  Worse, if a failure of FIOCLEX is ignored,
> > > >>>>>>>>> a file descriptor can be leaked to a child process that should not have
> > > >>>>>>>>> access to it, but which SELinux allows access to.  Userspace
> > > >>>>>>>>> SELinux-naive sandboxes are one way this could happen.  Therefore,
> > > >>>>>>>>> blocking FIOCLEX may *create* a security issue, and it cannot solve one.
> > > >>>>>>>>
> > > >>>>>>>> I can see you are frustrated with my initial take on this, but please
> > > >>>>>>>> understand that excluding an operation from the security policy is not
> > > >>>>>>>> something to take lightly and needs discussion.  I've added the
> > > >>>>>>>> SELinux refpolicy list to this thread as I believe their input would
> > > >>>>>>>> be helpful here.
> > > >>>>>>>
> > > >>>>>>> Absolutely it is not something that should be taken lightly, though I
> > > >>>>>>> strongly believe it is correct in this case.  Is one of my assumptions
> > > >>>>>>> mistaken?
> > > >>>>>>
> > > >>>>>> My concern is that there is a distro/admin somewhere which is relying
> > > >>>>>> on their SELinux policy enforcing access controls on these ioctls and
> > > >>>>>> removing these controls would cause them a regression.
> > > >>>>>
> > > >>>>> I obviously do not have visibility into all systems, but I suspect that
> > > >>>>> nobody is actually relying on this.  Setting and clearing CLOEXEC via
> > > >>>>> fcntl is not subject to SELinux restrictions, so blocking FIOCLEX
> > > >>>>> and FIONCLEX can be trivially bypassed unless fcntl(F_SETFD) is
> > > >>>>> blocked by seccomp or another LSM.  Clearing close-on-exec can also be
> > > >>>>> implemented with dup2(), and setting it can be implemented with dup3()
> > > >>>>> and F_DUPFD_CLOEXEC (which SELinux also allows).  In short, I believe
> > > >>>>> that unconditionally allowing FIOCLEX and FIONCLEX may fix real-world
> > > >>>>> problems, and that it is highly unlikely that anyone is relying on the
> > > >>>>> current behavior.
> > > >>>>
> > > >>>> I understand your point, but I remain concerned about making a kernel
> > > >>>> change for something that can be addressed via policy.  I'm also
> > > >>>> concerned that in the nine days this thread has been on both the mail
> > > >>>> SELinux developers and refpolicy lists no one other than you and I
> > > >>>> have commented on this patch.  In order to consider this patch
> > > >>>> further, I'm going to need to see comments from others, preferably
> > > >>>> those with a background in supporting SELinux policy.
> > > >>>>
> > > >>>
> > > >>> AFAIK/AFAICT Android makes no reference to F_SETFD, and tracing the code
> > > >>> does seem to be ignored, and the code for FIOCLEX FIONCLEX calls into
> > > >>> the same kernel routine set_close_on_exec().
> > > >>> Considering that Android's bionic contains support for "e" flag to
> > > >>> fopen, and it's
> > > >>> used in a lot of places, makes me more sure the check is skipped for F_SETFD
> > > >>>
> > > >>> However, Android does make reference to FIOCLEX FIONCLEX and every
> > > >>> domain has it enabled:
> > > >>> domain.te:allowxperm domain { file_type fs_type domain dev_type }:{
> > > >>> dir notdevfile_class_set blk_file } ioctl { FIOCLEX FIONCLEX };
> > > >>> domain.te:allowxperm domain tun_device:chr_file ioctl { FIOCLEX FIONCLEX };
> > > >>>
> > > >>> Refpolicy doesn't use xperm AFAICT.
> > > >>>
> > > >>> I stayed quiet, I wouldn't ack on this myself, but the premise seems
> > > >>> correct and we
> > > >>> can safely drop this. Note that I didn't review the code. But we need
> > > >>> to ensure we handle
> > > >>> policy correctly and not break anything. I'm not sure what the
> > > >>> compilers are doing
> > > >>> for validation of policy macro values, but we would probably want to
> > > >>> mark it deprecated,
> > > >>> but still allow loading of old compiled policies.
> > > >>
> > > >> Loading of policies is not impacted.  My patch simply skips the
> > > >> checks for FIOCLEX and FIONCLEX, instead unconditionally allowing the
> > > >> operation.  This is actually *more* selective than anything that can
> > > >> be done via policy, as my patch checks the entire ioctl number whereas
> > > >> policy can only check the low 16 bits.  As such, it is safer than using
> > > >> policy to allow FIOCLEX and FIONCLEX system-wide: if my patch causes an
> > > >> ioctl to be allowed, it is guaranteed that that ioctl will change the
> > > >> close-on-exec flag and have no other effect.
> > > >>
> > > >
> > > > What I meant by my comment is that patching the kernel is only 1/2 the
> > > > problem. We
> > > > still need to coordinate with existing policies to deprecate that out,
> > > > but since it's just
> > > > Android (AFIAK), that's pretty simple to do. I just want to make sure
> > > > we don't leave
> > > > confusing cruft floating around. I looked more at how they do xperms
> > > > in Android, and it's just
> > > > an m4 macro to a number. So we would want to coordinate a patch into the kernel
> > > > with a patch that drops that from Android policy.
> > >
> > > The kernel patch needs to come first, but there is no urgency at all
> > > for the Android policy patch.  The existing Android policy will work
> > > fine with a patched kernel.
> >
> > Yes it will work, no one said it wouldn't.
> > **If we make the change in the kernel, we should also do the cleanup
> > in policies.**
> > No cruft left behind, no dead rules.
> > We shouldn't take a patch here without ensuring that AOSP has a clean
> > path forward.
> > and putting a patch through for review gets us buy in and lets them
> > know about the
> > kernel change. This isn't about "technically it works". It's about community and
> > notice to AOSP. We give them a policy patch + link to the kernel patch, it keeps
> > everyone happy. But that's my opinion, let's just ask them (CC'd).
> >
>
> Jeff?
>
> > Dan/Nick do you guys care about these dead ioctl rules after this patch? How
> > would you like to proceed? Do you have any concerns we're not aware of?
> >
> > >  Removing the allowxperms for FIOCLEX and
> > > FIONCLEX will require ensuring that doing so does not make some domains
> > > not subject to xperm rules, and therefore allow ioctls that would
> > > previously have been forbidden.
> >
> > Yes, but this is very solvable. The set of xperms shouldn't change for
> > an affected
> > domain with the exception of FIOCLEX and FIONCLEX. sesearch will give
> > you that, I don't
> > know if sediff ever got updated for xperms.
> >
> > > --
> > > Sincerely,
> > > Demi Marie Obenour (she/her/hers)

-- 
paul-moore.com