On 2/3/2022 18:44, Paul Moore 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.
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
