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 |
There was a problem hiding this comment.
i'm not sure we can with ambient caps
|
|
||
| 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.
There was a problem hiding this comment.
ahhh yeah okay that makes sense
| 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?
There was a problem hiding this comment.
i was thinking of it from the other way around, more or less "clear out everything we don't need, since we are conferring higher privileges on anyone running this code" rather than "clear out the thing that looks unsafe"
There was a problem hiding this comment.
The usage expectation semantically is that in the scoped block, we have elevated privileges, and then we drop them outside the block. This is not the case with the environment: we hose the environment and never restore them.
Maybe fixing the behavior is more trouble than it's worth, but we should at least signpost this.
There was a problem hiding this comment.
oh, i see what you mean now. it could go either way; we could add some machinery to restore the environment that we cleared, though on the other hand i also do think it is defensible for elevating to not restore you to your old state if it was not clearly secure, for example. especially when we are aren't using any of that environment.
but yes 100% that behaviour should be documented in a comment, you're right
| } | ||
|
|
||
| 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.)
There was a problem hiding this comment.
you mean if we try executing anything else from inside that binary?
There was a problem hiding this comment.
other platforms may not have the necessary binaries in these exact locations.
There was a problem hiding this comment.
oh i missed the most important word in that sentence (portability)
2 participants
this pr does two things:
not going to wire it up to cvdalloc until the final PR, though