From eaa680c09e5701a62e8ac819211e5969eda84f2d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 2 Oct 2018 13:34:05 +0200 Subject: [PATCH 1/3] fileio: fix error propagation in link_tmpfile() --- src/basic/fileio.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index ea607f8cfb..d2fd4c47b3 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -1492,6 +1492,7 @@ int open_serialization_fd(const char *ident) { } int link_tmpfile(int fd, const char *path, const char *target) { + int r; assert(fd >= 0); assert(target); @@ -1504,8 +1505,9 @@ int link_tmpfile(int fd, const char *path, const char *target) { * operation currently (renameat2() does), and there is no nice way to emulate this. */ if (path) { - if (rename_noreplace(AT_FDCWD, path, AT_FDCWD, target) < 0) - return -errno; + r = rename_noreplace(AT_FDCWD, path, AT_FDCWD, target); + if (r < 0) + return r; } else { char proc_fd_path[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(fd) + 1]; From 2f15b6253af8d44bc63ce232ae236fdfb3528795 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 2 Oct 2018 13:34:18 +0200 Subject: [PATCH 2/3] fs-util: add racy RENAME_NOREPLACE fallback using access() Apparently FAT on some recent kernels can't do RENAME_NOREPLACE, and of course cannot do linkat()/unlinkat() either (as the hard link concept does not exist on FAT). Add a fallback using an explicit beforehand faccessat() check. This sucks, but what we can do if the safe operations are not available? Fixes: #10063 --- src/basic/fs-util.c | 53 ++++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 1fa76bda3d..3d83fc9b10 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -89,41 +89,44 @@ int rmdir_parents(const char *path, const char *stop) { } int rename_noreplace(int olddirfd, const char *oldpath, int newdirfd, const char *newpath) { - struct stat buf; - int ret; + int r; - ret = renameat2(olddirfd, oldpath, newdirfd, newpath, RENAME_NOREPLACE); - if (ret >= 0) + /* Try the ideal approach first */ + if (renameat2(olddirfd, oldpath, newdirfd, newpath, RENAME_NOREPLACE) >= 0) return 0; - /* renameat2() exists since Linux 3.15, btrfs added support for it later. - * If it is not implemented, fallback to another method. */ - if (!IN_SET(errno, EINVAL, ENOSYS)) + /* renameat2() exists since Linux 3.15, btrfs and FAT added support for it later. If it is not implemented, + * fall back to a different method. */ + if (!IN_SET(errno, EINVAL, ENOSYS, ENOTTY)) return -errno; - /* The link()/unlink() fallback does not work on directories. But - * renameat() without RENAME_NOREPLACE gives the same semantics on - * directories, except when newpath is an *empty* directory. This is - * good enough. */ - ret = fstatat(olddirfd, oldpath, &buf, AT_SYMLINK_NOFOLLOW); - if (ret >= 0 && S_ISDIR(buf.st_mode)) { - ret = renameat(olddirfd, oldpath, newdirfd, newpath); - return ret >= 0 ? 0 : -errno; + /* Let's try to use linkat()+unlinkat() as fallback. This doesn't work on directories and on some file systems + * that do not support hard links (such as FAT, most prominently), but for files it's pretty close to what we + * want — though not atomic (i.e. for a short period both the new and the old filename will exist). */ + if (linkat(olddirfd, oldpath, newdirfd, newpath, 0) >= 0) { + + if (unlinkat(olddirfd, oldpath, 0) < 0) { + r = -errno; /* Backup errno before the following unlinkat() alters it */ + (void) unlinkat(newdirfd, newpath, 0); + return r; + } + + return 0; } - /* If it is not a directory, use the link()/unlink() fallback. */ - ret = linkat(olddirfd, oldpath, newdirfd, newpath, 0); - if (ret < 0) + if (!IN_SET(errno, EINVAL, ENOSYS, ENOTTY, EPERM)) /* FAT returns EPERM on link()… */ return -errno; - ret = unlinkat(olddirfd, oldpath, 0); - if (ret < 0) { - /* backup errno before the following unlinkat() alters it */ - ret = errno; - (void) unlinkat(newdirfd, newpath, 0); - errno = ret; + /* OK, neither RENAME_NOREPLACE nor linkat()+unlinkat() worked. Let's then fallback to the racy TOCTOU + * vulnerable accessat(F_OK) check followed by classic, replacing renameat(), we have nothing better. */ + + if (faccessat(newdirfd, newpath, F_OK, AT_SYMLINK_NOFOLLOW) >= 0) + return -EEXIST; + if (errno != ENOENT) + return -errno; + + if (renameat(olddirfd, oldpath, newdirfd, newpath) < 0) return -errno; - } return 0; } From 4a5d77615ede1253ef24ab9421eab586b15b4e5c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 2 Oct 2018 16:10:39 +0200 Subject: [PATCH 3/3] test: add test case for rename_noreplace() --- src/test/test-fs-util.c | 72 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c index 4b61a3f43f..a058ad62d0 100644 --- a/src/test/test-fs-util.c +++ b/src/test/test-fs-util.c @@ -614,6 +614,77 @@ static void test_fsync_directory_of_file(void) { assert_se(fsync_directory_of_file(fd) >= 0); } +static void test_rename_noreplace(void) { + + const char* const table[] = { + "/reg", + "/dir", + "/fifo", + "/socket", + "/symlink", + NULL + }; + + _cleanup_(rm_rf_physical_and_freep) char *z = NULL; + const char *e, *j; + char **a, **b; + + e = getenv("RENAME_NOREPLACE_DIR"); + if (e) { + j = strjoina(e, "/testXXXXXX"); + + assert_se(mkdtemp_malloc(j, &z) >= 0); + } else + assert_se(mkdtemp_malloc(NULL, &z) >= 0); + + j = strjoina(z, table[0]); + assert_se(touch(j) >= 0); + + j = strjoina(z, table[1]); + assert_se(mkdir(j, 0777) >= 0); + + j = strjoina(z, table[2]); + (void) mkfifo(j, 0777); + + j = strjoina(z, table[3]); + (void) mknod(j, S_IFSOCK | 0777, 0); + + j = strjoina(z, table[4]); + (void) symlink("foobar", j); + + STRV_FOREACH(a, (char**) table) { + _cleanup_free_ char *x = NULL, *y = NULL; + + x = strjoin(z, *a); + assert_se(x); + + if (access(x, F_OK) < 0) { + assert_se(errno == ENOENT); + continue; + } + + STRV_FOREACH(b, (char**) table) { + y = strjoin(z, *b); + assert_se(y); + + if (access(y, F_OK) < 0) { + assert_se(errno == ENOENT); + continue; + } + + assert_se(rename_noreplace(AT_FDCWD, x, AT_FDCWD, y) == -EEXIST); + + y = mfree(y); + } + + y = strjoin(z, "/somethingelse"); + assert_se(y); + + assert_se(rename_noreplace(AT_FDCWD, x, AT_FDCWD, y) >= 0); + assert_se(rename_noreplace(AT_FDCWD, y, AT_FDCWD, x) >= 0); + } +} + int main(int argc, char *argv[]) { test_unlink_noerrno(); test_get_files_in_directory(); @@ -625,6 +696,7 @@ int main(int argc, char *argv[]) { test_touch_file(); test_unlinkat_deallocate(); test_fsync_directory_of_file(); + test_rename_noreplace(); return 0; }