Skip to content

Commit

Permalink
Whoops, remove a privilege escalation
Browse files Browse the repository at this point in the history
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
  • Loading branch information
SUPERCILEX committed Mar 24, 2023
1 parent 0e77b59 commit 933bccf
Show file tree
Hide file tree
Showing 5 changed files with 22 additions and 43 deletions.
2 changes: 1 addition & 1 deletion api.golden
Original file line number Diff line number Diff line change
Expand Up @@ -63,5 +63,5 @@ impl<T> core::convert::From<T> for forkfs::SessionOperand<'a, S>
pub fn forkfs::SessionOperand::from(t: T) -> T
pub fn forkfs::delete_sessions<S: core::convert::AsRef<str>>(sessions: forkfs::SessionOperand<'_, S>) -> error_stack::result::Result<(), forkfs::Error>
pub fn forkfs::list_sessions() -> error_stack::result::Result<(), forkfs::Error>
pub fn forkfs::run<T: core::convert::AsRef<std::ffi::os_str::OsStr>>(session: &str, command: &[T], stay_root: bool) -> error_stack::result::Result<(), forkfs::Error>
pub fn forkfs::run<T: core::convert::AsRef<std::ffi::os_str::OsStr>>(session: &str, command: &[T]) -> error_stack::result::Result<(), forkfs::Error>
pub fn forkfs::stop_sessions<S: core::convert::AsRef<str>>(sessions: forkfs::SessionOperand<'_, S>) -> error_stack::result::Result<(), forkfs::Error>
1 change: 0 additions & 1 deletion command-reference-short.golden
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ Arguments:

Options:
-s, --session <SESSION> The fork/sandbox to use [default: default]
--stay-root Run the command with root privileges
-h, --help Print help (use `--help` for more detail)

---
Expand Down
5 changes: 0 additions & 5 deletions command-reference.golden
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,6 @@ Options:

[default: default]

--stay-root
Run the command with root privileges

Note: this flag only applies when running as root.

-h, --help
Print help (use `-h` for a summary)

Expand Down
16 changes: 2 additions & 14 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,6 @@ struct Run {
#[arg(short = 's', long = "session", short_alias = 'n', aliases = & ["name", "id"])]
#[arg(default_value = "default")]
session: String,

/// Run the command with root privileges
///
/// Note: this flag only applies when running as root.
#[arg(long = "stay-root", aliases = & ["stay-sudo", "keep-root", "keep-sudo"])]
stay_root: bool,
}

#[derive(Subcommand, Debug)]
Expand Down Expand Up @@ -131,14 +125,8 @@ fn forkfs(ForkFs { cmd, help: _ }: ForkFs) -> Result<(), forkfs::Error> {
}
}

fn run(
Run {
command,
session,
stay_root,
}: Run,
) -> Result<(), forkfs::Error> {
forkfs::run(&session, command.as_slice(), stay_root)
fn run(Run { command, session }: Run) -> Result<(), forkfs::Error> {
forkfs::run(&session, command.as_slice())
}

fn sessions(sessions: Sessions) -> Result<(), forkfs::Error> {
Expand Down
41 changes: 19 additions & 22 deletions src/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use rustix::{

use crate::{get_sessions_dir, sessions::maybe_create_session, Error, IoErr};

pub fn run<T: AsRef<OsStr>>(session: &str, command: &[T], stay_root: bool) -> Result<(), Error> {
pub fn run<T: AsRef<OsStr>>(session: &str, command: &[T]) -> Result<(), Error> {
let uid = getuid();
validate_permissions(uid)?;

Expand All @@ -29,7 +29,7 @@ pub fn run<T: AsRef<OsStr>>(session: &str, command: &[T], stay_root: bool) -> Re
session_dir.push("merged");
enter_session(&session_dir)?;

run_command(command, uid, stay_root)
run_command(command, uid)
}

fn enter_session(target: &Path) -> Result<(), Error> {
Expand All @@ -41,17 +41,15 @@ fn enter_session(target: &Path) -> Result<(), Error> {
.map_io_err_lazy(|| format!("Failed to change current directory {target:?}"))
}

fn run_command(args: &[impl AsRef<OsStr>], prev_uid: Uid, stay_root: bool) -> Result<(), Error> {
fn run_command(args: &[impl AsRef<OsStr>], prev_uid: Uid) -> Result<(), Error> {
let mut command = Command::new(args[0].as_ref());

// Downgrade privilege level to pre-sudo if possible
if !stay_root {
if !prev_uid.is_root() {
command.uid(prev_uid.as_raw());
} else if let Some(uid) = env::var_os("SUDO_UID").as_ref().and_then(|s| s.to_str())
&& let Ok(uid) = uid.parse() {
command.uid(uid);
}
if !prev_uid.is_root() {
command.uid(prev_uid.as_raw());
} else if let Some(uid) = env::var_os("SUDO_UID").as_ref().and_then(|s| s.to_str())
&& let Ok(uid) = uid.parse() {
command.uid(uid);
}

Err(command.args(&args[1..]).exec()).map_io_err_lazy(|| {
Expand Down Expand Up @@ -81,10 +79,7 @@ fn validate_permissions(uid: Uid) -> Result<(), Error> {
.map_io_err("Failed to retrieve capabilities")?
.effective;
if effective_capabilities.contains(
CapabilityFlags::CHOWN
| CapabilityFlags::DAC_OVERRIDE
| CapabilityFlags::SYS_CHROOT
| CapabilityFlags::SYS_ADMIN,
CapabilityFlags::CHOWN | CapabilityFlags::SYS_CHROOT | CapabilityFlags::SYS_ADMIN,
) {
return Ok(());
}
Expand All @@ -104,17 +99,19 @@ Under the hood, ForkFS is implemented as a wrapper around OverlayFS. As a
consequence, elevated privileges are required and can be granted in one of
three ways (ordered by recommendation):
- $ sudo chown root {0}; sudo chmod u+s {0}
- $ sudo setcap \
cap_chown,cap_sys_chroot,cap_sys_admin,cap_dac_override,cap_fowner,cap_setpcap,\
cap_mknod,cap_lease,cap_setfcap+ep {0}
This transfers ownership of the `forkfs` binary to root and specifies that
the binary should be executed as its owner (i.e. root). This is preferable
because it allows you to pass along root privileges to the sandboxed
program when necessary.
This grants `forkfs` precisely the capabilities it needs.
- $ sudo setcap cap_chown,cap_dac_override,cap_sys_chroot,cap_sys_admin+ep {0}
cap_dac_override onwards are capabilities that are required for OverlayFS to
be able to perform those actions.
This grants `forkfs` precisely the capabilities it needs. Note that the
`stay-root` flag will not work.
- $ sudo chown root {0}; sudo chmod u+s {0}
This transfers ownership of the `forkfs` binary to root and specifies that
the binary should be executed as its owner (i.e. root).
- $ sudo -E forkfs ...
Expand Down

0 comments on commit 933bccf

Please sign in to comment.