diff --git a/sandboxed_api/sandbox2/policy.cc b/sandboxed_api/sandbox2/policy.cc index 398fbfe..564307e 100644 --- a/sandboxed_api/sandbox2/policy.cc +++ b/sandboxed_api/sandbox2/policy.cc @@ -106,18 +106,26 @@ std::vector Policy::GetDefaultPolicy() const { SANDBOX2_TRACE, LABEL(&l, past_execveat_l), - // Forbid some syscalls because unsafe or too risky. + // Forbid ptrace because it's unsafe or too risky. LOAD_SYSCALL_NR, JEQ32(__NR_ptrace, DENY), - JEQ32(__NR_bpf, DENY), - - // Disallow clone with CLONE_UNTRACED flag. - JNE32(__NR_clone, JUMP(&l, past_clone_untraced_l)), - // Regardless of arch, we only care about the lower 32-bits of the flags. - ARG_32(0), - JA32(CLONE_UNTRACED, DENY), - LABEL(&l, past_clone_untraced_l), }; + // If user policy doesn't mention it, then forbid bpf because it's unsafe or + // too risky. This uses LOAD_SYSCALL_NR from above. + if (!user_policy_handles_bpf_) { + policy.insert(policy.end(), {JEQ32(__NR_bpf, DENY)}); + } + policy.insert(policy.end(), + { + // Disallow clone with CLONE_UNTRACED flag. This uses + // LOAD_SYSCALL_NR from above. + JNE32(__NR_clone, JUMP(&l, past_clone_untraced_l)), + // Regardless of arch, we only care about the lower 32-bits + // of the flags. + ARG_32(0), + JA32(CLONE_UNTRACED, DENY), + LABEL(&l, past_clone_untraced_l), + }); if (bpf_resolve_jumps(&l, policy.data(), policy.size()) != 0) { LOG(FATAL) << "Cannot resolve bpf jumps"; diff --git a/sandboxed_api/sandbox2/policy.h b/sandboxed_api/sandbox2/policy.h index a50439b..989b14a 100644 --- a/sandboxed_api/sandbox2/policy.h +++ b/sandboxed_api/sandbox2/policy.h @@ -96,6 +96,7 @@ class Policy final { // The policy set by the user. std::vector user_policy_; + bool user_policy_handles_bpf_ = false; // Get the default policy, which blocks certain dangerous syscalls and // mismatched syscall tables. diff --git a/sandboxed_api/sandbox2/policy_test.cc b/sandboxed_api/sandbox2/policy_test.cc index a7cf852..54d95a2 100644 --- a/sandboxed_api/sandbox2/policy_test.cc +++ b/sandboxed_api/sandbox2/policy_test.cc @@ -40,7 +40,7 @@ using ::testing::Eq; namespace sandbox2 { namespace { -std::unique_ptr PolicyTestcasePolicy() { +PolicyBuilder CreatePolicyTestPolicyBuilder() { return PolicyBuilder() .DisableNamespaces() .AllowStaticStartup() @@ -60,8 +60,11 @@ std::unique_ptr PolicyTestcasePolicy() { #ifdef __NR_faccessat .BlockSyscallWithErrno(__NR_faccessat, ENOENT) #endif - .BlockSyscallWithErrno(__NR_prlimit64, EPERM) - .BuildOrDie(); + .BlockSyscallWithErrno(__NR_prlimit64, EPERM); +} + +std::unique_ptr PolicyTestcasePolicy() { + return CreatePolicyTestPolicyBuilder().BuildOrDie(); } #ifdef SAPI_X86_64 @@ -150,6 +153,25 @@ TEST(PolicyTest, BpfDisallowed) { EXPECT_THAT(result.reason_code(), Eq(__NR_bpf)); } +// Test that bpf(2) can return EPERM. +TEST(PolicyTest, BpfPermissionDenied) { + SKIP_SANITIZERS_AND_COVERAGE; + const std::string path = GetTestSourcePath("sandbox2/testcases/policy"); + std::vector args = {path, "7"}; + auto executor = absl::make_unique(path, args); + + auto policy = CreatePolicyTestPolicyBuilder() + .BlockSyscallWithErrno(__NR_bpf, EPERM) + .BuildOrDie(); + + Sandbox2 s2(std::move(executor), std::move(policy)); + auto result = s2.Run(); + + // bpf(2) is not a violation due to explicit policy. EPERM is expected. + ASSERT_THAT(result.final_status(), Eq(Result::OK)); + EXPECT_THAT(result.reason_code(), Eq(EPERM)); +} + TEST(PolicyTest, IsattyAllowed) { SKIP_SANITIZERS_AND_COVERAGE; const std::string path = GetTestSourcePath("sandbox2/testcases/policy"); diff --git a/sandboxed_api/sandbox2/policybuilder.cc b/sandboxed_api/sandbox2/policybuilder.cc index 2ec94aa..af4f736 100644 --- a/sandboxed_api/sandbox2/policybuilder.cc +++ b/sandboxed_api/sandbox2/policybuilder.cc @@ -82,6 +82,9 @@ PolicyBuilder& PolicyBuilder::BlockSyscallWithErrno(unsigned int num, int error) { if (handled_syscalls_.insert(num).second) { user_policy_.insert(user_policy_.end(), {SYSCALL(num, ERRNO(error))}); + if (num == __NR_bpf) { + user_policy_handles_bpf_ = true; + } } return *this; } @@ -728,6 +731,7 @@ absl::StatusOr> PolicyBuilder::TryBuild() { output->collect_stacktrace_on_timeout_ = collect_stacktrace_on_timeout_; output->collect_stacktrace_on_kill_ = collect_stacktrace_on_kill_; output->user_policy_ = std::move(user_policy_); + output->user_policy_handles_bpf_ = user_policy_handles_bpf_; auto pb_description = absl::make_unique(); diff --git a/sandboxed_api/sandbox2/policybuilder.h b/sandboxed_api/sandbox2/policybuilder.h index 9ac75bf..8c919d6 100644 --- a/sandboxed_api/sandbox2/policybuilder.h +++ b/sandboxed_api/sandbox2/policybuilder.h @@ -554,6 +554,7 @@ class PolicyBuilder final { // Seccomp fields std::vector user_policy_; + bool user_policy_handles_bpf_ = false; std::set handled_syscalls_; // Error handling diff --git a/sandboxed_api/sandbox2/testcases/policy.cc b/sandboxed_api/sandbox2/testcases/policy.cc index ae598cf..564b001 100644 --- a/sandboxed_api/sandbox2/testcases/policy.cc +++ b/sandboxed_api/sandbox2/testcases/policy.cc @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -79,6 +80,10 @@ void TestBpf() { exit(EXIT_FAILURE); } +void TestBpfError() { + exit(syscall(__NR_bpf, 0, nullptr, 0) == -1 ? errno : 0); +} + void TestIsatty() { isatty(0); @@ -119,6 +124,9 @@ int main(int argc, char** argv) { case 6: TestIsatty(); break; + case 7: + TestBpfError(); + break; default: printf("Unknown test: %d\n", testno); return EXIT_FAILURE;