Chris PeBenito <chpebeni@xxxxxxxxxxxxxxxxxxx> writes: > 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. 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@xxxxxxxxxxx Key fingerprint = FCD2 3660 5D6B 9D27 7FC6 E0FF DA7E 521F 10F6 4098 Dominick Grift
