diff --git a/docs/man/sudoers.5.md b/docs/man/sudoers.5.md index f73d441a2..3d6bdd7c0 100644 --- a/docs/man/sudoers.5.md +++ b/docs/man/sudoers.5.md @@ -430,7 +430,7 @@ sudo's behavior can be modified by Default_Entry lines, as explained earlier. A * editor - A colon (‘:’) separated list of editor path names used by **sudoedit** and **visudo**. For **sudoedit**, this list is used to find an editor when none of the SUDO_EDITOR, VISUAL or EDITOR environment variables are set to an editor that exists and is executable. For **visudo**, it is used as a white list of allowed editors; **visudo** will choose the editor that matches the user's SUDO_EDITOR, VISUAL or EDITOR environment variable if possible, or the first editor in the list that exists and is executable if not. Unless invoked as **sudoedit**, sudo does not preserve the SUDO_EDITOR, VISUAL or EDITOR environment variables unless they are present in the **env_keep** list. The default on Linux is _/usr/bin/editor_, on FreeBSD _/usr/vim/vi_. + A colon (‘:’) separated list of editor path names used by **sudoedit** and **visudo**. For **sudoedit**, this list is used to find an editor when none of the SUDO_EDITOR, VISUAL or EDITOR environment variables are set to an editor that exists and is executable. For **visudo**, it is used as a white list of allowed editors; **visudo** will choose the editor that matches the user's SUDO_EDITOR, VISUAL or EDITOR environment variable if possible, or the first editor in the list that exists and is executable if not. Unless invoked as **sudoedit**, sudo does not preserve the SUDO_EDITOR, VISUAL or EDITOR environment variables unless they are present in the **env_keep** list. The default on Linux is _/usr/bin/editor:/usr/bin/nano:/usr/bin/vi_. On FreeBSD the default is _/usr/bin/vi_. ## Strings that can be used in a boolean context: diff --git a/src/defaults/mod.rs b/src/defaults/mod.rs index 3bd0d4c43..fde9812f7 100644 --- a/src/defaults/mod.rs +++ b/src/defaults/mod.rs @@ -22,7 +22,7 @@ use settings_dsl::{ }; pub const SYSTEM_EDITOR: &str = if cfg!(target_os = "linux") { - "/usr/bin/editor" + "/usr/bin/editor:/usr/bin/nano:/usr/bin/vi" } else { "/usr/bin/vi" }; diff --git a/src/sudoers/mod.rs b/src/sudoers/mod.rs index e6e9dad5a..ffec883be 100644 --- a/src/sudoers/mod.rs +++ b/src/sudoers/mod.rs @@ -295,7 +295,7 @@ impl Sudoers { on_host: &system::Hostname, am_user: &User, target_user: &User, - ) -> PathBuf { + ) -> Option { self.specify_host_user_runas(on_host, am_user, Some(target_user)); select_editor(&self.settings, self.settings.env_editor()) @@ -304,7 +304,7 @@ impl Sudoers { /// Retrieve the chosen editor from a settings object, filtering based on whether the /// environment is trusted (sudoedit) or maybe less so (visudo) -fn select_editor(settings: &Settings, trusted_env: bool) -> PathBuf { +fn select_editor(settings: &Settings, trusted_env: bool) -> Option { let blessed_editors = settings.editor(); let is_whitelisted = |path: &Path| -> bool { @@ -329,7 +329,7 @@ fn select_editor(settings: &Settings, trusted_env: bool) -> PathBuf { }; if is_whitelisted(&editor) { - return editor; + return Some(editor); } } } @@ -339,13 +339,11 @@ fn select_editor(settings: &Settings, trusted_env: bool) -> PathBuf { for editor in blessed_editors.split(':') { let editor = PathBuf::from(editor); if is_valid_executable(&editor) { - return editor; + return Some(editor); } } - // fallback on hardcoded path -- always provide something to the caller - - PathBuf::from(defaults::SYSTEM_EDITOR) + None } // a `take_while` variant that does not consume the first non-matching item diff --git a/src/sudoers/policy.rs b/src/sudoers/policy.rs index 16196e554..2c569f342 100644 --- a/src/sudoers/policy.rs +++ b/src/sudoers/policy.rs @@ -147,7 +147,11 @@ impl Judgement { } pub(crate) fn preferred_editor(&self) -> std::path::PathBuf { + //if no editor could be selected, fall back to /bin/vi; + //note that /bin/vi is also likely to have been tried as part of + //the "editor" setting, so this is a last-resort super::select_editor(&self.settings, true) + .unwrap_or_else(|| std::path::PathBuf::from("/usr/bin/vi")) } } diff --git a/src/visudo/mod.rs b/src/visudo/mod.rs index 5a7763982..04c512153 100644 --- a/src/visudo/mod.rs +++ b/src/visudo/mod.rs @@ -146,32 +146,27 @@ fn run(file_arg: Option<&str>, perms: bool, owner: bool) -> io::Result<()> { .read(true) .write(true) .open(sudoers_path) - .map_err(|e| { - io::Error::new( - e.kind(), - format!("Failed to open existing sudoers file at {sudoers_path:?}: {e}"), + .map_err(|err| { + io_msg!( + err, + "Failed to open existing sudoers file at {sudoers_path:?}" ) })?; (file, true) } else { // Create a sudoers file if it doesn't exist. - let file = File::create(sudoers_path).map_err(|e| { - io::Error::new( - e.kind(), - format!("Failed to create sudoers file at {sudoers_path:?}: {e}"), - ) - })?; + let file = File::create(sudoers_path) + .map_err(|err| io_msg!(err, "Failed to create sudoers file at {sudoers_path:?}"))?; + // ogvisudo sets the permissions of the file so it can be read and written by the user and // read by the group if the `-f` argument was passed. if file_arg.is_some() { file.set_permissions(Permissions::from_mode(0o640)) - .map_err(|e| { - io::Error::new( - e.kind(), - format!( - "Failed to set permissions on new sudoers file at {sudoers_path:?}: {e}" - ), + .map_err(|err| { + io_msg!( + err, + "Failed to set permissions on new sudoers file at {sudoers_path:?}" ) })?; } @@ -260,21 +255,21 @@ fn edit_sudoers_file( let host_name = Hostname::resolve(); - let editor_path = if existed { + if existed { // If the sudoers file existed, read its contents and write them into the temporary file. sudoers_file.read_to_end(&mut sudoers_contents)?; // Rewind the sudoers file so it can be written later. sudoers_file.rewind()?; // Write to the temporary file. tmp_file.write_all(&sudoers_contents)?; + } - let (sudoers, _errors) = Sudoers::read(sudoers_contents.as_slice(), sudoers_path)?; - - sudoers.visudo_editor_path(&host_name, ¤t_user, ¤t_user) - } else { - // there is no /etc/sudoers config yet, so use a system default - PathBuf::from(crate::defaults::SYSTEM_EDITOR) - }; + let editor_path = Sudoers::read(sudoers_contents.as_slice(), sudoers_path)? + .0 + .visudo_editor_path(&host_name, ¤t_user, ¤t_user) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::NotFound, "no usable editor could be found") + })?; loop { Command::new(&editor_path) diff --git a/test-framework/sudo-compliance-tests/src/visudo/sudoers/editor.rs b/test-framework/sudo-compliance-tests/src/visudo/sudoers/editor.rs index ba21da09f..10f179464 100644 --- a/test-framework/sudo-compliance-tests/src/visudo/sudoers/editor.rs +++ b/test-framework/sudo-compliance-tests/src/visudo/sudoers/editor.rs @@ -71,10 +71,7 @@ fn no_valid_editor_in_list() { ); } else { // this output shows that visudo has rejected /dev/null as an editor - assert_eq!( - "visudo: specified editor (/usr/bin/editor) could not be used", - output.stderr() - ); + assert_eq!("visudo: no usable editor could be found", output.stderr()); } } @@ -92,22 +89,23 @@ fn editors_must_be_specified_by_absolute_path() { ); } else { // this output shows that visudo has rejected /dev/null as an editor - assert_eq!( - "visudo: specified editor (/usr/bin/editor) could not be used", - output.stderr() - ); + assert_eq!("visudo: no usable editor could be found", output.stderr()); } } #[test] fn on_invalid_editor_does_not_falls_back_to_configured_default_value() { - let env = Env("Defaults editor=true") + let expected = "configured editor was called"; + //NOTE: ogsudo DOES fallback to the default editor if a syntax error is + //made here, e.g. "Defaults editor=true" will simply not set the "editor" value. + //This is surprising. + let env = Env("Defaults editor=/bin/no-editor") .file( DEFAULT_EDITOR, - TextFile( + TextFile(format!( "#!/bin/sh -rm -f {LOGS_PATH}", - ) +echo '{expected}' >> {LOGS_PATH}" + )) .chmod(CHMOD_EXEC), ) .build(); @@ -117,11 +115,9 @@ rm -f {LOGS_PATH}", .output(&env) .assert_success(); - Command::new("visudo").output(&env).assert_success(); + Command::new("visudo").output(&env).assert_exit_code(1); - Command::new("sh") - .arg("-c") - .arg(format!("test -f {LOGS_PATH}")) - .output(&env) - .assert_success(); + let actual = Command::new("cat").arg(LOGS_PATH).output(&env).stdout(); + + assert_ne!(expected, actual); }