Add ScopedPrivileges RAII and sanitize env on elevation - #3138
Conversation
|
@3405691582 we can continue the discussion about whether or not the sanitization is necessary here. imo it's not a huge deal, but there's no harm in doing it given that we are using the PATH variable and also transitively conferring elevated caps onto other binaries. |
| Result<ScopedPrivileges> 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 |
|
|
||
| Result<ScopedPrivileges> ScopedPrivileges::Elevate() { | ||
| uid_t orig = getuid(); | ||
| // The child processes we exec run with elevated privilege (CAP_NET_ADMIN via |
There was a problem hiding this comment.
Since this comment talks about Linux capabilities, it should probably belong in the #if block below.
| // 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 |
There was a problem hiding this comment.
per the other pr, when I hear "sanitize" I think about clang's sanitizers, for example. We could still use the term if we qualified it, e.g., "only sanitize the environment when..." and "should_sanitize_env" for example.
| 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; |
There was a problem hiding this comment.
I don't think declaring in both branches of the #if will buy you much versus setting the default as non-const and then re-set it, then eliminate the #else branch.
| const bool should_sanitize = true; | ||
| #endif | ||
| if (should_sanitize) { | ||
| CF_EXPECTF(clearenv() == 0, "Couldn't clear environment: {}", |
There was a problem hiding this comment.
You're hosing the environment here, but we're not saving and restoring it. Maybe it's sufficient to just force PATH?
| } | ||
|
|
||
| namespace { | ||
| constexpr char kTrustedPath[] = "/usr/sbin:/usr/bin:/sbin:/bin"; |
There was a problem hiding this comment.
(this has a small possibility for future portability weirdness, but it's probably fine for now.)
2 participants
this pr does two things:
not going to wire it up to cvdalloc until the final PR, though