Repository: memorysafety/sudo-rs
Pull request #843 (2024-05-09 20:26:34)

Fix visudo bugs; update the CI
CC #833

This also makes some changes which should be fix the CI for at least Rust 1.80 by removing some dead code and other cobwebs. Every change has been put in a seperate commit for easy review.

The penultimate commit ("fix indentation") consists of whitespace-only changes.

In more detail, this PR functionally ensures that:
- Truncates the sudoers file correctly after it has shrunk
- Prints file source and position information of errors instead of just saying "syntax error".

Thereby Closing #833 

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/843?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety) Report
Attention: Patch coverage is `0%` with `2 lines` in your changes are missing coverage. Please review.
> Project coverage is 57.48%. Comparing base [(`4f93acb`)](https://app.codecov.io/gh/memorysafety/sudo-rs/commit/4f93acbc1a086d7316b4937fbe0a113105b59c73?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety) to head [(`865a7d3`)](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/843?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety).
> Report is 5 commits behind head on main.

| [Files](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/843?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety) | Patch % | Lines |
|---|---|---|
| [src/visudo/mod.rs](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/843?src=pr&el=tree&filepath=src%2Fvisudo%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety#diff-c3JjL3Zpc3Vkby9tb2QucnM=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/843?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #843      +/-   ##
==========================================
- Coverage   57.98%   57.48%   -0.50%     
==========================================
  Files          75       75              
  Lines       10560    10421     -139     
==========================================
- Hits         6123     5991     -132     
+ Misses       4437     4430       -7     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/memorysafety/sudo-rs/pull/843?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=memorysafety).

squell: CC PR #844. If that's merged, rebasing this on `main` should do the trick. I've included your CI-related commit there.

Patch:
diff --git a/src/defaults/settings_dsl.rs b/src/defaults/settings_dsl.rs
index 561eaa68..bc5d6fcb 100644
--- a/src/defaults/settings_dsl.rs
+++ b/src/defaults/settings_dsl.rs
@@ -1,5 +1,6 @@
 macro_rules! add_from {
     ($ctor:ident, $type:ty) => {
+        #[allow(non_local_definitions)]
         impl From<$type> for $crate::defaults::SudoDefault {
             fn from(value: $type) -> Self {
                 $crate::defaults::SudoDefault::$ctor(value.into())
@@ -8,6 +9,7 @@ macro_rules! add_from {
     };
 
     ($ctor:ident, $type:ty, negatable$(, $vetting_function:expr)?) => {
+        #[allow(non_local_definitions)]
         impl From<$type> for $crate::defaults::SudoDefault {
             fn from(value: $type) -> Self {
                 $crate::defaults::SudoDefault::$ctor(OptTuple {
@@ -17,6 +19,7 @@ macro_rules! add_from {
             }
         }
 
+        #[allow(non_local_definitions)]
         impl From<($type, $type)> for $crate::defaults::SudoDefault {
             fn from((value, neg): ($type, $type)) -> Self {
                 $crate::defaults::SudoDefault::$ctor(OptTuple {
diff --git a/src/sudo/cli/mod.rs b/src/sudo/cli/mod.rs
index 49a00f25..cdb6c6d3 100644
--- a/src/sudo/cli/mod.rs
+++ b/src/sudo/cli/mod.rs
@@ -10,6 +10,8 @@ pub mod help;
 #[cfg(test)]
 mod tests;
 
+// remove dead_code when sudoedit has been implemented
+#[allow(dead_code)]
 pub enum SudoAction {
     Edit(SudoEditOptions),
     Help(SudoHelpOptions),
@@ -205,6 +207,7 @@ impl TryFrom<SudoOptions> for SudoValidateOptions {
 }
 
 // sudo -e [-ABkNnS] [-r role] [-t type] [-C num] [-D directory] [-g group] [-h host] [-p prompt] [-R directory] [-T timeout] [-u user] file ...
+#[allow(dead_code)]
 pub struct SudoEditOptions {
     // -k
     pub reset_timestamp: bool,
diff --git a/src/sudo/env/environment.rs b/src/sudo/env/environment.rs
index 6b9da2c0..d762bfe9 100644
--- a/src/sudo/env/environment.rs
+++ b/src/sudo/env/environment.rs
@@ -133,6 +133,9 @@ fn is_safe_tz(value: &[u8]) -> bool {
     };
 
     if check_value.starts_with(&[b'/']) {
+        // clippy 1.79 wants to us to optimise this check away; but we don't know what this will always
+        // be possible; and the compiler is clever enough to do that for us anyway if it can be.
+        #[allow(clippy::const_is_empty)]
         if !PATH_ZONEINFO.is_empty() {
             if !check_value.starts_with(PATH_ZONEINFO.as_bytes())
                 || check_value.get(PATH_ZONEINFO.len()) != Some(&b'/')
diff --git a/src/sudo/mod.rs b/src/sudo/mod.rs
index ded2dd0d..3b48326f 100644
--- a/src/sudo/mod.rs
+++ b/src/sudo/mod.rs
@@ -16,7 +16,7 @@ use pipeline::{Pipeline, PolicyPlugin};
 use std::path::Path;
 
 mod cli;
-mod diagnostic;
+pub mod diagnostic;
 mod env;
 mod pam;
 mod pipeline;
diff --git a/src/sudoers/mod.rs b/src/sudoers/mod.rs
index 15f7e38e..096bbe51 100644
--- a/src/sudoers/mod.rs
+++ b/src/sudoers/mod.rs
@@ -590,160 +590,159 @@ fn analyze(
         }
     }
 
-    impl Sudoers {
-        fn include(
-            &mut self,
-            parent: &Path,
-            path: &Path,
-            diagnostics: &mut Vec<Error>,
-            count: &mut u8,
-        ) {
-            if *count >= INCLUDE_LIMIT {
-                diagnostics.push(Error {
-                    source: Some(parent.to_owned()),
-                    location: None,
-                    message: format!("include file limit reached opening '{}'", path.display()),
-                })
-            // FIXME: this will cause an error in `visudo` if we open a non-privileged sudoers file
-            // that includes another non-privileged sudoer files.
-            } else {
-                match open_subsudoers(path) {
-                    Ok(subsudoer) => {
-                        *count += 1;
-                        self.process(path, subsudoer, diagnostics, count)
-                    }
-                    Err(e) => {
-                        let message = if e.kind() == io::ErrorKind::NotFound {
-                            // improve the error message in this case
-                            format!("cannot open sudoers file '{}'", path.display())
-                        } else {
-                            e.to_string()
-                        };
+    fn include(
+        cfg: &mut Sudoers,
+        parent: &Path,
+        path: &Path,
+        diagnostics: &mut Vec<Error>,
+        count: &mut u8,
+    ) {
+        if *count >= INCLUDE_LIMIT {
+            diagnostics.push(Error {
+                source: Some(parent.to_owned()),
+                location: None,
+                message: format!("include file limit reached opening '{}'", path.display()),
+            })
+        // FIXME: this will cause an error in `visudo` if we open a non-privileged sudoers file
+        // that includes another non-privileged sudoer files.
+        } else {
+            match open_subsudoers(path) {
+                Ok(subsudoer) => {
+                    *count += 1;
+                    process(cfg, path, subsudoer, diagnostics, count)
+                }
+                Err(e) => {
+                    let message = if e.kind() == io::ErrorKind::NotFound {
+                        // improve the error message in this case
+                        format!("cannot open sudoers file '{}'", path.display())
+                    } else {
+                        e.to_string()
+                    };
 
-                        diagnostics.push(Error {
-                            source: Some(parent.to_owned()),
-                            location: None,
-                            message,
-                        })
-                    }
+                    diagnostics.push(Error {
+                        source: Some(parent.to_owned()),
+                        location: None,
+                        message,
+                    })
                 }
             }
         }
+    }
 
-        fn process(
-            &mut self,
-            cur_path: &Path,
-            sudoers: impl IntoIterator<Item = basic_parser::Parsed<Sudo>>,
-            diagnostics: &mut Vec<Error>,
-            safety_count: &mut u8,
-        ) {
-            for item in sudoers {
-                match item {
-                    Ok(line) => match line {
-                        Sudo::LineComment => {}
-
-                        Sudo::Spec(permission) => self.rules.push(permission),
-
-                        Sudo::Decl(UserAlias(mut def)) => self.aliases.user.1.append(&mut def),
-                        Sudo::Decl(HostAlias(mut def)) => self.aliases.host.1.append(&mut def),
-                        Sudo::Decl(CmndAlias(mut def)) => self.aliases.cmnd.1.append(&mut def),
-                        Sudo::Decl(RunasAlias(mut def)) => self.aliases.runas.1.append(&mut def),
-
-                        Sudo::Decl(Defaults(params)) => {
-                            for (name, value) in params {
-                                self.set_default(name, value)
-                            }
+    fn process(
+        cfg: &mut Sudoers,
+        cur_path: &Path,
+        sudoers: impl IntoIterator<Item = basic_parser::Parsed<Sudo>>,
+        diagnostics: &mut Vec<Error>,
+        safety_count: &mut u8,
+    ) {
+        for item in sudoers {
+            match item {
+                Ok(line) => match line {
+                    Sudo::LineComment => {}
+
+                    Sudo::Spec(permission) => cfg.rules.push(permission),
+
+                    Sudo::Decl(UserAlias(mut def)) => cfg.aliases.user.1.append(&mut def),
+                    Sudo::Decl(HostAlias(mut def)) => cfg.aliases.host.1.append(&mut def),
+                    Sudo::Decl(CmndAlias(mut def)) => cfg.aliases.cmnd.1.append(&mut def),
+                    Sudo::Decl(RunasAlias(mut def)) => cfg.aliases.runas.1.append(&mut def),
+
+                    Sudo::Decl(Defaults(params)) => {
+                        for (name, value) in params {
+                            set_default(cfg, name, value)
                         }
+                    }
 
-                        Sudo::Include(path) => self.include(
-                            cur_path,
-                            &resolve_relative(cur_path, path),
-                            diagnostics,
-                            safety_count,
-                        ),
-
-                        Sudo::IncludeDir(path) => {
-                            if path.contains("%h") {
-                                diagnostics.push(Error{
+                    Sudo::Include(path) => include(
+                        cfg,
+                        cur_path,
+                        &resolve_relative(cur_path, path),
+                        diagnostics,
+                        safety_count,
+                    ),
+
+                    Sudo::IncludeDir(path) => {
+                        if path.contains("%h") {
+                            diagnostics.push(Error{
                                     source: Some(cur_path.to_owned()),
                                     location: None,
                                     message: format!("cannot open sudoers file {path}: percent escape %h in includedir is unsupported")});
-                                continue;
-                            }
-
-                            let path = resolve_relative(cur_path, path);
-                            let Ok(files) = std::fs::read_dir(&path) else {
-                                diagnostics.push(Error {
-                                    source: Some(cur_path.to_owned()),
-                                    location: None,
-                                    message: format!("cannot open sudoers file {}", path.display()),
-                                });
-                                continue;
-                            };
-                            let mut safe_files = files
-                                .filter_map(|direntry| {
-                                    let path = direntry.ok()?.path();
-                                    let text = path.file_name()?.to_str()?;
-                                    if text.ends_with('~') || text.contains('.') {
-                                        None
-                                    } else {
-                                        Some(path)
-                                    }
-                                })
-                                .collect::<Vec<_>>();
-                            safe_files.sort();
-                            for file in safe_files {
-                                self.include(cur_path, file.as_ref(), diagnostics, safety_count)
-                            }
+                            continue;
                         }
-                    },
 
-                    Err(basic_parser::Status::Fatal(pos, message)) => diagnostics.push(Error {
-                        source: Some(cur_path.to_owned()),
-                        location: Some(pos),
-                        message,
-                    }),
-                    Err(_) => panic!("internal parser error"),
-                }
+                        let path = resolve_relative(cur_path, path);
+                        let Ok(files) = std::fs::read_dir(&path) else {
+                            diagnostics.push(Error {
+                                source: Some(cur_path.to_owned()),
+                                location: None,
+                                message: format!("cannot open sudoers file {}", path.display()),
+                            });
+                            continue;
+                        };
+                        let mut safe_files = files
+                            .filter_map(|direntry| {
+                                let path = direntry.ok()?.path();
+                                let text = path.file_name()?.to_str()?;
+                                if text.ends_with('~') || text.contains('.') {
+                                    None
+                                } else {
+                                    Some(path)
+                                }
+                            })
+                            .collect::<Vec<_>>();
+                        safe_files.sort();
+                        for file in safe_files {
+                            include(cfg, cur_path, file.as_ref(), diagnostics, safety_count)
+                        }
+                    }
+                },
+
+                Err(basic_parser::Status::Fatal(pos, message)) => diagnostics.push(Error {
+                    source: Some(cur_path.to_owned()),
+                    location: Some(pos),
+                    message,
+                }),
+                Err(_) => panic!("internal parser error"),
             }
         }
+    }
 
-        fn set_default(&mut self, name: String, value: ConfigValue) {
-            match value {
-                Flag(value) => {
-                    if value {
-                        self.settings.flags.insert(name);
-                    } else {
-                        self.settings.flags.remove(&name);
-                    }
+    fn set_default(cfg: &mut Sudoers, name: String, value: ConfigValue) {
+        match value {
+            Flag(value) => {
+                if value {
+                    cfg.settings.flags.insert(name);
+                } else {
+                    cfg.settings.flags.remove(&name);
                 }
-                List(mode, values) => {
-                    let slot: &mut _ = self.settings.list.entry(name).or_default();
-                    match mode {
-                        Mode::Set => *slot = values.into_iter().collect(),
-                        Mode::Add => slot.extend(values),
-                        Mode::Del => {
-                            for key in values {
-                                slot.remove(&key);
-                            }
+            }
+            List(mode, values) => {
+                let slot: &mut _ = cfg.settings.list.entry(name).or_default();
+                match mode {
+                    Mode::Set => *slot = values.into_iter().collect(),
+                    Mode::Add => slot.extend(values),
+                    Mode::Del => {
+                        for key in values {
+                            slot.remove(&key);
                         }
                     }
                 }
-                Text(value) => {
-                    self.settings.str_value.insert(name, value);
-                }
-                Enum(value) => {
-                    self.settings.enum_value.insert(name, value);
-                }
-                Num(value) => {
-                    self.settings.int_value.insert(name, value);
-                }
+            }
+            Text(value) => {
+                cfg.settings.str_value.insert(name, value);
+            }
+            Enum(value) => {
+                cfg.settings.enum_value.insert(name, value);
+            }
+            Num(value) => {
+                cfg.settings.int_value.insert(name, value);
             }
         }
     }
 
     let mut diagnostics = vec![];
-    result.process(path, sudoers, &mut diagnostics, &mut 0);
+    process(&mut result, path, sudoers, &mut diagnostics, &mut 0);
 
     let alias = &mut result.aliases;
     alias.user.0 = sanitize_alias_table(&alias.user.1, &mut diagnostics);
diff --git a/src/system/mod.rs b/src/system/mod.rs
index 5698beb3..e7dbaa25 100644
--- a/src/system/mod.rs
+++ b/src/system/mod.rs
@@ -420,8 +420,6 @@ impl User {
 pub struct Group {
     pub gid: GroupId,
     pub name: String,
-    pub passwd: String,
-    pub members: Vec<String>,
 }
 
 impl Group {
@@ -430,24 +428,9 @@ impl Group {
     /// In particular the grp.gr_mem pointer is assumed to be non-null, and pointing to a
     /// null-terminated list; the pointed-to strings are expected to be null-terminated.
     unsafe fn from_libc(grp: &libc::group) -> Group {
-        // find out how many members we have
-        let mut mem_count = 0;
-        while !(*grp.gr_mem.offset(mem_count)).is_null() {
-            mem_count += 1;
-        }
-
-        // convert the members to a slice and then put them into a vec of strings
-        let mut members = Vec::with_capacity(mem_count as usize);
-        let mem_slice = std::slice::from_raw_parts(grp.gr_mem, mem_count as usize);
-        for mem in mem_slice {
-            members.push(string_from_ptr(*mem));
-        }
-
         Group {
             gid: grp.gr_gid,
             name: string_from_ptr(grp.gr_name),
-            passwd: string_from_ptr(grp.gr_passwd),
-            members,
         }
     }
 
@@ -514,9 +497,7 @@ impl WithProcess {
 pub struct Process {
     pub pid: ProcessId,
     pub parent_pid: Option<ProcessId>,
-    pub group_id: ProcessId,
     pub session_id: ProcessId,
-    pub name: PathBuf,
 }
 
 impl Default for Process {
@@ -530,16 +511,10 @@ impl Process {
         Process {
             pid: Self::process_id(),
             parent_pid: Self::parent_id(),
-            group_id: Self::group_id(),
             session_id: Self::session_id(),
-            name: Self::process_name().unwrap_or_else(|| PathBuf::from("sudo")),
         }
     }
 
-    pub fn process_name() -> Option<PathBuf> {
-        std::env::args().next().map(PathBuf::from)
-    }
-
     /// Return the process identifier for the current process
     pub fn process_id() -> ProcessId {
         // NOTE libstd casts the `i32` that `libc::getpid` returns into `u32`
@@ -559,11 +534,6 @@ impl Process {
         }
     }
 
-    /// Return the process group id for the current process
-    pub fn group_id() -> ProcessId {
-        unsafe { libc::getpgid(0) }
-    }
-
     /// Get the session id for the current process
     pub fn session_id() -> ProcessId {
         unsafe { libc::getsid(0) }
@@ -754,9 +724,7 @@ mod tests {
                 },
                 Group {
                     name: name.to_string(),
-                    passwd: passwd.to_string(),
                     gid,
-                    members: mem.iter().map(|s| s.to_string()).collect(),
                 }
             )
         }
diff --git a/src/system/timestamp.rs b/src/system/timestamp.rs
index 603c5c86..2e6666ba 100644
--- a/src/system/timestamp.rs
+++ b/src/system/timestamp.rs
@@ -315,6 +315,7 @@ pub enum TouchResult {
     NotFound,
 }
 
+#[cfg_attr(not(test), allow(dead_code))]
 pub enum CreateResult {
     /// The record was found and it was refreshed
     Updated {
@@ -736,9 +737,10 @@ mod tests {
 
         std::thread::sleep(std::time::Duration::from_millis(1));
         let res = srf.create(tty_scope, auth_user).unwrap();
-        let CreateResult::Updated { .. } = res else {
+        let CreateResult::Updated { old_time, new_time } = res else {
             panic!("Expected record to be updated");
         };
+        assert_ne!(old_time, new_time);
 
         // reset the file
         assert!(srf.reset().is_ok());
diff --git a/src/visudo/mod.rs b/src/visudo/mod.rs
index b9dd1f99..4aec4abb 100644
--- a/src/visudo/mod.rs
+++ b/src/visudo/mod.rs
@@ -11,6 +11,7 @@ use std::{
 };
 
 use crate::{
+    sudo::diagnostic,
     sudoers::Sudoers,
     system::{
         can_execute,
@@ -106,9 +107,14 @@ fn check(file_arg: Option<&str>, perms: bool, owner: bool) -> io::Result<()> {
         return Ok(());
     }
 
-    let mut stderr = io::stderr();
-    for crate::sudoers::Error { message, .. } in errors {
-        writeln!(stderr, "syntax error: {message}")?;
+    for crate::sudoers::Error {
+        message,
+        source,
+        location,
+    } in errors
+    {
+        let path = source.as_deref().unwrap_or(sudoers_path);
+        diagnostic::diagnostic!("syntax error: {message}", path @ location);
     }
 
     Err(io::Error::new(io::ErrorKind::Other, "invalid sudoers file"))
@@ -283,6 +289,8 @@ fn edit_sudoers_file(
         writeln!(stderr, "visudo: {} unchanged", tmp_path.display())?;
     } else {
         sudoers_file.write_all(&tmp_contents)?;
+        let new_size = sudoers_file.stream_position()?;
+        sudoers_file.set_len(new_size)?;
     }
 
     lock.unlock()?;


Test patch:
diff --git a/src/sudo/env/tests.rs b/src/sudo/env/tests.rs
index a0028be0..15fc0f68 100644
--- a/src/sudo/env/tests.rs
+++ b/src/sudo/env/tests.rs
@@ -96,8 +96,6 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context {
     let current_group = Group {
         gid: 1000,
         name: "test".to_string(),
-        passwd: String::new(),
-        members: Vec::new(),
     };
 
     let root_user = User {
@@ -114,8 +112,6 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context {
     let root_group = Group {
         gid: 0,
         name: "root".to_string(),
-        passwd: String::new(),
-        members: Vec::new(),
     };
 
     Context {