diff --git a/CLAUDE.md b/CLAUDE.md index c9fb7b6..79c96e6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2678,9 +2678,14 @@ Source layout (all under `src/`): since Compose's own `ports:` syntax has none, so a later `-p` resolution picks the service's own sole `extern` network, the same "unqualified `-p`" default an ordinary CLI `-p` already has. An - explicit `--user`/`--group` still isn't wired up for compose services - — every service runs as whatever user its own image declares, same as - `-r/--run`'s own default. + explicit `--user`/`--group`, wired the same way: `user: "user[:group]"` + (`ComposeService::user`/`group`, `compose_file.h` — split on the + *first* `':'`, the exact same way an image's own declared `USER` is + split, `oci_image.cpp`'s own `read_oci_image_config()`) is passed + straight through to `run_mounted_container()`'s own `user`/`group` + parameters; when unset, that function already falls back to the + image's own declared user, the exact same default `-r/--run` has when + `--user` isn't given. 5. `compose_state_file_path()`/`record_compose_services()` — write the compose file's own real, absolute path as a header line (needed because `compose_state_file_path()`'s own *filename* only encodes a diff --git a/TODO.md b/TODO.md index 527cf38..45f1a6b 100644 --- a/TODO.md +++ b/TODO.md @@ -130,3 +130,32 @@ project's own state should stay root-only or move too. Not urgent -- this project's networking is already root-only in every other respect (bridges, iptables, etc.), so this is a narrower, lower-priority hardening pass for later, not a blocker for anything currently in progress. + +## `-x/--exec`/`--kill`'s `resolve_namespace_pid()` can stop one level too shallow when `--user` used priv-drop + +Found while verifying compose's new `user:` support (root, via the scoped +`doas` rule): a session started with `--user` (root, `--unshare-pid` in +effect, the default) has a real process tree of `outer bwrap` -> `bwrap's +own pid-1 supervisor for the new pid namespace` (never itself execs into +anything -- it's what keeps the namespace alive, since the kernel requires +some process to be pid 1) -> the actual target, forked by that supervisor, +which execs `slocker-lite-priv-drop` and then the real command. Confirmed +via `ps -eo pid,ppid,uid,cmd` that the real command genuinely runs as the +resolved uid/gid (priv-drop itself is correct, not the bug) -- but +`resolve_namespace_pid()` (`sandbox_process.{h,cpp}`), used by both +`-x/--exec`'s own default-identity resolution and `--kill`, stops at the +pid-1 supervisor (its namespaces already differ from the true outer pid, +satisfying that function's own single-hop comparison) instead of walking +one level deeper to the real target. Concretely: `-x/--exec -- id` +with no explicit `--user` on a session that resolved a non-root user via +priv-drop incorrectly reports `root` (reads the supervisor's own +still-root `/proc/.../status`), even though the session's real command is +genuinely running as the resolved non-root user the whole time. + +**Not urgent**: `-x/--exec --user ` and `--kill` are unaffected +in practice (`--kill`'s own cgroup/pid-namespace strategies already reach +every process regardless of tree depth; only `-x/--exec`'s *default* +identity guess is off). Needs a real fix to `resolve_namespace_pid()` (or +a documented, narrower workaround) to correctly walk past bwrap's own +pid-1 supervisor when `--unshare-pid` is in effect, rather than assuming +the first namespace-differing child is the real target. diff --git a/src/compose_file.cpp b/src/compose_file.cpp index be0a98b..49b4023 100644 --- a/src/compose_file.cpp +++ b/src/compose_file.cpp @@ -518,6 +518,22 @@ std::optional load_compose_file(const std::filesystem::path& path) service.container_name = std::string(scalar_value(*container_name_node)); } + if (const yaml_node_t* user_node = find_in_mapping(document, *value_node, "user")) { + if (user_node->type != YAML_SCALAR_NODE || scalar_value(*user_node).empty()) { + spdlog::error("compose file {}: service '{}' user must be a non-empty string", path.string(), + service_name); + return std::nullopt; + } + std::string user_spec(scalar_value(*user_node)); + auto colon = user_spec.find(':'); + if (colon == std::string::npos) { + service.user = user_spec; + } else { + service.user = user_spec.substr(0, colon); + service.group = user_spec.substr(colon + 1); + } + } + if (const yaml_node_t* command_node = find_in_mapping(document, *value_node, "command")) { auto command = parse_command(document, *command_node, service_name); if (!command) { diff --git a/src/compose_file.h b/src/compose_file.h index 5e0a75b..6523890 100644 --- a/src/compose_file.h +++ b/src/compose_file.h @@ -52,6 +52,17 @@ struct ComposeService { std::string name; // the mapping key under `services` std::string image; std::optional container_name; + // Parsed from `user: "user[:group]"`, split on the *first* ':' exactly + // the same way an image's own declared `USER` is split + // (oci_image.cpp's own read_oci_image_config()). Both unset when the + // compose file gives no `user:` key at all -- matching `-r/--run`'s + // own default when `--user` isn't given, the image's own declared user + // is used instead (falling back further to root only if the image + // declares none either), handled automatically by + // run_mounted_container() (commands.h) since it already implements + // that exact fallback for `-r/--run` itself. + std::optional user; + std::optional group; // Always normalized to an argv list: Compose's own list form is used // as-is; its scalar shell-string form is wrapped as {"sh", "-c", // }, matching Compose's "run through the image's shell" diff --git a/src/compose_orchestrator.cpp b/src/compose_orchestrator.cpp index b44fa0e..97d97f7 100644 --- a/src/compose_orchestrator.cpp +++ b/src/compose_orchestrator.cpp @@ -289,7 +289,7 @@ std::vector start_compose_services(const ComposeFile& com // any other service, which is why the only way out of this block // below is an explicit _exit(), never `continue`. int exit_code = run_mounted_container(image_tar, mounted, container_name, service.command, use_nsenter, - std::nullopt, std::nullopt, dns_hostname, volume_specs, + service.user, service.group, dns_hostname, volume_specs, service.environment_specs, true, network_specs, port_forward_specs, false, config); _exit(exit_code < 0 ? 1 : exit_code); diff --git a/src/compose_orchestrator.h b/src/compose_orchestrator.h index 54d9b28..26cba6c 100644 --- a/src/compose_orchestrator.h +++ b/src/compose_orchestrator.h @@ -154,9 +154,11 @@ struct ComposeStartedService { // never a network: prefix (Compose's own ports: syntax has no such // qualifier), so a later -p resolution picks the service's own sole // `extern` network, the same "unqualified -p" default an ordinary CLI -// `-p` already has. An explicit --user/--group isn't wired up for compose -// services yet, though -- every service still runs as whatever user its -// own image declares, same as -r/--run's own default. +// `-p` already has. `service.user`/`service.group` (ComposeService, parsed +// from a `user: "user[:group]"` key) are passed straight through to +// run_mounted_container()'s own `user`/`group` parameters -- when unset, +// that function already falls back to the image's own declared user, the +// exact same default `-r/--run` itself has when `--user` isn't given. // // Returns one ComposeStartedService per service that actually started -- // fewer than compose.services on any partial failure (already logged diff --git a/tests/unit/test_compose_file.cpp b/tests/unit/test_compose_file.cpp index cf77d60..ff13d8b 100644 --- a/tests/unit/test_compose_file.cpp +++ b/tests/unit/test_compose_file.cpp @@ -125,6 +125,50 @@ TEST_CASE("compose file: container_name -- optional, and duplicates across servi CHECK_FALSE(load_compose_file(colliding_with_name).has_value()); } +TEST_CASE("compose file: user -- optional, with and without a group, split the same way as an image's own USER", + "[unit]") { + ScratchXdgDirs scratch; + + auto unset = write_compose(scratch.path(), + "services:\n" + " web:\n" + " image: busybox:latest\n"); + auto loaded_unset = load_compose_file(unset); + REQUIRE(loaded_unset.has_value()); + CHECK_FALSE(loaded_unset->services[0].user.has_value()); + CHECK_FALSE(loaded_unset->services[0].group.has_value()); + + auto user_only = write_compose(scratch.path(), + "services:\n" + " web:\n" + " image: busybox:latest\n" + " user: \"1000\"\n"); + auto loaded_user_only = load_compose_file(user_only); + REQUIRE(loaded_user_only.has_value()); + REQUIRE(loaded_user_only->services[0].user.has_value()); + CHECK(*loaded_user_only->services[0].user == "1000"); + CHECK_FALSE(loaded_user_only->services[0].group.has_value()); + + auto user_and_group = write_compose(scratch.path(), + "services:\n" + " web:\n" + " image: busybox:latest\n" + " user: \"1000:1000\"\n"); + auto loaded_both = load_compose_file(user_and_group); + REQUIRE(loaded_both.has_value()); + REQUIRE(loaded_both->services[0].user.has_value()); + CHECK(*loaded_both->services[0].user == "1000"); + REQUIRE(loaded_both->services[0].group.has_value()); + CHECK(*loaded_both->services[0].group == "1000"); + + auto empty_user = write_compose(scratch.path(), + "services:\n" + " web:\n" + " image: busybox:latest\n" + " user: \"\"\n"); + CHECK_FALSE(load_compose_file(empty_user).has_value()); +} + TEST_CASE("compose file: command -- list form used as-is, scalar form wrapped in sh -c", "[unit]") { ScratchXdgDirs scratch;