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

[Demi Marie Obenour <demiobenour@gmail.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 5647 bytes --]

On 2/5/22 06:19, Dominick Grift wrote:
> 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.

Correct, but to emulate my patch one would need to use xperm rules
for all source and target classes.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 4963 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]