On Wed, 9 Aug 2017 10:01:14 -0500 Eric Blake <ebl...@redhat.com> wrote:
> On 08/09/2017 09:55 AM, Eric Blake wrote: > > On 08/09/2017 09:23 AM, Greg Kurz wrote: > >> This function has to ensure it doesn't follow a symlink that could be used > >> to escape the virtfs directory. This could be easily achieved if fchmodat() > >> on linux honored the AT_SYMLINK_NOFOLLOW flag as described in POSIX, but > >> it doesn't. > > > > Might be worth including a URL of the LKML discussion on the last > > version of that patch attempt. > > > >> > >> The current implementation covers most use-cases, but it notably fails if: > >> - the target path has access rights equal to 0000 (openat() returns > >> EPERM), > >> => once you've done chmod(0000) on a file, you can never chmod() again > >> - the target path is UNIX domain socket (openat() returns ENXIO) > >> => bind() of UNIX domain sockets fails if the file is on 9pfs > >> > >> The solution is to use O_PATH: openat() now succeeds in both cases, and we > >> can ensure the path isn't a symlink with fstat(). The associated entry in > >> "/proc/self/fd" can hence be safely passed to the regular chmod() syscall. > >> > > > > Hey - should we point this out as a viable solution to the glibc folks, > > since their current user-space emulation of AT_SYMLINK_NOFOLLOW is broken? > > > > > > Nope, unsafe when O_PATH_9P_UTIL is 0. This needs to be more like: > > > > /* Now we handle racing symlinks. On kernels without O_PATH, we will > > * fail on some corner cases, but that's better than dereferencing a > > * symlink that was injected during the TOCTTOU between our initial > > * fstatat() and openat_file(). > > */ > > if (O_PATH_9P_UTIL) { > > fstat, S_ISLINK, proc_path, chmod() > > } else { > > fchmod() > > } > > For that matter, I think you also want to avoid the O_WRONLY fallback > when O_PATH works, as in: > > >> - fd = openat_file(dirfd, name, O_RDONLY, 0); > >> + fd = openat_file(dirfd, name, O_RDONLY | O_PATH_9P_UTIL, 0); > >> if (fd == -1) { > > changing this to 'if (fd == -1 && !O_PATH_9P_UTIL)' > Yes, will do.
pgpc4KTXgkiRN.pgp
Description: OpenPGP digital signature