From d74dac096ab3b2eacbdf1f7377b041feb13895db Mon Sep 17 00:00:00 2001 From: Wiktor Garbacz Date: Thu, 2 Mar 2023 01:25:02 -0800 Subject: [PATCH] Rework stack_trace_test PiperOrigin-RevId: 513467290 Change-Id: Iab630412052fa5e7333514f3864ebdfb7f10e1ef --- sandboxed_api/sandbox2/BUILD.bazel | 4 +- sandboxed_api/sandbox2/CMakeLists.txt | 4 +- sandboxed_api/sandbox2/sandbox2_test.cc | 30 +--- sandboxed_api/sandbox2/stack_trace_test.cc | 145 ++++++++++-------- sandboxed_api/sandbox2/testcases/BUILD.bazel | 2 +- .../sandbox2/testcases/CMakeLists.txt | 3 +- sandboxed_api/sandbox2/testcases/symbolize.cc | 91 ++++++----- 7 files changed, 131 insertions(+), 148 deletions(-) diff --git a/sandboxed_api/sandbox2/BUILD.bazel b/sandboxed_api/sandbox2/BUILD.bazel index b0c960c..b387ec2 100644 --- a/sandboxed_api/sandbox2/BUILD.bazel +++ b/sandboxed_api/sandbox2/BUILD.bazel @@ -970,15 +970,13 @@ cc_test( ":stack_trace", ":testonly_allow_all_syscalls", "//sandboxed_api:testing", - "//sandboxed_api/sandbox2/util:bpf_helper", "//sandboxed_api/util:fileops", "//sandboxed_api/util:status_matchers", - "//sandboxed_api/util:temp_file", - "@com_google_absl//absl/cleanup", "@com_google_absl//absl/flags:flag", "@com_google_absl//absl/flags:reflection", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", + "@com_google_absl//absl/time", "@com_google_googletest//:gtest_main", ], ) diff --git a/sandboxed_api/sandbox2/CMakeLists.txt b/sandboxed_api/sandbox2/CMakeLists.txt index 416ece2..02446a2 100644 --- a/sandboxed_api/sandbox2/CMakeLists.txt +++ b/sandboxed_api/sandbox2/CMakeLists.txt @@ -1063,19 +1063,17 @@ if(BUILD_TESTING AND SAPI_BUILD_TESTING) sandbox2::testcase_symbolize ) target_link_libraries(sandbox2_stack_trace_test PRIVATE - absl::cleanup absl::flags absl::status absl::strings + absl::time sandbox2::allow_all_syscalls - sandbox2::bpf_helper sandbox2::global_forkserver sandbox2::namespace sandbox2::sandbox2 sandbox2::stack_trace sandbox2::util sapi::fileops - sapi::temp_file sapi::testing sapi::status_matchers sapi::test_main diff --git a/sandboxed_api/sandbox2/sandbox2_test.cc b/sandboxed_api/sandbox2/sandbox2_test.cc index 5a3af66..3177c3d 100644 --- a/sandboxed_api/sandbox2/sandbox2_test.cc +++ b/sandboxed_api/sandbox2/sandbox2_test.cc @@ -53,7 +53,6 @@ PolicyBuilder CreateDefaultPolicyBuilder(absl::string_view path) { using ::sapi::GetTestSourcePath; using ::testing::Eq; -using ::testing::HasSubstr; using ::testing::IsEmpty; using ::testing::IsTrue; using ::testing::Lt; @@ -118,28 +117,6 @@ TEST(ExecutorTest, ExecutorFdConstructor) { ASSERT_EQ(result.final_status(), Result::OK); } -// Tests that we return the correct state when the sandboxee timed out. -TEST(StackTraceTest, StackTraceOnTimeoutWorks) { - SKIP_ANDROID; - const std::string path = GetTestSourcePath("sandbox2/testcases/sleep"); - - std::vector args = {path}; - std::vector envs; - auto executor = std::make_unique(path, args, envs); - - SAPI_ASSERT_OK_AND_ASSIGN(auto policy, - PolicyBuilder() - // Don't restrict the syscalls at all. - .DangerDefaultAllowAll() - .TryBuild()); - Sandbox2 sandbox(std::move(executor), std::move(policy)); - ASSERT_TRUE(sandbox.RunAsync()); - sandbox.set_walltime_limit(absl::Seconds(1)); - auto result = sandbox.AwaitResult(); - EXPECT_EQ(result.final_status(), Result::TIMEOUT); - EXPECT_THAT(result.GetStackTrace(), HasSubstr("sleep")); -} - // Tests that we return the correct state when the sandboxee was killed by an // external signal. Also make sure that we do not have the stack trace. TEST(RunAsyncTest, SandboxeeExternalKill) { @@ -157,7 +134,7 @@ TEST(RunAsyncTest, SandboxeeExternalKill) { sandbox.Kill(); auto result = sandbox.AwaitResult(); EXPECT_EQ(result.final_status(), Result::EXTERNAL_KILL); - EXPECT_THAT(result.GetStackTrace(), IsEmpty()); + EXPECT_THAT(result.stack_trace(), IsEmpty()); } // Tests that we do not collect stack traces if it was disabled (signaled). @@ -176,7 +153,7 @@ TEST(RunAsyncTest, SandboxeeTimeoutDisabledStacktraces) { sandbox.set_walltime_limit(absl::Seconds(1)); auto result = sandbox.AwaitResult(); EXPECT_EQ(result.final_status(), Result::TIMEOUT); - EXPECT_THAT(result.GetStackTrace(), IsEmpty()); + EXPECT_THAT(result.stack_trace(), IsEmpty()); } // Tests that we do not collect stack traces if it was disabled (violation). @@ -196,7 +173,7 @@ TEST(RunAsyncTest, SandboxeeViolationDisabledStacktraces) { ASSERT_TRUE(sandbox.RunAsync()); auto result = sandbox.AwaitResult(); EXPECT_EQ(result.final_status(), Result::VIOLATION); - EXPECT_THAT(result.GetStackTrace(), IsEmpty()); + EXPECT_THAT(result.stack_trace(), IsEmpty()); } TEST(RunAsyncTest, SandboxeeNotKilledWhenStartingThreadFinishes) { @@ -205,7 +182,6 @@ TEST(RunAsyncTest, SandboxeeNotKilledWhenStartingThreadFinishes) { auto executor = std::make_unique(path, args); SAPI_ASSERT_OK_AND_ASSIGN(auto policy, CreateDefaultPolicyBuilder(path) - .CollectStacktracesOnExit(true) .TryBuild()); Sandbox2 sandbox(std::move(executor), std::move(policy)); std::thread sandbox_start_thread([&sandbox]() { sandbox.RunAsync(); }); diff --git a/sandboxed_api/sandbox2/stack_trace_test.cc b/sandboxed_api/sandbox2/stack_trace_test.cc index 1e7defb..48b56eb 100644 --- a/sandboxed_api/sandbox2/stack_trace_test.cc +++ b/sandboxed_api/sandbox2/stack_trace_test.cc @@ -24,12 +24,11 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" -#include "absl/cleanup/cleanup.h" #include "absl/flags/declare.h" #include "absl/flags/flag.h" #include "absl/flags/reflection.h" -#include "absl/strings/match.h" #include "absl/strings/str_cat.h" +#include "absl/time/time.h" #include "sandboxed_api/sandbox2/allow_all_syscalls.h" #include "sandboxed_api/sandbox2/executor.h" #include "sandboxed_api/sandbox2/global_forkclient.h" @@ -37,11 +36,9 @@ #include "sandboxed_api/sandbox2/policybuilder.h" #include "sandboxed_api/sandbox2/result.h" #include "sandboxed_api/sandbox2/sandbox2.h" -#include "sandboxed_api/sandbox2/util/bpf_helper.h" #include "sandboxed_api/testing.h" #include "sandboxed_api/util/fileops.h" #include "sandboxed_api/util/status_matchers.h" -#include "sandboxed_api/util/temp_file.h" ABSL_DECLARE_FLAG(bool, sandbox_libunwind_crash_handler); @@ -49,64 +46,70 @@ namespace sandbox2 { namespace { namespace file_util = ::sapi::file_util; -using ::sapi::CreateNamedTempFileAndClose; using ::sapi::GetTestSourcePath; +using ::testing::Contains; using ::testing::ElementsAre; using ::testing::Eq; -using ::testing::HasSubstr; using ::testing::IsEmpty; -using ::testing::IsTrue; -using ::testing::Not; +using ::testing::StartsWith; + +struct TestCase { + std::string arg = "1"; + int final_status = Result::SIGNALED; + std::string function_name = "CrashMe"; + std::string full_function_description = "CrashMe(char)"; + std::function modify_policy; + absl::Duration wall_time_limit = absl::ZeroDuration(); +}; + +class StackTraceTest : public ::testing::TestWithParam {}; // Test that symbolization of stack traces works. -void SymbolizationWorksCommon( - std::function modify_policy = {}) { +void SymbolizationWorksCommon(TestCase param) { const std::string path = GetTestSourcePath("sandbox2/testcases/symbolize"); - std::vector args = {path, "1"}; - - SAPI_ASSERT_OK_AND_ASSIGN(std::string temp_filename, - CreateNamedTempFileAndClose("/tmp/")); - absl::Cleanup temp_cleanup = [&temp_filename] { - remove(temp_filename.c_str()); - }; - ASSERT_THAT( - file_util::fileops::CopyFile("/proc/cpuinfo", temp_filename, 0444), - IsTrue()); + std::vector args = {path, param.arg}; auto policybuilder = PolicyBuilder() // Don't restrict the syscalls at all. - .DefaultAction(AllowAllSyscalls()) - .AddFile(path) - .AddLibrariesForBinary(path) - .AddFileAt(temp_filename, "/proc/cpuinfo"); - if (modify_policy) { - modify_policy(&policybuilder); + .DefaultAction(AllowAllSyscalls()); + if (param.modify_policy) { + param.modify_policy(&policybuilder); } SAPI_ASSERT_OK_AND_ASSIGN(auto policy, policybuilder.TryBuild()); Sandbox2 s2(std::make_unique(path, args), std::move(policy)); - auto result = s2.Run(); + ASSERT_TRUE(s2.RunAsync()); + s2.set_walltime_limit(param.wall_time_limit); + auto result = s2.AwaitResult(); - ASSERT_THAT(result.final_status(), Eq(Result::SIGNALED)); - ASSERT_THAT(result.GetStackTrace(), HasSubstr("CrashMe")); + EXPECT_THAT(result.final_status(), Eq(param.final_status)); + EXPECT_THAT(result.stack_trace(), Contains(StartsWith(param.function_name))); // Check that demangling works as well. - ASSERT_THAT(result.GetStackTrace(), HasSubstr("CrashMe()")); + EXPECT_THAT(result.stack_trace(), + Contains(StartsWith(param.full_function_description))); } -TEST(StackTraceTest, SymbolizationWorksNonSandboxedLibunwind) { +void SymbolizationWorksWithModifiedPolicy( + std::function modify_policy) { + TestCase test_case; + test_case.modify_policy = std::move(modify_policy); + SymbolizationWorksCommon(test_case); +} + +TEST_P(StackTraceTest, SymbolizationWorksNonSandboxedLibunwind) { SKIP_SANITIZERS_AND_COVERAGE; absl::FlagSaver fs; absl::SetFlag(&FLAGS_sandbox_libunwind_crash_handler, false); - SymbolizationWorksCommon(); + SymbolizationWorksCommon(GetParam()); } -TEST(StackTraceTest, SymbolizationWorksSandboxedLibunwind) { +TEST_P(StackTraceTest, SymbolizationWorksSandboxedLibunwind) { SKIP_SANITIZERS_AND_COVERAGE; absl::FlagSaver fs; absl::SetFlag(&FLAGS_sandbox_libunwind_crash_handler, true); - SymbolizationWorksCommon(); + SymbolizationWorksCommon(GetParam()); } TEST(StackTraceTest, SymbolizationWorksSandboxedLibunwindProcDirMounted) { @@ -114,7 +117,7 @@ TEST(StackTraceTest, SymbolizationWorksSandboxedLibunwindProcDirMounted) { absl::FlagSaver fs; absl::SetFlag(&FLAGS_sandbox_libunwind_crash_handler, true); - SymbolizationWorksCommon( + SymbolizationWorksWithModifiedPolicy( [](PolicyBuilder* builder) { builder->AddDirectory("/proc"); }); } @@ -123,7 +126,7 @@ TEST(StackTraceTest, SymbolizationWorksSandboxedLibunwindProcFileMounted) { absl::FlagSaver fs; absl::SetFlag(&FLAGS_sandbox_libunwind_crash_handler, true); - SymbolizationWorksCommon([](PolicyBuilder* builder) { + SymbolizationWorksWithModifiedPolicy([](PolicyBuilder* builder) { builder->AddFile("/proc/sys/vm/overcommit_memory"); }); } @@ -133,7 +136,7 @@ TEST(StackTraceTest, SymbolizationWorksSandboxedLibunwindSysDirMounted) { absl::FlagSaver fs; absl::SetFlag(&FLAGS_sandbox_libunwind_crash_handler, true); - SymbolizationWorksCommon( + SymbolizationWorksWithModifiedPolicy( [](PolicyBuilder* builder) { builder->AddDirectory("/sys"); }); } @@ -142,12 +145,12 @@ TEST(StackTraceTest, SymbolizationWorksSandboxedLibunwindSysFileMounted) { absl::FlagSaver fs; absl::SetFlag(&FLAGS_sandbox_libunwind_crash_handler, true); - SymbolizationWorksCommon([](PolicyBuilder* builder) { + SymbolizationWorksWithModifiedPolicy([](PolicyBuilder* builder) { builder->AddFile("/sys/devices/system/cpu/online"); }); } -static size_t FileCountInDirectory(const std::string& path) { +size_t FileCountInDirectory(const std::string& path) { std::vector fds; std::string error; CHECK(file_util::fileops::ListDirectoryEntries(path, &fds, &error)); @@ -161,9 +164,7 @@ TEST(StackTraceTest, ForkEnterNsLibunwindDoesNotLeakFDs) { // Very first sanitization might create some fds (e.g. for initial // namespaces). - SymbolizationWorksCommon([](PolicyBuilder* builder) { - builder->AddFile("/sys/devices/system/cpu/online"); - }); + SymbolizationWorksCommon({}); // Get list of open FDs in the global forkserver. pid_t forkserver_pid = GlobalForkClient::GetPid(); @@ -171,33 +172,11 @@ TEST(StackTraceTest, ForkEnterNsLibunwindDoesNotLeakFDs) { absl::StrCat("/proc/", forkserver_pid, "/fd"); size_t filecount_before = FileCountInDirectory(forkserver_fd_path); - SymbolizationWorksCommon([](PolicyBuilder* builder) { - builder->AddFile("/sys/devices/system/cpu/online"); - }); + SymbolizationWorksCommon({}); EXPECT_THAT(filecount_before, Eq(FileCountInDirectory(forkserver_fd_path))); } -// Test that symbolization skips writeable files (attack vector). -TEST(StackTraceTest, SymbolizationTrustedFilesOnly) { - SKIP_SANITIZERS_AND_COVERAGE; - const std::string path = GetTestSourcePath("sandbox2/testcases/symbolize"); - std::vector args = {path, "2"}; - - SAPI_ASSERT_OK_AND_ASSIGN(auto policy, - PolicyBuilder() - // Don't restrict the syscalls at all. - .DefaultAction(AllowAllSyscalls()) - .AddFile(path) - .AddLibrariesForBinary(path) - .TryBuild()); - Sandbox2 s2(std::make_unique(path, args), std::move(policy)); - auto result = s2.Run(); - - ASSERT_THAT(result.final_status(), Eq(Result::SIGNALED)); - ASSERT_THAT(result.GetStackTrace(), Not(HasSubstr("CrashMe"))); -} - TEST(StackTraceTest, CompactStackTrace) { EXPECT_THAT(CompactStackTrace({}), IsEmpty()); EXPECT_THAT(CompactStackTrace({"_start"}), ElementsAre("_start")); @@ -223,5 +202,41 @@ TEST(StackTraceTest, CompactStackTrace) { "(previous frame repeated 3 times)")); } +INSTANTIATE_TEST_SUITE_P( + Instantiation, StackTraceTest, + ::testing::Values( + TestCase{ + .arg = "1", + .final_status = Result::SIGNALED, + .function_name = "CrashMe", + .full_function_description = "CrashMe(char)", + }, + TestCase{ + .arg = "2", + .final_status = Result::VIOLATION, + .function_name = "ViolatePolicy", + .full_function_description = "ViolatePolicy(int)", + }, + TestCase{ + .arg = "3", + .final_status = Result::OK, + .function_name = "ExitNormally", + .full_function_description = "ExitNormally(int)", + .modify_policy = + [](PolicyBuilder* builder) { + builder->CollectStacktracesOnExit(true); + }, + }, + TestCase{ + .arg = "4", + .final_status = Result::TIMEOUT, + .function_name = "SleepForXSeconds", + .full_function_description = "SleepForXSeconds(int)", + .wall_time_limit = absl::Seconds(1), + }), + [](const ::testing::TestParamInfo& info) { + return info.param.function_name; + }); + } // namespace } // namespace sandbox2 diff --git a/sandboxed_api/sandbox2/testcases/BUILD.bazel b/sandboxed_api/sandbox2/testcases/BUILD.bazel index 8b552c4..017a6ba 100644 --- a/sandboxed_api/sandbox2/testcases/BUILD.bazel +++ b/sandboxed_api/sandbox2/testcases/BUILD.bazel @@ -168,9 +168,9 @@ cc_binary( testonly = True, srcs = ["symbolize.cc"], copts = sapi_platform_copts(), + features = ["fully_static_link"], deps = [ "//sandboxed_api/util:raw_logging", - "//sandboxed_api/util:temp_file", "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/strings", ], diff --git a/sandboxed_api/sandbox2/testcases/CMakeLists.txt b/sandboxed_api/sandbox2/testcases/CMakeLists.txt index b9bcbe7..ca6f610 100644 --- a/sandboxed_api/sandbox2/testcases/CMakeLists.txt +++ b/sandboxed_api/sandbox2/testcases/CMakeLists.txt @@ -208,10 +208,9 @@ set_target_properties(sandbox2_testcase_symbolize PROPERTIES OUTPUT_NAME symbolize ) target_link_libraries(sandbox2_testcase_symbolize PRIVATE + -static absl::core_headers absl::strings - sapi::temp_file - sapi::strerror sapi::base sapi::raw_logging ) diff --git a/sandboxed_api/sandbox2/testcases/symbolize.cc b/sandboxed_api/sandbox2/testcases/symbolize.cc index 55cb1d2..9a5d998 100644 --- a/sandboxed_api/sandbox2/testcases/symbolize.cc +++ b/sandboxed_api/sandbox2/testcases/symbolize.cc @@ -12,65 +12,59 @@ // See the License for the specific language governing permissions and // limitations under the License. -// A binary that crashes, either directly or by copying and re-executing, -// to test the stack tracing symbolizer. +// A binary that exits via different modes: crashes, causes violation, exits +// normally or times out, to test the stack tracing symbolizer. -#include -#include -#include -#include +#include #include -#include +#include #include "absl/base/attributes.h" #include "absl/strings/numbers.h" #include "sandboxed_api/util/raw_logging.h" -#include "sandboxed_api/util/temp_file.h" -ABSL_ATTRIBUTE_NOINLINE -void CrashMe() { - volatile char* null = nullptr; - *null = 0; +// Sometimes we don't have debug info to properly unwind through libc (a frame +// is skipped). +// Workaround by putting another frame on the call stack. +template +ABSL_ATTRIBUTE_NOINLINE ABSL_ATTRIBUTE_NO_TAIL_CALL void IndirectLibcCall( + F func) { + func(); } -void RunWritable() { - int exe_fd = open("/proc/self/exe", O_RDONLY); - SAPI_RAW_PCHECK(exe_fd >= 0, "Opening /proc/self/exe"); +ABSL_ATTRIBUTE_NOINLINE +void CrashMe(char x = 0) { + volatile char* null = nullptr; + *null = x; +} - std::string tmpname; - int tmp_fd; - std::tie(tmpname, tmp_fd) = sapi::CreateNamedTempFile("tmp").value(); - SAPI_RAW_PCHECK(fchmod(tmp_fd, S_IRWXU) == 0, "Fchmod on temporary file"); +ABSL_ATTRIBUTE_NOINLINE +ABSL_ATTRIBUTE_NO_TAIL_CALL +void ViolatePolicy(int x = 0) { + IndirectLibcCall([x]() { syscall(__NR_ptrace, x); }); +} - char buf[4096]; - while (true) { - ssize_t read_cnt = read(exe_fd, buf, sizeof(buf)); - SAPI_RAW_PCHECK(read_cnt >= 0, "Reading /proc/self/exe"); - if (read_cnt == 0) { - break; +ABSL_ATTRIBUTE_NOINLINE +ABSL_ATTRIBUTE_NO_TAIL_CALL +void ExitNormally(int x = 0) { + IndirectLibcCall([x]() { + // _exit is marked noreturn, which makes stack traces a bit trickier - + // work around by using a volatile read + if (volatile int y = 1) { + _exit(x); } - SAPI_RAW_PCHECK(write(tmp_fd, buf, read_cnt) == read_cnt, - "Writing temporary file"); - } + }); +} - SAPI_RAW_PCHECK(close(tmp_fd) == 0, "Closing temporary file"); - tmp_fd = open(tmpname.c_str(), O_RDONLY); - SAPI_RAW_PCHECK(tmp_fd >= 0, "Reopening temporary file"); - - char prog_name[] = "crashme"; - char testno[] = "1"; - char* argv[] = {prog_name, testno, nullptr}; - - SAPI_RAW_PCHECK(execv(tmpname.c_str(), argv) == 0, "Executing copied binary"); +ABSL_ATTRIBUTE_NOINLINE +ABSL_ATTRIBUTE_NO_TAIL_CALL +void SleepForXSeconds(int x = 0) { + IndirectLibcCall([x]() { sleep(x); }); } int main(int argc, char* argv[]) { - if (argc < 2) { - printf("argc < 3\n"); - return EXIT_FAILURE; - } - + SAPI_RAW_CHECK(argc >= 2, "Not enough arguments"); int testno; SAPI_RAW_CHECK(absl::SimpleAtoi(argv[1], &testno), "testno not a number"); switch (testno) { @@ -78,13 +72,16 @@ int main(int argc, char* argv[]) { CrashMe(); break; case 2: - RunWritable(); + ViolatePolicy(); + break; + case 3: + ExitNormally(); + break; + case 4: + SleepForXSeconds(10); break; default: - printf("Unknown test: %d\n", testno); - return EXIT_FAILURE; + SAPI_RAW_LOG(FATAL, "Unknown test case: %d", testno); } - - printf("OK: All tests went OK\n"); return EXIT_SUCCESS; }