diff --git a/docs/experimental-systemd-vm-processes.md b/docs/experimental-systemd-vm-processes.md index e2a7549c1..4fd693426 100644 --- a/docs/experimental-systemd-vm-processes.md +++ b/docs/experimental-systemd-vm-processes.md @@ -57,8 +57,20 @@ KillSignal=SIGTERM SendSIGKILL=yes TimeoutStopSec= Restart=no +User= # when cvm.user is set +OpenFile=/dev/tapN # one entry per NIC with networking.open_file ``` +`User=` replaces the Supervisor `sudo -u` path. systemd opens each `OpenFile=` +path with the manager's privileges and hands the descriptors to the service in +declaration order starting at fd 3, which is what QEMU's generated +`-netdev tap,id=netN,fd=M` arguments expect. That combination needs systemd +253 or newer. + +When both are set, the chardev stays root-owned on the host and the unit still +runs QEMU unprivileged. For software-TPM VMs the whole launcher unit runs as +`cvm.user`, so the VMM chowns the swtpm state directory before start. + The existing launcher remains responsible for swtpm readiness and graceful child shutdown. systemd owns the final cgroup lifetime. A stop request is submitted asynchronously so the VMM can report a VM as stopping while QEMU is @@ -88,8 +100,10 @@ atomic property handling and event-driven state updates. ## Limitations -- The host must run systemd with support for `ExitType=cgroup` and - `StandardOutput=append:`. +- The host must run systemd 253+ with support for `OpenFile=`, + `ExitType=cgroup`, and `StandardOutput=append:`. +- `networking.open_file` is manifest-only, requires `mode = "custom"`, and is + rejected with Supervisor, one-shot execution, and swtpm-backed VMs. - The VMM must be authorized to create and stop system services. - Transient services inherit the systemd manager environment rather than the VMM environment. Variables in `ProcessConfig.env` are forwarded; unrelated diff --git a/dstack/supervisor/client/src/main.rs b/dstack/supervisor/client/src/main.rs index 16d8c61a5..b076a8ddb 100644 --- a/dstack/supervisor/client/src/main.rs +++ b/dstack/supervisor/client/src/main.rs @@ -74,6 +74,8 @@ async fn main() -> Result<()> { pidfile: String::new(), cid: None, note: String::new(), + user: String::new(), + open_files: Vec::new(), }; print_json(&client.deploy(&config).await?)?; } diff --git a/dstack/supervisor/src/process.rs b/dstack/supervisor/src/process.rs index 94e0c61e5..6286d0087 100644 --- a/dstack/supervisor/src/process.rs +++ b/dstack/supervisor/src/process.rs @@ -46,6 +46,25 @@ pub struct ProcessConfig { pub cid: Option, #[serde(default)] pub note: String, + /// User the process manager runs the process as. + /// + /// Only the VMM's systemd backend implements this, by dropping privileges + /// in the transient unit. Supervisor rejects a config that sets it rather + /// than running the process with its own privileges. Skipped when empty so + /// existing records and requests keep serializing byte-identically. + #[serde(default, skip_serializing_if = "String::is_empty")] + #[builder(default)] + pub user: String, + /// Files the process manager opens before exec and passes to the process + /// as inherited file descriptors, in declaration order starting at fd 3. + /// + /// Only the VMM's systemd backend implements this. Supervisor rejects a + /// config that sets it rather than starting a process without the file + /// descriptors it asked for. Skipped when empty so existing records and + /// requests keep serializing byte-identically. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + #[builder(default)] + pub open_files: Vec, } #[derive(Debug, Clone, Serialize, Deserialize)] diff --git a/dstack/supervisor/src/supervisor.rs b/dstack/supervisor/src/supervisor.rs index 378013c05..18d7c528d 100644 --- a/dstack/supervisor/src/supervisor.rs +++ b/dstack/supervisor/src/supervisor.rs @@ -59,6 +59,18 @@ impl Supervisor { if id.is_empty() { return Err(anyhow::anyhow!("Process ID is empty")); } + if !config.user.is_empty() { + // Supervisor runs processes with its own privileges. Starting the + // process anyway would run a VM as root that asked to be confined + // to an unprivileged user. + bail!("user is not supported by supervisor"); + } + if !config.open_files.is_empty() { + // Supervisor spawns processes without pre-opened file descriptors, + // so honoring the rest of the config would start a process that is + // missing the files it depends on. + bail!("open_files is not supported by supervisor"); + } if self .info(&id) .is_some_and(|info| info.state.status.is_running()) diff --git a/dstack/vmm/Cargo.toml b/dstack/vmm/Cargo.toml index 941802b05..4efbfc1b1 100644 --- a/dstack/vmm/Cargo.toml +++ b/dstack/vmm/Cargo.toml @@ -25,7 +25,7 @@ sha2.workspace = true hex.workspace = true fs-err.workspace = true getrandom = { workspace = true, features = ["std"] } -nix = { workspace = true, features = ["user"] } +nix = { workspace = true, features = ["fs", "user", "dir"] } dirs.workspace = true which.workspace = true clap = { workspace = true, features = ["derive", "string"] } diff --git a/dstack/vmm/src/app.rs b/dstack/vmm/src/app.rs index bc88f0d1e..f57cf0683 100644 --- a/dstack/vmm/src/app.rs +++ b/dstack/vmm/src/app.rs @@ -22,7 +22,7 @@ use dstack_vmm_rpc::{ use fs_err as fs; use guest_api::client::DefaultClient as GuestClient; use id_pool::IdPool; -use nix::unistd::{Uid, User}; +use nix::unistd::Uid; use or_panic::ResultOrPanic; use ra_rpc::client::RaClient; use serde::{Deserialize, Serialize}; @@ -521,11 +521,7 @@ impl App { let qemu_uid = if self.config.cvm.user.is_empty() { Uid::effective().as_raw() } else { - User::from_name(&self.config.cvm.user) - .context("failed to resolve QEMU user")? - .with_context(|| format!("QEMU user {} does not exist", self.config.cvm.user))? - .uid - .as_raw() + qemu::resolve_cvm_user(&self.config.cvm.user)?.uid.as_raw() }; let mut prepared = Vec::new(); for (nic_index, network) in networks.iter().enumerate() { @@ -2166,6 +2162,7 @@ mod tests { dhcp_start: String::new(), restrict: false, netdev: String::new(), + open_file: String::new(), }]; workdir.put_manifest(&manifest)?; @@ -2428,6 +2425,7 @@ mod tests { dhcp_start: String::new(), restrict: false, netdev: String::new(), + open_file: String::new(), }]; let user_manifest = test_manifest(2048); let image = test_tdx_image(true); diff --git a/dstack/vmm/src/app/network.rs b/dstack/vmm/src/app/network.rs index 87925d9a6..e19d4942a 100644 --- a/dstack/vmm/src/app/network.rs +++ b/dstack/vmm/src/app/network.rs @@ -10,7 +10,9 @@ use anyhow::{bail, Result}; use sha2::{Digest, Sha256}; use super::Manifest; -use crate::config::{CvmConfig, Networking, NetworkingMode}; +use crate::config::{ + validate_open_file, CvmConfig, Networking, NetworkingMode, SD_LISTEN_FDS_START, +}; pub(crate) fn resolve_networking(networking: &Networking, cfg: &CvmConfig) -> Networking { let mut resolved = cfg.networking.clone(); @@ -28,9 +30,21 @@ pub(crate) fn resolve_networking(networking: &Networking, cfg: &CvmConfig) -> Ne if !networking.dhcp_start.is_empty() { resolved.dhcp_start = networking.dhcp_start.clone(); } - if !networking.netdev.is_empty() { + // A pre-opened chardev names one host device and belongs to exactly one + // NIC, so it is taken from the NIC verbatim and never inherited from the + // host defaults. + resolved.open_file = networking.open_file.clone(); + if !networking.open_file.is_empty() && networking.netdev.is_empty() { + // The chardev generates this NIC's netdev from the fd number later, so + // drop any inherited host netdev rather than leak a stale default. + resolved.netdev = String::new(); + } else if !networking.netdev.is_empty() { + // An explicit NIC netdev overrides the host default. A netdev set + // together with open_file is kept here, not dropped, so that + // validate_resolved_network rejects the conflicting pair. resolved.netdev = networking.netdev.clone(); } + // Neither set: inherit the host netdev unchanged. resolved } @@ -47,6 +61,17 @@ pub(crate) fn resolved_networks(manifest: &Manifest, cfg: &CvmConfig) -> Vec Result<()> { + if !networking.open_file.is_empty() { + validate_open_file("networking.open_file", &networking.open_file)?; + if networking.mode != NetworkingMode::Custom { + bail!("networking.open_file requires mode = \"custom\""); + } + if !networking.netdev.is_empty() { + // The netdev string is generated from the inherited fd number, + // which only the process manager knows. + bail!("networking.open_file and networking.netdev are mutually exclusive"); + } + } if networking.mode != NetworkingMode::Bridge { return Ok(()); } @@ -69,6 +94,32 @@ pub(crate) fn validate_resolved_networks(networks: &[Networking]) -> Result<()> Ok(()) } +/// Chardev paths the process manager must open before exec, in NIC order. +/// +/// The order is the contract: systemd hands the files to the service in +/// declaration order, so entry `i` of this list arrives as fd +/// `SD_LISTEN_FDS_START + i`. +pub(crate) fn open_files(networks: &[Networking]) -> Vec { + networks + .iter() + .filter(|networking| !networking.open_file.is_empty()) + .map(|networking| networking.open_file.clone()) + .collect() +} + +/// File descriptor the NIC at `index` receives, or `None` if it does not use a +/// pre-opened chardev. +pub(crate) fn open_file_fd(networks: &[Networking], index: usize) -> Option { + if networks.get(index)?.open_file.is_empty() { + return None; + } + let preceding = networks[..index] + .iter() + .filter(|networking| !networking.open_file.is_empty()) + .count(); + Some(SD_LISTEN_FDS_START + preceding as u32) +} + /// Derives a deterministic, locally administered unicast MAC address. /// /// Index zero preserves the legacy single-NIC derivation. Later interfaces @@ -95,7 +146,84 @@ pub(crate) fn mac_address_for_vm_index(vm_id: &str, prefix: &[u8], index: usize) #[cfg(test)] mod tests { - use super::mac_address_for_vm_index; + use super::{ + mac_address_for_vm_index, open_file_fd, open_files, validate_resolved_network, Networking, + NetworkingMode, + }; + + fn open_file_network(path: &str) -> Networking { + Networking { + mode: NetworkingMode::Custom, + bridge: String::new(), + mac_prefix: String::new(), + net: String::new(), + dhcp_start: String::new(), + restrict: false, + netdev: String::new(), + open_file: path.into(), + } + } + + #[test] + fn open_file_descriptors_are_numbered_in_nic_order() { + let mut networks = vec![ + open_file_network(""), + open_file_network("/dev/tap10"), + open_file_network(""), + open_file_network("/dev/tap11"), + ]; + networks[0].mode = NetworkingMode::User; + networks[2].mode = NetworkingMode::Bridge; + + assert_eq!(open_files(&networks), ["/dev/tap10", "/dev/tap11"]); + assert_eq!(open_file_fd(&networks, 0), None); + assert_eq!(open_file_fd(&networks, 1), Some(3)); + assert_eq!(open_file_fd(&networks, 2), None); + assert_eq!(open_file_fd(&networks, 3), Some(4)); + assert_eq!(open_file_fd(&networks, 4), None); + } + + #[test] + fn open_file_networks_are_validated() { + validate_resolved_network(&open_file_network("/dev/tap7498")).unwrap(); + + for path in [ + "dev/tap7498", + "/dev/tap 7498", + "/dev/tap7498:foo", + "/dev/tap7498,vhost=on", + "/dev/%i/tap7498", + ] { + validate_resolved_network(&open_file_network(path)).unwrap_err(); + } + + let mut wrong_mode = open_file_network("/dev/tap7498"); + wrong_mode.mode = NetworkingMode::Bridge; + wrong_mode.bridge = "br0".into(); + validate_resolved_network(&wrong_mode).unwrap_err(); + + let mut with_netdev = open_file_network("/dev/tap7498"); + with_netdev.netdev = "tap,id=net0,fd=3".into(); + validate_resolved_network(&with_netdev).unwrap_err(); + } + + #[test] + fn open_file_does_not_inherit_host_custom_netdev() { + use rocket::figment::{providers::Format, providers::Toml, Figment}; + + let mut cfg: crate::config::Config = + Figment::from(Toml::string(crate::config::DEFAULT_CONFIG)) + .extract() + .unwrap(); + cfg.cvm.networking.mode = NetworkingMode::Custom; + cfg.cvm.networking.netdev = "tap,id=net0,ifname=legacy,script=no".into(); + + let nic = open_file_network("/dev/tap7498"); + let resolved = super::resolve_networking(&nic, &cfg.cvm); + assert_eq!(resolved.open_file, "/dev/tap7498"); + assert!(resolved.netdev.is_empty()); + validate_resolved_network(&resolved).unwrap(); + } #[test] fn primary_mac_keeps_legacy_derivation_and_later_nics_are_distinct() { diff --git a/dstack/vmm/src/app/qemu.rs b/dstack/vmm/src/app/qemu.rs index 67115c0fe..7198b8796 100644 --- a/dstack/vmm/src/app/qemu.rs +++ b/dstack/vmm/src/app/qemu.rs @@ -9,13 +9,17 @@ use super::{ hugepage_numa_nodes, image::Image, mr_config::{snp_host_data, tdx_mr_config_id}, - network::{mac_address_for_vm_index, resolved_networks, validate_resolved_networks}, + network::{ + mac_address_for_vm_index, open_file_fd, open_files, resolved_networks, + validate_resolved_networks, + }, pci_numa_node, round_up, GpuConfig, VmWorkDir, }; use crate::{ app::Manifest, config::{ - CvmConfig, CvmPlatform, NetworkFilterMode, Networking, NetworkingMode, ProcessAnnotation, + parse_unit_user, CvmConfig, CvmPlatform, NetworkFilterMode, Networking, NetworkingMode, + ProcessAnnotation, ProcessManagerBackend, UnitUser, }, netd::{tap_name, InterfaceIdentity}, vm_launcher::{ChildCommand, LaunchSpec}, @@ -24,9 +28,14 @@ use anyhow::{bail, Context, Result}; use bon::Builder; use dstack_types::shared_filenames::HOST_SHARED_DISK_LABEL; use fs_err as fs; -use nix::unistd::User; +use nix::dir::Dir; +use nix::errno::Errno; +use nix::fcntl::{openat, AtFlags, OFlag}; +use nix::sys::stat::Mode; +use nix::unistd::{fchownat, Uid, User}; use serde::Serialize; use std::collections::HashMap; +use std::os::fd::{AsRawFd, RawFd}; use std::os::unix::fs::PermissionsExt; use std::{ fs::Permissions, @@ -242,6 +251,14 @@ impl PreparedQemuLaunch { .context("tpm key provider requested but swtpm is not installed")?; let state_dir = workdir.swtpm_state_dir(); fs::create_dir_all(&state_dir).context("failed to create swtpm state directory")?; + // systemd drops privileges for the whole launcher unit, including + // swtpm. Hand the state directory over so socket creation and TPM + // state updates are not denied on a root-owned path. Existing + // files from earlier root-owned boots are included. + if !cfg.user.is_empty() && cfg.pm != ProcessManagerBackend::Supervisor { + let user = resolve_cvm_user(&cfg.user)?; + chown_tree_to_user(&state_dir, &user)?; + } let socket = workdir.swtpm_socket(); if socket.exists() { fs::remove_file(&socket).context("failed to remove stale swtpm socket")?; @@ -308,6 +325,95 @@ fn prepare_data_disk(vm: &VmConfig, workdir: &VmWorkDir, cfg: &CvmConfig) -> Res Ok(()) } +pub(crate) fn resolve_cvm_user(user: &str) -> Result { + match parse_unit_user("cvm.user", user)? { + UnitUser::Name(name) => User::from_name(&name) + .context("failed to resolve QEMU user")? + .with_context(|| format!("QEMU user {name} does not exist")), + UnitUser::Uid(uid) => User::from_uid(Uid::from_raw(uid)) + .context("failed to resolve QEMU user")? + .with_context(|| format!("QEMU user uid {uid} does not exist")), + } +} + +const DIR_OFLAGS: OFlag = OFlag::O_RDONLY + .union(OFlag::O_NOFOLLOW) + .union(OFlag::O_DIRECTORY) + .union(OFlag::O_CLOEXEC); + +/// Makes `path` and its contents owned by the unprivileged VM user. +/// +/// Under systemd the transient unit drops privileges before exec, so paths the +/// VMM created as root must be handed over before launch. Supervisor keeps +/// root for the launcher/swtpm path and only sudo's QEMU, so it does not need +/// this. +/// +/// The walk never follows a symlink: every entry is chowned and every descent +/// happens relative to an `O_NOFOLLOW`-opened directory fd. The state directory +/// is writable by the unprivileged user between boots, so a symlink planted +/// there must not be able to redirect a root chown onto an arbitrary host path +/// (CWE-59). +fn chown_tree_to_user(path: &Path, user: &User) -> Result<()> { + // Chown the top path itself without following a symlink. + fchownat( + None, + path, + Some(user.uid), + Some(user.gid), + AtFlags::AT_SYMLINK_NOFOLLOW, + ) + .with_context(|| format!("failed to chown {}", path.display()))?; + + // Open the directory itself without following symlinks. A non-directory or + // a symlink has nothing to descend into and was already chowned above. + let dir_fd = match openat(None, path, DIR_OFLAGS, Mode::empty()) { + Ok(fd) => fd, + Err(Errno::ENOTDIR | Errno::ELOOP) => return Ok(()), + Err(err) => return Err(err).with_context(|| format!("failed to open {}", path.display())), + }; + chown_dir_contents(dir_fd, path, user) +} + +/// Chowns every entry reachable through `dir_fd`, taking ownership of it (the +/// fd is closed when the `Dir` drops). Every chown and descent is performed +/// relative to the trusted fd rather than by re-resolving a path, so a symlink +/// anywhere below `path` cannot redirect the walk outside the tree. +fn chown_dir_contents(dir_fd: RawFd, path: &Path, user: &User) -> Result<()> { + let mut dir = Dir::from_fd(dir_fd) + .with_context(|| format!("failed to read directory {}", path.display()))?; + let raw = dir.as_raw_fd(); + for entry in dir.iter() { + let entry = + entry.with_context(|| format!("failed to read entry under {}", path.display()))?; + let name = entry.file_name(); + let bytes = name.to_bytes(); + if bytes == b"." || bytes == b".." { + continue; + } + // Chown the entry relative to the trusted dir fd, never following a + // symlink, so a planted link cannot redirect the chown to its target. + fchownat( + Some(raw), + name, + Some(user.uid), + Some(user.gid), + AtFlags::AT_SYMLINK_NOFOLLOW, + ) + .with_context(|| format!("failed to chown entry under {}", path.display()))?; + // Descend only into a real subdirectory, opened without following + // symlinks. ELOOP means the entry is a symlink; ENOTDIR a regular file. + match openat(Some(raw), name, DIR_OFLAGS, Mode::empty()) { + Ok(child) => chown_dir_contents(child, path, user)?, + Err(Errno::ENOTDIR | Errno::ELOOP) => {} + Err(err) => { + return Err(err) + .with_context(|| format!("failed to open entry under {}", path.display())) + } + } + } + Ok(()) +} + fn prepare_shared_dir(workdir: &VmWorkDir) -> Result<()> { let shared_dir = workdir.shared_dir(); if !shared_dir.exists() { @@ -354,6 +460,14 @@ impl VmConfig { let Some(socket) = prepared.swtpm_socket.as_deref() else { return Ok(vec![process]); }; + if !process.open_files.is_empty() { + // The swtpm path puts vm-launcher between the process manager and + // QEMU. vm-launcher would inherit the descriptors and leak them + // into swtpm as well, and nothing keeps their numbers stable + // across the launcher's own file operations, so QEMU could be + // handed an unrelated fd. Reject instead of guessing. + bail!("networking.open_file is not supported for VMs that use swtpm"); + } let swtpm_path = prepared .swtpm_path .as_ref() @@ -361,9 +475,7 @@ impl VmConfig { let (socket_uid, socket_gid) = if cfg.user.is_empty() { (unsafe { libc::geteuid() }, unsafe { libc::getegid() }) } else { - let user = User::from_name(&cfg.user) - .context("failed to resolve QEMU user")? - .with_context(|| format!("QEMU user {} does not exist", cfg.user))?; + let user = resolve_cvm_user(&cfg.user)?; (user.uid.as_raw(), user.gid.as_raw()) }; @@ -414,6 +526,12 @@ impl VmConfig { pidfile: process.pidfile, cid: process.cid, note: process.note, + // The launcher unit owns the privilege drop, so vm-launcher and + // the swtpm and QEMU children it spawns all run as this user. + user: process.user, + // Rejected above: file descriptor passing does not survive the + // vm-launcher indirection. + open_files: Vec::new(), }; Ok(vec![launcher]) } @@ -628,12 +746,18 @@ impl QemuCommandBuilder<'_> { } } NetworkingMode::Custom => { - if !networking.netdev.contains(&format!("id={net_id}")) { - bail!( - "custom networking netdev must contain id={net_id} for interface index {index}" - ); + if let Some(fd) = open_file_fd(&self.prepared.networks, index) { + // The chardev is opened by the process manager, so the + // fd number is the only handle QEMU gets. + format!("tap,id={net_id},fd={fd}") + } else { + if !networking.netdev.contains(&format!("id={net_id}")) { + bail!( + "custom networking netdev must contain id={net_id} for interface index {index}" + ); + } + networking.netdev.clone() } - networking.netdev.clone() } }; command.arg("-netdev").arg(netdev); @@ -781,6 +905,7 @@ impl QemuCommandBuilder<'_> { fn process_config(&self, command: Command) -> Result { let workdir = &self.prepared.workdir; + let open_files = open_files(&self.prepared.networks); let mut arguments = vec![self.cfg.qemu_path.to_string_lossy().to_string()]; arguments.extend( command @@ -790,11 +915,29 @@ impl QemuCommandBuilder<'_> { if let Some(cpus) = &self.prepared.numa_cpus { arguments.splice(0..0, ["taskset", "-c", cpus].into_iter().map(String::from)); } + // The systemd backend drops privileges in the unit itself, so QEMU is + // exec'd directly. Supervisor has no such mechanism and keeps the sudo + // prefix, which is also why it cannot pass file descriptors: sudo + // closes every descriptor above stderr before exec, so QEMU would be + // told to use an fd that no longer exists. + let mut user = String::new(); if !self.cfg.user.is_empty() { - arguments.splice( - 0..0, - ["sudo", "-u", &self.cfg.user].into_iter().map(String::from), - ); + let unit_user = parse_unit_user("cvm.user", &self.cfg.user)?; + if self.cfg.pm == ProcessManagerBackend::Supervisor { + if !open_files.is_empty() { + bail!( + "networking.open_file requires cvm.pm = \"systemd\" or \"auto\" when cvm.user is set: sudo closes inherited file descriptors" + ); + } + let sudo_user = unit_user.sudo_value(); + arguments.splice( + 0..0, + ["sudo", "-u", &sudo_user].into_iter().map(String::from), + ); + } else { + // systemd User= takes a bare name or decimal UID, not sudo's #UID. + user = unit_user.systemd_value(); + } } let command = arguments.remove(0); @@ -819,6 +962,8 @@ impl QemuCommandBuilder<'_> { pidfile: workdir.pid_file().to_string_lossy().to_string(), cid: Some(self.vm.cid), note, + user, + open_files, }) } } @@ -1007,7 +1152,8 @@ mod tests { use crate::app::image::{Image, ImageInfo}; use crate::app::{needs_swtpm, GpuConfig, Manifest, PortMapping, VmVolume, VmWorkDir}; use crate::config::{ - Config, CvmPlatform, NetworkFilterMode, NetworkingMode, Protocol, DEFAULT_CONFIG, + Config, CvmPlatform, NetworkFilterMode, NetworkingMode, ProcessManagerBackend, Protocol, + DEFAULT_CONFIG, }; use crate::netd::{tap_name, InterfaceIdentity}; use dstack_types::{KeyProviderKind, TeeVariant}; @@ -1265,5 +1411,160 @@ mod tests { .args .windows(2) .any(|args| args == ["-tpmdev", "emulator,id=tpm0,chardev=chrtpm"])); + + // Pre-opened chardevs. The first NIC keeps user networking, so the + // two NICs that ask for a chardev take the first two descriptors + // systemd hands over. + prepared.swtpm_socket = None; + prepared.networks.push(config.cvm.networking.clone()); + prepared.networks[0].mode = NetworkingMode::User; + for (index, path) in [(1, "/dev/tap7498"), (2, "/dev/tap7499")] { + let networking = &mut prepared.networks[index]; + networking.mode = NetworkingMode::Custom; + networking.bridge = String::new(); + networking.netdev = String::new(); + networking.open_file = path.into(); + } + let process = QemuCommandBuilder { + vm: &vm, + cfg: &config.cvm, + gpus: &GpuConfig::default(), + prepared: &prepared, + } + .build() + .unwrap(); + assert!(process + .args + .windows(2) + .any(|args| args == ["-netdev", "tap,id=net1,fd=3"])); + assert!(process + .args + .windows(2) + .any(|args| args == ["-netdev", "tap,id=net2,fd=4"])); + assert_eq!(process.open_files, ["/dev/tap7498", "/dev/tap7499"]); + + // systemd drops privileges in the unit, so QEMU is exec'd directly and + // keeps the descriptors it was handed. + let mut systemd_config = config.clone(); + systemd_config.cvm.pm = ProcessManagerBackend::Systemd; + systemd_config.cvm.user = "qemu".into(); + let process = QemuCommandBuilder { + vm: &vm, + cfg: &systemd_config.cvm, + gpus: &GpuConfig::default(), + prepared: &prepared, + } + .build() + .unwrap(); + assert_eq!(process.user, "qemu"); + assert_eq!(process.command, "/not-installed/qemu-system-x86_64"); + assert!(!process.args.iter().any(|arg| arg == "sudo")); + + // Supervisor has no privilege-drop mechanism and falls back to sudo, + // which closes the descriptors before QEMU starts. + let mut sudo_config = config.clone(); + sudo_config.cvm.user = "qemu".into(); + let error = QemuCommandBuilder { + vm: &vm, + cfg: &sudo_config.cvm, + gpus: &GpuConfig::default(), + prepared: &prepared, + } + .build() + .unwrap_err(); + assert!(error.to_string().contains("cvm.pm"), "{error:#}"); + + // Without a pre-opened chardev, Supervisor keeps the sudo prefix. + for networking in &mut prepared.networks { + networking.open_file = String::new(); + networking.mode = NetworkingMode::User; + } + let process = QemuCommandBuilder { + vm: &vm, + cfg: &sudo_config.cvm, + gpus: &GpuConfig::default(), + prepared: &prepared, + } + .build() + .unwrap(); + assert_eq!(process.command, "sudo"); + assert_eq!(&process.args[..2], ["-u", "qemu"]); + assert!(process.user.is_empty()); + + // Numeric UIDs keep sudo's #UID form and systemd's bare digits. + sudo_config.cvm.user = "#1000".into(); + let process = QemuCommandBuilder { + vm: &vm, + cfg: &sudo_config.cvm, + gpus: &GpuConfig::default(), + prepared: &prepared, + } + .build() + .unwrap(); + assert_eq!(process.command, "sudo"); + assert_eq!(&process.args[..2], ["-u", "#1000"]); + + let mut uid_config = config.clone(); + uid_config.cvm.pm = ProcessManagerBackend::Systemd; + uid_config.cvm.user = "#1000".into(); + let process = QemuCommandBuilder { + vm: &vm, + cfg: &uid_config.cvm, + gpus: &GpuConfig::default(), + prepared: &prepared, + } + .build() + .unwrap(); + assert_eq!(process.user, "1000"); + assert!(!process.args.iter().any(|arg| arg == "sudo")); + } + + fn chown_test_user() -> nix::unistd::User { + // The swtpm chown must succeed against a real account. Targeting the + // current uid keeps the chown a permitted no-op whether the suite runs + // as root or unprivileged, so the tests below assert traversal shape, + // not privilege. + nix::unistd::User::from_uid(nix::unistd::Uid::current()) + .unwrap() + .expect("current uid resolves to a user") + } + + #[test] + fn chown_tree_does_not_follow_a_symlink() { + // A symlink whose target does not exist must be chowned as the link + // itself. Following it would chase the missing target and fail — the + // exact primitive that let a planted link redirect a root chown onto + // an arbitrary host path. + let dir = tempfile::tempdir().unwrap(); + std::os::unix::fs::symlink("/nonexistent/dstack-chown-victim", dir.path().join("link")) + .unwrap(); + super::chown_tree_to_user(dir.path(), &chown_test_user()) + .expect("must chown the symlink itself, not follow it"); + } + + #[test] + fn chown_tree_does_not_descend_into_a_symlinked_dir() { + // A symlink to a directory must not be walked into: the pointed-to + // directory holds a dangling link, so descending would chase it and + // fail. + let dir = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + std::os::unix::fs::symlink("/nonexistent/inner-victim", outside.path().join("inner")) + .unwrap(); + std::os::unix::fs::symlink(outside.path(), dir.path().join("dirlink")).unwrap(); + super::chown_tree_to_user(dir.path(), &chown_test_user()) + .expect("must not descend into a symlinked directory"); + } + + #[test] + fn chown_tree_still_walks_a_real_tree() { + // Regression guard: the hardening must keep chowning real nested + // entries rather than stop at the top directory. + let dir = tempfile::tempdir().unwrap(); + std::fs::create_dir(dir.path().join("sub")).unwrap(); + std::fs::write(dir.path().join("sub/file"), b"x").unwrap(); + std::fs::write(dir.path().join("top"), b"y").unwrap(); + super::chown_tree_to_user(dir.path(), &chown_test_user()) + .expect("must chown a normal tree"); } } diff --git a/dstack/vmm/src/config.rs b/dstack/vmm/src/config.rs index f3ca78d3b..63a95068a 100644 --- a/dstack/vmm/src/config.rs +++ b/dstack/vmm/src/config.rs @@ -333,7 +333,10 @@ pub struct CvmConfig { pub qmp_socket: bool, /// GPU configuration pub gpu: GpuConfig, - /// Use sudo to run the VM + /// User the VM process runs as. Empty keeps the VMM's own privileges. + /// Supervisor prefixes QEMU with `sudo -u`; systemd sets `User=` on the + /// transient unit. Accepts a POSIX user name, a numeric UID, or sudo's + /// `#UID` form. pub user: String, /// Auto restart configuration @@ -727,6 +730,9 @@ impl Config { } validate_networking(&self.cvm.networking)?; + if !self.cvm.user.is_empty() { + validate_unit_user("cvm.user", &self.cvm.user)?; + } if self.cvm.pm != ProcessManagerBackend::Systemd { anyhow::ensure!( !self.supervisor.sock.trim().is_empty(), @@ -798,6 +804,13 @@ fn validate_networking(networking: &Networking) -> Result<()> { "cvm.networking.mac_prefix must contain 1 to 3 two-digit hexadecimal bytes" ); } + // A pre-opened chardev names one specific host device, so it belongs to a + // single VM NIC. Inheriting it as a host-wide default would attach every + // VM to the same tap device. + anyhow::ensure!( + networking.open_file.is_empty(), + "cvm.networking.open_file must be set per VM NIC, not as a host-wide default" + ); match networking.mode { NetworkingMode::Bridge => anyhow::ensure!( !networking.bridge.trim().is_empty(), @@ -812,6 +825,96 @@ fn validate_networking(networking: &Networking) -> Result<()> { Ok(()) } +/// First file descriptor systemd hands to a service, per the LISTEN_FDS +/// convention shared by socket activation and `OpenFile=`. +pub(crate) const SD_LISTEN_FDS_START: u32 = 3; + +/// A `cvm.user` value after syntax checks, before looking the account up. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum UnitUser { + /// POSIX user name for `User=` / `sudo -u`. + Name(String), + /// Numeric UID. systemd takes the bare digits; sudo needs `#UID`. + Uid(u32), +} + +impl UnitUser { + /// Value for a systemd `User=` property. + pub(crate) fn systemd_value(&self) -> String { + match self { + Self::Name(name) => name.clone(), + Self::Uid(uid) => uid.to_string(), + } + } + + /// Value for `sudo -u`. + pub(crate) fn sudo_value(&self) -> String { + match self { + Self::Name(name) => name.clone(), + Self::Uid(uid) => format!("#{uid}"), + } + } +} + +/// Parses a user name or numeric UID before it reaches sudo or a unit property. +/// +/// Accepts a POSIX user name, a bare decimal UID (systemd `User=`), or sudo's +/// `#UID` form. The charset for names excludes `%` and property separators so +/// the value cannot expand as a systemd specifier or inject extra syntax. +pub(crate) fn parse_unit_user(name: &str, user: &str) -> Result { + if user.is_empty() { + bail!("{name} must not be empty"); + } + if let Some(digits) = user.strip_prefix('#') { + if digits.is_empty() || !digits.bytes().all(|byte| byte.is_ascii_digit()) { + bail!("{name} must be '#' when it starts with '#': {user}"); + } + let uid = digits + .parse::() + .with_context(|| format!("{name} contains an out-of-range uid: {user}"))?; + return Ok(UnitUser::Uid(uid)); + } + if user.bytes().all(|byte| byte.is_ascii_digit()) { + let uid = user + .parse::() + .with_context(|| format!("{name} contains an out-of-range uid: {user}"))?; + return Ok(UnitUser::Uid(uid)); + } + if user.starts_with('-') { + bail!("{name} must not start with '-': {user}"); + } + if !user + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b'-' | b'.')) + { + bail!("{name} must contain only alphanumerics, '_', '-' and '.': {user}"); + } + Ok(UnitUser::Name(user.to_string())) +} + +pub(crate) fn validate_unit_user(name: &str, user: &str) -> Result<()> { + parse_unit_user(name, user).map(|_| ()) +} + +/// Validates an `open_file` path before it reaches a systemd unit property. +/// +/// systemd parses `OpenFile=` as `path:fdname:options` and expands `%` +/// specifiers, so those characters would change the meaning of the property +/// rather than name a device. The check is deliberately conservative: the only +/// intended values are host device nodes such as `/dev/tap7498`. +pub(crate) fn validate_open_file(name: &str, path: &str) -> Result<()> { + if !path.starts_with('/') { + bail!("{name} must be an absolute path: {path}"); + } + if !path + .bytes() + .all(|byte| byte.is_ascii_graphic() && !matches!(byte, b':' | b',' | b'%' | b'\\')) + { + bail!("{name} must not contain whitespace or any of ':' ',' '%' '\\': {path}"); + } + Ok(()) +} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize, Serialize)] #[serde(rename_all = "lowercase")] pub enum NetworkingMode { @@ -849,6 +952,19 @@ pub struct Networking { // ── Custom fields ────────────────────────────────────────────── #[serde(default)] pub netdev: String, + + // ── Pre-opened chardev ───────────────────────────────────────── + /// Absolute path to an already existing tap character device, e.g. + /// `/dev/tap7498` for a macvtap interface created by an external net + /// daemon. The process manager opens it before exec and QEMU inherits it + /// as a file descriptor, so the netdev becomes `tap,id=netN,fd=M`. + /// + /// Only the systemd process manager can pass file descriptors, so this is + /// rejected on every other launch path instead of being silently dropped: + /// QEMU would otherwise open an unrelated fd and attach the guest to the + /// wrong network. + #[serde(default, skip_serializing_if = "String::is_empty")] + pub open_file: String, } impl Networking { @@ -1157,6 +1273,78 @@ mod tests { assert_eq!(parse("auto"), ProcessManagerBackend::Auto); } + #[test] + fn host_wide_open_file_is_rejected() { + let mut config = default_config(); + config.cvm.networking.open_file = "/dev/tap7498".into(); + assert!(config + .validate() + .unwrap_err() + .to_string() + .contains("open_file")); + } + + #[test] + fn unit_user_names_are_validated() { + validate_unit_user("cvm.user", "qemu-1.user_x").unwrap(); + assert_eq!( + parse_unit_user("cvm.user", "#1000").unwrap(), + UnitUser::Uid(1000) + ); + assert_eq!( + parse_unit_user("cvm.user", "1000").unwrap(), + UnitUser::Uid(1000) + ); + assert_eq!( + parse_unit_user("cvm.user", "#1000").unwrap().sudo_value(), + "#1000" + ); + assert_eq!( + parse_unit_user("cvm.user", "#1000") + .unwrap() + .systemd_value(), + "1000" + ); + for user in [ + "", + "#", + "#-1", + "-qemu", + "qemu:0", + "qemu user", + "%i", + "qemu$", + ] { + validate_unit_user("cvm.user", user).unwrap_err(); + } + + let mut config = default_config(); + config.cvm.user = "qemu:0".into(); + assert!(config + .validate() + .unwrap_err() + .to_string() + .contains("cvm.user")); + config.cvm.user = "#1000".into(); + config.validate().unwrap(); + } + + #[test] + fn empty_open_file_is_omitted_from_json() { + let networking = Networking { + mode: NetworkingMode::User, + bridge: String::new(), + mac_prefix: String::new(), + net: String::new(), + dhcp_start: String::new(), + restrict: false, + netdev: String::new(), + open_file: String::new(), + }; + let value = serde_json::to_value(&networking).unwrap(); + assert!(value.get("open_file").is_none()); + } + #[test] fn config_validation_rejects_invalid_static_invariants() { let mut config = default_config(); diff --git a/dstack/vmm/src/main_service.rs b/dstack/vmm/src/main_service.rs index ef52eed4c..d8daedfe0 100644 --- a/dstack/vmm/src/main_service.rs +++ b/dstack/vmm/src/main_service.rs @@ -370,6 +370,9 @@ fn networking_from_proto(proto: &rpc::NetworkingConfig) -> Result Result<()> { match self { - Self::Supervisor(client) => client.deploy(config).await, + Self::Supervisor(client) => { + ensure_supervisor_supported(config)?; + client.deploy(config).await + } Self::Systemd(manager) => manager.deploy(config).await, Self::Auto(manager) => manager.deploy(config).await, } @@ -101,6 +106,27 @@ impl ProcessManager { } } +/// Rejects a process asking for something Supervisor cannot provide. +/// +/// Supervisor spawns processes with its own privileges and without pre-opened +/// file descriptors. Launching anyway would run a VM as root that asked to be +/// confined, or leave QEMU pointing at whatever the fd number happens to be, +/// so this fails before anything is spawned. +fn ensure_supervisor_supported(config: &ProcessConfig) -> Result<()> { + for (what, unsupported) in [ + ("pre-opened files", !config.open_files.is_empty()), + ("a dedicated user", !config.user.is_empty()), + ] { + if unsupported { + bail!( + "process {} requires {what}, which only the systemd process manager supports; set cvm.pm = \"systemd\" or \"auto\"", + config.id + ); + } + } + Ok(()) +} + pub struct AutoProcessManager { systemd: Arc, supervisor: Option, @@ -355,47 +381,76 @@ impl SystemdProcessManager { Ok(output) } - async fn launch(&self, config: &ProcessConfig) -> Result<()> { - let unit = self.unit(&config.id); - // Failed transient units remain loaded until reset and otherwise - // prevent automatic restart from reusing the unit name. - let mut reset = Command::new("systemctl"); - reset.arg("reset-failed").arg(&unit); - let _ = reset.output().await; - let mut command = Command::new("systemd-run"); - command - .arg("--quiet") - .arg("--unit") - .arg(&unit) - .arg("--service-type=exec") - .arg("--property=KillMode=mixed") - .arg("--property=KillSignal=SIGTERM") - .arg("--property=SendSIGKILL=yes") - .arg(format!("--property=TimeoutStopSec={}", self.stop_timeout)) - .arg("--property=ExitType=cgroup") - .arg("--property=Restart=no") - .arg(format!("--description=dstack VM process {}", config.id)); + fn run_args(&self, config: &ProcessConfig, unit: &str) -> Result> { + let mut args = vec![ + "--quiet".into(), + "--unit".into(), + unit.to_string(), + "--service-type=exec".into(), + "--property=KillMode=mixed".into(), + "--property=KillSignal=SIGTERM".into(), + "--property=SendSIGKILL=yes".into(), + format!("--property=TimeoutStopSec={}", self.stop_timeout), + "--property=ExitType=cgroup".into(), + "--property=Restart=no".into(), + format!("--description=dstack VM process {}", config.id), + ]; if !config.cwd.is_empty() { - command.arg(format!("--working-directory={}", config.cwd)); + args.push(format!("--working-directory={}", config.cwd)); } if config.stdout.is_empty() { - command.arg("--property=StandardOutput=null"); + args.push("--property=StandardOutput=null".into()); } else { - command.arg(format!( + args.push(format!( "--property=StandardOutput=append:{}", config.stdout )); } if config.stderr.is_empty() { - command.arg("--property=StandardError=null"); + args.push("--property=StandardError=null".into()); } else { - command.arg(format!("--property=StandardError=append:{}", config.stderr)); + args.push(format!("--property=StandardError=append:{}", config.stderr)); } for (key, value) in &config.env { - command.arg(format!("--setenv={key}={value}")); + args.push(format!("--setenv={key}={value}")); } - command.arg("--").arg(&config.command).args(&config.args); + if !config.user.is_empty() { + // ProcessConfig.user is already normalized to a systemd User= + // value (name or bare UID). Re-parse to reject anything that + // would still inject property syntax. + let user = parse_unit_user("user", &config.user)?; + args.push(format!("--property=User={}", user.systemd_value())); + } + // systemd opens these before exec and passes them in declaration + // order starting at fd 3, which is what the QEMU netdev arguments + // reference. No fdname and no `graceful` option: a missing device must + // fail the unit instead of shifting every later descriptor by one. + // + // systemd.service(5): "The file or socket is opened by the service + // manager and the file descriptor is passed to the service." The open + // therefore happens with the manager's privileges, before the `User=` + // drop that lands just before exec, so a root-owned chardev such as + // /dev/tapN does not have to be chowned to the QEMU user. + for path in &config.open_files { + validate_open_file("open_files entry", path)?; + args.push(format!("--property=OpenFile={path}")); + } + args.push("--".into()); + args.push(config.command.clone()); + args.extend(config.args.iter().cloned()); + Ok(args) + } + + async fn launch(&self, config: &ProcessConfig) -> Result<()> { + let unit = self.unit(&config.id); + // Failed transient units remain loaded until reset and otherwise + // prevent automatic restart from reusing the unit name. + let mut reset = Command::new("systemctl"); + reset.arg("reset-failed").arg(&unit); + let _ = reset.output().await; + let mut command = Command::new("systemd-run"); + command.args(self.run_args(config, &unit)?); Self::command(command, "systemd-run").await?; if !config.pidfile.is_empty() { @@ -544,6 +599,35 @@ mod tests { #[test] fn unit_names_are_stable_and_do_not_embed_process_ids() { + let (_dir, manager) = test_manager(); + assert_eq!(manager.unit("vm/one"), manager.unit("vm/one")); + assert_ne!(manager.unit("vm/one"), manager.unit("vm-two")); + assert!(!manager.unit("vm/one").contains("vm/one")); + } + + fn test_config(open_files: &[&str]) -> ProcessConfig { + test_config_as("", open_files) + } + + fn test_config_as(user: &str, open_files: &[&str]) -> ProcessConfig { + ProcessConfig { + id: "vm/one".into(), + name: "vm".into(), + command: "/usr/bin/qemu".into(), + args: vec!["-netdev".into(), "tap,id=net0,fd=3".into()], + env: HashMap::new(), + cwd: String::new(), + stdout: String::new(), + stderr: String::new(), + pidfile: String::new(), + cid: None, + note: String::new(), + user: user.into(), + open_files: open_files.iter().map(|path| path.to_string()).collect(), + } + } + + fn test_manager() -> (tempfile::TempDir, SystemdProcessManager) { let dir = tempfile::tempdir().unwrap(); let manager = SystemdProcessManager::new( dir.path().to_path_buf(), @@ -551,9 +635,78 @@ mod tests { "infinity".into(), ) .unwrap(); - assert_eq!(manager.unit("vm/one"), manager.unit("vm/one")); - assert_ne!(manager.unit("vm/one"), manager.unit("vm-two")); - assert!(!manager.unit("vm/one").contains("vm/one")); + (dir, manager) + } + + #[test] + fn renders_open_files_as_ordered_unit_properties() { + let (_dir, manager) = test_manager(); + let args = manager + .run_args( + &test_config(&["/dev/tap7498", "/dev/tap7499"]), + "unit.service", + ) + .unwrap(); + let properties = args + .iter() + .take_while(|arg| *arg != "--") + .filter_map(|arg| arg.strip_prefix("--property=OpenFile=")) + .collect::>(); + assert_eq!(properties, ["/dev/tap7498", "/dev/tap7499"]); + assert_eq!(args.last().unwrap(), "tap,id=net0,fd=3"); + + assert!(!manager + .run_args(&test_config(&[]), "unit.service") + .unwrap() + .iter() + .any(|arg| arg.contains("OpenFile"))); + } + + #[test] + fn rejects_open_files_that_would_change_the_unit_property() { + let (_dir, manager) = test_manager(); + for path in ["relative/tap", "/dev/tap:0", "/dev/%i/tap"] { + manager + .run_args(&test_config(&[path]), "unit.service") + .unwrap_err(); + } + } + + #[test] + fn renders_the_privilege_drop_as_a_unit_property() { + let (_dir, manager) = test_manager(); + let args = manager + .run_args(&test_config_as("qemu", &["/dev/tap7498"]), "unit.service") + .unwrap(); + assert!(args.iter().any(|arg| arg == "--property=User=qemu")); + // The privilege drop replaces the sudo prefix rather than joining it. + assert!(!args.iter().any(|arg| arg == "sudo")); + + assert!(!manager + .run_args(&test_config(&[]), "unit.service") + .unwrap() + .iter() + .any(|arg| arg.contains("User="))); + + let args = manager + .run_args(&test_config_as("1000", &[]), "unit.service") + .unwrap(); + assert!(args.iter().any(|arg| arg == "--property=User=1000")); + + for user in ["qemu:0", "qemu user", "%i", "-qemu", "#"] { + manager + .run_args(&test_config_as(user, &[]), "unit.service") + .unwrap_err(); + } + } + + #[test] + fn supervisor_backend_rejects_what_it_cannot_provide() { + ensure_supervisor_supported(&test_config(&[])).unwrap(); + for config in [test_config(&["/dev/tap7498"]), test_config_as("qemu", &[])] { + let error = ensure_supervisor_supported(&config).unwrap_err(); + assert!(error.to_string().contains("systemd"), "{error:#}"); + } } #[test]