From 426b310d9bfe9b66dca77ba71030ee80b3582b7b Mon Sep 17 00:00:00 2001 From: Marc Schoolderman Date: Mon, 10 Feb 2025 20:30:38 +0100 Subject: [PATCH] remove conversion from CLI structs to an intermediary struct and build context object directly from command options + sudoers file --- src/common/context.rs | 160 +++++++++++++++++++++++--------------- src/common/resolve.rs | 31 ++++---- src/sudo/cli/mod.rs | 104 ------------------------- src/sudo/mod.rs | 6 +- src/sudo/pipeline.rs | 24 +++--- src/sudo/pipeline/list.rs | 4 +- 6 files changed, 125 insertions(+), 204 deletions(-) diff --git a/src/common/context.rs b/src/common/context.rs index b2c2ac44d..54cc4e6a1 100644 --- a/src/common/context.rs +++ b/src/common/context.rs @@ -1,35 +1,14 @@ use crate::common::{HARDENED_ENUM_VALUE_0, HARDENED_ENUM_VALUE_1, HARDENED_ENUM_VALUE_2}; +use crate::sudo::{SudoListOptions, SudoRunOptions, SudoValidateOptions}; +use crate::sudoers::Sudoers; use crate::system::{Group, Hostname, Process, User}; -use super::resolve::CurrentUser; use super::{ command::CommandAndArguments, - resolve::{resolve_launch_and_shell, resolve_target_user_and_group}, - Error, SudoPath, SudoString, + resolve::{resolve_shell, resolve_target_user_and_group, CurrentUser}, + Error, SudoPath, }; -#[derive(Clone, Copy)] -pub enum ContextAction { - List, - Run, - Validate, -} - -// this is a bit of a hack to keep the existing `Context` API working -#[derive(Clone)] -pub struct OptionsForContext { - pub chdir: Option, - pub group: Option, - pub login: bool, - pub non_interactive: bool, - pub positional_args: Vec, - pub reset_timestamp: bool, - pub shell: bool, - pub stdin: bool, - pub user: Option, - pub action: ContextAction, -} - #[derive(Debug)] pub struct Context { // cli options @@ -50,7 +29,7 @@ pub struct Context { pub password_feedback: bool, } -#[derive(Debug, Default, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] #[repr(u32)] pub enum LaunchType { #[default] @@ -60,7 +39,59 @@ pub enum LaunchType { } impl Context { - pub fn build_from_options(sudo_options: OptionsForContext) -> Result { + pub fn from_run_opts( + sudo_options: SudoRunOptions, + policy: &mut Sudoers, + ) -> Result { + let hostname = Hostname::resolve(); + let current_user = CurrentUser::resolve()?; + + let (target_user, target_group) = + resolve_target_user_and_group(&sudo_options.user, &sudo_options.group, ¤t_user)?; + + let launch = if sudo_options.login { + LaunchType::Login + } else if sudo_options.shell { + LaunchType::Shell + } else { + LaunchType::Direct + }; + + let shell = resolve_shell(launch, ¤t_user, &target_user); + + let override_path = policy.search_path(&hostname, ¤t_user, &target_user); + + let command = { + let system_path; + + let path = if let Some(path) = override_path { + path + } else { + system_path = std::env::var("PATH").unwrap_or_default(); + system_path.as_ref() + }; + + CommandAndArguments::build_from_args(shell, sudo_options.positional_args, path) + }; + + Ok(Context { + hostname, + command, + current_user, + target_user, + target_group, + use_session_records: !sudo_options.reset_timestamp, + launch, + chdir: sudo_options.chdir, + stdin: sudo_options.stdin, + non_interactive: sudo_options.non_interactive, + process: Process::new(), + use_pty: true, + password_feedback: false, + }) + } + + pub fn from_validate_opts(sudo_options: SudoValidateOptions) -> Result { let hostname = Hostname::resolve(); let current_user = CurrentUser::resolve()?; let (target_user, target_group) = @@ -74,7 +105,7 @@ impl Context { target_group, use_session_records: !sudo_options.reset_timestamp, launch: Default::default(), - chdir: sudo_options.chdir, + chdir: None, stdin: sudo_options.stdin, non_interactive: sudo_options.non_interactive, process: Process::new(), @@ -83,38 +114,46 @@ impl Context { }) } - pub fn supply_command( - self, - sudo_options: OptionsForContext, - secure_path: Option<&str>, + pub fn from_list_opts( + sudo_options: SudoListOptions, + policy: &mut Sudoers, ) -> Result { - let (launch, shell) = - resolve_launch_and_shell(&sudo_options, &self.current_user, &self.target_user); - - let command = match sudo_options.action { - ContextAction::Run | ContextAction::List - if !sudo_options.positional_args.is_empty() => - { - let system_path; - - let path = if let Some(path) = secure_path { - path - } else { - system_path = std::env::var("PATH").unwrap_or_default(); - system_path.as_ref() - }; - - CommandAndArguments::build_from_args(shell, sudo_options.positional_args, path) - } - - // FIXME `Default` is being used as `Option::None` - _ => Default::default(), + let hostname = Hostname::resolve(); + let current_user = CurrentUser::resolve()?; + let (target_user, target_group) = + resolve_target_user_and_group(&sudo_options.user, &sudo_options.group, ¤t_user)?; + + let override_path = policy.search_path(&hostname, ¤t_user, &target_user); + + let command = if sudo_options.positional_args.is_empty() { + Default::default() + } else { + let system_path; + + let path = if let Some(path) = override_path { + path + } else { + system_path = std::env::var("PATH").unwrap_or_default(); + system_path.as_ref() + }; + + CommandAndArguments::build_from_args(None, sudo_options.positional_args, path) }; - Ok(Self { - launch, + Ok(Context { + hostname, command, - ..self + current_user, + target_user, + target_group, + use_session_records: !sudo_options.reset_timestamp, + launch: Default::default(), + chdir: None, + stdin: sudo_options.stdin, + non_interactive: sudo_options.non_interactive, + process: Process::new(), + use_pty: true, + password_feedback: false, }) } } @@ -125,23 +164,18 @@ mod tests { sudo::SudoAction, system::{interface::UserId, Hostname}, }; - use std::collections::HashMap; use super::Context; #[test] - fn test_build_context() { + fn test_build_run_context() { let options = SudoAction::try_parse_from(["sudo", "echo", "hello"]) .unwrap() .try_into_run() .ok() .unwrap(); - let path = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"; - let (ctx_opts, _pipe_opts) = options.into(); - let context = Context::build_from_options(ctx_opts, Some(path)).unwrap(); - let mut target_environment = HashMap::new(); - target_environment.insert("SUDO_USER".to_string(), context.current_user.name.clone()); + let context = Context::from_run_opts(options, &mut Default::default()).unwrap(); if cfg!(target_os = "linux") { // this assumes /bin is a symlink on /usr/bin, like it is on modern Debian/Ubuntu diff --git a/src/common/resolve.rs b/src/common/resolve.rs index cfde086f1..926ae3821 100644 --- a/src/common/resolve.rs +++ b/src/common/resolve.rs @@ -11,10 +11,7 @@ use std::{ }; use super::SudoString; -use super::{ - context::{LaunchType, OptionsForContext}, - Error, -}; +use super::{context::LaunchType, Error}; #[derive(PartialEq, Debug)] enum NameOrId<'a, T: FromStr> { @@ -97,21 +94,21 @@ impl ops::Deref for AuthUser { type Shell = Option; -pub(super) fn resolve_launch_and_shell( - sudo_options: &OptionsForContext, +pub(super) fn resolve_shell( + launch_type: LaunchType, current_user: &User, target_user: &User, -) -> (LaunchType, Shell) { - if sudo_options.login { - (LaunchType::Login, Some(target_user.shell.clone())) - } else if sudo_options.shell { - let shell = env::var("SHELL") - .map(|s| s.into()) - .unwrap_or_else(|_| current_user.shell.clone()); - - (LaunchType::Shell, Some(shell)) - } else { - (LaunchType::Direct, None) +) -> Shell { + match launch_type { + LaunchType::Login => Some(target_user.shell.clone()), + + LaunchType::Shell => Some( + env::var("SHELL") + .map(|s| s.into()) + .unwrap_or_else(|_| current_user.shell.clone()), + ), + + LaunchType::Direct => None, } } diff --git a/src/sudo/cli/mod.rs b/src/sudo/cli/mod.rs index c1a6b3fdf..894d6ed36 100644 --- a/src/sudo/cli/mod.rs +++ b/src/sudo/cli/mod.rs @@ -2,7 +2,6 @@ use std::{borrow::Cow, mem}; -use crate::common::context::{ContextAction, OptionsForContext}; use crate::common::{SudoPath, SudoString}; pub mod help; @@ -799,106 +798,3 @@ fn reject_all(context: &str, opts: SudoOptions) -> Result<(), String> { Ok(()) } - -impl From for OptionsForContext { - fn from(opts: SudoListOptions) -> Self { - let SudoListOptions { - group, - non_interactive, - positional_args, - reset_timestamp, - stdin, - user, - - list: _, - other_user: _, - } = opts; - - Self { - action: ContextAction::List, - - group, - non_interactive, - positional_args, - reset_timestamp, - stdin, - user, - - chdir: None, - login: false, - shell: false, - } - } -} - -impl From for OptionsForContext { - fn from(opts: SudoValidateOptions) -> Self { - let SudoValidateOptions { - group, - non_interactive, - reset_timestamp, - stdin, - user, - } = opts; - - Self { - action: ContextAction::Validate, - - group, - non_interactive, - reset_timestamp, - stdin, - user, - - chdir: None, - login: false, - positional_args: vec![], - shell: false, - } - } -} - -pub struct OptionsForPipeline { - pub preserve_env: PreserveEnv, - pub user_requested_env_vars: Vec<(String, String)>, -} - -impl SudoRunOptions { - pub fn into(self) -> (OptionsForContext, OptionsForPipeline) { - let SudoRunOptions { - chdir, - group, - login, - non_interactive, - positional_args, - reset_timestamp, - shell, - stdin, - user, - - env_var_list, - preserve_env, - } = self; - - let ctx_opts = OptionsForContext { - action: ContextAction::Run, - - chdir, - group, - login, - non_interactive, - positional_args, - reset_timestamp, - shell, - stdin, - user, - }; - - let pipe_opts = OptionsForPipeline { - preserve_env, - user_requested_env_vars: env_var_list, - }; - - (ctx_opts, pipe_opts) - } -} diff --git a/src/sudo/mod.rs b/src/sudo/mod.rs index f6c33ce19..e6c12cff1 100644 --- a/src/sudo/mod.rs +++ b/src/sudo/mod.rs @@ -10,7 +10,7 @@ use crate::system::User; use crate::system::{time::Duration, timestamp::SessionRecordFile, Process}; use cli::help; #[cfg(test)] -pub use cli::SudoAction; +pub(crate) use cli::SudoAction; #[cfg(not(test))] use cli::SudoAction; use pam::PamAuthenticator; @@ -18,7 +18,9 @@ use pipeline::Pipeline; use std::path::Path; mod cli; -pub mod diagnostic; +pub(crate) use cli::{SudoListOptions, SudoRunOptions, SudoValidateOptions}; + +pub(crate) mod diagnostic; mod env; mod pam; mod pipeline; diff --git a/src/sudo/pipeline.rs b/src/sudo/pipeline.rs index b0d103862..084564f6f 100644 --- a/src/sudo/pipeline.rs +++ b/src/sudo/pipeline.rs @@ -62,31 +62,25 @@ fn judge(mut policy: Sudoers, context: &Context) -> Result { } impl Pipeline { - pub fn run(mut self, cmd_opts: SudoRunOptions) -> Result<(), Error> { + pub fn run(mut self, mut cmd_opts: SudoRunOptions) -> Result<(), Error> { let mut policy = read_sudoers()?; - let (ctx_opts, pipe_opts) = cmd_opts.into(); + let user_requested_env_vars = std::mem::take(&mut cmd_opts.env_var_list); - if !pipe_opts.preserve_env.is_nothing() { + if !cmd_opts.preserve_env.is_nothing() { eprintln_ignore_io_error!( "warning: `--preserve-env` has not yet been implemented and will be ignored" ) } - let context = Context::build_from_options(ctx_opts.clone())?; - - let path = policy.search_path( - &context.hostname, - &context.current_user, - &context.target_user, - ); - let mut context = context.supply_command(ctx_opts, path)?; + let mut context = Context::from_run_opts(cmd_opts, &mut policy)?; let policy = judge(policy, &context)?; let Authorization::Allowed(auth, controls) = policy.authorization() else { return Err(Error::Authorization(context.current_user.name.to_string())); }; + self.apply_policy_to_context(&mut context, &auth, &controls)?; self.auth_and_update_record_file(&mut context, &auth)?; @@ -95,9 +89,9 @@ impl Pipeline { let current_env = environment::system_environment(); let (checked_vars, trusted_vars) = if controls.trust_environment { - (vec![], pipe_opts.user_requested_env_vars) + (vec![], user_requested_env_vars) } else { - (pipe_opts.user_requested_env_vars, vec![]) + (user_requested_env_vars, vec![]) }; let mut target_env = environment::get_target_environment( @@ -144,8 +138,8 @@ impl Pipeline { pub fn run_validate(mut self, cmd_opts: SudoValidateOptions) -> Result<(), Error> { let policy = read_sudoers()?; - let ctx_opts: crate::common::context::OptionsForContext = cmd_opts.into(); - let mut context = Context::build_from_options(ctx_opts.clone())?; + + let mut context = Context::from_validate_opts(cmd_opts)?; match policy.validate_authorization() { Authorization::Forbidden => { diff --git a/src/sudo/pipeline/list.rs b/src/sudo/pipeline/list.rs index 64fd1771f..618d0538e 100644 --- a/src/sudo/pipeline/list.rs +++ b/src/sudo/pipeline/list.rs @@ -25,9 +25,7 @@ impl Pipeline { let mut sudoers = super::read_sudoers()?; - let ctx_opts: crate::common::context::OptionsForContext = cmd_opts.into(); - let mut context = Context::build_from_options(ctx_opts.clone())? - .supply_command(ctx_opts, /*sudoers.search_path()*/ None)?; // TODO + let mut context = Context::from_list_opts(cmd_opts, &mut sudoers)?; if original_command.is_some() && !context.command.resolved { return Err(Error::CommandNotFound(context.command.command));