make various parts of sudo less dependent on the Environment defini…

…tion

I have tried to change this into a newtype, but it doesn't add much safety (only adding a bit of boilerplate).
Security of the environment has to come from the cleanly written pipeline.

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,7 +16,11 @@ use std::{
 };
 
 use crate::{
-    common::Environment,
+    exec::no_pty::exec_no_pty,
+    log::dev_info,
+    system::{set_target_user, signal::SignalNumber, term::UserTerm},
+};
+use crate::{
     log::dev_warn,
     system::{
         interface::ProcessId,
@@ -25,11 +29,6 @@ use crate::{
         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};
 
 pub use interface::RunOptions;
@@ -44,7 +43,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/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};
 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
@@ -42,7 +49,7 @@ fn add_extra_env(
     context: &Context,
     cfg: &impl Policy,
     sudo_ps1: Option<OsString>,
-    environment: &mut Environment,
+    environment: &mut HashMap<OsString, OsString>,
 ) {
     // current user
     environment.insert("SUDO_COMMAND".into(), format_command(&context.command));
@@ -192,18 +199,16 @@ 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)>,
     context: &Context,
     settings: &impl Policy,
 ) -> Environment {
-    let mut environment = Environment::default();
-
     // 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

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())),
     )
 }

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()

src/sudo/pipeline.rs

@@ -1,10 +1,10 @@
-use std::ffi::OsStr;
+use std::ffi::{OsStr, OsString};
 use std::process::exit;
 
 use super::cli::{SudoRunOptions, SudoValidateOptions};
 use crate::common::context::OptionsForContext;
 use crate::common::resolve::CurrentUser;
-use crate::common::{Context, Environment, Error};
+use crate::common::{Context, Error};
 use crate::exec::{ExecOutput, ExitReason};
 use crate::log::{auth_info, auth_warn};
 use crate::sudo::env::environment;
@@ -32,7 +32,7 @@ pub trait PolicyPlugin {
 pub trait AuthPlugin {
     fn init(&mut self, context: &Context) -> Result<(), Error>;
     fn authenticate(&mut self, non_interactive: bool, max_tries: u16) -> Result<(), Error>;
-    fn pre_exec(&mut self, target_user: &str) -> Result<Environment, Error>;
+    fn pre_exec(&mut self, target_user: &str) -> Result<Vec<(OsString, OsString)>, Error>;
     fn cleanup(&mut self);
 }
 
@@ -73,7 +73,7 @@ impl<Policy: PolicyPlugin, Auth: AuthPlugin> Pipeline<Policy, Auth> {
         let additional_env = self.authenticator.pre_exec(&context.target_user.name)?;
 
         // build environment
-        let current_env = std::env::vars_os().collect();
+        let current_env = environment::system_environment();
         let target_env =
             environment::get_target_environment(current_env, additional_env, &context, &policy);