Skip to content

Commit 229aecf

Browse files
committed
feat(supervisor): accept numeric UIDs for process identity dropping
Allow run_as_user and run_as_group to be numeric UIDs/GIDs, removing the hard dependency on a baked-in 'sandbox' user in container images. Changes: - validate_sandbox_user(): accepts numeric UIDs without passwd lookup (logs OCSF event); keeps passwd check for "sandbox" name; rejects non-numeric non-sandbox strings that fail passwd lookup - prepare_filesystem(): passes numeric UIDs/GIDs directly to chown() instead of requiring a passwd entry - drop_privileges(): resolves numeric UIDs/GIDs directly via UID::from_raw / Gid::from_raw; skips initgroups when target uid matches current euid; uses guard conditions before setgid/setuid calls - session_user_and_home(): falls back to ("{uid}", "/sandbox") for numeric UIDs, avoiding a passwd lookup that will fail Re-exports MIN_SANDBOX_UID and MAX_SANDBOX_UID from openshell-policy so callers have consistent range constants. Phase 2 of #1959. Signed-off-by: Seth Jennings <sjenning@redhat.com>
1 parent 442b5f3 commit 229aecf

4 files changed

Lines changed: 300 additions & 53 deletions

File tree

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/openshell-supervisor-process/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ rust-version.workspace = true
1313
[dependencies]
1414
openshell-core = { path = "../openshell-core" }
1515
openshell-ocsf = { path = "../openshell-ocsf" }
16+
openshell-policy = { path = "../openshell-policy" }
1617

1718
anyhow = { workspace = true }
1819
base64 = { workspace = true }

crates/openshell-supervisor-process/src/process.rs

Lines changed: 181 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ use crate::netns::NetworkNamespace;
1111
use crate::sandbox;
1212
use miette::{IntoDiagnostic, Result};
1313
use nix::sys::signal::{self, Signal};
14-
use nix::unistd::{Group, Pid, User};
14+
use nix::unistd::{Gid, Group, Pid, Uid, User};
1515
use openshell_core::policy::{NetworkMode, SandboxPolicy};
1616
use std::collections::HashMap;
1717
use std::ffi::CString;
@@ -788,17 +788,36 @@ impl Drop for ProcessHandle {
788788
}
789789
}
790790

791-
/// Validate that the `sandbox` user exists in this image.
791+
/// Validate that the configured sandbox identity exists in this image.
792792
///
793-
/// All sandbox images must include a `sandbox` user for privilege dropping.
794-
/// This check runs at supervisor startup (inside the container) where we can
795-
/// inspect `/etc/passwd`. If the user is missing, the sandbox fails fast
796-
/// with a clear error instead of silently running child processes as root.
793+
/// When the identity is the literal `"sandbox"`, verifies the user exists
794+
/// in `/etc/passwd` (all sandbox images ship with one).
795+
///
796+
/// When the identity is a numeric UID, skips the passwd lookup entirely —
797+
/// the kernel will use the resolved UID regardless of whether an entry
798+
/// exists in `/etc/passwd`. Logs an OCSF event confirming numeric UID usage.
799+
/// Non-numeric, non-"sandbox" values are rejected.
797800
#[cfg(unix)]
798801
pub fn validate_sandbox_user(policy: &SandboxPolicy) -> Result<()> {
799-
let user_name = policy.process.run_as_user.as_deref().unwrap_or("sandbox");
802+
let identity = policy.process.run_as_user.as_deref().unwrap_or("sandbox");
803+
804+
// Numeric UID — no passwd entry required; kernel resolves directly.
805+
if openshell_policy::is_valid_sandbox_identity(identity)
806+
&& identity.parse::<u32>().is_ok()
807+
{
808+
openshell_ocsf::ocsf_emit!(
809+
openshell_ocsf::ConfigStateChangeBuilder::new(openshell_ocsf::ctx::ctx())
810+
.severity(openshell_ocsf::SeverityId::Informational)
811+
.status(openshell_ocsf::StatusId::Success)
812+
.state(openshell_ocsf::StateId::Enabled, "validated")
813+
.message(format!("Accepted numeric UID {identity} (no passwd entry required)"))
814+
.build()
815+
);
816+
return Ok(());
817+
}
800818

801-
if user_name.is_empty() || user_name == "sandbox" {
819+
// "sandbox" name — must exist in /etc/passwd.
820+
if identity == "sandbox" {
802821
match User::from_name("sandbox") {
803822
Ok(Some(_)) => {
804823
openshell_ocsf::ocsf_emit!(
@@ -820,11 +839,34 @@ pub fn validate_sandbox_user(policy: &SandboxPolicy) -> Result<()> {
820839
return Err(miette::miette!("failed to look up 'sandbox' user: {e}"));
821840
}
822841
}
842+
} else if !identity.is_empty() {
843+
// Non-numeric, non-sandbox string — attempt passwd lookup.
844+
// This catches cases where someone accidentally put "root" or similar.
845+
match User::from_name(identity) {
846+
Ok(Some(_)) => {
847+
tracing::warn!(
848+
identity,
849+
"non-sandbox user accepted via passwd entry; \
850+
consider using a numeric UID for UID-injected images"
851+
);
852+
}
853+
Ok(None) => {
854+
return Err(miette::miette!(
855+
"unrecognized sandbox identity '{identity}'; \
856+
expected 'sandbox' or a numeric UID in range [{MIN_SANDBOX_UID}, {MAX_SANDBOX_UID}]"
857+
));
858+
}
859+
Err(e) => {
860+
return Err(miette::miette!("failed to look up identity '{identity}': {e}"));
861+
}
862+
}
823863
}
824864

825865
Ok(())
826866
}
827867

868+
pub use openshell_policy::{MIN_SANDBOX_UID, MAX_SANDBOX_UID};
869+
828870
/// Prepare a `read_write` path for the sandboxed process.
829871
///
830872
/// Returns `true` when the path was created by the supervisor and therefore
@@ -863,9 +905,13 @@ fn prepare_read_write_path(path: &Path) -> Result<bool> {
863905
/// Creates `read_write` directories if they don't exist and sets ownership
864906
/// on newly-created paths to the configured sandbox user/group. This runs as
865907
/// the supervisor (root) before forking the child process.
908+
///
909+
/// Accepts both name-based identities (resolved via `/etc/passwd`) and numeric
910+
/// UIDs/GIDs (passed directly to `chown` without a passwd lookup).
866911
#[cfg(unix)]
867912
pub fn prepare_filesystem(policy: &SandboxPolicy) -> Result<()> {
868913
use nix::unistd::chown;
914+
use nix::unistd::{Gid, Uid};
869915

870916
let user_name = match policy.process.run_as_user.as_deref() {
871917
Some(name) if !name.is_empty() => Some(name),
@@ -881,27 +927,26 @@ pub fn prepare_filesystem(policy: &SandboxPolicy) -> Result<()> {
881927
return Ok(());
882928
}
883929

884-
// Resolve user and group
885-
let uid = if let Some(name) = user_name {
886-
Some(
887-
User::from_name(name)
888-
.into_diagnostic()?
889-
.ok_or_else(|| miette::miette!("Sandbox user not found: {name}"))?
890-
.uid,
891-
)
892-
} else {
893-
None
930+
// Resolve UID: numeric values are passed directly; names resolve via passwd.
931+
let uid = match user_name {
932+
Some(name) if name.parse::<u32>().is_ok() => {
933+
Some(Uid::from_raw(name.parse().into_diagnostic()?))
934+
}
935+
Some(name) => User::from_name(name)
936+
.into_diagnostic()?
937+
.map(|u| u.uid),
938+
_ => None,
894939
};
895940

896-
let gid = if let Some(name) = group_name {
897-
Some(
898-
Group::from_name(name)
899-
.into_diagnostic()?
900-
.ok_or_else(|| miette::miette!("Sandbox group not found: {name}"))?
901-
.gid,
902-
)
903-
} else {
904-
None
941+
// Resolve GID: numeric values are passed directly; names resolve via group.
942+
let gid = match group_name {
943+
Some(name) if name.parse::<u32>().is_ok() => {
944+
Some(Gid::from_raw(name.parse().into_diagnostic()?))
945+
}
946+
Some(name) => Group::from_name(name)
947+
.into_diagnostic()?
948+
.map(|g| g.gid),
949+
_ => None,
905950
};
906951

907952
// Create missing read_write paths and only chown the ones we created.
@@ -954,27 +999,57 @@ pub fn drop_privileges(policy: &SandboxPolicy) -> Result<()> {
954999
return Ok(());
9551000
}
9561001

957-
let user = if let Some(name) = user_name {
958-
User::from_name(name)
1002+
// Resolve UID: numeric values are used directly; names resolve via passwd.
1003+
let target_uid = match user_name {
1004+
Some(name) if name.parse::<u32>().is_ok() => Uid::from_raw(
1005+
name.parse().into_diagnostic()?,
1006+
),
1007+
Some(name) => User::from_name(name)
9591008
.into_diagnostic()?
9601009
.ok_or_else(|| miette::miette!("Sandbox user not found: {name}"))?
961-
} else {
962-
User::from_uid(nix::unistd::geteuid())
963-
.into_diagnostic()?
964-
.ok_or_else(|| miette::miette!("Failed to resolve current user"))?
1010+
.uid,
1011+
None => nix::unistd::geteuid(),
9651012
};
9661013

967-
let group = if let Some(name) = group_name {
968-
Group::from_name(name)
1014+
// Resolve group: if a numeric GID is configured use it directly.
1015+
// Otherwise try name resolution, then fall back to current user's primary group.
1016+
let target_gid = match group_name {
1017+
Some(name) if name.parse::<u32>().is_ok() => Gid::from_raw(
1018+
name.parse().into_diagnostic()?,
1019+
),
1020+
Some(name) => Group::from_name(name)
9691021
.into_diagnostic()?
9701022
.ok_or_else(|| miette::miette!("Sandbox group not found: {name}"))?
971-
} else {
972-
Group::from_gid(user.gid)
1023+
.gid,
1024+
None => match target_uid.as_raw() {
1025+
0 => nix::unistd::getegid(),
1026+
_ => Group::from_gid(
1027+
User::from_uid(target_uid)
1028+
.into_diagnostic()?
1029+
.ok_or_else(|| miette::miette!("Failed to resolve user from UID {target_uid}"))?
1030+
.gid,
1031+
)
9731032
.into_diagnostic()?
974-
.ok_or_else(|| miette::miette!("Failed to resolve user primary group"))?
1033+
.map_or_else(nix::unistd::getegid, |g| g.gid),
1034+
},
9751035
};
9761036

977-
if user_name.is_some() {
1037+
// Resolve the user record for initgroups (if name-based) or skip it (numeric UID).
1038+
let user = if user_name.is_some() {
1039+
Some(
1040+
User::from_uid(target_uid)
1041+
.into_diagnostic()?
1042+
.ok_or_else(|| miette::miette!("Failed to resolve user record for UID {target_uid}"))?,
1043+
)
1044+
} else {
1045+
None
1046+
};
1047+
1048+
// Set supplementary groups only when we have a name-based identity.
1049+
// Numeric UIDs may not have a passwd entry, so initgroups would fail.
1050+
if let Some(ref user) = user
1051+
&& target_uid != nix::unistd::geteuid()
1052+
{
9781053
let user_cstr =
9791054
CString::new(user.name.clone()).map_err(|_| miette::miette!("Invalid user name"))?;
9801055
#[cfg(any(
@@ -993,46 +1068,52 @@ pub fn drop_privileges(policy: &SandboxPolicy) -> Result<()> {
9931068
target_os = "redox"
9941069
)))]
9951070
{
996-
nix::unistd::initgroups(user_cstr.as_c_str(), group.gid).into_diagnostic()?;
1071+
nix::unistd::initgroups(user_cstr.as_c_str(), target_gid).into_diagnostic()?;
9971072
}
9981073
}
9991074

1000-
nix::unistd::setgid(group.gid).into_diagnostic()?;
1075+
if target_gid != nix::unistd::getegid() {
1076+
nix::unistd::setgid(target_gid).into_diagnostic()?;
1077+
}
10011078

10021079
// Verify effective GID actually changed (defense-in-depth, CWE-250 / CERT POS37-C)
10031080
let effective_gid = nix::unistd::getegid();
1004-
if effective_gid != group.gid {
1081+
if effective_gid != target_gid {
10051082
return Err(miette::miette!(
10061083
"Privilege drop verification failed: expected effective GID {}, got {}",
1007-
group.gid,
1084+
target_gid,
10081085
effective_gid
10091086
));
10101087
}
10111088

10121089
#[cfg(target_os = "linux")]
10131090
drop_capability_bounding_set()?;
10141091

1015-
if user_name.is_some() {
1016-
nix::unistd::setuid(user.uid).into_diagnostic()?;
1092+
if let Some(_user) = user {
1093+
if target_uid != nix::unistd::geteuid() {
1094+
nix::unistd::setuid(target_uid).into_diagnostic()?;
1095+
}
10171096

10181097
// Verify effective UID actually changed (defense-in-depth, CWE-250 / CERT POS37-C)
10191098
let effective_uid = nix::unistd::geteuid();
1020-
if effective_uid != user.uid {
1099+
if effective_uid != target_uid {
10211100
return Err(miette::miette!(
10221101
"Privilege drop verification failed: expected effective UID {}, got {}",
1023-
user.uid,
1102+
target_uid,
10241103
effective_uid
10251104
));
10261105
}
10271106

10281107
// Verify root cannot be re-acquired (CERT POS37-C hardening).
10291108
// If we dropped from root, setuid(0) must fail; success means privileges
10301109
// were not fully relinquished.
1031-
if nix::unistd::setuid(nix::unistd::Uid::from_raw(0)).is_ok() && user.uid.as_raw() != 0 {
1110+
if nix::unistd::setuid(Uid::from_raw(0)).is_ok()
1111+
&& target_uid.as_raw() != 0
1112+
{
10321113
return Err(miette::miette!(
10331114
"Privilege drop verification failed: process can still re-acquire root (UID 0) \
10341115
after switching to UID {}",
1035-
user.uid
1116+
target_uid
10361117
));
10371118
}
10381119
}
@@ -1601,7 +1682,7 @@ mod tests {
16011682
let current_user = User::from_uid(nix::unistd::geteuid())
16021683
.unwrap()
16031684
.expect("current user entry");
1604-
let restricted_group = Group::from_gid(nix::unistd::Gid::from_raw(0))
1685+
let restricted_group = Group::from_gid(Gid::from_raw(0))
16051686
.unwrap()
16061687
.expect("gid 0 group entry");
16071688
if restricted_group.gid == nix::unistd::getegid() {
@@ -1736,4 +1817,54 @@ mod tests {
17361817
Some(PathBuf::from("/run/spire"))
17371818
);
17381819
}
1820+
1821+
// ---- Numeric UID tests (Phase 2) ----
1822+
1823+
#[test]
1824+
fn drop_privileges_accepts_numeric_uid() {
1825+
// When running as non-root, a numeric UID/GID that matches the
1826+
// current process should succeed without any passwd lookup.
1827+
if nix::unistd::geteuid().is_root() {
1828+
return;
1829+
}
1830+
1831+
let uid_raw = nix::unistd::geteuid().as_raw();
1832+
let gid_raw = nix::unistd::getegid().as_raw();
1833+
1834+
let policy = policy_with_process(ProcessPolicy {
1835+
run_as_user: Some(uid_raw.to_string()),
1836+
run_as_group: Some(gid_raw.to_string()),
1837+
});
1838+
1839+
assert!(
1840+
drop_privileges(&policy).is_ok(),
1841+
"should accept current process UID/GID as numeric strings"
1842+
);
1843+
}
1844+
1845+
#[test]
1846+
fn drop_privileges_numeric_uid_skips_initgroups() {
1847+
// When running as non-root with a numeric user but group matches,
1848+
// initgroups should not be called (guard: target_uid != geteuid()).
1849+
if nix::unistd::geteuid().is_root() {
1850+
return;
1851+
}
1852+
1853+
let current_uid = nix::unistd::geteuid().as_raw();
1854+
1855+
// Use a different group name that exists (the current one).
1856+
let current_group = Group::from_gid(nix::unistd::getegid())
1857+
.expect("should resolve current group")
1858+
.expect("current group should exist");
1859+
1860+
let policy = policy_with_process(ProcessPolicy {
1861+
run_as_user: Some(current_uid.to_string()), // numeric UID, no passwd entry needed
1862+
run_as_group: Some(current_group.name), // name-based group
1863+
});
1864+
1865+
assert!(
1866+
drop_privileges(&policy).is_ok(),
1867+
"should accept numeric UID with name-based group (initgroups guarded)"
1868+
);
1869+
}
17391870
}

0 commit comments

Comments
 (0)