Sudoedit compliance tests (batch 2) (#1216)

squell · web-flow · commit 5bcad63b12ac · 2025-07-24T18:12:43.000+02:00
diff --git a/src/sudo/pipeline/edit.rs b/src/sudo/pipeline/edit.rs
@@ -3,6 +3,7 @@ use std::process::exit;
 use super::super::cli::SudoEditOptions;
 use crate::common::{Context, Error};
 use crate::exec::ExitReason;
+use crate::log::{user_error, user_info};
 use crate::sudoers::Authorization;
 use crate::system::audit;
 
@@ -34,16 +35,20 @@ pub fn run_edit(edit_opts: SudoEditOptions) -> Result<(), Error> {
                 &context.target_group,
             ) {
                 Ok(file) => opened_files.push((path, file)),
-                Err(error) => eprintln_ignore_io_error!("error opening {arg}: {error}"),
+                // ErrorKind::FilesystemLoop was only stabilized in 1.83
+                Err(error) if error.raw_os_error() == Some(libc::ELOOP) => {
+                    user_error!("{arg}: editing symbolic links is not permitted")
+                }
+                Err(error) => user_error!("error opening {arg}: {error}"),
             }
         } else {
-            eprintln_ignore_io_error!("invalid path: {arg}");
+            user_error!("invalid path: {arg}");
         }
     }
 
     if opened_files.len() != context.files_to_edit.len() {
-        eprintln_ignore_io_error!("please address the problems and try again");
-        return Ok(());
+        user_info!("please address the problems and try again");
+        return Err(Error::Silent);
     }
 
     // run command and return corresponding exit code
diff --git a/src/system/audit.rs b/src/system/audit.rs
@@ -10,7 +10,9 @@ use std::os::unix::{
 };
 use std::path::{Component, Path};
 
-use super::{cerr, inject_group, set_supplementary_groups, Group, GroupId, User, UserId};
+use super::{
+    cerr, inject_group, interface::UnixUser, set_supplementary_groups, Group, GroupId, User, UserId,
+};
 use crate::common::resolve::CurrentUser;
 
 /// Temporary change privileges --- essentially a 'mini sudo'
@@ -109,6 +111,7 @@ pub fn secure_open_cookie_file(path: impl AsRef<Path>) -> io::Result<File> {
         .read(true)
         .write(true)
         .create(true)
+        .truncate(false)
         .mode(mode(Category::Owner, Op::Write) | mode(Category::Owner, Op::Read));
 
     secure_open_impl(path.as_ref(), &mut open_options, true, true)
@@ -214,7 +217,16 @@ pub fn secure_open_for_sudoedit(
     target_group: &Group,
 ) -> io::Result<File> {
     sudo_call(target_user, target_group, || {
-        traversed_secure_open(path, current_user)
+        if current_user.is_root() {
+            OpenOptions::new()
+                .read(true)
+                .write(true)
+                .create(true)
+                .truncate(false)
+                .open(path)
+        } else {
+            traversed_secure_open(path, current_user)
+        }
     })?
 }
 
@@ -248,7 +260,7 @@ fn traversed_secure_open(path: impl AsRef<Path>, forbidden_user: &User) -> io::R
         {
             Err(io::Error::new(
                 ErrorKind::PermissionDenied,
-                "cannot open a file in a path writeable by the user",
+                "cannot open a file in a path writable by the user",
             ))
         } else {
             Ok(())
@@ -291,13 +303,13 @@ mod test {
         assert!(std::fs::File::open("/etc/hosts").is_ok());
         assert!(secure_open_sudoers("/etc/hosts", false).is_ok());
 
-        // /tmp should be readable, but not secure (writeable by group other than root)
+        // /tmp should be readable, but not secure (writable by group other than root)
         assert!(std::fs::File::open("/tmp").is_ok());
         assert!(secure_open_sudoers("/tmp", false).is_err());
 
         #[cfg(target_os = "linux")]
         {
-            // /var/log/wtmp should be readable, but not secure (writeable by group other than root)
+            // /var/log/wtmp should be readable, but not secure (writable by group other than root)
             // It doesn't exist on many non-Linux systems however.
             if std::fs::File::open("/var/log/wtmp").is_ok() {
                 assert!(secure_open_sudoers("/var/log/wtmp", false).is_err());
diff --git a/test-framework/sudo-compliance-tests/src/sudoedit.rs b/test-framework/sudo-compliance-tests/src/sudoedit.rs
@@ -6,6 +6,8 @@ use crate::{
     Result, DEFAULT_EDITOR, GROUPNAME, PANIC_EXIT_CODE, SUDOERS_ALL_ALL_NOPASSWD, USERNAME,
 };
 
+mod limits;
+
 const LOGS_PATH: &str = "/tmp/logs.txt";
 const CHMOD_EXEC: &str = "555";
 const EDITOR_DUMMY: &str = "#!/bin/sh
diff --git a/test-framework/sudo-compliance-tests/src/sudoedit/limits.rs b/test-framework/sudo-compliance-tests/src/sudoedit/limits.rs
@@ -0,0 +1,163 @@
+use sudo_test::{Command, Directory, Env, TextFile, ROOT_GROUP};
+
+use crate::{DEFAULT_EDITOR, OTHER_USERNAME, SUDOERS_ALL_ALL_NOPASSWD, USERNAME};
+
+const CHMOD_EXEC: &str = "555";
+const EDITOR_DUMMY: &str = "#!/bin/sh
+echo \"#\" >> \"$1\"";
+
+#[test]
+fn cannot_edit_writable_paths() {
+    let env = Env(SUDOERS_ALL_ALL_NOPASSWD)
+        .user(USERNAME)
+        .directory(Directory("/tmp/bar").chmod("755"))
+        .file(DEFAULT_EDITOR, TextFile(EDITOR_DUMMY).chmod(CHMOD_EXEC))
+        .build();
+
+    for file in ["/tmp/foo.sh", "/tmp/bar/foo.sh", "/var/tmp/foo.sh"] {
+        let output = Command::new("sudoedit")
+            .as_user(USERNAME)
+            .arg(file)
+            .output(&env);
+
+        output.assert_exit_code(1);
+
+        if sudo_test::is_original_sudo() {
+            if file != "/tmp/bar/foo.sh" {
+                assert_contains!(
+                    output.stderr(),
+                    "editing files in a writable directory is not permitted"
+                );
+            } else {
+                // I don't know why ogsudo gives this error -- probably because opening failed
+                assert_contains!(output.stderr(), "No such file or directory");
+            }
+        } else {
+            assert_contains!(
+                output.stderr(),
+                "cannot open a file in a path writable by the user"
+            );
+        }
+    }
+}
+
+#[test]
+fn can_edit_writable_paths_as_root() {
+    // note: we already have tests that sudoedit "works" so we are skipping
+    // the content check here: the point here is that sudoedit does not stop
+    // the user.
+
+    let env = Env(SUDOERS_ALL_ALL_NOPASSWD)
+        .user(USERNAME)
+        .directory(Directory("/tmp/bar").chmod("755"))
+        .file(DEFAULT_EDITOR, TextFile(EDITOR_DUMMY).chmod(CHMOD_EXEC))
+        .build();
+
+    let file = "/tmp/foo.sh";
+    Command::new("sudoedit")
+        .arg(file)
+        .output(&env)
+        .assert_success();
+}
+
+#[test]
+fn cannot_edit_symlinks() {
+    let env = Env(SUDOERS_ALL_ALL_NOPASSWD)
+        .user(USERNAME)
+        .file(DEFAULT_EDITOR, TextFile(EDITOR_DUMMY).chmod(CHMOD_EXEC))
+        .build();
+
+    let file = "/usr/bin/sudoedit";
+
+    let output = Command::new("sudoedit")
+        .as_user(USERNAME)
+        .arg(file)
+        .output(&env);
+
+    assert_contains!(output.stderr(), "editing symbolic links is not permitted");
+
+    output.assert_exit_code(1);
+}
+
+#[test]
+fn cannot_edit_files_target_user_cannot_access() {
+    let file = "/test.txt";
+
+    let env = Env(SUDOERS_ALL_ALL_NOPASSWD)
+        .user(USERNAME)
+        .user(OTHER_USERNAME)
+        .group(USERNAME)
+        .group(OTHER_USERNAME)
+        .file(DEFAULT_EDITOR, TextFile(EDITOR_DUMMY).chmod(CHMOD_EXEC))
+        .file(
+            file,
+            TextFile("")
+                .chown(format!("{USERNAME}:{ROOT_GROUP}"))
+                .chmod("460"),
+        )
+        .build();
+
+    let test_cases = [
+        // incorrect user
+        &["-u", OTHER_USERNAME][..],
+        // correct user, but does not have write permits
+        &["-u", USERNAME][..],
+        // incorrect group
+        &["-u", OTHER_USERNAME, "-g", USERNAME][..],
+        // group permission doesn't override matching user permissions
+        &["-u", USERNAME, "-g", ROOT_GROUP][..],
+        &["-g", ROOT_GROUP][..],
+    ];
+
+    for args in test_cases {
+        let output = Command::new("sudoedit")
+            .args(args)
+            .arg(file)
+            .as_user(USERNAME)
+            .output(&env);
+
+        assert_contains!(output.stderr(), "Permission denied");
+        output.assert_exit_code(1);
+    }
+}
+
+#[test]
+fn can_edit_files_target_user_or_group_can_access() {
+    // note: we already have tests that sudoedit "works" so we are skipping
+    // the content check here: the point here is that sudoedit does not stop
+    // the user.
+
+    let file = "/test.txt";
+    let env = Env(SUDOERS_ALL_ALL_NOPASSWD)
+        .user(USERNAME)
+        .user(OTHER_USERNAME)
+        .group(OTHER_USERNAME)
+        .file(DEFAULT_EDITOR, TextFile(EDITOR_DUMMY).chmod(CHMOD_EXEC))
+        .file(
+            file,
+            TextFile("")
+                .chown(format!("{OTHER_USERNAME}:{OTHER_USERNAME}"))
+                .chmod("660"),
+        )
+        .build();
+
+    for user in ["root", OTHER_USERNAME] {
+        Command::new("sudoedit")
+            .args(["-u", user, file])
+            .as_user(USERNAME)
+            .output(&env)
+            .assert_success();
+    }
+
+    Command::new("sudoedit")
+        .args(["-g", OTHER_USERNAME, file])
+        .as_user(USERNAME)
+        .output(&env)
+        .assert_success();
+
+    Command::new("sudoedit")
+        .args(["-u", USERNAME, "-g", OTHER_USERNAME, file])
+        .as_user(USERNAME)
+        .output(&env)
+        .assert_success();
+}
