Detect "system editor" at runtime on Linux (#1392)

Author: squell

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:
 

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"
 };

src/sudoers/mod.rs

@@ -295,7 +295,7 @@ impl Sudoers {
         on_host: &system::Hostname,
         am_user: &User,
         target_user: &User,
-    ) -> PathBuf {
+    ) -> Option<PathBuf> {
         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<PathBuf> {
     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

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"))
     }
 }
 

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, &current_user, &current_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, &current_user, &current_user)
+        .ok_or_else(|| {
+            io::Error::new(io::ErrorKind::NotFound, "no usable editor could be found")
+        })?;
 
     loop {
         Command::new(&editor_path)

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);
 }