+Linus, Andy, Al from the other thread On Mon, Jul 16, 2018 at 12:03 AM Ingo Molnar <mi...@kernel.org> wrote: > * Jann Horn <ja...@google.com> wrote: > > > - A malicious user can pass an arbitrary file to a setuid binary as > > stdin/stdout/stderr. When the setuid binary (expecting stdin/stdout to > > be something normal, like a proper file or a pipe) then calls read(0, > > <buf>, <len>), if the kernel disregards the length argument and writes > > beyond the end of the buffer, it can corrupt adjacent userspace data, > > potentially allowing a user to escalate their privileges; a write > > handler is somewhat less interesting because it can probably (as in > > this case) only leak out-of-bounds data from the caller, not corrupt > > it, but it's still a concern in theory. > > BTW., a naive question: would it make sense to simply disallow 'special' > fds to be passed to setuid binaries, and fix any user-space that breaks? > (i.e. only allow regular files and pipes/sockets.)
If we do that, we'd probably want to do the same for file descriptor passing through /dev/binder and SCM_RIGHTS. There are already LSM hooks for most of that because SELinux filters these. I guess the big question is, how exactly do you decide whether a file is "special"? Something like a whitelist for files that are not special based on their filesystem or their file_operations? /dev/null and /dev/full are character devices, but should probably be things you can pass to setuid binaries; /dev/fuse is a character device that is passed over a unix domain socket by the fusermount setuid helper; on the other hand, the infiniband "rdma_cm" miscdev had a buggy write handler. If the necessary whitelist is small enough, it might be a sensible hardening measure. > Also, don't allow splice() on special files either, except if the driver > explicitly opts in to it. In the thread "[PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]", Andy Lutomirski suggested something similar (https://lore.kernel.org/lkml/338bc3c4-f3e7-48f0-a82e-2c7295b66...@amacapital.net/): handler | (Al- can’t we just stop allowing splice() at all on things that don’t use iov_iter?) Linus suggested (https://lore.kernel.org/lkml/ca+55afxgqlfu1pjt5421k7kbsd94nwu4fw0h-zje-qswwbf...@mail.gmail.com/): | We could add a FMODE_SPLICE_READ/WRITE bit, and let people opt in to | splice. We probably should have. > Sounds a lot more robust in the long run than playing whack-a-mole with the > *inevitable* hole in special read() and write() handlers in our 3,000+ device > drivers...
