From 4946dd4197efee693432b3ddff56f30205d3bdfd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Oct 2024 22:58:25 +0200 Subject: [PATCH] fs-util: tweak how openat_report_new() operates when O_CREAT is used on a dangling symlink One of the big mistakes of Linux is that when you create a file with open() and O_CREAT and the file already exists as dangling symlink that the symlink will be followed and the file created that it points to. This has resulted in many vulnerabilities, and triggered the creation of the O_MOFOLLOW flag, addressing the problem. O_NOFOLLOW is less than ideal in many ways, but in particular one: when actually creating a file it makes sense to set, because it is a problem to follow final symlinks in that case. But if the file is already existing, it actually does make sense to follow the symlinks. With openat_report_new() we distinguish these two cases anyway (the whole function exists only to distinguish the create and the exists-already case after all), hence let's do something about this: let's simply never create files "through symlinks". This can be implemented very easily: just pass O_NOFOLLOW to the 2nd openat() call, where we actually create files. And then basically remove 0dd82dab91eaac5e7b17bd5e9a1e07c6d2b78dca again, because we don't need to care anymore, we already will see ELOOP when we touch a symlink. Note that this change means that openat_report_new() will thus start to deviate from plain openat() behaviour in this one small detail: when actually creating files we will *never* follow the symlink. That should be a systematic improvement of security. Fixes: #34088 --- src/basic/fs-util.c | 46 ++++++++++++++--------------------------- src/test/test-fs-util.c | 3 +++ src/test/test-id128.c | 5 ++--- 3 files changed, 21 insertions(+), 33 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 1d0533e4f0..9397c4b384 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -1082,17 +1082,20 @@ int openat_report_new(int dirfd, const char *pathname, int flags, mode_t mode, b /* Just like openat(), but adds one thing: optionally returns whether we created the file anew or if * it already existed before. This is only relevant if O_CREAT is set without O_EXCL, and thus will - * shortcut to openat() otherwise */ - - if (!ret_newly_created) - return RET_NERRNO(openat(dirfd, pathname, flags, mode)); + * shortcut to openat() otherwise. + * + * Note that this routine is a bit more strict with symlinks than regular openat() is. If O_NOFOLLOW + * is not specified, then we'll follow the symlink when opening an existing file but we will *not* + * follow it when creating a new one (because that's a terrible UNIX misfeature and generally a + * security hole). */ if (!FLAGS_SET(flags, O_CREAT) || FLAGS_SET(flags, O_EXCL)) { fd = openat(dirfd, pathname, flags, mode); if (fd < 0) return -errno; - *ret_newly_created = FLAGS_SET(flags, O_CREAT); + if (ret_newly_created) + *ret_newly_created = FLAGS_SET(flags, O_CREAT); return fd; } @@ -1100,42 +1103,25 @@ int openat_report_new(int dirfd, const char *pathname, int flags, mode_t mode, b /* First, attempt to open without O_CREAT/O_EXCL, i.e. open existing file */ fd = openat(dirfd, pathname, flags & ~(O_CREAT | O_EXCL), mode); if (fd >= 0) { - *ret_newly_created = false; + if (ret_newly_created) + *ret_newly_created = false; return fd; } if (errno != ENOENT) return -errno; - /* So the file didn't exist yet, hence create it with O_CREAT/O_EXCL. */ - fd = openat(dirfd, pathname, flags | O_CREAT | O_EXCL, mode); + /* So the file didn't exist yet, hence create it with O_CREAT/O_EXCL/O_NOFOLLOW. */ + fd = openat(dirfd, pathname, flags | O_CREAT | O_EXCL | O_NOFOLLOW, mode); if (fd >= 0) { - *ret_newly_created = true; + if (ret_newly_created) + *ret_newly_created = true; return fd; } if (errno != EEXIST) return -errno; - /* Hmm, so now we got EEXIST? This can indicate two things. First, if the path points to a - * dangling symlink, the first openat() will fail with ENOENT because the symlink is resolved - * and the second openat() will fail with EEXIST because symlinks are not followed when - * O_CREAT|O_EXCL is specified. Let's check for this explicitly and fall back to opening with - * just O_CREAT and assume we're the ones that created the file. */ - - struct stat st; - if (fstatat(dirfd, pathname, &st, AT_SYMLINK_NOFOLLOW) < 0) - return -errno; - - if (S_ISLNK(st.st_mode)) { - fd = openat(dirfd, pathname, flags | O_CREAT, mode); - if (fd < 0) - return -errno; - - *ret_newly_created = true; - return fd; - } - - /* If we're not operating on a symlink, someone might have created the file between the first - * and second call to openat(). Let's try again but with a limit so we don't spin forever. */ + /* Hmm, so now we got EEXIST? Then someone might have created the file between the first and + * second call to openat(). Let's try again but with a limit so we don't spin forever. */ if (--attempts == 0) /* Give up eventually, somebody is playing with us */ return -EEXIST; diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c index 3da3caf4ab..55202240d3 100644 --- a/src/test/test-fs-util.c +++ b/src/test/test-fs-util.c @@ -670,6 +670,9 @@ TEST(openat_report_new) { ASSERT_OK_ERRNO(symlinkat("target", tfd, "link")); fd = openat_report_new(tfd, "link", O_RDWR|O_CREAT, 0666, &b); + ASSERT_ERROR(fd, EEXIST); + + fd = openat_report_new(tfd, "target", O_RDWR|O_CREAT, 0666, &b); ASSERT_OK(fd); fd = safe_close(fd); ASSERT_TRUE(b); diff --git a/src/test/test-id128.c b/src/test/test-id128.c index a6ed640bd6..eb3bfec2bb 100644 --- a/src/test/test-id128.c +++ b/src/test/test-id128.c @@ -286,9 +286,8 @@ TEST(id128_at) { ASSERT_OK_ERRNO(unlinkat(tfd, "etc/machine-id", 0)); ASSERT_OK(id128_write_at(tfd, "etc2/machine-id", ID128_FORMAT_PLAIN, id)); ASSERT_OK_ERRNO(unlinkat(tfd, "etc/machine-id", 0)); - ASSERT_OK(id128_write_at(tfd, "etc/hoge-id", ID128_FORMAT_PLAIN, id)); - ASSERT_OK_ERRNO(unlinkat(tfd, "etc/machine-id", 0)); - ASSERT_OK(id128_write_at(tfd, "etc2/hoge-id", ID128_FORMAT_PLAIN, id)); + ASSERT_ERROR(id128_write_at(tfd, "etc/hoge-id", ID128_FORMAT_PLAIN, id), EEXIST); + ASSERT_OK(id128_write_at(tfd, "etc2/machine-id", ID128_FORMAT_PLAIN, id)); /* id128_read_at() */ i = SD_ID128_NULL; /* Not necessary in real code, but for testing that the id is really assigned. */ -- 2.25.1