On 2/5/22 06:19, Dominick Grift wrote: > 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. 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:
OpenPGP_0xB288B55FFF9C22C1.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
