diff --git a/src/libstore/builtins/fetchurl.cc b/src/libstore/builtins/fetchurl.cc index 126fb922eba..16e2b2ee4df 100644 --- a/src/libstore/builtins/fetchurl.cc +++ b/src/libstore/builtins/fetchurl.cc @@ -4,6 +4,7 @@ #include "nix/store/globals.hh" #include "nix/util/archive.hh" #include "nix/util/compression.hh" +#include "nix/util/file-system.hh" namespace nix { @@ -65,8 +66,7 @@ static void builtinFetchurl(const BuiltinBuilderContext & ctx) auto executable = ctx.drv.env.find("executable"); if (executable != ctx.drv.env.end() && executable->second == "1") { - if (chmod(storePath.c_str(), 0755) == -1) - throw SysError("making '%1%' executable", storePath); + chmod(storePath, 0755); } }; diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index ede73a89c45..c18f781e924 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -174,8 +174,7 @@ LocalStore::LocalStore(ref config) if (st.st_uid != 0 || st.st_gid != gr->gr_gid || (st.st_mode & ~S_IFMT) != perm) { if (chown(config->realStoreDir.get().c_str(), 0, gr->gr_gid) == -1) throw SysError("changing ownership of path '%1%'", config->realStoreDir); - if (chmod(config->realStoreDir.get().c_str(), perm) == -1) - throw SysError("changing permissions on path '%1%'", config->realStoreDir); + chmod(config->realStoreDir.get(), perm); } } } diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index 58111b61a69..8d4813e5d6b 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -3,6 +3,7 @@ #include "nix/util/signals.hh" #include "nix/store/posix-fs-canonicalise.hh" #include "nix/util/posix-source-accessor.hh" +#include "nix/util/file-system.hh" #include #include @@ -20,8 +21,7 @@ namespace nix { static void makeWritable(const Path & path) { auto st = lstat(path); - if (chmod(path.c_str(), st.st_mode | S_IWUSR) == -1) - throw SysError("changing writability of '%1%'", path); + chmod(path, st.st_mode | S_IWUSR); } struct MakeReadOnly diff --git a/src/libstore/posix-fs-canonicalise.cc b/src/libstore/posix-fs-canonicalise.cc index a274468c329..1dcfec36e0c 100644 --- a/src/libstore/posix-fs-canonicalise.cc +++ b/src/libstore/posix-fs-canonicalise.cc @@ -24,8 +24,7 @@ static void canonicaliseTimestampAndPermissions(const Path & path, const struct bool isDir = S_ISDIR(st.st_mode); if ((mode != 0444 || isDir) && mode != 0555) { mode = (st.st_mode & S_IFMT) | 0444 | (st.st_mode & S_IXUSR || isDir ? 0111 : 0); - if (chmod(path.c_str(), mode) == -1) - throw SysError("changing mode of '%1%' to %2$o", path, mode); + chmod(path, mode); } } diff --git a/src/libstore/unix/build/chroot-derivation-builder.cc b/src/libstore/unix/build/chroot-derivation-builder.cc index 91866d1c060..7e9e13d4a73 100644 --- a/src/libstore/unix/build/chroot-derivation-builder.cc +++ b/src/libstore/unix/build/chroot-derivation-builder.cc @@ -85,7 +85,7 @@ struct ChrootDerivationBuilder : virtual DerivationBuilderImpl instead.) */ std::filesystem::path chrootTmpDir = chrootRootDir / "tmp"; createDirs(chrootTmpDir); - chmod_(chrootTmpDir, 01777); + chmod(chrootTmpDir, 01777); /* Create a /etc/passwd with entries for the build user and the nobody account. The latter is kind of a hack to support @@ -119,7 +119,7 @@ struct ChrootDerivationBuilder : virtual DerivationBuilderImpl build user. */ std::filesystem::path chrootStoreDir = chrootRootDir / std::filesystem::path(store.storeDir).relative_path(); createDirs(chrootStoreDir); - chmod_(chrootStoreDir, 01775); + chmod(chrootStoreDir, 01775); if (buildUser && chown(chrootStoreDir.c_str(), 0, buildUser->getGID()) == -1) throw SysError("cannot change ownership of %1%", PathFmt(chrootStoreDir)); diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index ce3d5b729dc..9da51643d2f 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -570,12 +570,6 @@ SingleDrvOutputs DerivationBuilderImpl::unprepareBuild() return builtOutputs; } -static void chmod_(const std::filesystem::path & path, mode_t mode) -{ - if (chmod(path.c_str(), mode) == -1) - throw SysError("setting permissions on %s", PathFmt(path)); -} - /* Move/rename path 'src' to 'dst'. Temporarily make 'src' writable if it's a directory and we're not root (to be able to update the directory's parent link ".."). */ @@ -586,12 +580,12 @@ static void movePath(const std::filesystem::path & src, const std::filesystem::p bool changePerm = (geteuid() && S_ISDIR(st.st_mode) && !(st.st_mode & S_IWUSR)); if (changePerm) - chmod_(src, st.st_mode | S_IWUSR); + chmod(src, st.st_mode | S_IWUSR); std::filesystem::rename(src, dst); if (changePerm) - chmod_(dst, st.st_mode); + chmod(dst, st.st_mode); } static void replaceValidPath(const std::filesystem::path & storePath, const std::filesystem::path & tmpPath) @@ -824,8 +818,7 @@ std::optional DerivationBuilderImpl::startBuild() std::string slaveName = getPtsName(builderOut.get()); if (buildUser) { - if (chmod(slaveName.c_str(), 0600)) - throw SysError("changing mode of pseudoterminal slave"); + chmod(slaveName, 0600); if (chown(slaveName.c_str(), buildUser->getUID(), 0)) throw SysError("changing owner of pseudoterminal slave"); @@ -1926,14 +1919,14 @@ void DerivationBuilderImpl::cleanupBuild(bool force) * This hardens against an attack which smuggles a file descriptor * to make use of the temporary directory. */ - chmod(topTmpDir.c_str(), 0000); + chmod(topTmpDir, 0000); /* Don't keep temporary directories for builtins because they might have privileged stuff (like a copy of netrc). */ if (settings.keepFailed && !force && !drv.isBuiltin()) { printError("note: keeping build directory %s", PathFmt(tmpDir)); - chmod(topTmpDir.c_str(), 0755); - chmod(tmpDir.c_str(), 0755); + chmod(topTmpDir, 0755); + chmod(tmpDir, 0755); } else deletePath(topTmpDir); topTmpDir = ""; diff --git a/src/libstore/unix/build/linux-derivation-builder.cc b/src/libstore/unix/build/linux-derivation-builder.cc index 3e33ff49800..c16c9ef8e59 100644 --- a/src/libstore/unix/build/linux-derivation-builder.cc +++ b/src/libstore/unix/build/linux-derivation-builder.cc @@ -586,7 +586,7 @@ struct ChrootLinuxDerivationBuilder : ChrootDerivationBuilder, LinuxDerivationBu auto dst = chrootRootDir / i.first.relative_path(); createDirs(dst.parent_path()); writeFile(dst, std::string_view((const char *) sh, sizeof(sh))); - chmod_(dst, 0555); + chmod(dst, 0555); } else # endif { @@ -629,7 +629,7 @@ struct ChrootLinuxDerivationBuilder : ChrootDerivationBuilder, LinuxDerivationBu /* Make sure /dev/pts/ptmx is world-writable. With some Linux versions, it is created with permissions 0. */ - chmod_(chrootRootDir / "dev" / "pts" / "ptmx", 0666); + chmod(chrootRootDir / "dev" / "pts" / "ptmx", 0666); } else { if (errno != EINVAL) throw SysError("mounting /dev/pts"); @@ -640,7 +640,7 @@ struct ChrootLinuxDerivationBuilder : ChrootDerivationBuilder, LinuxDerivationBu /* Make /etc unwritable */ if (!drvOptions.useUidRange(drv)) - chmod_(chrootRootDir / "etc", 0555); + chmod(chrootRootDir / "etc", 0555); /* Unshare this mount namespace. This is necessary because pivot_root() below changes the root of the mount diff --git a/src/libutil-tests/unix/file-descriptor.cc b/src/libutil-tests/unix/file-descriptor.cc index 185b50d9322..75a83dcf756 100644 --- a/src/libutil-tests/unix/file-descriptor.cc +++ b/src/libutil-tests/unix/file-descriptor.cc @@ -97,8 +97,8 @@ TEST(fchmodatTryNoFollow, works) sink.createSymlink(CanonPath("dirlink"), "dir"); } - ASSERT_EQ(chmod((tmpDir / "file").c_str(), 0644), 0); - ASSERT_EQ(chmod((tmpDir / "dir").c_str(), 0755), 0); + ASSERT_NO_THROW(chmod(tmpDir / "file", 0644)); + ASSERT_NO_THROW(chmod(tmpDir / "dir", 0755)); AutoCloseFD dirFd = openDirectory(tmpDir); ASSERT_TRUE(dirFd); @@ -152,7 +152,7 @@ TEST(fchmodatTryNoFollow, fallbackWithoutProc) sink.createSymlink(CanonPath("link"), "file"); } - ASSERT_EQ(chmod((tmpDir / "file").c_str(), 0644), 0); + ASSERT_NO_THROW(chmod(tmpDir / "file", 0644)); Pid pid = startProcess( [&] { diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 0049d45343d..52281ad7635 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -739,6 +739,19 @@ std::filesystem::path makeParentCanonical(const std::filesystem::path & rawPath) } } +void chmod(const std::filesystem::path & path, mode_t mode) +{ + if ( +#ifdef _WIN32 + ::_wchmod +#else + ::chmod +#endif + (path.c_str(), mode) + == -1) + throw SysError("setting permissions on %s", PathFmt(path)); +} + bool chmodIfNeeded(const std::filesystem::path & path, mode_t mode, mode_t mask) { auto pathString = path.string(); @@ -747,9 +760,7 @@ bool chmodIfNeeded(const std::filesystem::path & path, mode_t mode, mode_t mask) if (((prevMode ^ mode) & mask) == 0) return false; - if (chmod(pathString.c_str(), mode) != 0) - throw SysError("could not set permissions on '%s' to %o", pathString, mode); - + chmod(path, mode); return true; } diff --git a/src/libutil/include/nix/util/file-system.hh b/src/libutil/include/nix/util/file-system.hh index fd3b7f32a27..6f682991ece 100644 --- a/src/libutil/include/nix/util/file-system.hh +++ b/src/libutil/include/nix/util/file-system.hh @@ -418,6 +418,17 @@ extern PathFilter defaultPathFilter; */ bool chmodIfNeeded(const std::filesystem::path & path, mode_t mode, mode_t mask = S_IRWXU | S_IRWXG | S_IRWXO); +/** + * Set permissions on a path, throwing an exception on error. + * + * @param path Path to the file to change the permissions for. + * @param mode New file mode. + * + * @todo stop using this and start using `fchmodatTryNoFollow` (or a different + * wrapper) to avoid TOCTOU issues. + */ +void chmod(const std::filesystem::path & path, mode_t mode); + /** * @brief A directory iterator that can be used to iterate over the * contents of a directory. It is similar to std::filesystem::directory_iterator diff --git a/src/libutil/unix-domain-socket.cc b/src/libutil/unix-domain-socket.cc index 50df7438bd0..bc9033e3b8f 100644 --- a/src/libutil/unix-domain-socket.cc +++ b/src/libutil/unix-domain-socket.cc @@ -38,8 +38,7 @@ AutoCloseFD createUnixDomainSocket(const Path & path, mode_t mode) bind(fdSocket.get(), path); - if (chmod(path.c_str(), mode) == -1) - throw SysError("changing permissions on '%1%'", path); + chmod(path, mode); if (listen(toSocket(fdSocket.get()), 100) == -1) throw SysError("cannot listen on socket '%1%'", path);