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

[William Roberts <bill.c.roberts@gmail.com>]

On Mon, Feb 7, 2022 at 3:42 PM William Roberts <bill.c.roberts@gmail.com> 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?

>
> On Mon, Feb 7, 2022 at 3:12 PM Demi Marie Obenour <demiobenour@gmail.com> wrote:
> >
> > On 2/7/22 13:35, William Roberts wrote:
> > > On Mon, Feb 7, 2022 at 11:09 AM Demi Marie Obenour
> > > <demiobenour@gmail.com> wrote:
> > >>
> > >> On 2/7/22 12:00, William Roberts wrote:
> > >>> On Mon, Feb 7, 2022 at 9:08 AM Paul Moore <paul@paul-moore.com> wrote:
> > >>>>
> > >>>> On Wed, Feb 2, 2022 at 5:13 AM Demi Marie Obenour <demiobenour@gmail.com> wrote:
> > >>>>> On 2/1/22 12:26, Paul Moore wrote:
> > >>>>>> On Sat, Jan 29, 2022 at 10:40 PM Demi Marie Obenour
> > >>>>>> <demiobenour@gmail.com> wrote:
> > >>>>>>> On 1/26/22 17:41, Paul Moore wrote:
> > >>>>>>>> On Tue, Jan 25, 2022 at 5:50 PM Demi Marie Obenour
> > >>>>>>>> <demiobenour@gmail.com> wrote:
> > >>>>>>>>> On 1/25/22 17:27, Paul Moore wrote:
> > >>>>>>>>>> On Tue, Jan 25, 2022 at 4:34 PM Demi Marie Obenour
> > >>>>>>>>>> <demiobenour@gmail.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.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
> > >>>>>>>>>>> ---
> > >>>>>>>>>>>  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)