Repository: memorysafety/sudo-rs
Pull request #110 (2023-03-17 14:30:39)

check sudoers permissions
Closing #77 

This actually also fixes the test-case so it actually fails (without the addition); in the original test sudo-rs would fail because the sudoers file it read contained no rules.

squell: Note, this PR also fixes a small issue in the test framework (see aa1be8f).
squell: Take care that the test-cases are built on the pre #103 framework and that will conflict, no matter the order in which these PR's are handled.

Patch:
diff --git a/lib/sudo-cutils/src/lib.rs b/lib/sudo-cutils/src/lib.rs
index 9f47b8cb..7f9e8487 100644
--- a/lib/sudo-cutils/src/lib.rs
+++ b/lib/sudo-cutils/src/lib.rs
@@ -53,18 +53,45 @@ pub unsafe fn string_from_ptr(ptr: *const libc::c_char) -> String {
     }
 }
 
-pub fn into_leaky_cstring(s: &str) -> *const libc::c_char {
-    let alloc_len = s.len() as isize;
-    let mem = unsafe { libc::malloc(alloc_len as usize + 1) as *mut u8 };
+/// Create a C string copy of a Rust string copy, allocated by libc::malloc()
+///
+/// # Safety
+/// This function assumes that the caller will clean up the returned pointer
+/// via a call to libc::free.
+pub unsafe fn into_leaky_cstring(s: &str) -> *const libc::c_char {
+    let alloc_len: isize = s.len().try_into().expect("absurd string size");
+    let mem = libc::malloc(alloc_len as usize + 1) as *mut u8;
     if mem.is_null() {
         panic!("libc malloc failed");
     } else {
-        for (i, e) in s.bytes().enumerate() {
-            let signed_i = i as isize;
-            unsafe { *mem.offset(signed_i) = e };
-        }
-        unsafe { *mem.offset(alloc_len) = 0 };
+        std::ptr::copy_nonoverlapping(s.as_ptr(), mem, alloc_len as usize);
+        *mem.offset(alloc_len) = 0;
     }
 
     mem as *mut libc::c_char
 }
+
+#[cfg(test)]
+mod test {
+    use super::{into_leaky_cstring, string_from_ptr};
+
+    #[test]
+    fn test_str_to_ptr() {
+        let strp = |ptr| unsafe { string_from_ptr(ptr) };
+        assert_eq!(strp(std::ptr::null()), "");
+        assert_eq!(strp("\0".as_ptr() as *const libc::c_char), "");
+        assert_eq!(strp("hello\0".as_ptr() as *const libc::c_char), "hello");
+    }
+
+    #[test]
+    fn test_leaky_cstring() {
+        let test = |text| unsafe {
+            let ptr = into_leaky_cstring(text);
+            let result = string_from_ptr(ptr);
+            libc::free(ptr as *mut libc::c_void);
+            result
+        };
+        assert_eq!(test(""), "");
+        assert_eq!(test("hello"), "hello");
+    }
+}
diff --git a/lib/sudo-pam/src/converse.rs b/lib/sudo-pam/src/converse.rs
index 18f0b24f..9a3e380f 100644
--- a/lib/sudo-pam/src/converse.rs
+++ b/lib/sudo-pam/src/converse.rs
@@ -249,7 +249,7 @@ pub(crate) extern "C" fn converse<C: Converser>(
             // Unwrap here should be ok because we previously allocated an array of the same size
             let our_resp = &conversation.messages.get(i as usize).unwrap().response;
             if let Some(r) = our_resp {
-                let cstr = sudo_cutils::into_leaky_cstring(r);
+                let cstr = unsafe { sudo_cutils::into_leaky_cstring(r) };
                 response.resp = cstr as *mut _;
             }
         }
diff --git a/lib/sudo-system/src/audit.rs b/lib/sudo-system/src/audit.rs
new file mode 100644
index 00000000..e4520a5f
--- /dev/null
+++ b/lib/sudo-system/src/audit.rs
@@ -0,0 +1,43 @@
+use std::fs::File;
+use std::io::{self, Error, ErrorKind};
+use std::os::unix::fs::{MetadataExt, PermissionsExt};
+use std::path::Path;
+
+// of course we can also write "file & 0o040 != 0", but this makes the intent explicit
+enum Op {
+    _Read = 4,
+    Write = 2,
+    _Exec = 1,
+}
+enum Category {
+    _Owner = 2,
+    Group = 1,
+    World = 0,
+}
+
+fn mode(who: Category, what: Op) -> u32 {
+    (what as u32) << (3 * who as u32)
+}
+
+pub fn secure_open(path: &Path) -> io::Result<File> {
+    let file = File::open(path)?;
+    let meta = file.metadata()?;
+    let permbits = meta.permissions().mode();
+    let error = |msg| Error::new(ErrorKind::PermissionDenied, msg);
+
+    if meta.uid() != 0 {
+        Err(error(format!("{} must be owned by root", path.display())))
+    } else if meta.gid() != 0 && (permbits & mode(Category::Group, Op::Write) != 0) {
+        Err(error(format!(
+            "{} cannot be group-writable",
+            path.display()
+        )))
+    } else if permbits & mode(Category::World, Op::Write) != 0 {
+        Err(error(format!(
+            "{} cannot be world-writable",
+            path.display()
+        )))
+    } else {
+        Ok(file)
+    }
+}
diff --git a/lib/sudo-system/src/lib.rs b/lib/sudo-system/src/lib.rs
index 862f278f..c735184a 100644
--- a/lib/sudo-system/src/lib.rs
+++ b/lib/sudo-system/src/lib.rs
@@ -10,6 +10,9 @@ use libc::pid_t;
 pub use libc::PATH_MAX;
 use sudo_cutils::*;
 
+mod audit;
+pub use audit::secure_open;
+
 pub fn hostname() -> String {
     let max_hostname_size = sysconf(libc::_SC_HOST_NAME_MAX).unwrap_or(256);
     let mut buf = vec![0; max_hostname_size as usize];
diff --git a/lib/sudoers/src/lib.rs b/lib/sudoers/src/lib.rs
index 5d4c5cad..7d731987 100644
--- a/lib/sudoers/src/lib.rs
+++ b/lib/sudoers/src/lib.rs
@@ -86,9 +86,8 @@ impl Sudoers {
 }
 
 fn read_sudoers(path: &Path) -> Result<Vec<basic_parser::Parsed<Sudo>>, std::io::Error> {
-    use std::fs::File;
     use std::io::Read;
-    let mut source = File::open(path)?;
+    let mut source = sudo_system::secure_open(path)?;
 
     // it's a bit frustrating that BufReader.chars() does not exist
     let mut buffer = String::new();
diff --git a/sudo/src/main.rs b/sudo/src/main.rs
index fc739288..c6c2a054 100644
--- a/sudo/src/main.rs
+++ b/sudo/src/main.rs
@@ -16,7 +16,7 @@ fn parse_sudoers() -> Result<Sudoers, Error> {
     let sudoers_path = "/etc/sudoers.test";
 
     let (sudoers, syntax_errors) = Sudoers::new(sudoers_path)
-        .map_err(|e| Error::Configuration(format!("no sudoers file {e}")))?;
+        .map_err(|e| Error::Configuration(format!("no valid sudoers file: {e}")))?;
 
     for sudoers::Error(_pos, error) in syntax_errors {
         eprintln!("Parse error: {error}");


Test patch:
diff --git a/test-framework/sudo-compliance-tests/src/sudoers.rs b/test-framework/sudo-compliance-tests/src/sudoers.rs
index 2d97fa47..c5200fe8 100644
--- a/test-framework/sudo-compliance-tests/src/sudoers.rs
+++ b/test-framework/sudo-compliance-tests/src/sudoers.rs
@@ -1,12 +1,12 @@
 use sudo_test::{Command, Env, TextFile, User};
 
-use crate::{Result, PASSWORD, USERNAME};
+use crate::{Result, PASSWORD, SUDOERS_ROOT_ALL_NOPASSWD, USERNAME};
 
 mod user_list;
 
 #[test]
 fn cannot_sudo_if_sudoers_file_is_world_writable() -> Result<()> {
-    let env = Env(TextFile("").chmod("446")).build()?;
+    let env = Env(TextFile(SUDOERS_ROOT_ALL_NOPASSWD).chmod("446")).build()?;
 
     let output = Command::new("sudo").arg("true").exec(&env)?;
     assert_eq!(Some(1), output.status().code());
@@ -18,6 +18,56 @@ fn cannot_sudo_if_sudoers_file_is_world_writable() -> Result<()> {
     Ok(())
 }
 
+#[test]
+fn cannot_sudo_if_sudoers_file_is_group_writable() -> Result<()> {
+    let env = Env(TextFile(SUDOERS_ROOT_ALL_NOPASSWD)
+        .chmod("464")
+        .chown("root:1234"))
+    .user(User(USERNAME).password(PASSWORD))
+    .build()?;
+
+    let output = Command::new("sudo").arg("true").exec(&env)?;
+    assert_eq!(Some(1), output.status().code());
+
+    if sudo_test::is_original_sudo() {
+        assert_contains!(
+            output.stderr(),
+            "/etc/sudoers is owned by gid 1234, should be 0"
+        );
+    }
+
+    Ok(())
+}
+
+#[test]
+fn can_sudo_if_sudoers_file_is_owner_writable() -> Result<()> {
+    let env = Env(TextFile(SUDOERS_ROOT_ALL_NOPASSWD).chmod("644")).build()?;
+
+    let output = Command::new("sudo").arg("true").exec(&env)?;
+    assert_eq!(Some(0), output.status().code());
+
+    Ok(())
+}
+
+#[test]
+fn cannot_sudo_if_sudoers_file_is_not_owned_by_root() -> Result<()> {
+    let env = Env(TextFile(SUDOERS_ROOT_ALL_NOPASSWD).chown("1234:root"))
+        .user(User(USERNAME).password(PASSWORD))
+        .build()?;
+
+    let output = Command::new("sudo").arg("true").exec(&env)?;
+    assert_eq!(Some(1), output.status().code());
+
+    if sudo_test::is_original_sudo() {
+        assert_contains!(
+            output.stderr(),
+            "/etc/sudoers is owned by uid 1234, should be 0"
+        );
+    }
+
+    Ok(())
+}
+
 #[ignore]
 #[test]
 fn user_specifications_evaluated_bottom_to_top() -> Result<()> {