From 1a02ba3e6e1ee01e878efcff6a20ab7ff3083303 Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Mon, 1 Apr 2019 15:30:21 -0700 Subject: [PATCH] Trim trailing newline when reading /proc/[pid]/{uid,gid}_map in test. This reveals a bug in the tests that require CAP_SET{UID,GID}: After the child process enters the new user namespace, it ceases to have the relevant capability in the parent user namespace, so the privileged write must be done by the parent process. Change tests accordingly. PiperOrigin-RevId: 241412765 Change-Id: I587c1f24aa6f2180fb2e5e5c0162691ba5bac1bc --- test/syscalls/linux/BUILD | 2 + test/syscalls/linux/proc_pid_uid_gid_map.cc | 155 +++++++++++++++----- 2 files changed, 124 insertions(+), 33 deletions(-) diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD index 7dd63dd0a..1e386193b 100644 --- a/test/syscalls/linux/BUILD +++ b/test/syscalls/linux/BUILD @@ -1455,6 +1455,8 @@ cc_binary( linkstatic = 1, deps = [ "//test/util:capability_util", + "//test/util:cleanup", + "//test/util:file_descriptor", "//test/util:fs_util", "//test/util:logging", "//test/util:multiprocess_util", diff --git a/test/syscalls/linux/proc_pid_uid_gid_map.cc b/test/syscalls/linux/proc_pid_uid_gid_map.cc index e6a5265fa..e782f2318 100644 --- a/test/syscalls/linux/proc_pid_uid_gid_map.cc +++ b/test/syscalls/linux/proc_pid_uid_gid_map.cc @@ -20,12 +20,17 @@ #include #include +#include +#include #include #include "gtest/gtest.h" +#include "absl/strings/ascii.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_split.h" #include "test/util/capability_util.h" +#include "test/util/cleanup.h" +#include "test/util/file_descriptor.h" #include "test/util/fs_util.h" #include "test/util/logging.h" #include "test/util/multiprocess_util.h" @@ -44,10 +49,53 @@ PosixErrorOr InNewUserNamespace(const std::function& fn) { }); } +PosixErrorOr> CreateProcessInNewUserNamespace() { + int pipefd[2]; + if (pipe(pipefd) < 0) { + return PosixError(errno, "pipe failed"); + } + const auto cleanup_pipe_read = + Cleanup([&] { EXPECT_THAT(close(pipefd[0]), SyscallSucceeds()); }); + auto cleanup_pipe_write = + Cleanup([&] { EXPECT_THAT(close(pipefd[1]), SyscallSucceeds()); }); + pid_t child_pid = fork(); + if (child_pid < 0) { + return PosixError(errno, "fork failed"); + } + if (child_pid == 0) { + // Close our copy of the pipe's read end, which doesn't really matter. + TEST_PCHECK(close(pipefd[0]) >= 0); + TEST_PCHECK(unshare(CLONE_NEWUSER) == 0); + MaybeSave(); + // Indicate that we've switched namespaces by unblocking the parent's read. + TEST_PCHECK(close(pipefd[1]) >= 0); + while (true) { + SleepSafe(absl::Minutes(1)); + } + } + auto cleanup_child = Cleanup([child_pid] { + EXPECT_THAT(kill(child_pid, SIGKILL), SyscallSucceeds()); + int status; + ASSERT_THAT(RetryEINTR(waitpid)(child_pid, &status, 0), + SyscallSucceedsWithValue(child_pid)); + EXPECT_TRUE(WIFSIGNALED(status) && WTERMSIG(status) == SIGKILL) + << "status = " << status; + }); + // Close our copy of the pipe's write end, then wait for the child to close + // its copy, indicating that it's switched namespaces. + cleanup_pipe_write.Release()(); + char buf; + if (RetryEINTR(read)(pipefd[0], &buf, 1) < 0) { + return PosixError(errno, "reading from pipe failed"); + } + MaybeSave(); + return std::make_tuple(child_pid, std::move(cleanup_child)); +} + // TEST_CHECK-fails on error, since this function is used in contexts that // require async-signal-safety. -void DenySelfSetgroups() { - int fd = open("/proc/self/setgroups", O_WRONLY); +void DenySetgroupsByPath(const char* path) { + int fd = open(path, O_WRONLY); if (fd < 0 && errno == ENOENT) { // On kernels where this file doesn't exist, writing "deny" to it isn't // necessary to write to gid_map. @@ -61,13 +109,19 @@ void DenySelfSetgroups() { TEST_PCHECK(close(fd) == 0); } +void DenySelfSetgroups() { DenySetgroupsByPath("/proc/self/setgroups"); } + +void DenyPidSetgroups(pid_t pid) { + DenySetgroupsByPath(absl::StrCat("/proc/", pid, "/setgroups").c_str()); +} + // Returns a valid UID/GID that isn't id. uint32_t another_id(uint32_t id) { return (id + 1) % 65535; } struct TestParam { std::string desc; - std::string map_filename; int cap; + std::function get_map_filename; std::function get_current_id; }; @@ -75,11 +129,29 @@ std::string DescribeTestParam(const ::testing::TestParamInfo& info) { return info.param.desc; } -class ProcSelfUidGidMapTest : public ::testing::TestWithParam { +std::vector UidGidMapTestParams() { + return {TestParam{"UID", CAP_SETUID, + [](absl::string_view pid) { + return absl::StrCat("/proc/", pid, "/uid_map"); + }, + []() -> uint32_t { return getuid(); }}, + TestParam{"GID", CAP_SETGID, + [](absl::string_view pid) { + return absl::StrCat("/proc/", pid, "/gid_map"); + }, + []() -> uint32_t { return getgid(); }}}; +} + +class ProcUidGidMapTest : public ::testing::TestWithParam { + protected: + uint32_t CurrentID() { return GetParam().get_current_id(); } +}; + +class ProcSelfUidGidMapTest : public ProcUidGidMapTest { protected: PosixErrorOr InNewUserNamespaceWithMapFD( const std::function& fn) { - std::string map_filename = GetParam().map_filename; + std::string map_filename = GetParam().get_map_filename("self"); return InNewUserNamespace([&] { int fd = open(map_filename.c_str(), O_RDWR); TEST_PCHECK(fd >= 0); @@ -88,9 +160,10 @@ class ProcSelfUidGidMapTest : public ::testing::TestWithParam { TEST_PCHECK(close(fd) == 0); }); } +}; - uint32_t CurrentID() { return GetParam().get_current_id(); } - +class ProcPidUidGidMapTest : public ProcUidGidMapTest { + protected: PosixErrorOr HaveSetIDCapability() { return HaveCapability(GetParam().cap); } @@ -100,11 +173,17 @@ class ProcSelfUidGidMapTest : public ::testing::TestWithParam { // IDs into a child user namespace, since even with CAP_SET*ID this is only // possible if those IDs are mapped into the current one. PosixErrorOr AllIDsMapped() { - ASSIGN_OR_RETURN_ERRNO(std::string id_map, GetContents(GetParam().map_filename)); + ASSIGN_OR_RETURN_ERRNO(std::string id_map, + GetContents(GetParam().get_map_filename("self"))); + absl::StripTrailingAsciiWhitespace(&id_map); std::vector id_map_parts = absl::StrSplit(id_map, ' ', absl::SkipEmpty()); return id_map_parts == std::vector({"0", "0", "4294967295"}); } + + PosixErrorOr OpenMapFile(pid_t pid) { + return Open(GetParam().get_map_filename(absl::StrCat(pid)), O_RDWR); + } }; TEST_P(ProcSelfUidGidMapTest, IsInitiallyEmpty) { @@ -117,7 +196,6 @@ TEST_P(ProcSelfUidGidMapTest, IsInitiallyEmpty) { } TEST_P(ProcSelfUidGidMapTest, IdentityMapOwnID) { - // This is the only write permitted if the writer does not have CAP_SET*ID. SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(CanCreateUserNamespace())); uint32_t id = CurrentID(); std::string line = absl::StrCat(id, " ", id, " 1"); @@ -148,7 +226,6 @@ TEST_P(ProcSelfUidGidMapTest, TrailingNewlineAndNULIgnored) { TEST_P(ProcSelfUidGidMapTest, NonIdentityMapOwnID) { SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(CanCreateUserNamespace())); - SKIP_IF(ASSERT_NO_ERRNO_AND_VALUE(HaveSetIDCapability())); uint32_t id = CurrentID(); uint32_t id2 = another_id(id); std::string line = absl::StrCat(id2, " ", id, " 1"); @@ -160,9 +237,11 @@ TEST_P(ProcSelfUidGidMapTest, NonIdentityMapOwnID) { IsPosixErrorOkAndHolds(0)); } -TEST_P(ProcSelfUidGidMapTest, MapOtherIDUnprivileged) { +TEST_P(ProcSelfUidGidMapTest, MapOtherID) { SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(CanCreateUserNamespace())); - SKIP_IF(ASSERT_NO_ERRNO_AND_VALUE(HaveSetIDCapability())); + // Whether or not we have CAP_SET*ID is irrelevant: the process running in the + // new (child) user namespace won't have any capabilities in the current + // (parent) user namespace, which is needed. uint32_t id = CurrentID(); uint32_t id2 = another_id(id); std::string line = absl::StrCat(id, " ", id2, " 1"); @@ -174,25 +253,41 @@ TEST_P(ProcSelfUidGidMapTest, MapOtherIDUnprivileged) { IsPosixErrorOkAndHolds(0)); } -TEST_P(ProcSelfUidGidMapTest, MapOtherIDPrivileged) { +INSTANTIATE_TEST_CASE_P(All, ProcSelfUidGidMapTest, + ::testing::ValuesIn(UidGidMapTestParams()), + DescribeTestParam); + +TEST_P(ProcPidUidGidMapTest, MapOtherIDPrivileged) { + // Like ProcSelfUidGidMapTest_MapOtherID, but since we have CAP_SET*ID in the + // parent user namespace (this one), we can map IDs that aren't ours. SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(CanCreateUserNamespace())); SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveSetIDCapability())); SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(AllIDsMapped())); + + pid_t child_pid; + Cleanup cleanup_child; + std::tie(child_pid, cleanup_child) = + ASSERT_NO_ERRNO_AND_VALUE(CreateProcessInNewUserNamespace()); + uint32_t id = CurrentID(); uint32_t id2 = another_id(id); std::string line = absl::StrCat(id, " ", id2, " 1"); - EXPECT_THAT( - InNewUserNamespaceWithMapFD([&](int fd) { - DenySelfSetgroups(); - TEST_PCHECK(write(fd, line.c_str(), line.size()) == line.size()); - }), - IsPosixErrorOkAndHolds(0)); + DenyPidSetgroups(child_pid); + auto fd = ASSERT_NO_ERRNO_AND_VALUE(OpenMapFile(child_pid)); + EXPECT_THAT(write(fd.get(), line.c_str(), line.size()), + SyscallSucceedsWithValue(line.size())); } -TEST_P(ProcSelfUidGidMapTest, MapAnyIDsPrivileged) { +TEST_P(ProcPidUidGidMapTest, MapAnyIDsPrivileged) { SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(CanCreateUserNamespace())); SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveSetIDCapability())); SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(AllIDsMapped())); + + pid_t child_pid; + Cleanup cleanup_child; + std::tie(child_pid, cleanup_child) = + ASSERT_NO_ERRNO_AND_VALUE(CreateProcessInNewUserNamespace()); + // Test all of: // // - Mapping ranges of length > 1 @@ -201,21 +296,15 @@ TEST_P(ProcSelfUidGidMapTest, MapAnyIDsPrivileged) { // // - Non-identity mappings char entries[] = "2 0 2\n4 6 2"; - EXPECT_THAT( - InNewUserNamespaceWithMapFD([&](int fd) { - DenySelfSetgroups(); - TEST_PCHECK(write(fd, entries, sizeof(entries)) == sizeof(entries)); - }), - IsPosixErrorOkAndHolds(0)); + DenyPidSetgroups(child_pid); + auto fd = ASSERT_NO_ERRNO_AND_VALUE(OpenMapFile(child_pid)); + EXPECT_THAT(write(fd.get(), entries, sizeof(entries)), + SyscallSucceedsWithValue(sizeof(entries))); } -INSTANTIATE_TEST_CASE_P( - All, ProcSelfUidGidMapTest, - ::testing::Values(TestParam{"UID", "/proc/self/uid_map", CAP_SETUID, - []() -> uint32_t { return getuid(); }}, - TestParam{"GID", "/proc/self/gid_map", CAP_SETGID, - []() -> uint32_t { return getgid(); }}), - DescribeTestParam); +INSTANTIATE_TEST_CASE_P(All, ProcPidUidGidMapTest, + ::testing::ValuesIn(UidGidMapTestParams()), + DescribeTestParam); } // namespace testing } // namespace gvisor