Add environment variable passing (#918)

src/common/context.rs

@@ -111,7 +111,8 @@ mod tests {
             .ok()
             .unwrap();
         let path = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin";
-        let context = Context::build_from_options(options.into(), path.to_string()).unwrap();
+        let (ctx_opts, _pipe_opts) = options.into();
+        let context = Context::build_from_options(ctx_opts, path.to_string()).unwrap();
 
         let mut target_environment = HashMap::new();
         target_environment.insert("SUDO_USER".to_string(), context.current_user.name.clone());

src/common/error.rs

@@ -24,6 +24,7 @@ pub enum Error {
     GroupNotFound(String),
     Authorization(String),
     InteractionRequired,
+    EnvironmentVar(Vec<String>),
     Configuration(String),
     Options(String),
     Pam(PamError),
@@ -65,6 +66,18 @@ impl fmt::Display for Error {
             Error::GroupNotFound(g) => write!(f, "group '{g}' not found"),
             Error::Authorization(u) => write!(f, "I'm sorry {u}. I'm afraid I can't do that"),
             Error::InteractionRequired => write!(f, "interactive authentication is required"),
+            Error::EnvironmentVar(vs) => {
+                write!(
+                    f,
+                    "you are not allowed to set the following environment variables:"
+                )?;
+                let mut sep = "";
+                for v in vs {
+                    write!(f, "{sep} {v}")?;
+                    sep = ",";
+                }
+                Ok(())
+            }
             Error::Configuration(e) => write!(f, "invalid configuration: {e}"),
             Error::Options(e) => write!(f, "{e}"),
             Error::Pam(e) => write!(f, "PAM error: {e}"),

src/common/mod.rs

@@ -1,5 +1,4 @@
 #![forbid(unsafe_code)]
-use std::{collections::HashMap, ffi::OsString};
 
 pub use command::CommandAndArguments;
 pub use context::Context;
@@ -15,8 +14,6 @@ mod path;
 pub mod resolve;
 mod string;
 
-pub type Environment = HashMap<OsString, OsString>;
-
 // Hardened enum values used for critical enums to mitigate attacks like Rowhammer.
 // See for example https://arxiv.org/pdf/2309.02545.pdf
 // The values are copied from https://github.com/sudo-project/sudo/commit/7873f8334c8d31031f8cfa83bd97ac6029309e4f#diff-b8ac7ab4c3c4a75aed0bb5f7c5fd38b9ea6c81b7557f775e46c6f8aa115e02cd

src/exec/mod.rs

@@ -16,18 +16,15 @@ use std::{
 };
 
 use crate::{
-    common::Environment,
+    exec::no_pty::exec_no_pty,
+    log::dev_info,
     log::dev_warn,
     system::{
         interface::ProcessId,
         killpg,
         signal::{consts::*, signal_name},
         wait::{Wait, WaitError, WaitOptions},
     },
-};
-use crate::{
-    exec::no_pty::exec_no_pty,
-    log::dev_info,
     system::{set_target_user, signal::SignalNumber, term::UserTerm},
 };
 use crate::{log::user_error, system::kill};
@@ -44,7 +41,10 @@ use self::{
 ///
 /// Returns the [`ExitReason`] of the command and a function that restores the default handler for
 /// signals once its called.
-pub fn run_command(options: &impl RunOptions, env: Environment) -> io::Result<ExecOutput> {
+pub fn run_command(
+    options: &impl RunOptions,
+    env: impl IntoIterator<Item = (impl AsRef<OsStr>, impl AsRef<OsStr>)>,
+) -> io::Result<ExecOutput> {
     // FIXME: should we pipe the stdio streams?
     let qualified_path = options.command()?;
     let mut command = Command::new(qualified_path);

src/pam/mod.rs

@@ -1,5 +1,4 @@
 use std::{
-    collections::HashMap,
     ffi::{CStr, CString, OsStr, OsString},
     os::raw::c_char,
     os::unix::prelude::OsStrExt,
@@ -316,8 +315,8 @@ impl<C: Converser> PamContext<C> {
     }
 
     /// Get a full listing of the current PAM environment
-    pub fn env(&mut self) -> PamResult<HashMap<OsString, OsString>> {
-        let mut res = HashMap::new();
+    pub fn env(&mut self) -> PamResult<Vec<(OsString, OsString)>> {
+        let mut res = Vec::new();
         // SAFETY: `self.pamh` contains a correct handle (obtained from `pam_start`).
         // The man page for pam_getenvlist states that:
         //    The format of the memory is a malloc()'d array of char pointers, the last element
@@ -348,7 +347,7 @@ impl<C: Converser> PamContext<C> {
                 }
             };
             if let Some((k, v)) = data {
-                res.insert(k, v);
+                res.push((k, v));
             }
 
             // SAFETY: curr_str was obtained via libc::malloc() so we are responsible for freeing it.

src/su/context.rs

@@ -1,4 +1,5 @@
 use std::{
+    collections::HashMap,
     env,
     ffi::OsString,
     fs, io,
@@ -9,14 +10,16 @@ use crate::exec::RunOptions;
 use crate::log::user_warn;
 use crate::system::{Group, Process, User};
 use crate::{
-    common::{error::Error, resolve::CurrentUser, Environment},
+    common::{error::Error, resolve::CurrentUser},
     system::interface::ProcessId,
 };
 use crate::{
     common::{resolve::is_valid_executable, SudoPath},
     system::interface::UserId,
 };
 
+type Environment = HashMap<OsString, OsString>;
+
 use super::cli::SuRunOptions;
 
 const VALID_LOGIN_SHELLS_LIST: &str = "/etc/shells";

src/sudo/cli/mod.rs

@@ -858,8 +858,13 @@ impl From<SudoValidateOptions> for OptionsForContext {
     }
 }
 
-impl From<SudoRunOptions> for OptionsForContext {
-    fn from(opts: SudoRunOptions) -> Self {
+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,
@@ -871,11 +876,11 @@ impl From<SudoRunOptions> for OptionsForContext {
             stdin,
             user,
 
-            env_var_list: _,
-            preserve_env: _,
-        } = opts;
+            env_var_list,
+            preserve_env,
+        } = self;
 
-        Self {
+        let ctx_opts = OptionsForContext {
             action: ContextAction::Run,
 
             chdir,
@@ -887,6 +892,13 @@ impl From<SudoRunOptions> for OptionsForContext {
             shell,
             stdin,
             user,
-        }
+        };
+
+        let pipe_opts = OptionsForPipeline {
+            preserve_env,
+            user_requested_env_vars: env_var_list,
+        };
+
+        (ctx_opts, pipe_opts)
     }
 }

src/sudo/env/environment.rs

@@ -1,10 +1,10 @@
 use std::{
-    collections::{hash_map::Entry, HashSet},
+    collections::{hash_map::Entry, HashMap, HashSet},
     ffi::{OsStr, OsString},
     os::unix::prelude::OsStrExt,
 };
 
-use crate::common::{CommandAndArguments, Context, Environment};
+use crate::common::{CommandAndArguments, Context, Error};
 use crate::sudoers::Policy;
 use crate::system::PATH_MAX;
 
@@ -14,6 +14,13 @@ const PATH_MAILDIR: &str = env!("PATH_MAILDIR");
 const PATH_ZONEINFO: &str = env!("PATH_ZONEINFO");
 const PATH_DEFAULT: &str = env!("SUDO_PATH_DEFAULT");
 
+pub type Environment = HashMap<OsString, OsString>;
+
+/// obtain the system environment
+pub fn system_environment() -> Environment {
+    std::env::vars_os().collect()
+}
+
 /// check byte slice contains with given byte slice
 fn contains_subsequence(haystack: &[u8], needle: &[u8]) -> bool {
     haystack
@@ -165,6 +172,10 @@ fn should_keep(key: &OsStr, value: &OsStr, cfg: &impl Policy) -> bool {
         return false;
     }
 
+    if cfg.secure_path().is_some() && key == "PATH" {
+        return false;
+    }
+
     if key == "TZ" {
         return in_table(key, cfg.env_keep())
             || (in_table(key, cfg.env_check()) && is_safe_tz(value.as_bytes()));
@@ -192,18 +203,17 @@ fn should_keep(key: &OsStr, value: &OsStr, cfg: &impl Policy) -> bool {
 /// Environment variables with a value beginning with ‘()’ are removed
 pub fn get_target_environment(
     current_env: Environment,
-    additional_env: Environment,
+    additional_env: impl IntoIterator<Item = (OsString, OsString)>,
+    user_override: Vec<(String, String)>,
     context: &Context,
     settings: &impl Policy,
-) -> Environment {
-    let mut environment = Environment::default();
-
+) -> Result<Environment, Error> {
     // retrieve SUDO_PS1 value to set a PS1 value as additional environment
     let sudo_ps1 = current_env.get(OsStr::new("SUDO_PS1")).cloned();
 
     // variables preserved from the invoking user's environment by the
     // env_keep list take precedence over those in the PAM environment
-    environment.extend(additional_env);
+    let mut environment: HashMap<_, _> = additional_env.into_iter().collect();
 
     environment.extend(
         current_env
@@ -213,7 +223,19 @@ pub fn get_target_environment(
 
     add_extra_env(context, settings, sudo_ps1, &mut environment);
 
-    environment
+    let mut rejected_vars = Vec::new();
+    for (key, value) in user_override {
+        if should_keep(OsStr::new(&key), OsStr::new(&value), settings) {
+            environment.insert(key.into(), value.into());
+        } else {
+            rejected_vars.push(key);
+        }
+    }
+    if !rejected_vars.is_empty() {
+        return Err(Error::EnvironmentVar(rejected_vars));
+    }
+
+    Ok(environment)
 }
 
 #[cfg(test)]
@@ -225,6 +247,7 @@ mod tests {
     struct TestConfiguration {
         keep: HashSet<String>,
         check: HashSet<String>,
+        path: Option<String>,
     }
 
     impl Policy for TestConfiguration {
@@ -237,38 +260,46 @@ mod tests {
         }
 
         fn secure_path(&self) -> Option<String> {
-            None
+            self.path.clone()
         }
 
         fn use_pty(&self) -> bool {
             true
         }
     }
 
-    #[test]
-    fn test_filtering() {
-        let config = TestConfiguration {
-            keep: HashSet::from(["AAP".to_string(), "NOOT".to_string()]),
-            check: HashSet::from(["MIES".to_string(), "TZ".to_string()]),
-        };
-
-        let check_should_keep = |key: &str, value: &str, expected: bool| {
+    impl TestConfiguration {
+        pub fn check_should_keep(&self, key: &str, value: &str, expected: bool) {
             assert_eq!(
-                should_keep(OsStr::new(key), OsStr::new(value), &config),
+                should_keep(OsStr::new(key), OsStr::new(value), self),
                 expected,
                 "{} should {}",
                 key,
                 if expected { "be kept" } else { "not be kept" }
             );
+        }
+    }
+
+    #[test]
+    fn test_filtering() {
+        let mut config = TestConfiguration {
+            keep: HashSet::from(["AAP".to_string(), "NOOT".to_string()]),
+            check: HashSet::from(["MIES".to_string(), "TZ".to_string()]),
+            path: Some("/bin".to_string()),
         };
 
-        check_should_keep("AAP", "FOO", true);
-        check_should_keep("MIES", "BAR", true);
-        check_should_keep("AAP", "()=foo", false);
-        check_should_keep("TZ", "Europe/Amsterdam", true);
-        check_should_keep("TZ", "../Europe/Berlin", false);
-        check_should_keep("MIES", "FOO/BAR", false);
-        check_should_keep("MIES", "FOO%", false);
+        config.check_should_keep("AAP", "FOO", true);
+        config.check_should_keep("MIES", "BAR", true);
+        config.check_should_keep("AAP", "()=foo", false);
+        config.check_should_keep("TZ", "Europe/Amsterdam", true);
+        config.check_should_keep("TZ", "../Europe/Berlin", false);
+        config.check_should_keep("MIES", "FOO/BAR", false);
+        config.check_should_keep("MIES", "FOO/BAR", false);
+
+        config.keep.insert("PATH".to_string());
+        config.check_should_keep("PATH", "FOO", false);
+        config.path = None;
+        config.check_should_keep("PATH", "FOO", true);
     }
 
     #[allow(clippy::useless_format)]

src/sudo/env/tests.rs

@@ -1,8 +1,8 @@
 use crate::common::resolve::CurrentUser;
-use crate::common::{CommandAndArguments, Context, Environment};
+use crate::common::{CommandAndArguments, Context};
 use crate::sudo::{
     cli::{SudoAction, SudoRunOptions},
-    env::environment::get_target_environment,
+    env::environment::{get_target_environment, Environment},
 };
 use crate::system::interface::{GroupId, UserId};
 use crate::system::{Group, Hostname, Process, User};
@@ -66,7 +66,7 @@ fn parse_env_commands(input: &str) -> Vec<(&str, Environment)> {
         .map(|e| {
             let (cmd, vars) = e.split_once('\n').unwrap();
 
-            let vars: Environment = vars
+            let vars = vars
                 .lines()
                 .map(|line| line.trim().split_once('=').unwrap())
                 .map(|(k, v)| (k.into(), v.into()))
@@ -142,7 +142,7 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context {
 fn environment_to_set(environment: Environment) -> HashSet<String> {
     HashSet::from_iter(
         environment
-            .iter()
+            .into_iter()
             .map(|(k, v)| format!("{}={}", k.to_str().unwrap(), v.to_str().unwrap())),
     )
 }
@@ -160,8 +160,14 @@ fn test_environment_variable_filtering() {
             .unwrap();
         let settings = crate::sudoers::Judgement::default();
         let context = create_test_context(&options);
-        let resulting_env =
-            get_target_environment(initial_env.clone(), HashMap::new(), &context, &settings);
+        let resulting_env = get_target_environment(
+            initial_env.clone(),
+            HashMap::new(),
+            Vec::new(),
+            &context,
+            &settings,
+        )
+        .unwrap();
 
         let resulting_env = environment_to_set(resulting_env);
         let expected_env = environment_to_set(expected_env);

src/sudo/pam.rs

@@ -1,4 +1,3 @@
-use std::collections::HashMap;
 use std::ffi::OsString;
 
 use crate::common::context::LaunchType;
@@ -59,7 +58,7 @@ impl<C: Converser> AuthPlugin for PamAuthenticator<C> {
         Ok(())
     }
 
-    fn pre_exec(&mut self, target_user: &str) -> Result<HashMap<OsString, OsString>, Error> {
+    fn pre_exec(&mut self, target_user: &str) -> Result<Vec<(OsString, OsString)>, Error> {
         let pam = self
             .pam
             .as_mut()