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

[Demi Marie Obenour]

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.  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.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

Attachment: OpenPGP_0xB288B55FFF9C22C1.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature