From: Chris PeBenito <chpebeni@linux.microsoft.com>
To: Paul Moore <paul@paul-moore.com>,
Demi Marie Obenour <demiobenour@gmail.com>
Cc: Stephen Smalley <stephen.smalley.work@gmail.com>,
Eric Paris <eparis@parisplace.org>,
selinux@vger.kernel.org, linux-kernel@vger.kernel.org,
selinux-refpolicy@vger.kernel.org
Subject: Re: [PATCH] SELinux: Always allow FIOCLEX and FIONCLEX
Date: Fri, 4 Feb 2022 08:48:27 -0500 [thread overview]
Message-ID: <478e1651-a383-05ff-d011-6dda771b8ce8@linux.microsoft.com> (raw)
In-Reply-To: <CAHC9VhSdgD4Nfaxbnnn4r-OK8koSZ7+zQoPShDbGi9PvkJFpng@mail.gmail.com>
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.
--
Chris PeBenito
next prev parent reply other threads:[~2022-02-04 13:48 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-25 21:34 [PATCH] SELinux: Always allow FIOCLEX and FIONCLEX Demi Marie Obenour
2022-01-25 22:27 ` Paul Moore
2022-01-25 22:50 ` Demi Marie Obenour
2022-01-26 22:41 ` Paul Moore
2022-01-30 3:40 ` Demi Marie Obenour
2022-02-01 17:26 ` Paul Moore
2022-02-02 10:13 ` Demi Marie Obenour
2022-02-03 23:44 ` Paul Moore
2022-02-04 13:48 ` Chris PeBenito [this message]
2022-02-05 11:19 ` Dominick Grift
2022-02-05 13:13 ` Demi Marie Obenour
2022-02-08 14:17 ` William Roberts
2022-02-08 15:47 ` Chris PeBenito
2022-02-08 16:47 ` Dominick Grift
2022-02-08 23:44 ` David Laight
2022-02-14 7:11 ` Jeffrey Vander Stoep
2022-02-15 20:34 ` Paul Moore
2022-02-17 15:04 ` Christian Göttsche
2022-02-17 22:25 ` Paul Moore
2022-02-17 23:55 ` Demi Marie Obenour
2022-02-18 15:06 ` Richard Haines
2022-02-18 15:39 ` Richard Haines
2022-02-20 1:15 ` Demi Marie Obenour
2022-02-07 17:00 ` William Roberts
2022-02-07 17:08 ` Demi Marie Obenour
2022-02-07 18:35 ` William Roberts
2022-02-07 21:12 ` Demi Marie Obenour
2022-02-07 21:42 ` William Roberts
2022-02-07 21:50 ` William Roberts
2022-02-08 0:01 ` Paul Moore
2022-02-08 14:05 ` William Roberts
2022-02-08 16:26 ` Paul Moore
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=478e1651-a383-05ff-d011-6dda771b8ce8@linux.microsoft.com \
--to=chpebeni@linux.microsoft.com \
--cc=demiobenour@gmail.com \
--cc=eparis@parisplace.org \
--cc=linux-kernel@vger.kernel.org \
--cc=paul@paul-moore.com \
--cc=selinux-refpolicy@vger.kernel.org \
--cc=selinux@vger.kernel.org \
--cc=stephen.smalley.work@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).