diff --git a/src/services/browser_tracker_service.cc b/src/services/browser_tracker_service.cc index 50489a0..5921f3e 100644 --- a/src/services/browser_tracker_service.cc +++ b/src/services/browser_tracker_service.cc @@ -13,6 +13,7 @@ #include #include #include +#include namespace mocktail { namespace services { @@ -211,6 +212,48 @@ bool FindExistingDirectoryAncestor(const std::filesystem::path& directory, } } +// Creates every component of `directory` below `existing_ancestor` with mode +// 0700, whatever the process umask. mkdir masks its mode argument, so a umask +// of 002 would leave the directory group writable and rejected by the +// IsPrivateDirectory checks that guard this file's credentials, while a umask +// carrying owner bits would leave it unwritable by its own owner. Only +// components this call creates are tightened; one that already exists keeps +// its mode and is judged by IsPrivateDirectory like any other. +bool CreatePrivateDirectories(const std::filesystem::path& directory, + const std::filesystem::path& existing_ancestor) { + std::vector missing; + for (std::filesystem::path candidate = directory; + candidate != existing_ancestor;) { + if (candidate.empty()) return false; + missing.push_back(candidate); + // Mirror FindExistingDirectoryAncestor's walk, including the implicit "." + // parent of a bare relative name, so the two agree on which components are + // missing. + std::filesystem::path parent = candidate.parent_path(); + if (parent.empty() && candidate != ".") parent = "."; + if (parent.empty() || parent == candidate) return false; + candidate = parent; + } + for (auto component = missing.rbegin(); component != missing.rend(); + ++component) { + if (mkdir(component->c_str(), 0700) != 0) { + if (errno != EEXIST) return false; + continue; + } + const int created = open(component->c_str(), + O_RDONLY | O_DIRECTORY | O_CLOEXEC | O_NOFOLLOW); + if (created < 0) return false; + struct stat metadata = {}; + const bool tightened = fstat(created, &metadata) == 0 && + S_ISDIR(metadata.st_mode) && + metadata.st_uid == geteuid() && + fchmod(created, 0700) == 0; + close(created); + if (!tightened) return false; + } + return true; +} + bool FsyncDirectoryChain(const std::filesystem::path& first, const std::filesystem::path& last) { std::filesystem::path current = first; @@ -322,8 +365,7 @@ bool AtomicWriteContents(const std::filesystem::path& path, return false; } const bool parent_existed = existing_ancestor == parent; - std::filesystem::create_directories(parent, filesystem_error); - if (filesystem_error) { + if (!CreatePrivateDirectories(parent, existing_ancestor)) { error->assign(directory_error); return false; } @@ -449,10 +491,11 @@ BrowserTrackerResult BrowserTrackerService::EnsureInitialized( return result; } const bool lock_parent_existed = existing_lock_ancestor == lock_parent; - std::filesystem::create_directories(lock_parent, filesystem_error); + const bool lock_parent_created = + CreatePrivateDirectories(lock_parent, existing_lock_ancestor); struct stat lock_parent_status = {}; const bool safe_lock_parent = - !filesystem_error && + lock_parent_created && lstat(lock_parent.c_str(), &lock_parent_status) == 0 && IsPrivateDirectory(lock_parent_status); const int lock = safe_lock_parent diff --git a/tests/browser_tracker_service_test.cc b/tests/browser_tracker_service_test.cc index 0d892b9..b44f7f8 100644 --- a/tests/browser_tracker_service_test.cc +++ b/tests/browser_tracker_service_test.cc @@ -41,18 +41,26 @@ class FakeHttpClient final : public HttpClient { class BrowserTrackerServiceTest : public testing::Test { protected: void SetUp() override { + // The service refuses to hold credentials in a group- or world-writable + // directory, so the directories these tests create must be private + // whatever umask the developer or CI runner happens to use. + previous_umask_ = umask(077); root_ = std::filesystem::temp_directory_path() / ("mocktail-browser-tracker-" + std::to_string(getpid()) + "-" + ::testing::UnitTest::GetInstance()->current_test_info()->name()); std::filesystem::remove_all(root_); } - void TearDown() override { std::filesystem::remove_all(root_); } + void TearDown() override { + std::filesystem::remove_all(root_); + umask(previous_umask_); + } std::filesystem::path Storage() const { return root_ / "appData/LocalStorage/appStorage.json"; } std::filesystem::path Cookies() const { return root_ / "roblox.cookie"; } std::filesystem::path root_; + mode_t previous_umask_ = 0; }; TEST_F(BrowserTrackerServiceTest, ReusesValidStoredIdentityWithoutNetwork) { @@ -171,6 +179,58 @@ TEST_F(BrowserTrackerServiceTest, EXPECT_FALSE(std::filesystem::exists(Storage())); } +TEST_F(BrowserTrackerServiceTest, CreatesPrivateStorageDirectoryUnderAnyUmask) { + std::filesystem::create_directories(root_); + ASSERT_EQ(chmod(root_.c_str(), 0700), 0); + std::ofstream(Cookies()) << ".ROBLOSECURITY=secret"; + FakeHttpClient http; + http.response = {true, + 200, + R"({"browserTrackerId":987654321})", + "", + {"Set-Cookie: RBXEventTrackerV2=" + "CreateDate=1&rbxid=2&browserid=987654321; Path=/"}}; + + // A permissive umask must not widen the directories the service creates for + // its own credentials; it refuses to use any that are group or world + // writable. + const mode_t previous_umask = umask(0); + const BrowserTrackerResult result = + BrowserTrackerService(http).EnsureInitialized(Storage(), Cookies(), true); + umask(previous_umask); + + ASSERT_TRUE(result) << result.error; + struct stat storage_parent = {}; + ASSERT_EQ(stat(Storage().parent_path().c_str(), &storage_parent), 0); + EXPECT_EQ(storage_parent.st_mode & 0777, 0700U); +} + +TEST_F(BrowserTrackerServiceTest, + CreatesUsableStorageDirectoryUnderRestrictiveUmask) { + std::filesystem::create_directories(root_); + ASSERT_EQ(chmod(root_.c_str(), 0700), 0); + std::ofstream(Cookies()) << ".ROBLOSECURITY=secret"; + FakeHttpClient http; + http.response = {true, + 200, + R"({"browserTrackerId":987654321})", + "", + {"Set-Cookie: RBXEventTrackerV2=" + "CreateDate=1&rbxid=2&browserid=987654321; Path=/"}}; + + // mkdir applies the umask to its mode argument, so a umask that strips owner + // bits would otherwise leave the service's own directory unwritable. + const mode_t previous_umask = umask(0200); + const BrowserTrackerResult result = + BrowserTrackerService(http).EnsureInitialized(Storage(), Cookies(), true); + umask(previous_umask); + + ASSERT_TRUE(result) << result.error; + struct stat storage_parent = {}; + ASSERT_EQ(stat(Storage().parent_path().c_str(), &storage_parent), 0); + EXPECT_EQ(storage_parent.st_mode & 0777, 0700U); +} + TEST_F(BrowserTrackerServiceTest, RejectsCorruptStoredIdentityFailClosed) { std::filesystem::create_directories(Storage().parent_path()); std::ofstream(Storage()) << R"({"BrowserTrackerId":"0"})";