Add support for editor (#1198)

Author: squell

src/common/resolve.rs

@@ -210,7 +210,7 @@ pub(crate) fn resolve_path(command: &Path, path: &str) -> Option<PathBuf> {
     // replaced, or passed unchanged to the program that sudo executes.
 
     path.split(':')
-        .map(PathBuf::from)
+        .map(Path::new)
         // ignore all relative paths ("", "." or "./")
         .filter(|path| path.is_absolute())
         // construct a possible executable absolute path candidate

src/defaults/mod.rs

@@ -21,6 +21,12 @@ use settings_dsl::{
     result_of, storage_of,
 };
 
+pub const SYSTEM_EDITOR: &str = if cfg!(target_os = "linux") {
+    "/usr/bin/editor"
+} else {
+    "/usr/bin/vi"
+};
+
 defaults! {
     always_query_group_plugin = false  #ignored
     always_set_home           = false  #ignored
@@ -34,7 +40,6 @@ defaults! {
     use_pty                   = true
     visiblepw                 = false  #ignored
     pwfeedback                = false
-    env_editor                = true
     rootpw                    = false
     targetpw                  = false
     noexec                    = false
@@ -55,6 +60,9 @@ defaults! {
     passwd_timeout            = (5*60) (!= 0) {fractional_minutes}
     timestamp_timeout         = (15*60) (!= 0) {fractional_minutes}
 
+    editor                    = SYSTEM_EDITOR
+    env_editor                = true
+
     env_keep                  = ["COLORS", "DISPLAY", "HOSTNAME", "KRB5CCNAME", "LS_COLORS", "PATH",
                                  "PS1", "PS2", "XAUTHORITY", "XAUTHORIZATION", "XDG_CURRENT_DESKTOP"]
 

src/defaults/settings_dsl.rs

@@ -4,7 +4,8 @@ macro_rules! storage_of {
     ($id:ident, [ $($value: expr),* ]) => { std::collections::HashSet<String> };
     ($id:ident, $(=int $check: expr;)+ $_: expr) => { i64 };
     ($id:ident, $(=enum $k: ident;)+ $_: ident) => { $crate::defaults::enums::$id };
-    ($id:ident, $_: expr) => { Option<Box<str>> };
+    ($id:ident, None) => { Option<Box<str>> };
+    ($id:ident, $_: expr) => { Box<str> };
 }
 
 macro_rules! referent_of {
@@ -13,7 +14,8 @@ macro_rules! referent_of {
     ($id:ident, [ $($value: expr),* ]) => { &std::collections::HashSet<String> };
     ($id:ident, $(=int $check: expr;)+ $_: expr) => { i64 };
     ($id:ident, $(=enum $k: ident;)+ $_: ident) => { $crate::defaults::enums::$id };
-    ($id:ident, $_: expr) => { Option<&str> };
+    ($id:ident, None) => { Option<&str> };
+    ($id:ident, $_: expr) => { &str };
 }
 
 macro_rules! initializer_of {
@@ -23,7 +25,7 @@ macro_rules! initializer_of {
     ($id:ident, $(=int $check: expr;)+ $value: expr) => { $value };
     ($id:ident, $(=enum $k: ident;)+ $value: ident) => { $crate::defaults::enums::$id::$value };
     ($id:ident, None) => { None };
-    ($id:ident, $value: expr) => { Some($value.into()) };
+    ($id:ident, $value: expr) => { $value.into() };
     ($id:ident, $($_: tt)*) => { return None };
 }
 
@@ -40,9 +42,12 @@ macro_rules! result_of {
     ($id:expr, $(=value $k: expr;)+ $_: expr) => {
         $id
     };
-    ($id:expr, $_: expr) => {
+    ($id:expr, None) => {
         $id.as_deref()
     };
+    ($id:expr, $_: expr) => {
+        $id.as_ref()
+    };
 }
 
 macro_rules! modifier_of {
@@ -103,7 +108,7 @@ macro_rules! modifier_of {
     ($id:ident, $value: expr) => {
         $crate::defaults::SettingKind::Text(|text| {
             let text = text.into();
-            Some(Box::new(move |obj: &mut Settings| obj.$id = Some(text)))
+            Some(Box::new(move |obj: &mut Settings| obj.$id = text))
         })
     };
 }

src/sudo/pipeline/edit.rs

@@ -33,9 +33,16 @@ pub fn run_edit(edit_opts: SudoEditOptions) -> Result<(), Error> {
     let command_exit_reason = {
         super::log_command_execution(&context);
 
+        let editor = policy.preferred_editor();
+
         eprintln_ignore_io_error!(
-            "this would launch sudoedit as requested, to edit the files: {:?}",
-            context.files_to_edit.as_slice()
+            "this would launch sudoedit as requested, to edit the files: {:?} using editor {}",
+            context
+                .files_to_edit
+                .into_iter()
+                .flatten()
+                .collect::<Vec<_>>(),
+            editor.display(),
         );
 
         Ok::<_, std::io::Error>(ExitReason::Code(42))

src/sudoers/mod.rs

@@ -13,11 +13,11 @@ use std::collections::{HashMap, HashSet};
 use std::io;
 use std::path::{Path, PathBuf};
 
-use crate::common::resolve::resolve_path;
+use crate::common::resolve::{is_valid_executable, resolve_path};
 use crate::defaults;
 use crate::log::auth_warn;
+use crate::system;
 use crate::system::interface::{GroupId, UnixGroup, UnixUser, UserId};
-use crate::system::{self, can_execute};
 use ast::*;
 use tokens::*;
 
@@ -290,33 +290,62 @@ impl Sudoers {
         user_specs.flat_map(|cmd_specs| group_cmd_specs_per_runas(cmd_specs, &self.aliases.cmnd))
     }
 
-    pub(crate) fn solve_editor_path<User: UnixUser + PartialEq<User>>(
+    pub(crate) fn visudo_editor_path<User: UnixUser + PartialEq<User>>(
         mut self,
         on_host: &system::Hostname,
         am_user: &User,
         target_user: &User,
-    ) -> Option<PathBuf> {
+    ) -> PathBuf {
         self.specify_host_user_runas(on_host, am_user, Some(target_user));
-        if self.settings.env_editor() {
-            for key in ["SUDO_EDITOR", "VISUAL", "EDITOR"] {
-                if let Some(var) = std::env::var_os(key) {
-                    let path = Path::new(&var);
-                    if can_execute(path) {
-                        return Some(path.to_owned());
-                    }
-                    let path = resolve_path(
-                        path,
-                        &std::env::var("PATH").unwrap_or(env!("SUDO_PATH_DEFAULT").to_string()),
-                    );
-                    if let Some(path) = path {
-                        return Some(path);
-                    }
-                }
+
+        select_editor(&self.settings, self.settings.env_editor())
+    }
+}
+
+/// 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 {
+    let blessed_editors = settings.editor();
+
+    let is_whitelisted = |path: &Path| -> bool {
+        trusted_env || blessed_editors.split(':').any(|x| Path::new(x) == path)
+    };
+
+    // find editor in environment, if possible
+
+    for key in ["SUDO_EDITOR", "VISUAL", "EDITOR"] {
+        if let Some(editor) = std::env::var_os(key) {
+            let editor = PathBuf::from(editor);
+
+            let editor = if is_valid_executable(&editor) {
+                editor
+            } else if let Some(editor) = resolve_path(
+                &editor,
+                &std::env::var("PATH").unwrap_or(env!("SUDO_PATH_DEFAULT").to_string()),
+            ) {
+                editor
+            } else {
+                continue;
+            };
+
+            if is_whitelisted(&editor) {
+                return editor;
             }
         }
+    }
 
-        None
+    // no acceptable editor found in environment, fallback on config
+
+    for editor in blessed_editors.split(':') {
+        let editor = PathBuf::from(editor);
+        if is_valid_executable(&editor) {
+            return editor;
+        }
     }
+
+    // fallback on hardcoded path -- always provide something to the caller
+
+    PathBuf::from(defaults::SYSTEM_EDITOR)
 }
 
 // a `take_while` variant that does not consume the first non-matching item

src/sudoers/policy.rs

@@ -142,6 +142,11 @@ impl Judgement {
             Authorization::Forbidden
         }
     }
+
+    #[cfg_attr(not(feature = "sudoedit"), allow(unused))]
+    pub(crate) fn preferred_editor(&self) -> std::path::PathBuf {
+        super::select_editor(&self.settings, true)
+    }
 }
 
 impl Sudoers {

src/system/mod.rs

@@ -2,12 +2,12 @@
 #[cfg(target_os = "linux")]
 use std::str::FromStr;
 use std::{
-    ffi::{c_int, c_uint, CStr, CString},
+    ffi::{c_int, c_uint, CStr},
     fmt, fs, io,
     mem::MaybeUninit,
     ops,
-    os::unix::{self, prelude::OsStrExt},
-    path::{Path, PathBuf},
+    os::unix,
+    path::PathBuf,
 };
 
 use crate::{
@@ -41,15 +41,6 @@ pub mod wait;
 #[cfg(not(any(target_os = "freebsd", target_os = "linux")))]
 compile_error!("sudo-rs only works on Linux and FreeBSD");
 
-pub(crate) fn can_execute<P: AsRef<Path>>(path: P) -> bool {
-    let Ok(path) = CString::new(path.as_ref().as_os_str().as_bytes()) else {
-        return false;
-    };
-
-    // SAFETY: we are passing a proper pointer to access
-    unsafe { libc::access(path.as_ptr(), libc::X_OK) == 0 }
-}
-
 pub(crate) fn _exit(status: libc::c_int) -> ! {
     // SAFETY: this function is safe to call
     unsafe { libc::_exit(status) }

src/visudo/mod.rs

@@ -18,7 +18,6 @@ use crate::{
     sudo::{candidate_sudoers_file, diagnostic},
     sudoers::{self, Sudoers},
     system::{
-        can_execute,
         file::{create_temporary_dir, Chown, FileLock},
         interface::{GroupId, UserId},
         signal::{consts::*, register_handlers, SignalStream},
@@ -248,7 +247,6 @@ fn edit_sudoers_file(
 ) -> io::Result<()> {
     let mut stderr = io::stderr();
 
-    let mut editor_path = None;
     let mut sudoers_contents = Vec::new();
 
     // Since visudo is meant to run as root, resolve shouldn't fail
@@ -262,31 +260,36 @@ fn edit_sudoers_file(
 
     let host_name = Hostname::resolve();
 
-    if existed {
+    let editor_path = 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)?;
+        let (sudoers, _errors) = Sudoers::read(sudoers_contents.as_slice(), sudoers_path)?;
 
-        if errors.is_empty() {
-            editor_path = sudoers.solve_editor_path(&host_name, &current_user, &current_user);
-        }
-    }
-
-    let editor_path = match editor_path {
-        Some(path) => path,
-        None => editor_path_fallback()?,
+        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)
     };
 
     loop {
         Command::new(&editor_path)
             .arg("--")
             .arg(tmp_path)
-            .spawn()?
+            .spawn()
+            .map_err(|_| {
+                io::Error::new(
+                    io::ErrorKind::NotFound,
+                    format!(
+                        "specified editor ({}) could not be used",
+                        editor_path.display()
+                    ),
+                )
+            })?
             .wait_with_output()?;
 
         let (sudoers, errors) = File::open(tmp_path)
@@ -354,27 +357,6 @@ fn edit_sudoers_file(
     Ok(())
 }
 
-fn editor_path_fallback() -> io::Result<PathBuf> {
-    let editors = if cfg!(target_os = "linux") {
-        &["/usr/bin/editor"][..]
-    } else if cfg!(target_os = "freebsd") {
-        &["/usr/bin/vi"]
-    } else {
-        unimplemented!("unknown default editor for target")
-    };
-    for &editor in editors {
-        let path = Path::new(editor);
-        if can_execute(path) {
-            return Ok(path.to_owned());
-        }
-    }
-
-    Err(io::Error::new(
-        io::ErrorKind::NotFound,
-        "cannot find text editor",
-    ))
-}
-
 // To detect potential lock-outs if the user called "sudo visudo".
 // Note that SUDO_USER will normally be set by sudo.
 //