Implement support for the umask and umask_override defaults (#1151)

Author: squell

src/common/context.rs

@@ -1,7 +1,7 @@
 use std::io;
 
 use crate::common::{HARDENED_ENUM_VALUE_0, HARDENED_ENUM_VALUE_1, HARDENED_ENUM_VALUE_2};
-use crate::exec::RunOptions;
+use crate::exec::{RunOptions, Umask};
 use crate::sudo::{SudoListOptions, SudoRunOptions, SudoValidateOptions};
 use crate::sudoers::Sudoers;
 use crate::system::{Group, Hostname, Process, User};
@@ -32,6 +32,7 @@ pub struct Context {
     // policy
     pub use_pty: bool,
     pub noexec: bool,
+    pub umask: Umask,
 }
 
 #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)]
@@ -95,6 +96,7 @@ impl Context {
             process: Process::new(),
             use_pty: true,
             noexec: false,
+            umask: Umask::Preserve,
         })
     }
 
@@ -120,6 +122,7 @@ impl Context {
             process: Process::new(),
             use_pty: true,
             noexec: false,
+            umask: Umask::Preserve,
         })
     }
 
@@ -165,6 +168,7 @@ impl Context {
             process: Process::new(),
             use_pty: true,
             noexec: false,
+            umask: Umask::Preserve,
         })
     }
 
@@ -181,6 +185,7 @@ impl Context {
             is_login: self.launch == LaunchType::Login,
             user: &self.target_user,
             group: &self.target_group,
+            umask: self.umask,
 
             use_pty: self.use_pty,
             noexec: self.noexec,

src/defaults/mod.rs

@@ -43,6 +43,8 @@ defaults! {
 
     setenv                    = false
     apparmor_profile          = None (!= None)
+    umask                     = 0o022 (!= 0o777) {octal_mode}
+    umask_override            = false
 
     passwd_tries              = 3 [0..=1000]
 
@@ -66,6 +68,12 @@ defaults! {
                                 "PYTHONINSPECT", "PYTHONUSERBASE", "RUBYLIB", "RUBYOPT", "*=()*"] #ignored
 }
 
+fn octal_mode(input: &str) -> Option<i64> {
+    <libc::mode_t>::from_str_radix(input.strip_prefix('0')?, 8)
+        .ok()
+        .map(Into::into)
+}
+
 /// A custom parser to parse seconds as fractional "minutes", the format used by
 /// passwd_timeout and timestamp_timeout.
 fn fractional_minutes(input: &str) -> Option<i64> {

src/exec/mod.rs

@@ -18,7 +18,9 @@ use std::{
 };
 
 use crate::{
-    common::bin_serde::BinPipe,
+    common::{
+        bin_serde::BinPipe, HARDENED_ENUM_VALUE_0, HARDENED_ENUM_VALUE_1, HARDENED_ENUM_VALUE_2,
+    },
     exec::no_pty::exec_no_pty,
     log::{dev_info, dev_warn, user_error},
     system::{
@@ -47,6 +49,18 @@ impl SpawnNoexecHandler {
     fn spawn(self) {}
 }
 
+#[derive(Debug, Copy, Clone)]
+#[cfg_attr(test, derive(PartialEq))]
+#[repr(u32)]
+pub enum Umask {
+    /// Keep the umask of the parent process.
+    Preserve = HARDENED_ENUM_VALUE_0,
+    /// Mask out more of the permission bits in the new umask.
+    Extend(libc::mode_t) = HARDENED_ENUM_VALUE_1,
+    /// Override the umask of the parent process entirely with the given umask.
+    Override(libc::mode_t) = HARDENED_ENUM_VALUE_2,
+}
+
 pub struct RunOptions<'a> {
     pub command: &'a Path,
     pub arguments: &'a [String],
@@ -55,6 +69,7 @@ pub struct RunOptions<'a> {
     pub is_login: bool,
     pub user: &'a User,
     pub group: &'a Group,
+    pub umask: Umask,
 
     pub use_pty: bool,
     pub noexec: bool,
@@ -131,6 +146,29 @@ pub fn run_command(
         }
     }
 
+    // SAFETY: Umask is async-signal-safe.
+    unsafe {
+        let umask = options.umask;
+
+        command.pre_exec(move || {
+            match umask {
+                Umask::Preserve => {}
+                Umask::Extend(umask) => {
+                    // The only options to get the existing umask are overwriting it or
+                    // parsing a /proc file. Given that this is a single-threaded context,
+                    // overwrite it with a safe value is fine and the simpler option.
+                    let existing_umask = libc::umask(0o777);
+                    libc::umask(existing_umask | umask);
+                }
+                Umask::Override(umask) => {
+                    libc::umask(umask);
+                }
+            }
+
+            Ok(())
+        });
+    }
+
     let sudo_pid = ProcessId::new(std::process::id() as i32);
 
     if options.use_pty {

src/su/context.rs

@@ -7,7 +7,7 @@ use std::{
 };
 
 use crate::common::{error::Error, resolve::CurrentUser};
-use crate::exec::RunOptions;
+use crate::exec::{RunOptions, Umask};
 use crate::log::user_warn;
 use crate::system::{Group, Process, User};
 use crate::{common::resolve::is_valid_executable, system::interface::UserId};
@@ -216,6 +216,7 @@ impl SuContext {
             is_login: self.options.login,
             user: &self.user,
             group: &self.group,
+            umask: Umask::Preserve,
 
             use_pty: true,
             noexec: false,

src/sudo/env/environment.rs

@@ -280,6 +280,7 @@ mod tests {
                         chdir: crate::sudoers::DirChange::Strict(None),
                         trust_environment: false,
                         use_pty: true,
+                        umask: crate::exec::Umask::Preserve,
                         #[cfg(feature = "apparmor")]
                         apparmor_profile: None,
                         noexec: false,

src/sudo/env/tests.rs

@@ -1,5 +1,6 @@
 use crate::common::resolve::CurrentUser;
 use crate::common::{CommandAndArguments, Context};
+use crate::exec::Umask;
 use crate::sudo::{
     cli::{SudoAction, SudoRunOptions},
     env::environment::{get_target_environment, Environment},
@@ -134,6 +135,7 @@ fn create_test_context(sudo_options: SudoRunOptions) -> Context {
         use_pty: true,
         noexec: false,
         bell: false,
+        umask: Umask::Preserve,
     }
 }
 
@@ -170,6 +172,7 @@ fn test_environment_variable_filtering() {
                 use_pty: true,
                 chdir: crate::sudoers::DirChange::Strict(None),
                 trust_environment: false,
+                umask: crate::exec::Umask::Preserve,
                 #[cfg(feature = "apparmor")]
                 apparmor_profile: None,
                 noexec: false,

src/sudo/pipeline.rs

@@ -238,6 +238,7 @@ fn apply_policy_to_context(
     // could be needed, but here we copy these -- perhaps we should split up the Context type
     context.use_pty = controls.use_pty;
     context.noexec = controls.noexec;
+    context.umask = controls.umask;
 
     Ok(())
 }

src/sudoers/policy.rs

@@ -4,6 +4,7 @@ use super::Judgement;
 use crate::common::{
     SudoPath, HARDENED_ENUM_VALUE_0, HARDENED_ENUM_VALUE_1, HARDENED_ENUM_VALUE_2,
 };
+use crate::exec::Umask;
 use crate::sudoers::ast::{ExecControl, Tag};
 use crate::system::{time::Duration, Hostname, User};
 /// Data types and traits that represent what the "terms and conditions" are after a successful
@@ -59,6 +60,7 @@ pub struct Restrictions<'a> {
     pub env_check: &'a HashSet<String>,
     pub chdir: DirChange<'a>,
     pub path: Option<&'a str>,
+    pub umask: Umask,
     #[cfg(feature = "apparmor")]
     pub apparmor_profile: Option<String>,
 }
@@ -109,6 +111,20 @@ impl Judgement {
                         Some(super::ChDir::Path(path)) => DirChange::Strict(Some(path)),
                     },
                     path: self.settings.secure_path(),
+                    umask: {
+                        let mask = self
+                            .settings
+                            .umask()
+                            .try_into()
+                            .expect("the umask parser should have prevented overflow");
+                        if mask == 0o777 {
+                            Umask::Preserve
+                        } else if self.settings.umask_override() {
+                            Umask::Override(mask)
+                        } else {
+                            Umask::Extend(mask)
+                        }
+                    },
                     #[cfg(feature = "apparmor")]
                     apparmor_profile: tag
                         .apparmor_profile

test-framework/sudo-compliance-tests/src/sudo.rs

@@ -28,4 +28,5 @@ mod sudo_ps1;
 mod sudoers;
 mod syslog;
 mod timestamp;
+mod umask;
 mod use_pty;

test-framework/sudo-compliance-tests/src/sudo/umask.rs

@@ -0,0 +1,53 @@
+use sudo_test::{Command, Env};
+
+use crate::SUDOERS_ALL_ALL_NOPASSWD;
+
+fn test_umask(config: &str, user_umask: &str, target_umask: &str) {
+    let env = Env([SUDOERS_ALL_ALL_NOPASSWD, config]).build();
+
+    let output = Command::new("sh")
+        .args(["-c", &format!("umask {user_umask}; sudo sh -c umask")])
+        .output(&env);
+    output.assert_success();
+
+    assert_eq!(output.stdout(), target_umask);
+}
+
+#[test]
+fn umask_unchanged() {
+    test_umask("Defaults umask=0777", "0123", "0123");
+    test_umask("Defaults !umask", "0123", "0123");
+}
+
+#[test]
+fn stricter_umask_respected() {
+    test_umask("Defaults umask=0776", "0022", "0776");
+}
+
+#[test]
+fn overlapping_umask_unioned() {
+    test_umask("Defaults umask=0770", "0022", "0772");
+}
+
+#[test]
+fn looser_umask_unchanged() {
+    test_umask("Defaults umask=0000", "0022", "0022");
+}
+
+#[test]
+fn umask_override() {
+    test_umask(
+        "Defaults umask=0700\nDefaults umask_override",
+        "0022",
+        "0700",
+    );
+}
+
+#[test]
+fn umask_override_0777() {
+    test_umask(
+        "Defaults umask=0777\nDefaults umask_override",
+        "0022",
+        "0022",
+    );
+}