From 5bd5cdb7c1f114ca3829cdab37545139de8e4c4b Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Tue, 31 Dec 2024 12:05:25 -0500 Subject: [PATCH] chroot: fix parsing of --userspec argument Fix the parsing of the `--userspec=USER:GROUP` argument so that the both the user and the group are optional, and update the error message to match that of GNU `chroot`. This commit also removes the incorrect `clap` arguments for `--user` and `--group`. In `chroot --user=USER`, the `--user` is an abbreviation of `--userspec`, and in `chroot --group=GROUP`, the `--group` is an abbreviation of `--groups`. Closes #7040. --- src/uu/chroot/src/chroot.rs | 167 ++++++++++++++++++++--------------- src/uu/chroot/src/error.rs | 8 +- tests/by-util/test_chroot.rs | 37 ++++++-- 3 files changed, 129 insertions(+), 83 deletions(-) diff --git a/src/uu/chroot/src/chroot.rs b/src/uu/chroot/src/chroot.rs index 58e11e101..04db1380f 100644 --- a/src/uu/chroot/src/chroot.rs +++ b/src/uu/chroot/src/chroot.rs @@ -11,7 +11,7 @@ use clap::{crate_version, Arg, ArgAction, Command}; use std::ffi::CString; use std::io::Error; use std::os::unix::prelude::OsStrExt; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::process; use uucore::error::{set_exit_code, UClapError, UResult, UUsageError}; use uucore::fs::{canonicalize, MissingHandling, ResolveMode}; @@ -23,14 +23,79 @@ static USAGE: &str = help_usage!("chroot.md"); mod options { pub const NEWROOT: &str = "newroot"; - pub const USER: &str = "user"; - pub const GROUP: &str = "group"; pub const GROUPS: &str = "groups"; pub const USERSPEC: &str = "userspec"; pub const COMMAND: &str = "command"; pub const SKIP_CHDIR: &str = "skip-chdir"; } +/// A user and group specification, where each is optional. +enum UserSpec { + NeitherGroupNorUser, + UserOnly(String), + GroupOnly(String), + UserAndGroup(String, String), +} + +struct Options { + /// Path to the new root directory. + newroot: PathBuf, + /// Whether to change to the new root directory. + skip_chdir: bool, + /// List of groups under which the command will be run. + groups: Vec, + /// The user and group (each optional) under which the command will be run. + userspec: Option, +} + +/// Parse a user and group from the argument to `--userspec`. +/// +/// The `spec` must be of the form `[USER][:[GROUP]]`, otherwise an +/// error is returned. +fn parse_userspec(spec: &str) -> UResult { + match &spec.splitn(2, ':').collect::>()[..] { + // "" + [""] => Ok(UserSpec::NeitherGroupNorUser), + // "usr" + [usr] => Ok(UserSpec::UserOnly(usr.to_string())), + // ":" + ["", ""] => Ok(UserSpec::NeitherGroupNorUser), + // ":grp" + ["", grp] => Ok(UserSpec::GroupOnly(grp.to_string())), + // "usr:" + [usr, ""] => Ok(UserSpec::UserOnly(usr.to_string())), + // "usr:grp" + [usr, grp] => Ok(UserSpec::UserAndGroup(usr.to_string(), grp.to_string())), + // everything else + _ => Err(ChrootError::InvalidUserspec(spec.to_string()).into()), + } +} + +impl Options { + /// Parse parameters from the command-line arguments. + fn from(matches: &clap::ArgMatches) -> UResult { + let newroot = match matches.get_one::(options::NEWROOT) { + Some(v) => Path::new(v).to_path_buf(), + None => return Err(ChrootError::MissingNewRoot.into()), + }; + let groups = match matches.get_one::(options::GROUPS) { + None => vec![], + Some(s) => s.split(",").map(str::to_string).collect(), + }; + let skip_chdir = matches.get_flag(options::SKIP_CHDIR); + let userspec = match matches.get_one::(options::USERSPEC) { + None => None, + Some(s) => Some(parse_userspec(s)?), + }; + Ok(Self { + newroot, + skip_chdir, + groups, + userspec, + }) + } +} + #[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { let matches = uu_app().try_get_matches_from(args).with_exit_code(125)?; @@ -39,17 +104,17 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let default_option: &'static str = "-i"; let user_shell = std::env::var("SHELL"); - let newroot: &Path = match matches.get_one::(options::NEWROOT) { - Some(v) => Path::new(v), - None => return Err(ChrootError::MissingNewRoot.into()), - }; + let options = Options::from(&matches)?; - let skip_chdir = matches.get_flag(options::SKIP_CHDIR); // We are resolving the path in case it is a symlink or /. or /../ - if skip_chdir - && canonicalize(newroot, MissingHandling::Normal, ResolveMode::Logical) - .unwrap() - .to_str() + if options.skip_chdir + && canonicalize( + &options.newroot, + MissingHandling::Normal, + ResolveMode::Logical, + ) + .unwrap() + .to_str() != Some("/") { return Err(UUsageError::new( @@ -58,8 +123,8 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { )); } - if !newroot.is_dir() { - return Err(ChrootError::NoSuchDirectory(format!("{}", newroot.display())).into()); + if !options.newroot.is_dir() { + return Err(ChrootError::NoSuchDirectory(format!("{}", options.newroot.display())).into()); } let commands = match matches.get_many::(options::COMMAND) { @@ -85,7 +150,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let chroot_args = &command[1..]; // NOTE: Tests can only trigger code beyond this point if they're invoked with root permissions - set_context(newroot, &matches)?; + set_context(&options)?; let pstatus = match process::Command::new(chroot_command) .args(chroot_args) @@ -125,20 +190,6 @@ pub fn uu_app() -> Command { .required(true) .index(1), ) - .arg( - Arg::new(options::USER) - .short('u') - .long(options::USER) - .help("User (ID or name) to switch before running the program") - .value_name("USER"), - ) - .arg( - Arg::new(options::GROUP) - .short('g') - .long(options::GROUP) - .help("Group (ID or name) to switch to") - .value_name("GROUP"), - ) .arg( Arg::new(options::GROUPS) .short('G') @@ -175,43 +226,18 @@ pub fn uu_app() -> Command { ) } -fn set_context(root: &Path, options: &clap::ArgMatches) -> UResult<()> { - let userspec_str = options.get_one::(options::USERSPEC); - let user_str = options - .get_one::(options::USER) - .map(|s| s.as_str()) - .unwrap_or_default(); - let group_str = options - .get_one::(options::GROUP) - .map(|s| s.as_str()) - .unwrap_or_default(); - let groups_str = options - .get_one::(options::GROUPS) - .map(|s| s.as_str()) - .unwrap_or_default(); - let skip_chdir = options.contains_id(options::SKIP_CHDIR); - let userspec = match userspec_str { - Some(u) => { - let s: Vec<&str> = u.split(':').collect(); - if s.len() != 2 || s.iter().any(|&spec| spec.is_empty()) { - return Err(ChrootError::InvalidUserspec(u.to_string()).into()); - }; - s +fn set_context(options: &Options) -> UResult<()> { + enter_chroot(&options.newroot, options.skip_chdir)?; + set_groups_from_str(&options.groups)?; + match &options.userspec { + None | Some(UserSpec::NeitherGroupNorUser) => {} + Some(UserSpec::UserOnly(user)) => set_user(user)?, + Some(UserSpec::GroupOnly(group)) => set_main_group(group)?, + Some(UserSpec::UserAndGroup(user, group)) => { + set_main_group(group)?; + set_user(user)?; } - None => Vec::new(), - }; - - let (user, group) = if userspec.is_empty() { - (user_str, group_str) - } else { - (userspec[0], userspec[1]) - }; - - enter_chroot(root, skip_chdir)?; - - set_groups_from_str(groups_str)?; - set_main_group(group)?; - set_user(user)?; + } Ok(()) } @@ -239,7 +265,7 @@ fn set_main_group(group: &str) -> UResult<()> { if !group.is_empty() { let group_id = match entries::grp2gid(group) { Ok(g) => g, - _ => return Err(ChrootError::NoSuchGroup(group.to_string()).into()), + _ => return Err(ChrootError::NoSuchGroup.into()), }; let err = unsafe { setgid(group_id) }; if err != 0 { @@ -261,13 +287,13 @@ fn set_groups(groups: &[libc::gid_t]) -> libc::c_int { unsafe { setgroups(groups.len() as libc::size_t, groups.as_ptr()) } } -fn set_groups_from_str(groups: &str) -> UResult<()> { +fn set_groups_from_str(groups: &[String]) -> UResult<()> { if !groups.is_empty() { let mut groups_vec = vec![]; - for group in groups.split(',') { + for group in groups { let gid = match entries::grp2gid(group) { Ok(g) => g, - Err(_) => return Err(ChrootError::NoSuchGroup(group.to_string()).into()), + Err(_) => return Err(ChrootError::NoSuchGroup.into()), }; groups_vec.push(gid); } @@ -281,8 +307,7 @@ fn set_groups_from_str(groups: &str) -> UResult<()> { fn set_user(user: &str) -> UResult<()> { if !user.is_empty() { - let user_id = - entries::usr2uid(user).map_err(|_| ChrootError::NoSuchUser(user.to_string()))?; + let user_id = entries::usr2uid(user).map_err(|_| ChrootError::NoSuchUser)?; let err = unsafe { setuid(user_id as libc::uid_t) }; if err != 0 { return Err( diff --git a/src/uu/chroot/src/error.rs b/src/uu/chroot/src/error.rs index 1b83e7625..cc1acbee8 100644 --- a/src/uu/chroot/src/error.rs +++ b/src/uu/chroot/src/error.rs @@ -28,10 +28,10 @@ pub enum ChrootError { MissingNewRoot, /// Failed to find the specified user. - NoSuchUser(String), + NoSuchUser, /// Failed to find the specified group. - NoSuchGroup(String), + NoSuchGroup, /// The given directory does not exist. NoSuchDirectory(String), @@ -74,8 +74,8 @@ impl Display for ChrootError { "Missing operand: NEWROOT\nTry '{} --help' for more information.", uucore::execution_phrase(), ), - Self::NoSuchUser(s) => write!(f, "no such user: {}", s.maybe_quote(),), - Self::NoSuchGroup(s) => write!(f, "no such group: {}", s.maybe_quote(),), + Self::NoSuchUser => write!(f, "invalid user"), + Self::NoSuchGroup => write!(f, "invalid group"), Self::NoSuchDirectory(s) => write!( f, "cannot change root directory to {}: no such directory", diff --git a/tests/by-util/test_chroot.rs b/tests/by-util/test_chroot.rs index 42da75f48..31facdd55 100644 --- a/tests/by-util/test_chroot.rs +++ b/tests/by-util/test_chroot.rs @@ -55,13 +55,34 @@ fn test_no_such_directory() { #[test] fn test_invalid_user_spec() { - let (at, mut ucmd) = at_and_ucmd!(); + let ts = TestScenario::new(util_name!()); - at.mkdir("a"); + if let Ok(result) = run_ucmd_as_root(&ts, &["--userspec=ARABA:", "/"]) { + result + .failure() + .code_is(125) + .stderr_is("chroot: invalid user"); + } else { + print!("Test skipped; requires root user"); + } - let result = ucmd.arg("a").arg("--userspec=ARABA:").fails(); - result.code_is(125); - assert!(result.stderr_str().starts_with("chroot: invalid userspec")); + if let Ok(result) = run_ucmd_as_root(&ts, &["--userspec=ARABA:ARABA", "/"]) { + result + .failure() + .code_is(125) + .stderr_is("chroot: invalid user"); + } else { + print!("Test skipped; requires root user"); + } + + if let Ok(result) = run_ucmd_as_root(&ts, &["--userspec=:ARABA", "/"]) { + result + .failure() + .code_is(125) + .stderr_is("chroot: invalid group"); + } else { + print!("Test skipped; requires root user"); + } } #[test] @@ -77,10 +98,9 @@ fn test_invalid_user() { print!("Test skipped; requires root user"); } + // `--user` is an abbreviation of `--userspec`. if let Ok(result) = run_ucmd_as_root(&ts, &["--user=nobody:+65535", dir, "pwd"]) { - result - .failure() - .stderr_contains("no such user: nobody:+65535"); + result.failure().stderr_is("chroot: invalid user"); } else { print!("Test skipped; requires root user"); } @@ -116,6 +136,7 @@ fn test_preference_of_userspec() { at.mkdir("a"); + // `--user` is an abbreviation of `--userspec`. let result = ucmd .arg("a") .arg("--user")