From d6f3ad37a46a37dc286bf0acf1bcf60094270de2 Mon Sep 17 00:00:00 2001 From: Arjun Dhaliwal Date: Tue, 1 Sep 2026 15:16:19 -0700 Subject: [PATCH 1/2] Add ScopedPrivileges RAII and sanitize env on elevation --- .../host/commands/cvdalloc/privilege.cpp | 50 +++++++++++++++++-- .../host/commands/cvdalloc/privilege.h | 24 ++++++++- 2 files changed, 70 insertions(+), 4 deletions(-) diff --git a/base/cvd/cuttlefish/host/commands/cvdalloc/privilege.cpp b/base/cvd/cuttlefish/host/commands/cvdalloc/privilege.cpp index 8734b1be5db..1170b1bef46 100644 --- a/base/cvd/cuttlefish/host/commands/cvdalloc/privilege.cpp +++ b/base/cvd/cuttlefish/host/commands/cvdalloc/privilege.cpp @@ -15,20 +15,25 @@ */ #include "cuttlefish/host/commands/cvdalloc/privilege.h" +#include #include #include +#include +#include #include #if defined(__linux__) #include #include #include +#include #include #include -#include #include #endif +#include #include +#include #include "absl/log/log.h" @@ -95,10 +100,10 @@ Result ValidateCvdallocBinary(std::string_view path) { #if defined(__linux__) (void)st; /* Try and determine if the cvdalloc binary has any capabilities. */ - struct vfs_cap_data cap; + struct vfs_cap_data cap = {}; ssize_t s = getxattr(path.data(), XATTR_NAME_CAPS, &cap, sizeof(cap)); CF_EXPECTF( - s != 1 && (cap.data[0].permitted & (1 << CAP_NET_ADMIN)) != 0, + s != -1 && (cap.data[0].permitted & (1 << CAP_NET_ADMIN)) != 0, "cvdalloc binary does not have permissions to allocate resources.\n" "As root, please\n\n setcap cap_net_admin,cap_net_bind_service," "cap_net_raw=+ep `realpath {}`", @@ -157,4 +162,43 @@ int DropPrivileges(uid_t orig) { return setuid(orig); } +namespace { +constexpr char kTrustedPath[] = "/usr/sbin:/usr/bin:/sbin:/bin"; +} // namespace + +Result ScopedPrivileges::Elevate() { + uid_t orig = getuid(); + // The child processes we exec run with elevated privilege (CAP_NET_ADMIN via + // ambient caps) but with AT_SECURE=0, so the dynamic linker won't scrub their + // environment for us. Sanitize with an allowlist. +#if defined(__linux__) + // On Linux, only sanitize when this exec actually gained privilege (e.g. via + // file caps), as signalled by AT_SECURE. + const bool should_sanitize = getauxval(AT_SECURE) != 0; +#else + // Elsewhere we can't rely on AT_SECURE, so sanitize unconditionally. + const bool should_sanitize = true; +#endif + if (should_sanitize) { + CF_EXPECTF(clearenv() == 0, "Couldn't clear environment: {}", + StrError(errno)); + CF_EXPECTF(setenv("PATH", kTrustedPath, /*overwrite=*/1) == 0, + "Couldn't set PATH: {}", StrError(errno)); + } + CF_EXPECTF(BeginElevatedPrivileges() != -1, + "Couldn't elevate permissions: {}", StrError(errno)); + return ScopedPrivileges(orig); +} + +ScopedPrivileges::ScopedPrivileges(uid_t orig) : orig_(orig) {} + +ScopedPrivileges::ScopedPrivileges(ScopedPrivileges&& other) noexcept + : orig_(std::exchange(other.orig_, std::nullopt)) {} + +ScopedPrivileges::~ScopedPrivileges() { + if (orig_.has_value() && DropPrivileges(*orig_) == -1) { + LOG(ERROR) << "cvdalloc: couldn't drop privileges: " << StrError(errno); + } +} + } // namespace cuttlefish diff --git a/base/cvd/cuttlefish/host/commands/cvdalloc/privilege.h b/base/cvd/cuttlefish/host/commands/cvdalloc/privilege.h index c78a266a900..0140d08b2ca 100644 --- a/base/cvd/cuttlefish/host/commands/cvdalloc/privilege.h +++ b/base/cvd/cuttlefish/host/commands/cvdalloc/privilege.h @@ -13,8 +13,12 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#include +#ifndef CUTTLEFISH_HOST_COMMANDS_CVDALLOC_PRIVILEGE_H_ +#define CUTTLEFISH_HOST_COMMANDS_CVDALLOC_PRIVILEGE_H_ +#include + +#include #include #include "cuttlefish/result/result_type.h" @@ -25,4 +29,22 @@ int BeginElevatedPrivileges(); int DropPrivileges(uid_t orig); Result ValidateCvdallocBinary(std::string_view path); +class ScopedPrivileges { + public: + static Result Elevate(); + + ScopedPrivileges(ScopedPrivileges&& other) noexcept; + ScopedPrivileges& operator=(ScopedPrivileges&& other) = delete; + ScopedPrivileges(const ScopedPrivileges&) = delete; + ScopedPrivileges& operator=(const ScopedPrivileges&) = delete; + ~ScopedPrivileges(); + + private: + explicit ScopedPrivileges(uid_t orig); + + std::optional orig_; +}; + } // namespace cuttlefish + +#endif // CUTTLEFISH_HOST_COMMANDS_CVDALLOC_PRIVILEGE_H_ From 76af9c434d5a4db0a5358cf2b0fd62e798c9b365 Mon Sep 17 00:00:00 2001 From: Arjun Dhaliwal Date: Wed, 9 Sep 2026 16:01:53 -0700 Subject: [PATCH 2/2] address review comments + add clarification that Elevate() wipes the process env --- .../host/commands/cvdalloc/privilege.cpp | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/base/cvd/cuttlefish/host/commands/cvdalloc/privilege.cpp b/base/cvd/cuttlefish/host/commands/cvdalloc/privilege.cpp index 1170b1bef46..b7c89fe1a2c 100644 --- a/base/cvd/cuttlefish/host/commands/cvdalloc/privilege.cpp +++ b/base/cvd/cuttlefish/host/commands/cvdalloc/privilege.cpp @@ -166,20 +166,21 @@ namespace { constexpr char kTrustedPath[] = "/usr/sbin:/usr/bin:/sbin:/bin"; } // namespace +// Activate this instance and gain privileges. +// WARNING: We treat elevating privileges as a one-way +// action. Activating an instance of ScopedPrivileges +// will scrub its process' environment. Result ScopedPrivileges::Elevate() { uid_t orig = getuid(); + bool should_sanitize_env = true; +#if defined(__linux__) // The child processes we exec run with elevated privilege (CAP_NET_ADMIN via // ambient caps) but with AT_SECURE=0, so the dynamic linker won't scrub their - // environment for us. Sanitize with an allowlist. -#if defined(__linux__) - // On Linux, only sanitize when this exec actually gained privilege (e.g. via - // file caps), as signalled by AT_SECURE. - const bool should_sanitize = getauxval(AT_SECURE) != 0; -#else - // Elsewhere we can't rely on AT_SECURE, so sanitize unconditionally. - const bool should_sanitize = true; + // environment for us. Only sanitize the environment when this exec actually + // gained privilege (e.g. via file caps), as signalled by AT_SECURE. + should_sanitize_env = getauxval(AT_SECURE) != 0; #endif - if (should_sanitize) { + if (should_sanitize_env) { CF_EXPECTF(clearenv() == 0, "Couldn't clear environment: {}", StrError(errno)); CF_EXPECTF(setenv("PATH", kTrustedPath, /*overwrite=*/1) == 0,