From 283d1eb046c1a36d35e1f924ca6a5b3006a9fd1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20L=C3=B6nnhager?= Date: Wed, 21 Aug 2024 17:40:11 +0200 Subject: [PATCH] Spawn test processes as an unprivileged user by default Fixes an issue where the connection-checker is allowed to leak traffic on macOS --- test/scripts/ssh-setup.sh | 19 ++++++++ test/test-manager/src/vm/provision.rs | 3 +- test/test-rpc/src/lib.rs | 4 ++ test/test-runner/Cargo.toml | 2 +- test/test-runner/src/main.rs | 16 +++++-- test/test-runner/src/util.rs | 69 +++++++++++++++++++++++++++ 6 files changed, 106 insertions(+), 7 deletions(-) diff --git a/test/scripts/ssh-setup.sh b/test/scripts/ssh-setup.sh index 714756f45e46..5ac5dea15efa 100644 --- a/test/scripts/ssh-setup.sh +++ b/test/scripts/ssh-setup.sh @@ -9,6 +9,7 @@ RUNNER_DIR="$1" APP_PACKAGE="$2" PREVIOUS_APP="$3" UI_RUNNER="$4" +UNPRIVILEGED_USER="$5" # Copy over test runner to correct place @@ -21,6 +22,9 @@ for file in test-runner connection-checker $APP_PACKAGE $PREVIOUS_APP $UI_RUNNER cp -f "$SCRIPT_DIR/$file" "$RUNNER_DIR" done +# Unprivileged users need execute rights for connection checker +chmod 551 "${RUNNER_DIR}/connection-checker" + chown -R root "$RUNNER_DIR/" # Create service @@ -69,11 +73,18 @@ function setup_macos { EOF + create_test_user_macos + echo "Starting test runner service" launchctl load -w $RUNNER_PLIST_PATH } +function create_test_user_macos { + echo "Adding test user account" + sysadminctl -addUser "$UNPRIVILEGED_USER" -fullName "$UNPRIVILEGED_USER" -password "$UNPRIVILEGED_USER" +} + function setup_systemd { RUNNER_SERVICE_PATH="/etc/systemd/system/testrunner.service" @@ -94,10 +105,18 @@ EOF semanage fcontext -a -t bin_t "$RUNNER_DIR/.*" &> /dev/null || true + create_test_user_linux + systemctl enable testrunner.service systemctl start testrunner.service } +function create_test_user_linux { + echo "Adding test user account" + useradd -m "$UNPRIVILEGED_USER" + echo "$UNPRIVILEGED_USER:$UNPRIVILEGED_USER" | chpasswd +} + if [[ "$(uname -s)" == "Darwin" ]]; then setup_macos exit 0 diff --git a/test/test-manager/src/vm/provision.rs b/test/test-manager/src/vm/provision.rs index 143f023fa49f..359cf16c33d8 100644 --- a/test/test-manager/src/vm/provision.rs +++ b/test/test-manager/src/vm/provision.rs @@ -10,6 +10,7 @@ use std::{ net::{IpAddr, SocketAddr, TcpStream}, path::{Path, PathBuf}, }; +use test_rpc::UNPRIVILEGED_USER; /// Returns the directory in the test runner where the test-runner binary is installed. pub async fn provision( @@ -156,7 +157,7 @@ fn blocking_ssh( // Run the setup script in the test runner let cmd = format!( - r#"sudo {} {remote_dir} "{app_package_path}" "{app_package_to_upgrade_from_path}" "{gui_package_path}""#, + r#"sudo {} {remote_dir} "{app_package_path}" "{app_package_to_upgrade_from_path}" "{gui_package_path}" "{UNPRIVILEGED_USER}""#, bootstrap_script_dest.display(), ); log::debug!("Running setup script on remote, cmd: {cmd}"); diff --git a/test/test-rpc/src/lib.rs b/test/test-rpc/src/lib.rs index cc263b845e1e..1ec8fb73216a 100644 --- a/test/test-rpc/src/lib.rs +++ b/test/test-rpc/src/lib.rs @@ -13,6 +13,10 @@ pub mod net; pub mod package; pub mod transport; +/// Unprivileged user. This is used for things like spawning processes. +/// This is also used as the password for the same user, as is common practice. +pub const UNPRIVILEGED_USER: &str = "mole"; + #[derive(thiserror::Error, Debug, Serialize, Deserialize, PartialEq, Eq)] pub enum Error { #[error("Test runner RPC failed")] diff --git a/test/test-runner/Cargo.toml b/test/test-runner/Cargo.toml index b16a8c23b2b3..7206cb394a8d 100644 --- a/test/test-runner/Cargo.toml +++ b/test/test-runner/Cargo.toml @@ -58,7 +58,7 @@ features = ["codec"] default-features = false [target.'cfg(unix)'.dependencies] -nix = { workspace = true } +nix = { workspace = true, features = ["user"] } [target.'cfg(target_os = "linux")'.dependencies] rs-release = "0.1.7" diff --git a/test/test-runner/src/main.rs b/test/test-runner/src/main.rs index 59fc83672d77..79bc17b4ef50 100644 --- a/test/test-runner/src/main.rs +++ b/test/test-runner/src/main.rs @@ -17,7 +17,7 @@ use test_rpc::{ net::SockHandleId, package::Package, transport::GrpcForwarder, - AppTrace, Service, SpawnOpts, + AppTrace, Service, SpawnOpts, UNPRIVILEGED_USER, }; use tokio::{ io::{AsyncBufReadExt, AsyncReadExt, AsyncWriteExt, BufReader}, @@ -386,11 +386,17 @@ impl Service for TestServer { } cmd.stderr(Stdio::piped()); + cmd.kill_on_drop(true); - let mut child = cmd.kill_on_drop(true).spawn().map_err(|error| { - log::error!("Failed to spawn {}: {error}", opts.path); - test_rpc::Error::Syscall - })?; + let mut child = util::as_unprivileged_user(UNPRIVILEGED_USER, || cmd.spawn()) + .map_err(|error| { + log::error!("Failed to drop privileges: {error}"); + test_rpc::Error::Syscall + })? + .map_err(|error| { + log::error!("Failed to spawn {}: {error}", opts.path); + test_rpc::Error::Syscall + })?; let pid = child .id() diff --git a/test/test-runner/src/util.rs b/test/test-runner/src/util.rs index 03a334321412..84cc048b49c6 100644 --- a/test/test-runner/src/util.rs +++ b/test/test-runner/src/util.rs @@ -21,3 +21,72 @@ impl OnDrop { } } } + +#[derive(thiserror::Error, Debug)] +#[error(transparent)] +pub struct Error { + inner: InnerError, +} + +#[cfg(unix)] +#[derive(thiserror::Error, Debug)] +enum InnerError { + #[error("Failed to get the specified user")] + GetUser(#[source] nix::Error), + #[error("The specified user was not found")] + MissingUser, + #[error("Failed to set uid")] + SetUid(#[source] nix::Error), + #[error("Failed to set gid")] + SetGid(#[source] nix::Error), +} + +#[cfg(target_os = "windows")] +#[derive(thiserror::Error, Debug)] +enum InnerError {} + +impl From for Error { + fn from(inner: InnerError) -> Self { + Self { inner } + } +} + +#[cfg(target_os = "windows")] +pub fn as_unprivileged_user(unpriv_user: &str, func: impl FnOnce() -> T) -> Result { + // NOTE: no-op + let _ = unpriv_user; + Ok(func()) +} + +#[cfg(unix)] +pub fn as_unprivileged_user(unpriv_user: &str, func: impl FnOnce() -> T) -> Result { + let original_uid = nix::unistd::getuid(); + let original_gid = nix::unistd::getgid(); + + let user = nix::unistd::User::from_name(unpriv_user) + .map_err(InnerError::GetUser)? + .ok_or(InnerError::MissingUser)?; + let uid = user.uid; + let gid = user.gid; + + nix::unistd::setegid(gid).map_err(InnerError::SetGid)?; + let restore_gid = OnDrop::new(|| { + if let Err(error) = nix::unistd::setegid(original_gid) { + log::error!("Failed to restore gid: {error}"); + } + }); + + nix::unistd::seteuid(uid).map_err(InnerError::SetUid)?; + let restore_uid = OnDrop::new(|| { + if let Err(error) = nix::unistd::seteuid(original_uid) { + log::error!("Failed to restore uid: {error}"); + } + }); + + let result = Ok(func()); + + drop(restore_uid); + drop(restore_gid); + + result +}