From b0091ec0bb9bb3fbb60c86323418201209d1bee4 Mon Sep 17 00:00:00 2001 From: Jeff Turner Date: Wed, 5 Nov 2025 23:42:49 +1100 Subject: [PATCH 1/5] test: Check that 'test' takes supplementary groups into account. #9147 Run with: cargo test --test tests test_permission_with_supplementary_group --- tests/by-util/test_test.rs | 85 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/tests/by-util/test_test.rs b/tests/by-util/test_test.rs index 21ea1893e99..1a14815dc3a 100644 --- a/tests/by-util/test_test.rs +++ b/tests/by-util/test_test.rs @@ -1034,3 +1034,88 @@ fn test_unary_op_as_literal_in_three_arg_form() { new_ucmd!().args(&["-f", "=", "a"]).fails_with_code(1); new_ucmd!().args(&["-f", "=", "a", "-o", "b"]).succeeds(); } + +#[test] +#[cfg(any( + target_os = "linux", + target_os = "hurd", + target_os = "solaris", + target_os = "illumos", + target_os = "freebsd", + target_os = "macos", + target_os = "netbsd", + target_os = "openbsd", + target_os = "dragonfly" +))] +fn test_permission_with_supplementary_group() { + // Test that permission checks (-r, -w, -x) correctly consider supplementary groups, + // not just the primary group (egid). See #9147 + // + // This test requires: + // - Running as a non-root user + // - User must have at least one supplementary group + // - sudo access to create test files/dirs with specific ownership + use std::process::Command; + + let scene = TestScenario::new(util_name!()); + + // Must be run as non-root user (otherwise owner permissions would apply) + if scene.cmd("whoami").run().stdout_str().trim() == "root" { + return; + } + + // Get user's supplementary groups + let Ok(groups_output) = Command::new("id").arg("-nG").output() else { + return; + }; + + let groups_str = String::from_utf8_lossy(&groups_output.stdout); + let groups: Vec<&str> = groups_str.split_whitespace().collect(); + + // Need at least 2 groups (primary + at least one supplementary) + if groups.len() < 2 { + return; + } + + let supplementary_group = groups[1]; + let test_path = scene.fixtures.plus("test_supp_group_dir"); + let test_path_str = test_path.to_str().unwrap(); + + // Try to create directory with root owner and supplementary group, mode 750 + let install_result = scene + .cmd("sudo") + .args(&[ + "-E", + "--non-interactive", + "install", + "-d", + "-m", + "750", + "-o", + "root", + "-g", + supplementary_group, + test_path_str, + ]) + .run(); + + if !install_result.succeeded() { + return; + } + + // Test that -r correctly detects directory as readable (via supplementary group) + scene.ucmd().args(&["-r", test_path_str]).succeeds(); + + // Test that -x correctly detects directory as executable (via supplementary group) + scene.ucmd().args(&["-x", test_path_str]).succeeds(); + + // Test that -w correctly detects directory as NOT writable (group has r-x, not w) + scene.ucmd().args(&["!", "-w", test_path_str]).succeeds(); + + // Clean up + let _ = scene + .cmd("sudo") + .args(&["-E", "--non-interactive", "rm", "-rf", test_path_str]) + .run(); +} +>>>>>>> Conflict 1 of 1 ends From a51c571cbe54372f4785d0352663086ee9bcb59b Mon Sep 17 00:00:00 2001 From: Jeff Turner Date: Thu, 6 Nov 2025 00:29:25 +1100 Subject: [PATCH 2/5] test: Check that 'test' respects ACLs. #8960 Run with: cargo test --test tests test_permission_with_acl --- tests/by-util/test_test.rs | 100 +++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/tests/by-util/test_test.rs b/tests/by-util/test_test.rs index 21ea1893e99..d9324c25628 100644 --- a/tests/by-util/test_test.rs +++ b/tests/by-util/test_test.rs @@ -1034,3 +1034,103 @@ fn test_unary_op_as_literal_in_three_arg_form() { new_ucmd!().args(&["-f", "=", "a"]).fails_with_code(1); new_ucmd!().args(&["-f", "=", "a", "-o", "b"]).succeeds(); } + +// Only test platforms supporting setfacl. +#[test] +#[cfg(any( + target_os = "linux", + target_os = "cygwin" +))] +fn test_permission_with_acl() { + // Test that permission checks (-r, -w, -x) correctly handle ACL (Access Control List) + // permissions, not just traditional Unix permissions. This requires using euidaccess() + // or equivalent, not manual permission bit checking. + // + // This test requires: + // - Running as a non-root user + // - sudo access + // - setfacl command available + use std::process::Command; + + let scene = TestScenario::new(util_name!()); + + // Must be run as non-root user (otherwise owner permissions would apply) + if scene.cmd("whoami").run().stdout_str().trim() == "root" { + return; + } + + // Check if setfacl is available + if Command::new("which").arg("setfacl").output().is_err() { + return; + } + + // Get current username for ACL + let Ok(username_output) = Command::new("whoami").output() else { + return; + }; + let username = String::from_utf8_lossy(&username_output.stdout) + .trim() + .to_string(); + + let test_path = scene.fixtures.plus("test_acl_dir"); + let test_path_str = test_path.to_str().unwrap(); + + // Create directory with root:root ownership and 750 permissions (no access for others) + let install_result = scene + .cmd("sudo") + .args(&[ + "-E", + "--non-interactive", + "install", + "-d", + "-m", + "750", + "-o", + "root", + "-g", + "root", + test_path_str, + ]) + .run(); + + if !install_result.succeeded() { + return; + } + + // Grant ACL permissions to current user (read + execute) + let setfacl_result = scene + .cmd("sudo") + .args(&[ + "-E", + "--non-interactive", + "setfacl", + "-m", + &format!("u:{username}:rx"), + test_path_str, + ]) + .run(); + + if !setfacl_result.succeeded() { + // Clean up and skip test + let _ = scene + .cmd("sudo") + .args(&["-E", "--non-interactive", "rm", "-rf", test_path_str]) + .run(); + return; + } + + // Test that -r correctly detects directory as readable (via ACL) + scene.ucmd().args(&["-r", test_path_str]).succeeds(); + + // Test that -x correctly detects directory as executable (via ACL) + scene.ucmd().args(&["-x", test_path_str]).succeeds(); + + // Test that -w correctly detects directory as NOT writable (ACL grants r-x, not w) + scene.ucmd().args(&["!", "-w", test_path_str]).succeeds(); + + // Clean up + let _ = scene + .cmd("sudo") + .args(&["-E", "--non-interactive", "rm", "-rf", test_path_str]) + .run(); +} From ecb6581b6850312dc1bc3fc1fa8ce7140fe3563f Mon Sep 17 00:00:00 2001 From: Jeff Turner Date: Thu, 6 Nov 2025 00:13:36 +1100 Subject: [PATCH 3/5] test: check supplementary groups and ACLs for -r/-w/-x permissions. Replace manual permission checking logic with platform-specific libc functions that properly handle supplementary groups and ACLs: - Linux, Hurd, Redox, Cygwin, Solaris, Illumos: euidaccess() - FreeBSD: eaccess() - macOS, iOS, NetBSD, OpenBSD, DragonFly: faccessat() with AT_EACCESS - Android: faccessat() - Other OSes: fall back to libc:access() These functions delegate permission checks to the kernel, which correctly evaluates all of the user's group memberships (both primary and supplementary), as well as ACLs and other advanced permission features. Fixes #8960 Fixes #9147 --- .../workspace.wordlist.txt | 5 ++ src/uu/test/src/test.rs | 77 +++++++++++++++---- 2 files changed, 66 insertions(+), 16 deletions(-) diff --git a/.vscode/cspell.dictionaries/workspace.wordlist.txt b/.vscode/cspell.dictionaries/workspace.wordlist.txt index 28c468d4f9c..b0d3b5f074a 100644 --- a/.vscode/cspell.dictionaries/workspace.wordlist.txt +++ b/.vscode/cspell.dictionaries/workspace.wordlist.txt @@ -152,7 +152,9 @@ vmsplice # * vars/libc COMFOLLOW +EACCESS EXDEV +FDCWD FILENO FTSENT HOSTSIZE @@ -197,7 +199,10 @@ blocksize canonname chroot dlsym +eaccess +euidaccess execvp +faccessat fdatasync freeaddrinfo getaddrinfo diff --git a/src/uu/test/src/test.rs b/src/uu/test/src/test.rs index 0e4e809d760..92b0adf178e 100644 --- a/src/uu/test/src/test.rs +++ b/src/uu/test/src/test.rs @@ -258,26 +258,71 @@ enum PathCondition { #[cfg(not(windows))] fn path(path: &OsStr, condition: &PathCondition) -> bool { - use std::fs::Metadata; + use std::ffi::CString; + use std::os::unix::ffi::OsStrExt; use std::os::unix::fs::FileTypeExt; const S_ISUID: u32 = 0o4000; const S_ISGID: u32 = 0o2000; const S_ISVTX: u32 = 0o1000; - enum Permission { - Read = 0o4, - Write = 0o2, - Execute = 0o1, - } + // Helper function to check file access permissions. + // Uses platform-specific access functions that properly handle supplementary groups + // and ACLs. + let check_access = |path: &OsStr, mode: i32| -> bool { + let path_bytes = path.as_bytes(); + let Ok(c_path) = CString::new(path_bytes) else { + return false; // Path contains null byte + }; - let perm = |metadata: Metadata, p: Permission| { - if geteuid() == metadata.uid() { - metadata.mode() & ((p as u32) << 6) != 0 - } else if getegid() == metadata.gid() { - metadata.mode() & ((p as u32) << 3) != 0 - } else { - metadata.mode() & (p as u32) != 0 + unsafe { + #[cfg(any( + target_os = "linux", + target_os = "hurd", + target_os = "redox", + target_os = "cygwin", + target_os = "solaris", + target_os = "illumos" + ))] + { + libc::euidaccess(c_path.as_ptr(), mode) == 0 + } + + #[cfg(target_os = "freebsd")] + { + libc::eaccess(c_path.as_ptr(), mode) == 0 + } + + #[cfg(any( + target_os = "macos", + target_os = "ios", + target_os = "netbsd", + target_os = "openbsd", + target_os = "dragonfly" + ))] + { + libc::faccessat(libc::AT_FDCWD, c_path.as_ptr(), mode, libc::AT_EACCESS) == 0 + } + + // Fallback for other OSes + #[cfg(not(any( + target_os = "linux", + target_os = "hurd", + target_os = "redox", + target_os = "cygwin", + target_os = "solaris", + target_os = "illumos", + target_os = "freebsd", + target_os = "macos", + target_os = "ios", + target_os = "netbsd", + target_os = "openbsd", + target_os = "dragonfly" + )))] + { + // fallback: use regular access() + libc::access(c_path.as_ptr(), mode) == 0 + } } }; @@ -308,12 +353,12 @@ fn path(path: &OsStr, condition: &PathCondition) -> bool { PathCondition::Sticky => metadata.mode() & S_ISVTX != 0, PathCondition::UserOwns => metadata.uid() == geteuid(), PathCondition::Fifo => file_type.is_fifo(), - PathCondition::Readable => perm(metadata, Permission::Read), + PathCondition::Readable => check_access(path, libc::R_OK), PathCondition::Socket => file_type.is_socket(), PathCondition::NonEmpty => metadata.size() > 0, PathCondition::UserIdFlag => metadata.mode() & S_ISUID != 0, - PathCondition::Writable => perm(metadata, Permission::Write), - PathCondition::Executable => perm(metadata, Permission::Execute), + PathCondition::Writable => check_access(path, libc::W_OK), + PathCondition::Executable => check_access(path, libc::X_OK), } } From f59d7eaf843315550c00e5c601109ec38e2b782d Mon Sep 17 00:00:00 2001 From: Jeff Turner Date: Tue, 30 Dec 2025 18:35:04 +1100 Subject: [PATCH 4/5] test: redox libc does not have euidaccess --- src/uu/test/src/test.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/uu/test/src/test.rs b/src/uu/test/src/test.rs index 92b0adf178e..08c362819d8 100644 --- a/src/uu/test/src/test.rs +++ b/src/uu/test/src/test.rs @@ -279,7 +279,6 @@ fn path(path: &OsStr, condition: &PathCondition) -> bool { #[cfg(any( target_os = "linux", target_os = "hurd", - target_os = "redox", target_os = "cygwin", target_os = "solaris", target_os = "illumos" @@ -308,7 +307,6 @@ fn path(path: &OsStr, condition: &PathCondition) -> bool { #[cfg(not(any( target_os = "linux", target_os = "hurd", - target_os = "redox", target_os = "cygwin", target_os = "solaris", target_os = "illumos", From e3d63d7e952f1703d26bb2a7176efcad4c958f91 Mon Sep 17 00:00:00 2001 From: Jeff Turner Date: Sat, 17 Jan 2026 15:54:22 +1100 Subject: [PATCH 5/5] test: get tests working in sudo-less environments --- tests/by-util/test_test.rs | 117 +++++++++++++++++++++++-------------- 1 file changed, 74 insertions(+), 43 deletions(-) diff --git a/tests/by-util/test_test.rs b/tests/by-util/test_test.rs index 207bd817ea1..19aebbe6576 100644 --- a/tests/by-util/test_test.rs +++ b/tests/by-util/test_test.rs @@ -1037,10 +1037,7 @@ fn test_unary_op_as_literal_in_three_arg_form() { // Only test platforms supporting setfacl. #[test] -#[cfg(any( - target_os = "linux", - target_os = "cygwin" -))] +#[cfg(any(target_os = "linux", target_os = "cygwin"))] fn test_permission_with_acl() { // Test that permission checks (-r, -w, -x) correctly handle ACL (Access Control List) // permissions, not just traditional Unix permissions. This requires using euidaccess() @@ -1054,16 +1051,28 @@ fn test_permission_with_acl() { let scene = TestScenario::new(util_name!()); - // Must be run as non-root user (otherwise owner permissions would apply) - if scene.cmd("whoami").run().stdout_str().trim() == "root" { - return; - } + let is_root = scene.cmd("whoami").run().stdout_str().trim() == "root"; // Check if setfacl is available if Command::new("which").arg("setfacl").output().is_err() { return; } + // Skip if we cannot run non-interactive sudo and we're not already root. + let sudo_ok = if is_root { + true + } else { + Command::new("sudo") + .env("LC_ALL", "C") + .args(["-E", "--non-interactive", "whoami"]) + .output() + .ok() + .is_some_and(|output| output.status.success() && output.stdout == b"root\n") + }; + if !sudo_ok { + return; + } + // Get current username for ACL let Ok(username_output) = Command::new("whoami").output() else { return; @@ -1076,46 +1085,64 @@ fn test_permission_with_acl() { let test_path_str = test_path.to_str().unwrap(); // Create directory with root:root ownership and 750 permissions (no access for others) - let install_result = scene - .cmd("sudo") - .args(&[ - "-E", - "--non-interactive", - "install", - "-d", - "-m", - "750", - "-o", - "root", - "-g", - "root", - test_path_str, - ]) - .run(); + let install_result = if is_root { + scene + .cmd("install") + .args(&["-d", "-m", "750", "-o", "root", "-g", "root", test_path_str]) + .run() + } else { + scene + .cmd("sudo") + .args(&[ + "-E", + "--non-interactive", + "install", + "-d", + "-m", + "750", + "-o", + "root", + "-g", + "root", + test_path_str, + ]) + .run() + }; if !install_result.succeeded() { return; } // Grant ACL permissions to current user (read + execute) - let setfacl_result = scene - .cmd("sudo") - .args(&[ - "-E", - "--non-interactive", - "setfacl", - "-m", - &format!("u:{username}:rx"), - test_path_str, - ]) - .run(); + let setfacl_result = if is_root { + scene + .cmd("setfacl") + .args(&["-m", &format!("u:{username}:rx"), test_path_str]) + .run() + } else { + scene + .cmd("sudo") + .args(&[ + "-E", + "--non-interactive", + "setfacl", + "-m", + &format!("u:{username}:rx"), + test_path_str, + ]) + .run() + }; if !setfacl_result.succeeded() { // Clean up and skip test - let _ = scene - .cmd("sudo") - .args(&["-E", "--non-interactive", "rm", "-rf", test_path_str]) - .run(); + let _ = if is_root { + scene.cmd("rm").args(&["-rf", test_path_str]).run() + } else { + scene + .cmd("sudo") + .args(&["-E", "--non-interactive", "rm", "-rf", test_path_str]) + .run() + }; return; } @@ -1129,10 +1156,14 @@ fn test_permission_with_acl() { scene.ucmd().args(&["!", "-w", test_path_str]).succeeds(); // Clean up - let _ = scene - .cmd("sudo") - .args(&["-E", "--non-interactive", "rm", "-rf", test_path_str]) - .run(); + let _ = if is_root { + scene.cmd("rm").args(&["-rf", test_path_str]).run() + } else { + scene + .cmd("sudo") + .args(&["-E", "--non-interactive", "rm", "-rf", test_path_str]) + .run() + }; } #[test]