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

[Dominick Grift <dominick.grift@defensec.nl>]

Chris PeBenito <chpebeni@linux.microsoft.com> writes:

> On 2/3/2022 18:44, Paul Moore 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.
>> Also, while I'm sure you are already well aware of this, I think it
>> is
>> worth mentioning that SELinux does apply access controls when file
>> descriptors are inherited across an exec() boundary.
>
>
> I don't have a strong opinion either way.  If one were to allow this
> using a policy rule, it would result in a major policy breakage.  The 
> rule would turn on extended perm checks across the entire system,
> which the SELinux Reference Policy isn't written for.  I can't speak
> to the Android policy, but I would imagine it would be the similar
> problem there too.

Excuse me if I am wrong but AFAIK adding a xperm rule does not turn on
xperm checks across the entire system.

If i am not mistaken it will turn on xperm checks only for the
operations that have the same source and target/target class.

This is also why i don't (with the exception TIOSCTI for termdev
chr_file) use xperms by default.

1. it is really easy to selectively filter ioctls by adding xperm rules
for end users (and since ioctls are often device/driver specific they
know best what is needed and what not)

2. if you filter ioctls in upstream policy for example like i do with
TIOSCTI using for example (allowx foo bar (ioctl chr_file (not
(0xXXXX)))) then you cannot easily exclude additional ioctls later where source is
foo and target/tclass is bar/chr_file because there is already a rule in
place allowing the ioctl (and you cannot add rules)

>
>
> --
> Chris PeBenito
>

-- 
gpg --locate-keys dominick.grift@defensec.nl
Key fingerprint = FCD2 3660 5D6B 9D27 7FC6  E0FF DA7E 521F 10F6 4098
Dominick Grift