From 0a91bca5db9e31d8415f013e2cd6cbadc19dca44 Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Thu, 23 Apr 2015 08:00:56 -0700 Subject: [PATCH] test: avoid vuln-inducing use of temporary directory Summary: Without this change, someone on the machine on which I run "make check" could cause me to overwrite arbitrary files owned by me, via a symlink attack. Instead of using a predictable temporary directory and accepting to use a preexisting one, always create a new one using mkdtemp. If $TEST_IOCTL_FRIENDLY_TMPDIR is set and usable, attempt first to find a usable temporary directory therein. If not, or if unusable, then try /var/tmp and /tmp. If none of those is usable abort with a diagnostic. To do that, I added a new class. Its constructor finds a suitable directory or aborts, the sole member prints that directory's name, and the destructor unlinks what should be an empty directory. Note that while the code before this did not remove its temporary directory, there was only one per $UID. Now, there would be at least one per run or one per test, depending on implementation, so it is important to remove them. Test Plan: Run this on a fedora rawhide system, where /tmp is a tmpfs file system, and /var/tmp is ext4. # This gives a diagnostic that /dev/shm is not suitable # and ends up using /var/tmp. TEST_IOCTL_FRIENDLY_TMPDIR=/dev/shm ./env_test # Uses /var/tmp; same as when envvar not set. TEST_IOCTL_FRIENDLY_TMPDIR=/var/tmp ./env_test # Uses /tmp unless it's tmpfs, in which case it gives # a diagnostic and uses /var/tmp. TEST_IOCTL_FRIENDLY_TMPDIR=/tmp ./env_test Reviewers: ljin, rven, igor.sugak, yhchiang, sdong, igor Reviewed By: igor Subscribers: dhruba Differential Revision: https://reviews.facebook.net/D37287 --- util/env_test.cc | 125 +++++++++++++++++++++++++++++++---------------- 1 file changed, 83 insertions(+), 42 deletions(-) diff --git a/util/env_test.cc b/util/env_test.cc index d75134f0c..081a10f59 100644 --- a/util/env_test.cc +++ b/util/env_test.cc @@ -13,6 +13,7 @@ #include #include #include +#include #ifdef OS_LINUX #include @@ -472,15 +473,7 @@ TEST_F(EnvPosixTest, DecreaseNumBgThreads) { // Travis doesn't support fallocate or getting unique ID from files for whatever // reason. #ifndef TRAVIS -// To make sure the Env::GetUniqueId() related tests work correctly, The files -// should be stored in regular storage like "hard disk" or "flash device". -// Otherwise we cannot get the correct id. -// -// The following function act as the replacement of test::TmpDir() that may be -// customized by user to be on a storage that doesn't work with GetUniqueId(). -// -// TODO(kailiu) This function still assumes /tmp/ reside in regular -// storage system. + namespace { bool IsSingleVarint(const std::string& s) { Slice slice(s); @@ -500,47 +493,100 @@ bool IsUniqueIDValid(const std::string& s) { const size_t MAX_ID_SIZE = 100; char temp_id[MAX_ID_SIZE]; -std::string GetOnDiskTestDir() { - char base[100]; - snprintf(base, sizeof(base), "/tmp/rocksdbtest-%d", - static_cast(geteuid())); - // Directory may already exist - Env::Default()->CreateDirIfMissing(base); - return base; -} +} // namespace // Determine whether we can use the FS_IOC_GETVERSION ioctl -// on a file in GetOnDiskTestDir(). Create a temporary file, +// on a file in directory DIR. Create a temporary file therein, // try to apply the ioctl (save that result), cleanup and // return the result. Return true if it is supported, and // false if anything fails. -bool ioctl_support__FS_IOC_GETVERSION(void) { - std::string file_ro = GetOnDiskTestDir() + "/XXXXXX"; - auto *file = new char[file_ro.size() + 1]; - memcpy(file, file_ro.data(), file_ro.size() + 1); - int fd = mkstemp(file); +// Note that this function "knows" that dir has just been created +// and is empty, so we create a simply-named test file: "f". +bool ioctl_support__FS_IOC_GETVERSION(const std::string& dir) { + const std::string file = dir + "/f"; + int fd; + do { + fd = open(file.c_str(), O_CREAT | O_RDWR | O_TRUNC, 0644); + } while (fd < 0 && errno == EINTR); long int version; bool ok = (fd >= 0 && ioctl(fd, FS_IOC_GETVERSION, &version) >= 0); close(fd); - unlink(file); - delete[] file; + unlink(file.c_str()); return ok; } -} // namespace +// To ensure that Env::GetUniqueId-related tests work correctly, the files +// should be stored in regular storage like "hard disk" or "flash device", +// and not on a tmpfs file system (like /dev/shm and /tmp on some systems). +// Otherwise we cannot get the correct id. +// +// This function serves as the replacement for test::TmpDir(), which may be +// customized to be on a file system that doesn't work with GetUniqueId(). + +class IoctlFriendlyTmpdir { + public: + explicit IoctlFriendlyTmpdir() { + char dir_buf[100]; + std::list candidate_dir_list = {"/var/tmp", "/tmp"}; + + const char *fmt = "%s/rocksdb.XXXXXX"; + const char *tmp = getenv("TEST_IOCTL_FRIENDLY_TMPDIR"); + // If $TEST_IOCTL_FRIENDLY_TMPDIR/rocksdb.XXXXXX fits, use + // $TEST_IOCTL_FRIENDLY_TMPDIR; subtract 2 for the "%s", and + // add 1 for the trailing NUL byte. + if (tmp && strlen(tmp) + strlen(fmt) - 2 + 1 <= sizeof dir_buf) { + // use $TEST_IOCTL_FRIENDLY_TMPDIR value + candidate_dir_list.push_front(tmp); + } + + for (const std::string& d : candidate_dir_list) { + snprintf(dir_buf, sizeof dir_buf, fmt, d.c_str()); + if (mkdtemp(dir_buf)) { + if (ioctl_support__FS_IOC_GETVERSION(dir_buf)) { + dir_ = dir_buf; + return; + } else { + // Diagnose ioctl-related failure only if this is the + // directory specified via that envvar. + if (tmp == d) { + fprintf(stderr, "TEST_IOCTL_FRIENDLY_TMPDIR-specified directory is " + "not suitable: %s\n", d.c_str()); + } + rmdir(dir_buf); // ignore failure + } + } else { + // mkdtemp failed: diagnose it, but don't give up. + fprintf(stderr, "mkdtemp(%s/...) failed: %s\n", d.c_str(), + strerror(errno)); + } + } + + fprintf(stderr, "failed to find an ioctl-friendly temporary directory;" + " specify one via the TEST_IOCTL_FRIENDLY_TMPDIR envvar\n"); + std::abort(); + } + + ~IoctlFriendlyTmpdir() { + rmdir(dir_.c_str()); + } + const std::string& name() { + return dir_; + } + + private: + std::string dir_; +}; // Only works in linux platforms TEST_F(EnvPosixTest, RandomAccessUniqueID) { - if (!ioctl_support__FS_IOC_GETVERSION()) { - return; - } // Create file. const EnvOptions soptions; - std::string fname = GetOnDiskTestDir() + "/" + "testfile"; + IoctlFriendlyTmpdir ift; + std::string fname = ift.name() + "/testfile"; unique_ptr wfile; ASSERT_OK(env_->NewWritableFile(fname, &wfile, soptions)); @@ -579,12 +625,12 @@ TEST_F(EnvPosixTest, RandomAccessUniqueID) { // only works in linux platforms #ifdef ROCKSDB_FALLOCATE_PRESENT TEST_F(EnvPosixTest, AllocateTest) { - std::string fname = GetOnDiskTestDir() + "/preallocate_testfile"; + IoctlFriendlyTmpdir ift; + std::string fname = ift.name() + "/preallocate_testfile"; // Try fallocate in a file to see whether the target file system supports it. // Skip the test if fallocate is not supported. - std::string fname_test_fallocate = - GetOnDiskTestDir() + "/preallocate_testfile_2"; + std::string fname_test_fallocate = ift.name() + "/preallocate_testfile_2"; int fd = -1; do { fd = open(fname_test_fallocate.c_str(), O_CREAT | O_RDWR | O_TRUNC, 0644); @@ -658,17 +704,14 @@ bool HasPrefix(const std::unordered_set& ss) { // Only works in linux platforms TEST_F(EnvPosixTest, RandomAccessUniqueIDConcurrent) { - if (!ioctl_support__FS_IOC_GETVERSION()) { - return; - } // Check whether a bunch of concurrently existing files have unique IDs. const EnvOptions soptions; // Create the files + IoctlFriendlyTmpdir ift; std::vector fnames; for (int i = 0; i < 1000; ++i) { - fnames.push_back( - GetOnDiskTestDir() + "/" + "testfile" + ToString(i)); + fnames.push_back(ift.name() + "/" + "testfile" + ToString(i)); // Create file. unique_ptr wfile; @@ -700,12 +743,10 @@ TEST_F(EnvPosixTest, RandomAccessUniqueIDConcurrent) { // Only works in linux platforms TEST_F(EnvPosixTest, RandomAccessUniqueIDDeletes) { - if (!ioctl_support__FS_IOC_GETVERSION()) { - return; - } const EnvOptions soptions; - std::string fname = GetOnDiskTestDir() + "/" + "testfile"; + IoctlFriendlyTmpdir ift; + std::string fname = ift.name() + "/" + "testfile"; // Check that after file is deleted we don't get same ID again in a new file. std::unordered_set ids;