ls: only show "+" for a real ACL, and show it before "."

src/uu/ls/src/display.rs

@@ -314,14 +314,15 @@ fn pad_left(string: &str, count: usize) -> String {
 }
 
 /// Returns the alternate-access indicator that follows the 9-bit permission
-/// string in long-format output: `.` for a non-trivial security context, `+`
-/// for an ACL, otherwise a space (which acts as a placeholder so columns line
-/// up across items that don't carry an indicator themselves).
+/// string in long-format output: `+` for an ACL (even when the file also has a
+/// security context, which is an extended attribute too), `.` for a non-trivial
+/// security context alone, otherwise a space (which acts as a placeholder so
+/// columns line up across items that don't carry an indicator themselves).
 fn alt_access_indicator(item: &PathData, config: &Config, is_acl_set: bool) -> u8 {
-    if item.security_context(config).len() > 1 {
-        b'.'
-    } else if is_acl_set {
+    if is_acl_set {
         b'+'
+    } else if item.security_context(config).len() > 1 {
+        b'.'
     } else {
         b' '
     }

src/uucore/src/lib/features/fsxattr.rs

@@ -3,7 +3,7 @@
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
 
-// spell-checker:ignore getxattr posix_acl_default posix_acl_access ENOTSUP EOPNOTSUPP renamer
+// spell-checker:ignore getxattr posix_acl_default posix_acl_access nfs4_acl ENOTSUP EOPNOTSUPP renamer
 
 //! Set of functions to manage xattr on files and dirs
 
@@ -249,8 +249,11 @@ pub fn apply_xattrs_fd_ignore_unsupported(
 
 /// Checks if a file has an Access Control List (ACL).
 ///
-/// On macOS this asks the ACL API; elsewhere it looks at the file's extended
-/// attributes.
+/// On macOS this asks the ACL API. On Linux and Android it looks for the
+/// attributes that store an ACL (for `NFSv4`, one that names someone besides the
+/// owner, the group and everyone), so a security label (`security.selinux`) or a
+/// `user.*` attribute alone is not an ACL. Elsewhere it looks at the file's
+/// extended attributes.
 ///
 /// # Arguments
 ///
@@ -259,14 +262,17 @@ pub fn apply_xattrs_fd_ignore_unsupported(
 ///
 /// # Returns
 ///
-/// `true` if the file has an extended ACL (on macOS) or any extended attributes
-/// (elsewhere), `false` otherwise.
+/// `true` if the file has an extended ACL (on macOS, Linux and Android) or any
+/// extended attributes (elsewhere), `false` otherwise.
 pub fn has_acl<P: AsRef<Path>>(file: P, dereference: bool) -> bool {
     // On macOS most files carry some xattr (e.g. com.apple.provenance) and the
     // ACL is not one of them, so ask the ACL API instead.
     #[cfg(target_os = "macos")]
     return macos_acl::has_extended_acl(file.as_ref(), dereference);
 
+    #[cfg(not(target_os = "macos"))]
+    let file = file.as_ref();
+
     // don't use exacl here, it is doing more getxattr call then needed
     #[cfg(not(target_os = "macos"))]
     let attrs = if dereference {
@@ -275,13 +281,75 @@ pub fn has_acl<P: AsRef<Path>>(file: P, dereference: bool) -> bool {
         xattr::list(file)
     };
 
-    #[cfg(not(target_os = "macos"))]
-    attrs.is_ok_and(|acl| {
+    // The attributes the kernel stores an ACL in. Other attributes, such as the
+    // SELinux or SMACK label, say nothing about an ACL.
+    #[cfg(any(target_os = "linux", target_os = "android"))]
+    return attrs.is_ok_and(|mut names| {
+        names.any(|name| {
+            if name == OsStr::new("system.posix_acl_access")
+                || name == OsStr::new("system.posix_acl_default")
+            {
+                return true;
+            }
+            if name != OsStr::new("system.nfs4_acl") {
+                return false;
+            }
+            // NFSv4 lists this attribute for every file, so look at its content. One that
+            // cannot be read is no evidence of an ACL.
+            let value = if dereference {
+                xattr::get_deref(file, &name)
+            } else {
+                xattr::get(file, &name)
+            };
+            value
+                .ok()
+                .flatten()
+                .is_some_and(|raw| nfs4_acl_is_nontrivial(&raw))
+        })
+    });
+
+    #[cfg(not(any(target_os = "macos", target_os = "linux", target_os = "android")))]
+    attrs.is_ok_and(|mut acl| {
         // if we have extra attributes, we have an acl
-        acl.count() > 0
+        acl.next().is_some()
     })
 }
 
+/// Whether an `NFSv4` ACL (the XDR value of `system.nfs4_acl`) names anyone besides
+/// the owner, the group and everyone, which is all the mode bits can express.
+/// A value that cannot be parsed counts as non-trivial.
+#[cfg(any(target_os = "linux", target_os = "android"))]
+fn nfs4_acl_is_nontrivial(raw: &[u8]) -> bool {
+    // Big-endian 32-bit words; strings are length-prefixed and padded to 4 bytes.
+    fn word(raw: &mut &[u8]) -> Option<usize> {
+        let (head, rest) = raw.split_first_chunk::<4>()?;
+        *raw = rest;
+        usize::try_from(u32::from_be_bytes(*head)).ok()
+    }
+
+    fn named_principal(mut raw: &[u8]) -> Option<bool> {
+        let count = word(&mut raw)?;
+        for _ in 0..count {
+            // ACE type, flags and access mask, then the principal.
+            let [_, _, _, len] = [
+                word(&mut raw)?,
+                word(&mut raw)?,
+                word(&mut raw)?,
+                word(&mut raw)?,
+            ];
+            let padded = len.checked_add(3)? & !3;
+            let who = raw.get(..len)?;
+            raw = raw.get(padded..)?;
+            if !matches!(who, b"OWNER@" | b"GROUP@" | b"EVERYONE@") {
+                return Some(true);
+            }
+        }
+        Some(false)
+    }
+
+    named_principal(raw).unwrap_or(true)
+}
+
 #[cfg(target_os = "macos")]
 mod macos_acl {
     use std::ffi::{CString, c_char, c_int, c_void};
@@ -555,15 +623,22 @@ mod tests {
 
         File::create(&file_path).unwrap();
 
-        // FIXME: this fails on a system that uses SELinux
         assert!(!has_acl(&file_path, true));
 
         let test_attr = "user.test_acl";
         let test_value = b"test value";
         xattr::set(&file_path, test_attr, test_value).unwrap();
 
-        // On macOS an ACL isn't an xattr, so an xattr alone is not an ACL.
-        assert_eq!(has_acl(&file_path, true), cfg!(not(target_os = "macos")));
+        // An ACL isn't any xattr: on macOS it is not an xattr at all, and on
+        // Linux only the posix_acl/nfs4_acl attributes are ACLs.
+        assert_eq!(
+            has_acl(&file_path, true),
+            cfg!(not(any(
+                target_os = "macos",
+                target_os = "linux",
+                target_os = "android"
+            )))
+        );
         assert!(!has_security_cap_acl(&file_path, true));
 
         // FreeBSD/NetBSD's xattr library does not support the "security" namespace
@@ -580,6 +655,75 @@ mod tests {
         }
     }
 
+    #[test]
+    #[cfg(any(target_os = "linux", target_os = "android"))]
+    fn test_nfs4_acl_is_nontrivial() {
+        // count, then per entry: type, flags, access mask, principal (length, padded).
+        fn acl(principals: &[&str]) -> Vec<u8> {
+            let mut raw = u32::try_from(principals.len())
+                .unwrap()
+                .to_be_bytes()
+                .to_vec();
+            for who in principals {
+                for word in [0u32, 0, 0x1f, u32::try_from(who.len()).unwrap()] {
+                    raw.extend(word.to_be_bytes());
+                }
+                raw.extend(who.as_bytes());
+                raw.resize(raw.len().next_multiple_of(4), 0);
+            }
+            raw
+        }
+
+        assert!(!nfs4_acl_is_nontrivial(&acl(&[])));
+        assert!(!nfs4_acl_is_nontrivial(&acl(&[
+            "OWNER@",
+            "GROUP@",
+            "EVERYONE@"
+        ])));
+        assert!(nfs4_acl_is_nontrivial(&acl(&[
+            "OWNER@",
+            "alice@example.com",
+            "EVERYONE@"
+        ])));
+        assert!(nfs4_acl_is_nontrivial(&acl(&["1000"])));
+
+        // Truncated or garbage values are not trusted to be trivial.
+        let full = acl(&["OWNER@", "GROUP@"]);
+        assert!(nfs4_acl_is_nontrivial(&full[..full.len() - 1]));
+        assert!(nfs4_acl_is_nontrivial(&[0, 0]));
+        assert!(nfs4_acl_is_nontrivial(&[0xff; 8]));
+    }
+
+    #[test]
+    #[cfg(any(target_os = "linux", target_os = "android"))]
+    fn test_file_has_acl_posix_attribute() {
+        let temp_dir = tempdir().unwrap();
+        let file_path = temp_dir.path().join("test_file.txt");
+        File::create(&file_path).unwrap();
+
+        // An extended POSIX ACL in its xattr form: version 2, then (tag, perm, id)
+        // entries. The named user entry is what makes it more than the mode bits,
+        // which the kernel would not store as an ACL.
+        let mut acl = 2u32.to_le_bytes().to_vec();
+        for (tag, perm, id) in [
+            (0x01u16, 6u16, u32::MAX), // user::rw-
+            (0x02, 4, 1000),           // user:1000:r--
+            (0x04, 4, u32::MAX),       // group::r--
+            (0x10, 4, u32::MAX),       // mask::r--
+            (0x20, 4, u32::MAX),       // other::r--
+        ] {
+            acl.extend(tag.to_le_bytes());
+            acl.extend(perm.to_le_bytes());
+            acl.extend(id.to_le_bytes());
+        }
+        // Not every file system stores ACLs (or lets the test set them).
+        if xattr::set(&file_path, "system.posix_acl_access", &acl).is_err() {
+            return;
+        }
+        assert!(has_acl(&file_path, true));
+        assert!(has_acl(&file_path, false));
+    }
+
     #[test]
     fn test_apply_and_retrieve_xattrs_fd() {
         use std::fs::OpenOptions;

tests/by-util/test_ls.rs

@@ -5431,6 +5431,22 @@ fn test_ls_selinux_context_indicator() {
         );
     }
 
+    // A file with an ACL as well as a context is listed with `+`: the context is
+    // an extended attribute too, so it must not hide the ACL.
+    at.touch("file_acl");
+    if run_attr_tool(
+        "setfacl",
+        &["-m", "u:bin:r--", &at.plus_as_string("file_acl")],
+    ) {
+        let result = scene.ucmd().args(&["-l", "file_acl"]).succeeds();
+        let first_line = result.stdout_str().lines().next().unwrap().to_string();
+        assert_eq!(
+            first_line.chars().nth(10),
+            Some('+'),
+            "Expected '+' indicator for an ACL plus SELinux context, got: {first_line}",
+        );
+    }
+
     // Test that ls -lnZ properly shows the context
     for file in ["file", "link"] {
         let result = scene.ucmd().args(&["-lnZ", file]).succeeds();
@@ -7182,6 +7198,95 @@ fn test_acl_display_macos() {
         .stdout_matches(&re_without_acl);
 }
 
+/// Runs an attribute tool (`setfacl`, `setfattr`) on a test file. Returns false, after
+/// saying why, when it cannot be used here, so that the caller can skip its test.
+#[cfg(any(target_os = "linux", target_os = "android"))]
+fn run_attr_tool(tool: &str, args: &[&str]) -> bool {
+    use std::process::Command;
+
+    // calling the command directly. xattr requires some dev packages to be installed
+    // and it adds a complex dependency just for a test
+    match Command::new(tool).args(args).status().map(|s| s.code()) {
+        Ok(Some(0)) => true,
+        Ok(_) => {
+            println!("test skipped: {tool} failed");
+            false
+        }
+        Err(e) => {
+            println!("test skipped: {tool} failed with {e}");
+            false
+        }
+    }
+}
+
+// A real ACL is listed with `+`, so the check below that other attributes are not
+// is not just passing because `+` never shows up.
+#[cfg(any(target_os = "linux", target_os = "android"))]
+#[test]
+fn test_acl_display_access_acl_on_file() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+    at.touch("with_acl");
+    at.touch("without_acl");
+
+    if !run_attr_tool(
+        "setfacl",
+        &["-m", "u:bin:r--", &at.plus_as_string("with_acl")],
+    ) {
+        return;
+    }
+
+    let result = scene
+        .ucmd()
+        .args(&["-l", "with_acl", "without_acl"])
+        .succeeds();
+    let out = result.stdout_str();
+    assert!(
+        Regex::new(r"(?m)^-[rwx-]{9}\+ .*with_acl$")
+            .unwrap()
+            .is_match(out),
+        "{out}"
+    );
+    assert!(
+        Regex::new(r"(?m)^-[rwx-]{9}[^+].*without_acl$")
+            .unwrap()
+            .is_match(out),
+        "{out}"
+    );
+}
+
+// An extended attribute that is not an ACL (a security label is one too) must not
+// be listed with the ACL `+`.
+#[cfg(any(target_os = "linux", target_os = "android"))]
+#[test]
+fn test_acl_display_ignores_other_xattrs() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+    at.touch("with_xattr");
+
+    if !run_attr_tool(
+        "setfattr",
+        &[
+            "-n",
+            "user.test",
+            "-v",
+            "1",
+            &at.plus_as_string("with_xattr"),
+        ],
+    ) {
+        return;
+    }
+
+    // Whatever follows the mode bits (nothing, or `.` for a security context on
+    // an SELinux host), it is not the ACL `+`.
+    let re_without_acl = Regex::new(r"(?m)^-[rwx-]{9}[^+].*with_xattr$").unwrap();
+    scene
+        .ucmd()
+        .args(&["-l", "with_xattr"])
+        .succeeds()
+        .stdout_matches(&re_without_acl);
+}
+
 // Regression test for https://github.com/uutils/coreutils/issues/10980
 // Each file with an ACL must not inflate the link-count column width.
 #[cfg(all(unix, not(target_vendor = "apple")))]