Skip to content

Commit

Permalink
Spawn test processes as an unprivileged user by default
Browse files Browse the repository at this point in the history
Fixes an issue where the connection-checker is allowed to leak traffic
on macOS
  • Loading branch information
dlon committed Aug 22, 2024
1 parent 5c93059 commit 283d1eb
Show file tree
Hide file tree
Showing 6 changed files with 106 additions and 7 deletions.
19 changes: 19 additions & 0 deletions test/scripts/ssh-setup.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand Down Expand Up @@ -69,11 +73,18 @@ function setup_macos {
</plist>
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"

Expand All @@ -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
Expand Down
3 changes: 2 additions & 1 deletion test/test-manager/src/vm/provision.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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}");
Expand Down
4 changes: 4 additions & 0 deletions test/test-rpc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down
2 changes: 1 addition & 1 deletion test/test-runner/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
16 changes: 11 additions & 5 deletions test/test-runner/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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()
Expand Down
69 changes: 69 additions & 0 deletions test/test-runner/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,72 @@ impl<F: FnOnce() + Send> OnDrop<F> {
}
}
}

#[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<InnerError> for Error {
fn from(inner: InnerError) -> Self {
Self { inner }
}
}

#[cfg(target_os = "windows")]
pub fn as_unprivileged_user<T>(unpriv_user: &str, func: impl FnOnce() -> T) -> Result<T, Error> {
// NOTE: no-op
let _ = unpriv_user;
Ok(func())
}

#[cfg(unix)]
pub fn as_unprivileged_user<T>(unpriv_user: &str, func: impl FnOnce() -> T) -> Result<T, Error> {
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
}

0 comments on commit 283d1eb

Please sign in to comment.